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

Regression: v4 Beta 1 rejects POSITION element of v2.5 defined SQL2003 CREATE TRIGGER syntax [CORE6243] #6487

Closed
firebird-issue-importer opened this issue Feb 3, 2020 · 24 comments

Comments

@firebird-issue-importer
Copy link

@firebird-issue-importer firebird-issue-importer commented Feb 3, 2020

Submitted by: Tony Whyman (twhyman)

In a simple test of the Firebird 4 engine, I dumped the example employee database (from a Firebird 3 source) and edited the first CREATE TRIGGER to:

CREATE TRIGGER SET_CUST_NO
ACTIVE BEFORE INSERT POSITION 0
ON CUSTOMER
AS
BEGIN
if (new.cust_no is null) then
new.cust_no = gen_id(cust_no_gen, 1);
END ^

When feeding the SQL back in to create a new database using the Firebird 4 isql, this caused the following error message:

Statement failed, SQLSTATE = 42000
Dynamic SQL Error
-SQL error code = -104
-Token unknown - line 2, column 22
-POSITION

I also fed exactly the same script back to a Firebird 3 Server/isql and the script completed with no errors.

Commits: d99fcf9

====== Test Details ======

See test for CORE5545.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 3, 2020

Commented by: Tony Whyman (twhyman)

By "dumped" I did of course mean use isql -A to extract the DDL

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 3, 2020

Commented by: @pavel-zotov

May be this is the same as i've commented in CORE5545 22/Jun/17 05:11 AM ?

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 3, 2020

Commented by: Tony Whyman (twhyman)

It does look similar.

The engine should be compliant with

https://firebirdsql.org/file/documentation/reference_manuals/fblangref25-en/html/fblangref25-ddl-trgr.html#fblangref25-ddl-trgr-create

It all works fine in Firebird 3 so a regression has slipped in somewhere.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 3, 2020

Modified by: Sean Leyne (seanleyne)

description: In a simple test of the Firebird 4 engine, I dumped the example employee database (from a Firebird 3 source) and edited the first CREATE TRIGGER to:

CREATE TRIGGER SET_CUST_NO
ACTIVE BEFORE INSERT POSITION 0
ON CUSTOMER
AS
BEGIN
if (new.cust_no is null) then
new.cust_no = gen_id(cust_no_gen, 1);
END ^

so that it was SQL 2003 compliant.

When feeding the SQL back in to create a new database using the Firebird 4 isql, this caused the following error message:

Statement failed, SQLSTATE = 42000
Dynamic SQL Error
-SQL error code = -104
-Token unknown - line 2, column 22
-POSITION

I also fed exactly the same script back to a Firebird 3 Server/isql and the script completed with no errors.

=>

In a simple test of the Firebird 4 engine, I dumped the example employee database (from a Firebird 3 source) and edited the first CREATE TRIGGER to:

CREATE TRIGGER SET_CUST_NO
ACTIVE BEFORE INSERT POSITION 0
ON CUSTOMER
AS
BEGIN
if (new.cust_no is null) then
new.cust_no = gen_id(cust_no_gen, 1);
END ^

When feeding the SQL back in to create a new database using the Firebird 4 isql, this caused the following error message:

Statement failed, SQLSTATE = 42000
Dynamic SQL Error
-SQL error code = -104
-Token unknown - line 2, column 22
-POSITION

I also fed exactly the same script back to a Firebird 3 Server/isql and the script completed with no errors.

summary: Firebird 4 Beta 1 rejects SQL 2003 compliant CREATE TRIGGER syntax => Regression: v4 Beta 1 rejects valid CREATE TRIGGER syntax

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: @asfernandes

The syntax is invalid accordingly to SQL 2013:

<trigger definition> ::=
CREATE TRIGGER <trigger name> <trigger action time> <trigger event>
ON <table name> [ REFERENCING <transition table or variable list> ]
<triggered action>
<trigger action time> ::=
BEFORE
| AFTER
| INSTEAD OF
<trigger event> ::=
INSERT
| DELETE
| UPDATE [ OF <trigger column list> ]

BEFORE INSERT ON are fixed parts of a same clause. You should not inject different clauses (POSITION) between them.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Modified by: @asfernandes

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Won't Fix [ 2 ]

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: Tony Whyman (twhyman)

It is not unreasonable that Firebird should support SQL 2013 syntax. However, I can't find anything in the release notes with respect to SQL2013 compliance nor is this incompatibility with Firebird 3 mentioned in Chapter 12 (compatibility issues). At the very least, the Release Notes need updating.

Is this the only change for SQL2013, or are there any others?

Also, is there any reason to withdraw a feature present in both Firebird 2.5 and Firebird 3 and which will cause a compatibility problem for any users that adopted the SQL2003 syntax? On initial inspection, I can't see a good reason why the parser should not be able to cope with both SQL2003 and SQL2013 variants.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: @asfernandes

Show us where the SQL 2003 syntax has the POSITION clause.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: Tony Whyman (twhyman)

Copied from the Firebird 2.5 Language Reference (https://firebirdsql.org/file/documentation/reference_manuals/fblangref25-en/html/fblangref25-ddl-trgr.html)

CREATE TRIGGER trigname {
<relation_trigger_legacy> |
<relation_trigger_sql2003> |
<database_trigger> }
AS
[<declarations>]
BEGIN
[<PSQL_statements>]
END

<relation_trigger_legacy> ::=
FOR {tablename | viewname}
[ACTIVE | INACTIVE]
{BEFORE | AFTER} <mutation_list>
[POSITION number]

<relation_trigger_sql2003> ::=
[ACTIVE | INACTIVE]
{BEFORE | AFTER} <mutation_list>
[POSITION number]
ON {tablename | viewname}

<database_trigger> ::=
[ACTIVE | INACTIVE] ON db_event [POSITION number]

<mutation_list> ::=
<mutation> [OR <mutation> [OR <mutation>]]

<mutation> ::= { INSERT | UPDATE | DELETE }

<db_event> ::= {
CONNECT |
DISCONNECT |
TRANSACTION START |
TRANSACTION COMMIT |
TRANSACTION ROLLBACK
}

<declarations> ::= {<declare_var> | <declare_cursor>};
[{<declare_var> | <declare_cursor>}; ...]

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: Sean Leyne (seanleyne)

Adriano,

Interbase/Firebird has always supported POSITION as part of Trigger definition (back to 1998), are you saying that with v4 this will no longer be supported?

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Modified by: Sean Leyne (seanleyne)

summary: Regression: v4 Beta 1 rejects valid CREATE TRIGGER syntax => Regression: v4 Beta 1 rejects POSITION element in CREATE TRIGGER syntax

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Modified by: Sean Leyne (seanleyne)

summary: Regression: v4 Beta 1 rejects POSITION element in CREATE TRIGGER syntax => Regression: v4 Beta 1 rejects POSITION element of v2.5 defined SQL2003 CREATE TRIGGER syntax

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: @asfernandes

> Interbase/Firebird has always supported POSITION as part of Trigger definition (back to 1998), are you saying that with v4 this will no longer be supported?

Of course it is not the case.

POSITION is an extension, and it should not be between the fixed standard parts BEFORE INSERT ON <table>, in the same way it's not allowed INSERT BEFORE ON <table>, INSERT ON <table> BEFORE or another bizarre variant.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: Tony Whyman (twhyman)

So how is "POSITION" supported in the SQL2013 variant?

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: @asfernandes

> So how is "POSITION" supported in the SQL2013 variant?

As the last subclause.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: Sean Leyne (seanleyne)

So, the language reference should read as follows (correct?):

CREATE TRIGGER trigname {
<relation_trigger_legacy> |
<relation_trigger_sql2003> |
<database_trigger> }
[POSITION number]
AS
[<declarations>]
BEGIN
[<PSQL_statements>]
END

<relation_trigger_legacy> ::=
FOR {tablename | viewname}
[ACTIVE | INACTIVE]
{BEFORE | AFTER} <mutation_list>

<relation_trigger_sql2003> ::=
[ACTIVE | INACTIVE]
{BEFORE | AFTER} <mutation_list>
ON {tablename | viewname}

<database_trigger> ::=
[ACTIVE | INACTIVE] ON db_event

...

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 5, 2020

Commented by: @reevespaul

It doesn't really matter if the old syntax was an extension, or legacy, or whatever we want to call it. It has been part of the trigger syntax of firebird since InterBase days.

Conformance to a level of the SQL standard is an ideal, not an obligation. I don' t think they hand out prizes for conformance, that's for sure. But I do know that breaking code like this doesn't win any friends, especially when the only justification is conformance to an arbitrary standard.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 6, 2020

Commented by: Tony Whyman (twhyman)

Running a few tests:

Test 1

CREATE TRIGGER SET_CUST_NO
ACTIVE BEFORE INSERT ON CUSTOMER POSITION 0
AS
BEGIN
if (new.cust_no is null) then
new.cust_no = gen_id(cust_no_gen, 1);
END ^

works for Firebird 4 Beta 1, but fails with Firebird 3.0.5.

Test 2

CREATE TRIGGER SET_CUST_NO
ACTIVE BEFORE INSERT POSITION 0 ON CUSTOMER
AS
BEGIN
if (new.cust_no is null) then
new.cust_no = gen_id(cust_no_gen, 1);
END ^

works for Firebird 3.0.5, but fails with Firebird 4 Beta 1

Test 3

CREATE TRIGGER SET_CUST_NO FOR CUSTOMER
ACTIVE BEFORE INSERT POSITION 0
AS
BEGIN
if (new.cust_no is null) then
new.cust_no = gen_id(cust_no_gen, 1);
END ^

works for both Firebird 3.0.5 and Firebird 4 Beta 1.

I found this issue when testing the IBX TIBExtract component with Firebird 4 Beta 1. Several iterations ago and when adding support for database triggers, I had changed this to use the "SQL2003 syntax". If nothing changes then it looks like I will have to revert back to using the legacy syntax, as that is the only one that works reliably over Firebird 2.5, 3 and 4. (isql also uses the legacy syntax when it too extracts the database schema).

I agree with Paul Reeve's comment. Your users never forgive you for breaking existing code unless there is a very good reason - and coding elegance is never one of them. By all means add support for the "correct syntax" i.e. "ON CUSTOMER POSITION 0" and encourage its use - but Firebird 4 should still accept the old "POSITION 0 ON CUSTOMER" style as well.

Whatever the outcome, please also make sure that the Release Notes reflect the changes to the CREATE TRIGGER syntax.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 6, 2020

Commented by: @asfernandes

> Conformance to a level of the SQL standard is an ideal, not an obligation. I don' t think they hand out prizes for conformance, that's for sure. But I do know that breaking code like this doesn't win any friends, especially when the only justification is conformance to an arbitrary standard.

I do not know where you read that!

I can revisit details of CORE5545 (and probably emails about the subject) because the people so interested now didn't payed attention then!

Of course it will take much more time than if the interested people payed attention 3 years ago.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 6, 2020

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Resolved [ 5 ]

QA Status: No test => Covered by another test(s)

Test Details: See test for CORE5545.

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 6, 2020

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Closed [ 6 ]

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 6, 2020

Commented by: Tony Whyman (twhyman)

> I can revisit details of CORE5545 (and probably emails about the subject) because the people so interested now didn't payed attention then!

>Of course it will take much more time than if the interested people payed attention 3 years ago.

Except that CORE5545 did include the request "please ensure this is fixed in a backwards compatible way".

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 11, 2020

Modified by: @asfernandes

status: Closed [ 6 ] => Reopened [ 4 ]

assignee: Adriano dos Santos Fernandes [ asfernandes ]

resolution: Won't Fix [ 2 ] =>

@firebird-issue-importer
Copy link
Author

@firebird-issue-importer firebird-issue-importer commented Feb 11, 2020

Modified by: @asfernandes

status: Reopened [ 4 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

Fix Version: 4.0 Beta 2 [ 10888 ]

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

Successfully merging a pull request may close this issue.

None yet
2 participants