FirebirdSQL / firebird

Firebird server, client and tools
https://firebirdsql.org
1.26k stars 217 forks source link

AUTOTERM adds a terminator character to the sql text of the statement #8325

Closed ant-zuev closed 3 days ago

ant-zuev commented 3 days ago

We get different MON$SQL_TEXT values without using AUTOTERM and when using it. A simple example for the statement "select MON$SQL_TEXT from MON$STATEMENTS;":

SQL> set list on;
SQL> set blob all;
SQL> select MON$SQL_TEXT from MON$STATEMENTS;

MON$SQL_TEXT                    0:3
select MON$SQL_TEXT from MON$STATEMENTS

SQL> commit;
SQL> set autoterm on;
SQL> select MON$SQL_TEXT from MON$STATEMENTS;

MON$SQL_TEXT                    0:5
select MON$SQL_TEXT from MON$STATEMENTS;
aafemt commented 3 days ago

By design. With AUTOTERM ON you don't need the semicolon after the statement but in your script you still have it and it is considered as a part of the statement, not a terminator. Exactly what you see in monitoring tables.

ant-zuev commented 3 days ago

By design. With AUTOTERM ON you don't need the semicolon after the statement but in your script you still have it and it is considered as a part of the statement, not a terminator. Exactly what you see in monitoring tables.

However, I still can't execute a statement without semicolon with AUTOTERM ON:

SQL> set autoterm on;
SQL> select MON$SQL_TEXT from MON$STATEMENTS
CON> 

As far as I understand, AUTOTERM ON is needed to use PSQL without changing the terminator, not to execute statements without it.

aafemt commented 3 days ago

However, I still can't execute a statement without semicolon with AUTOTERM ON

And this is a real AUTOTERM bug IMHO.

mrotteveel commented 3 days ago

However, I still can't execute a statement without semicolon with AUTOTERM ON

And this is a real AUTOTERM bug IMHO.

I don't think so, because how would ISQL determine that you're done with the statement as opposed to wanting to add a WHERE clause (or if you already have a WHERE clause, an additional OR/AND, or a UNION, etc, etc.)

asfernandes commented 3 days ago

However, I still can't execute a statement without semicolon with AUTOTERM ON

And this is a real AUTOTERM bug IMHO.

I don't think so, because how would ISQL determine that you're done with the statement as opposed to wanting to add a WHERE clause (or if you already have a WHERE clause, an additional OR/AND, or a UNION, etc, etc.)

Correct. Auto term only allow a semicolon to not finish the statement when more things can follow, like happens in a psql block.

I see no bug.

ant-zuev commented 3 days ago

However, I still can't execute a statement without semicolon with AUTOTERM ON

And this is a real AUTOTERM bug IMHO.

I don't think so, because how would ISQL determine that you're done with the statement as opposed to wanting to add a WHERE clause (or if you already have a WHERE clause, an additional OR/AND, or a UNION, etc, etc.)

Correct. Auto term only allow a semicolon to not finish the statement when more things can follow, like happens in a psql block.

I see no bug.

And what about semicolon in the mon$sql_text?

aafemt commented 3 days ago

I don't think so, because how would ISQL determine that you're done with the statement as opposed to wanting to add a WHERE clause (or if you already have a WHERE clause, an additional OR/AND, or a UNION, etc, etc.)

By the terminator which is already set. In this case the semicolon after DSQL query in this testcase would be normally recognized and trimmed. But it is too late and I have no desire to repeat that discussion again.

mrotteveel commented 3 days ago

By the terminator which is already set. In this case the semicolon after DSQL query in this testcase would be normally recognized and trimmed. But it is too late and I have no desire to repeat that discussion again.

Reread your own comment:

However, I still can't execute a statement without semicolon with AUTOTERM ON

And this is a real AUTOTERM bug IMHO.

In other words, you implied that "can't execute a statement without semicolon" is the real bug, while it isn't.

I would consider the original described issue that the semicolon is not trimmed a bug, but not a very severe one.

asfernandes commented 3 days ago

And what about semicolon in the mon$sql_text?

This is documented and the flag is even called IStatement::PREPARE_REQUIRE_SEMICOLON.

Also, semicolon is valid, and if received, specially with this flag, should be registered as such.

What is really wrong and questionable is the old behavior where this works:

set term !;
select mon$sql_text from mon$statements;;!
-- Result: select * from mon$statements;
dyemanov commented 1 day ago

While this may be by design, I'd insist this is still the issue hard to explain to end users (who do not care about IStatement::PREPARE_REQUIRE_SEMICOLON).

Moreover, we have more than 80 tests in the Firebird QA failing if AUTOTERM is enabled. Of course, they could be fixed/adjusted to ignore the trailing semicolon. But I still don't really like the different MON$ output with and without AUTOTERM for the same user input.

dyemanov commented 23 hours ago

Just to be clear - I don't suggest to change the parsing rules, just to trim the trailing semicolon from the stored SQL text. The fact that semicolon is required does not (IMHO) make it a part of the user statement, it's still just a terminator.

asfernandes commented 23 hours ago

While this may be by design, I'd insist this is still the issue hard to explain to end users (who do not care about IStatement::PREPARE_REQUIRE_SEMICOLON).

Are really users interested in such small things, specially when they are using a new feature, that is exclusively from ISQL?

I'd say our users uses ISQL very few times.

And I don't consider it a bug, specially because 1) it was already parser feature 2) the other behavior is much more weird as I shown.

Moreover, we have more than 80 tests in the Firebird QA failing if AUTOTERM is enabled. Of course, they could be fixed/adjusted to ignore the trailing semicolon. But I still don't really like the different MON$ output with and without AUTOTERM for the same user input.

Existing tests do not need to be changed, nor I think they should. There should be exclusive tests for this new feature.

This feature makes parsing slow and is documented to be avoided in scripts. It's for interactive use of ISQL.

mrotteveel commented 20 hours ago

Existing tests do not need to be changed, nor I think they should. There should be exclusive tests for this new feature.

Being able to reuse tests to see if things still work as expected if you use a different way of handling input is a great thing. It's a great way to detect edge cases that you would never consider when writing tests explicitly for such a feature.