-
-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract external routines in one pass #8054
base: master
Are you sure you want to change the base?
Extract external routines in one pass #8054
Conversation
src/dsql/DdlNodes.epp
Outdated
@@ -2941,7 +2941,7 @@ bool CreateAlterProcedureNode::executeAlter(thread_db* tdbb, DsqlCompilerScratch | |||
if (secondPass) | |||
{ | |||
P.RDB$PROCEDURE_TYPE.NULL = FALSE; | |||
P.RDB$PROCEDURE_TYPE = (USHORT) prc_selectable; | |||
P.RDB$PROCEDURE_TYPE = returns.getCount() != 0 ? (USHORT) prc_selectable : (USHORT) prc_executable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that procedure without output parameters cannot be selectable?
select count(*) from anyUDR()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sim1984 yes, this is exactly the bug we were fixing. I suggested a single pass extraction as an extension to the original patch.
* | ||
**************************************/ | ||
|
||
fb_assert(isqlGlob.major_ods >= ODS_VERSION12); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this is supposed to work with pre ODS13 databases ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current ISQL (FB 3-4-5) cannot extract procedures/functions from pre-ODS12 databases, so nothing really changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both list_external_procedures()
and list_external_functions()
is called unconditionally by EXTRACT_ddl()
, see
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, it was discussed in fb-devel and we agreed to simplify the code this way, but it was ages ago...
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. |
I surely missed the "compilation stage" external calls, could you please point me where it happens? |
If you put breakpoint in UdrEngine.cpp at
It will be called from DFW. This also prevents problematic routines to be created:
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. |
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. |
Extract external procedures and functions in one pass because they have no BLR and cannot have dependencies.