Commented by: Roman Rokytskyy (rrokytskyy)

Mark, I have committed fix for this bug. However, there is one ambiguous situation when statements contains RETURNING clause with some columns and different columns are specified as parameter. Currently I have implemented it so, that when RETURNING clause is specified, it overrules the columns passed as parameters. Question is - should we leave it as it is or should we add some more logic?

Commented by: @mrotteveel

I assume you are talking about the methods on Statement and Connection which include parameters like int[] columnIndexes and String[] columnNames, right? I think that if RETURNING is specified in the actual query passed to these methods, then we should assume the developer using it knows what he is doing and we shouldn't second guess him. Of course in most cases the developer shouldn't be specifying RETURNING for these methods as it would be superfluous to the use of these methods; the alternative would be to never allow the caller to include RETURNING in the generated keys related methods, but that might be too inflexible.

BTW: Did you also look into the problems that we are including too many columns in the RETURNING clause (I think because it is using getColumns() instead of getPrimaryKeys() of the metadata), and the wrong error returned when the table does not exist?

Commented by: Roman Rokytskyy (rrokytskyy)

I am not sure that we should return only the PK values. Firebird does not have any syntax to mark column as auto-generated. The generation happens in the BEFORE INSERT triggers and can affect any column. So returning all columns for the Connection.prepareStatement(String, int) or Statement.execute(String, int) seems to be ok. It does not affect other methods, since the column names are passed explicitly into the methods.

As to the unknown table name - I will check this. Most likely the issue is caused by the DatabaseMetaData.getColumns(...) method.

Commented by: Roman Rokytskyy (rrokytskyy)

Ok, I also checked the unknown table name - now it is handled "automagically", since it does not get any RETURNING columns and executes the code as is, then the correct error is returned. Please check my fixes, and close the ticket, if everything is fine.

Commented by: @mrotteveel

I think that the primary purpose is to retrieve generated primary keys. Given the limitations of Firebird, I think it would be simpler to have a limitation that we only return the primary key by default, and not return everything. If an application developer requires more fields, he could always use a specific RETURNING clause, or use one of the methods taking int[] columnIndexes or String[] columnName as a parameter.

On the other hand, looking through the source of PostgreSQL JDBC I noticed that they simply add 'RETURNING *' at the end of the query (something which Firebird does not support) for methods with the signature String sql, int autoGeneratedKeys. I have also seen in the documentation that MS SQL Server JDBC will only return values from a single IDENTITY column. I will see what some of the other database do (eg Oracle, MySQL). As I already have seen two different ways of handling it, I think we can keep on returning everything.

Of course we *could* make it even more complex and only return those fields which are not specified in the INSERT, and maybe include those fields which have a different value than specified in the INSERT, but I think that is taking it a bit too far ;)

I will test your current version in the coming week.

Commented by: @mrotteveel

I will make some changes to reduce the code duplication between Statement and Connection.

I also noticed two bugs in the int[] columnIndexes methods. It is using a 0-based index, while columns in JDBC are 1-based, and the counter to keep track of them is never increased. I will change it to use the ORDINAL_POSITION as returned by DatabaseMetaData.getColumns(...).

Commented by: @mrotteveel

I refactored functionality common to both Connection and Statement to separate class, I will commit the changes after I have added a number of unit tests.

Commented by: @mrotteveel

Cases including RETURNING still get an additional RETURNING clause: The parser doesn't correctly attribute columns from the RETURNING clause, as ANTLR will only assign true to _inReturning after retrieving the column list (which depends on the _inReturning value to assign to the normal columnlist or the return column list.

For a simple testcase with a query INSERT INTO GENERATED_KEYS_TABLE(NAME, TEXTVALUE) VALUES (?, ?) RETURNING id
the generated JaybirdStatementModel has an empty returningColumns List, and the columns List contains NAME, TEXTVALUE and id(!). As far as I can see this happens as the parser first consumes the columnList before reaching the assignment _inReturning = true which makes the parser add columns to the returningColumns

I am not 100% (as I have never done anything with ANTLR), but it looks like the following piece of JaybirdSql.g3:

returningClause
: RETURNING columnList
{
_inReturning = true;
}
;

should be changed to:

returningClause
: RETURNING {_inReturning = true;} columnList {_inReturning = false;}
;

Could you take a look at this? I am not familiar enough with ANTLR and it would come down to trial and error.

I have committed my changes so far including the refactoring to move the generated keys code to a single place (AbstractGeneratedKeysQuery); two of the tests in TestGeneratedKeysQuery are currently failing because of this problem.

Commented by: @mrotteveel

* Reversed decision to return false for execute() (to indicate no ResultSet if only generated keys are available) as getResultSet will return a ResultSet (the one containing the keys) and to avoid potential backwards compatibility issues for statements using INSERT ... RETURNING in 2.1.6
* Added more tests for AbstractGeneratedKeysQuery
* Added (integration) tests for generated keys from Statement (FBStatement)
* Added (integration) tests for generated keys from PreparedStatement (FBPreparedStatement)
NOTE: some tests are currently failing due to outstanding parser-issue

Commented by: @mrotteveel

Another thing that popped into my head:
Are we going to act differently when connected to a 1.5 DB (which does not support RETURNING), for 2.0 (which only supports RETURNING for INSERT ... VALUES and for 2.1 or higher (which also supports RETURNING for UPDATE, DELETE and UPDATE OR INSERT and for INSERT ... SELECT)?

Commented by: Roman Rokytskyy (rrokytskyy)

I have fixed the grammar, now all tests pass OK - your suggestion was right. Also I have upgraded to latest ANTLR version (3.4), that required more work compared to the real fix :)

As to supporting of RETURNING clause in UPDATE - that requires more work on the grammar. We do not want to implement the complete UPDATE statement syntax of Firebird, since that would require a lot of work, but we have to create grammar that correctly represents the relevant parts of the syntax, so that we can detect the table name and whether RETURNING clause is specified or not.

I see no point in getGeneratedKeys() for DELETE statement (does Firebird support it?!)

The UPDATE OR INSERT should be easy to implement, I will look into it (however the MERGE syntax would require more coding).

The INSERT INTO ... SELECT might work right now, but I will create a test case for it.

And yes, we should think about throwing an error when generated keys are asked for the Firebird 1.5 or lower... On the other hand, we will get an error anyway.

Commented by: Roman Rokytskyy (rrokytskyy)

Ok, INSERT INTO ... SELECT won't work when RETURNING part is specified there. Reason is that we stop parsing when we detect SELECT keyword. In order to be able to detect RETURNING keyword we have to implement almost full parsing of the SELECT statement, which is not that trivial task.

So, considering the effort it requires and my estimation that this is very rare use case, I think we can wait until somebody really asks for it support.

Same with UPDATE - we have to implement the WHERE clause from the SELECT statement. Not sure whether the efforts are worth it.

Commented by: @mrotteveel

I have re-run my tests last week. I also fixed two that did not take into account my change to just return true for execute(). I will see if I can come up with additional testcases when I write the releasenotes.

And yes: Firebird supports RETURNING for all DML update statements except MERGE:
eg DELETE: http://www.firebirdsql.org/file/documentation/reference_manuals/reference_material/html/langrefupd25-delete.html#langrefupd25-delete-returning

But I agree its uses may be a bit esoteric, so lets wait until people request it.

I was wondering about one thing: There is a JaybirdSql.g and JaybirdSql.g3 grammar, they are almost identical, but there are some differences. Your last changes were in JaybirdSql.g, so can JaybirdSql.g3 be removed?

Commented by: @mrotteveel

I will look if I can throw FBDriverNotCapableException (or maybe a new one, eg FBServerNotCapableException ;)) for non-support onder Firebird 1.5.

Commented by: @mrotteveel

Added version check to throw FBDriverNotCapableException when using Firebird version lower than 2.0.

Roman, could you look at my question about the two grammar files JaybirdSql.g and JaybirdSql.g3?

Commented by: @mrotteveel

Removed duplicate grammar file JaybirdSql.g3