Issue Details (XML | Word | Printable)

Key: CORE-6243
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Major Major
Assignee: Adriano dos Santos Fernandes
Reporter: Tony Whyman
Votes: 0
Watchers: 4
Operations

If you were logged in you would be able to see more operations.
Firebird Core

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

Created: 03/Feb/20 10:55 AM   Updated: 11/Feb/20 04:13 PM
Component/s: Engine
Affects Version/s: 4.0 Beta 1
Fix Version/s: 4.0 Beta 2

Environment:
Linux Mint 19.3 64-bit
Firebird binary installed from firebirdsql.org

QA Status: Covered by another test(s)
Test Details: See test for CORE-5545.


 Description  « Hide
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.

 All   Comments   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Tony Whyman added a comment - 03/Feb/20 11:00 AM
By "dumped" I did of course mean use isql -A to extract the DDL

Pavel Zotov added a comment - 03/Feb/20 01:39 PM
May be this is the same as i've commented in http://tracker.firebirdsql.org/browse/CORE-5545 22/Jun/17 05:11 AM ?

Tony Whyman added a comment - 03/Feb/20 02:13 PM
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.

Adriano dos Santos Fernandes added a comment - 05/Feb/20 03:17 PM
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.

Tony Whyman added a comment - 05/Feb/20 03:32 PM
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.

Adriano dos Santos Fernandes added a comment - 05/Feb/20 03:42 PM
Show us where the SQL 2003 syntax has the POSITION clause.

Tony Whyman added a comment - 05/Feb/20 03:50 PM
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>}; ...]

Sean Leyne added a comment - 05/Feb/20 03:54 PM
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?

Adriano dos Santos Fernandes added a comment - 05/Feb/20 04:08 PM - edited
> 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.

Tony Whyman added a comment - 05/Feb/20 04:17 PM
So how is "POSITION" supported in the SQL2013 variant?

Adriano dos Santos Fernandes added a comment - 05/Feb/20 04:31 PM
> So how is "POSITION" supported in the SQL2013 variant?

As the last subclause.

Sean Leyne added a comment - 05/Feb/20 04:36 PM
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

...

Paul Reeves added a comment - 05/Feb/20 08:20 PM
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.

Tony Whyman added a comment - 05/Feb/20 11:19 PM
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.

Adriano dos Santos Fernandes added a comment - 06/Feb/20 01:53 AM
> 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 CORE-5545 (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.

Tony Whyman added a comment - 06/Feb/20 09:07 AM
> I can revisit details of CORE-5545 (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 CORE-5545 did include the request "please ensure this is fixed in a backwards compatible way".