Extract external routines in one pass

[vasiliy-yashkov]

Extract external procedures and functions in one pass because they have no BLR and cannot have dependencies.

[aafemt]

Are you sure that procedure without output parameters cannot be selectable?
select count(*) from anyUDR()

[vasiliy-yashkov]

select count(*) from anyUDR()

This causes SQL error code = -84; procedure TEST_UDR does not return any values. Even if the procedure has prc_selectable type.

[sim1984]

This solves a completely different problem. #7699
As for creating external procedures and functions in one-pass, I agree with @asfernandes. This is not necessary and can even be harmful in some cases.

[dyemanov]

@sim1984 yes, this is exactly the bug we were fixing. I suggested a single pass extraction as an extension to the original patch.

[hvlad]

How this is supposed to work with pre ODS13 databases ?

[dyemanov]

Current ISQL (FB 3-4-5) cannot extract procedures/functions from pre-ODS12 databases, so nothing really changed.

[hvlad]

Both list_external_procedures() and list_external_functions() is called unconditionally by EXTRACT_ddl(), see

https://github.com/FirebirdSQL/firebird/pull/8054/files#diff-580a2883156750bdebc94df7a24f090f8b288b49558292ae66fde9b21cbbc432R184

so, what happens in case of pre-ODS13 DB ?
Error will be raised ?
Nothing, as query below returns no rows ?
Why assert then ? It will not make DEBUG build happy, AFAIU.
I just want it to be clear.

[dyemanov]

Error "field does not exist". You may find the same assert in list_procedure_bodies() which is also executed unconditionally. AFAIU, the only purpose of the assert is to warn that the extract function is not able to deal with older ODS. We may remove these asserts separately, if you disagree with their usage in general. They are not related to this PR and existed before.

[hvlad]

Strange logic, as for me - use assert and runtime error instead of condition and correct handling of old ODS.
I agree, it is not related with this PR. I never looked into this code before and thus wondering.

[dyemanov]

IIRC, it was discussed in fb-devel and we agreed to simplify the code this way, but it was ages ago...

[asfernandes]

I think this is going to cause problems.

When external routines are created, engine immediately calls external code, that can interact with existing metadata.

[dyemanov]

I think this is going to cause problems.

When external routines are created, engine immediately calls external code, that can interact with existing metadata.

Hmmm. Maybe theoretically this is possible, but is it really our problem? When UDR is created we have no idea about the logic inside the external module. How can we be expected to pre-create some metadata in advance? What if the external code would depend on some pre-existing data too?

[asfernandes]

I think this is going to cause problems.
When external routines are created, engine immediately calls external code, that can interact with existing metadata.

Hmmm. Maybe theoretically this is possible, but is it really our problem? When UDR is created we have no idea about the logic inside the external module. How can we be expected to pre-create some metadata in advance? What if the external code would depend on some pre-existing data too?

Nothing different than PSQL routines. We shouldn't care about routines content, but we do to make their actual usage work as we don't know their creation/update order.

For example, it may receive another procedure name in its body, inspect it and precompute things for fast transformation of its result to json in call time.

The thing is that external routines may do things at "compilation" phase, and this things may be interaction with metadata.

Note also that UDR is a layer on top of external routines. External routines may do more things and it still is a public API.

[dyemanov]

I surely missed the "compilation stage" external calls, could you please point me where it happens?

[asfernandes]

I surely missed the "compilation stage" external calls, could you please point me where it happens?

If you put breakpoint in UdrEngine.cpp at

IExternalFunction* Engine::makeFunction(ThrowStatusWrapper* status, IExternalContext* context,
	IRoutineMetadata* metadata, IMetadataBuilder* inBuilder, IMetadataBuilder* outBuilder)
{
	return FB_NEW SharedFunction(status, this, context, metadata, inBuilder, outBuilder);
}

It will be called from DFW.

This also prevents problematic routines to be created:

create function div (
    n1 integer,
    n2 integer
) returns double precision
    external name 'udf_compat!ERROR'
    engine udr;

I may accept a change to only load/validate routines when they are called for the first time. But that should also happen when call is present in another PSQL routine, i.e., PSQL_FUNC is created and calls EXT_FUNC, then EXT_FUN should be loaded only when it's called.

It may have the advantage of declare something that is not yet present/configured in the machine. At same time, it will defer erroneous declaration.

But this patch alone is problematic IMO.

[dyemanov]

Thanks. I asked because there may be a difference. PSQL routines are compiled into BLR during DDL execution -- this is the first stage when dependent objects should already exist (METD should be able to find them). Then at DFW time dependencies are tracked (BLR is parsed) and this is the second stage. For external routines we have only the second stage (DFW time) dependencies access. At this time all dependencies already exist in the database. They're not committed yet, but visible for the current transaction. So I'm wondering whether it's enough to satisfy the possible external calls, given that ISQL extracts all metadata to be executed in one transaction.