Skip to content
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

Long-term solution for #2632 #8130

Closed
wants to merge 2 commits into from
Closed

Conversation

aafemt
Copy link
Contributor

@aafemt aafemt commented May 24, 2024

Current workaround should be preserved because of stored BLR in old databases.

@asfernandes
Copy link
Member

asfernandes commented May 25, 2024

What is a long-term solution for a bug already fixed 14 years ago?

@aafemt
Copy link
Contributor Author

aafemt commented May 25, 2024

You didn't fix it. You merely workarounded it skipping unneeded assignments. This PR eliminates the assignments themselves.

@aafemt aafemt closed this May 26, 2024
@aafemt aafemt reopened this May 26, 2024
@asfernandes
Copy link
Member

My solution fixed #2632:

SQL> CREATE DOMAIN BOOL AS INTEGER NOT NULL CHECK (value = 0 or value = 1)!
SQL> 
SQL> CREATE PROCEDURE TEST returns (b bool)
CON> as
CON> begin
CON> if (1 = 0) then
CON> suspend;
CON> end!
SQL> 
SQL> select * from test!
SQL> execute procedure test!
Statement failed, SQLSTATE = 42000
validation error for variable B, value "*** null ***"
-At procedure 'TEST' line: 4, col: 1

Your "long-term solution" creates a bug:

SQL> CREATE DOMAIN BOOL AS INTEGER NOT NULL CHECK (value = 0 or value = 1)!
SQL> 
SQL> CREATE PROCEDURE TEST returns (b bool)
CON> as
CON> begin
CON> if (1 = 0) then
CON> suspend;
CON> end!
SQL> select * from test!
SQL> execute procedure test!

           B 
============ 
           0 

@asfernandes
Copy link
Member

How ever a NOT NULL output, without DEFAULT, unassigned parameter can become 0?

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

Because user application (isql) assigned it to 0 in output buffer before call and called procedure never changed it. Much better question is why selectable stored procedure performs assign to output parameters if SUSPEND is never called.

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

My solution fixed #2632:

Let me remember your comment from there:

If no value is returned, looks like a bug for me if they are validated.

So what is a reason to validate values that are never assigned?

@asfernandes
Copy link
Member

Because user application (isql) assigned it to 0 in output buffer before call and called procedure never changed it. Much better question is why selectable stored procedure performs assign to output parameters if SUSPEND is never called.

This is no-sense, this is output parameter, not IN-OUT parameter.

@hvlad
Copy link
Member

hvlad commented May 27, 2024

Because user application (isql) assigned it to 0 in output buffer before call and called procedure never changed it.

Procedure should return no rows, and isql perfectly handles such conditions.
But it looks like in your case procedure returns one row, that isql prints.

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

This is no-sense, this is output parameter, not IN-OUT parameter.

RAM cannot be "uninitialized". It always contain some value. And keep to contain it if no assignment was performed.

Let me quote documentation:

Each parameter has a data type. The NOT NULL constraint can also be specified for any parameter, to prevent NULL being passed or assigned to it.

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

But it looks like in your case procedure returns one row, that isql prints.

Notice that it has been called with EXECUTE PROCEDURE second time in the script. Such things always were a gray zone in Firebird.

@asfernandes
Copy link
Member

Such things always were a gray zone in Firebird.

It's the first time I've heard that. Looks like your only opinion.

@hvlad
Copy link
Member

hvlad commented May 27, 2024

But it looks like in your case procedure returns one row, that isql prints.

Notice that it has been called with EXECUTE PROCEDURE second time in the script.

Yes, missed it

Such things always were a gray zone in Firebird.

Could you explain more ?

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

Could you explain more ?

Difference between selectable and executable stored procedures is not explicitly stated in creation statement but derived from existence of "suspend" inside. Executing of selectable procedure have defined behavior only if SUSPEND is called otherwise results are undefined and may be confusing. Behavior of selecting from executable procedure is not defined though predictable.

EXECUTE BLOCK has a little different rules which adds more confusion because its type derived not from SUSPEND inside but only from existence of output parameters.

@hvlad
Copy link
Member

hvlad commented May 27, 2024

Difference between selectable and executable stored procedures is not explicitly stated in creation statement but derived from existence of "suspend" inside.

I.e. it is defined and not a "gray zone".

Executing of selectable procedure have defined behavior only if SUSPEND is called otherwise results are undefined and may be confusing.

Not agree. EXECUTE PROCEDURE always returns contents of its output params - this is defined behavior and not depends on presence of SUSPEND.

Behavior of selecting from executable procedure is not defined though predictable.

If procedure contains no SUSPEND statement (or execution flow doesn't runs one) then SELECT returns no rows.
This is defined behavior.

EXECUTE BLOCK has a little different rules which adds more confusion because its type derived not from SUSPEND inside but only from existence of output parameters.

Not related with current discussion.

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

Not related with current discussion.

Yes, not directly related, I just come to this PR trying to eliminate EOS variable from SELECT and EB. That's why side-effect for SP left unnoticed.

If you consider the behavior of SP as a right one then perhaps following script should be registered as a separate issue :

SQL> create procedure test returns (a integer) as begin a = 5; end^
SQL> execute procedure test^

           A
============
           5

SQL> execute block returns (a integer) as begin a = 5; end^
SQL>

@hvlad
Copy link
Member

hvlad commented May 27, 2024

Not related with current discussion.

Yes, not directly related, I just come to this PR trying to eliminate EOS variable from SELECT and EB.

For what ?

That's why side-effect for SP left unnoticed.

If you consider the behavior of SP as a right one then perhaps following script should be registered as a separate issue :

SQL> create procedure test returns (a integer) as begin a = 5; end^
SQL> execute procedure test^

           A
============
           5

SQL> execute block returns (a integer) as begin a = 5; end^
SQL>

I see no issues in results above.

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

For what ?

To make blr_send/blr_receive work with message and buffer provided by EXE_receive()/EXE_send(). They have no room for EOS.

I see no issues in results above.

The same PSQL code producing different results. It is not an issue for you?

@asfernandes
Copy link
Member

asfernandes commented May 27, 2024

I see no issues in results above.

The confusion (for me) is because AFAIR in first version (before final release) of EXECUTE BLOCK, the behavior was like procedures. If there were no SUSPEND, it was going to return one row.

@hvlad
Copy link
Member

hvlad commented May 27, 2024

I see no issues in results above.

The confusion (for me) is because AFAIR in first version (before final release) of EXECUTE BLOCK, the behavior was like procedures. If there were no SUSPEND, it was going to return one row.

I really don't remember it ;)

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

The confusion (for me) is because AFAIR in first version (before final release) of EXECUTE BLOCK, the behavior was like procedures. If there were no SUSPEND, it was going to return one row.

Wasn't it only described as "select" that time?

@hvlad
Copy link
Member

hvlad commented May 27, 2024

For what ?

To make blr_send/blr_receive work with message and buffer provided by EXE_receive()/EXE_send(). They have no room for EOS.

Still not clear.

I see no issues in results above.

The same PSQL code producing different results. It is not an issue for you?

No. There is rule and it is well defined.
For selectable procedure: no SUSPEND - no output row, N SUSPEND's - N rows. N could be zero.
For non-selectable procedure: exacly one row.
If EXEC BLOCK have outputs - it is selectable, else it produces no rows.
All is defined and easy when you follow it.

@aafemt
Copy link
Contributor Author

aafemt commented May 27, 2024

Still not clear.

I expect it to solve #8082 and improve performance at once.

No. There is rule and it is well defined.

It is an internal rule. And too inconsistent IMHO. Besides, documentation disagree:

Executes a block of PSQL code as if it were a stored procedure, optionally with input and output parameters and variable declarations. This allows the user to perform "on-the-fly" PSQL within a DSQL context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants