Add Firebird support - Fix #135

[fdcastel]

Work in progress. Do not merge.

• Initial work done by @staticlibs in 85997d1
• Add firebird-windows-amd64 job in GitHub Actions Workflow
  • Windows is needed because Firebird ODBC on Linux is broken.
• Add comprehensive tests for Firebird data types
  • Firebird data types coverage status.
• Add odbc_copy tests for Firebird

[fdcastel]

@staticlibs, could you please review this when you have a chance?

Regarding the failing SQL test:

Run ./configure/venv/Scripts/python.exe ./resources/scripts/run_sqllogic_tests.py --dbms Firebird
Running SQLLogic tests from directory: 'D:\a\odbc-scanner\odbc-scanner\test\sql\firebird'
ODBC_CONN_STRING: 'Driver={Firebird ODBC Driver};Database=localhost:C:/tmp/test.fdb;UID=SYSDBA;***;CHARSET=UTF8;'
[1/6] test\sql\close.test
[2/6] test\sql\connect.test
[3/6] test\sql\firebird\11_integer.test
D:\a\odbc-scanner\odbc-scanner\test\sql\firebird\11_integer.test:28 Query failed, but did not produce the right error: Invalid Descriptor Index
Instead it produced: Binder Error: 'SQLPrepare' failed, query: 'SELECT CAST('foo' AS NVARCHAR(max)), CAST(42 AS INTEGER) FROM rdb$database', return: -1, diagnostics: 'HY000(-104): [ODBC Firebird Driver][Firebird]Dynamic SQL Error
-SQL error code = -104
-Token unknown - line 1, column 30
-('

LINE 1: SELECT * FROM odbc_query(getvariable('conn'), 'SELECT CAST(''foo'' AS...
                      ^
ERROR

I’m not sure how to move forward with this one.

I tried updating the expected error to "Binder Error", but it still fails with the same error message.

Firebird doesn’t have NVARCHAR(max), but from what I understand, the intent of the test is just to provoke a statement error.

[staticlibs]

This test was added (to other DBs) to cover the column binding support. NVARCHAR(max) was uncorrectly copied from MSSQL. Also "Invalid descriptor index" message is specific to MSSQL. I would think this test can just be removed from Firebird.

[staticlibs]

The PR looks good to me, special thanks for covering C API tests! Once the SQL tests are passing - I can merge it promptly.

I think it make sense to chain it after the DuckDB Windows job and enable CCache caching for it. But these are minor things that I can add in follow up PRs.

[fdcastel]

I think it make sense to chain it after the DuckDB Windows job and enable CCache caching for it.

This would be great!

I have to admit I tried poking around with CCache, but it went hilariously wrong. 😄
(Full disclosure: I have zero experience with it 😅)

[fdcastel]

Another question: should the SQL tests be wrapped with

require-env FIREBIRD_AVAILABLE

?

If yes, where should this be defined?

I see a similar pattern with POSTGRES_AVAILABLE and MYSQL_AVAILABLE, but I’m not sure whether these are actually used. I can’t find anywhere in the codebase where those variables are set.

[staticlibs]

No, FIREBIRD_AVAILABLE is not needed, it is the remaining from when the SQL test suite was shared between DBs. The runner script only runs test from a single DB directory now.

[fdcastel]

What should this query return?

SELECT * FROM odbc_query(getvariable('conn'), 
  'SELECT CAST(? AS INT128) FROM rdb$database',
  params=row(9223372036854775807::BIGINT))

I'm getting

SQL Error: Invalid Input Error: 'SQLExecute' failed, query: 'SELECT CAST(? AS INT128) FROM rdb$database', return: -1, diagnostics: 'HY000(-303): [ODBC Firebird Driver][Firebird]Dynamic SQL Error
-SQL error code = -303
-conversion error from string "9#x002#x002#x003#x003#x007#x002#x000#x003#x006#x008#x005#x004#x007#x007#x005#x008#x000#x007#x00"'

Is this a bug, or one more Firebird ODBC oddity?

[fdcastel]

P.S.: I’m inclined to leave INT128 support aside for now...

Those “char” conversions don’t smell right, and even the Firebird docs say this type isn’t defined in the SQL standard.

Opinions?

[staticlibs]

This error comes from the driver, perhaps the VARCHAR parameter may work here, I think leaving out INT128 is fine.

[fdcastel]

Rebased with latest main.

[fdcastel]

Added initial tests for odbc_copy.

I had to make it very simple for now. Firebird does not support multiple VALUES in INSERT statements.

The create_table=TRUE option doesn’t seem to work with Firebird. I suspect this is because Firebird requires DDL to be committed and a new transaction to be started before DML can see the newly created objects. If you want to dig into this, try running some odbc_copy tests from the “Db2” suite.

[fdcastel]

I’m taking this PR out of draft.

If you plan to merge it, please make sure to skip this commit:

• [WIP] Disables all other tests except Firebird.

Let me know if you’d like any history cleanup or additional changes.

[staticlibs]

Thanks!

Let me know if you’d like any history cleanup or additional changes.

I think I will just merge it as is and add a few CI changes on top of it.

I suspect this is because Firebird requires DDL to be committed and a new transaction to be started before DML can see the newly created objects.

This should be straigthforward to fix, I guess some kind of option to issue COMMIT after the DDL is run may help here - need to get Windows env setup first to try this.

[staticlibs]

@fdcastel

Just FYI, I filed #140 that fixes CTAS for Firebird and also adds support for generating INSERT UNION ALL queries to make odbc_copy working in batch mode. I updated the firebird_copy.test, but did not run any benchmarks. With batch_size=1 it will generate simple INSERTs. Going to publish it for nightly in about an hour.

[staticlibs]

Going to publish it for nightly in about an hour.

Passed CI, going to be published as 96743f7.

[staticlibs]

One issue found, column types mapping not initialized when create_table=FALSE, so this case completely broken with Firebird. Will fix shortly.

[staticlibs]

Filed #142 to fix create_table=FALSE.

Another problem found is that the "tail" insert query is generated incorrectly for Firebird (parameters count mismatch) - will investigate that in a couple of hours.

[staticlibs]

Another problem found is that the "tail" insert query is generated incorrectly for Firebird

Not specific to Firebird, tracked in #143, will comment there.

[staticlibs]

@fdcastel

Just FYI, GH actions are not running right now, and performance still needs to be checked. So going to postpone submitting the final version for 1.5 to tomorrow evening.

[fdcastel]

For the record: initial testing tells me that the transaction changes made into odbc_copy don’t introduce any noticeable overhead when using create_table = FALSE and a parameterized dest_query (my original use case).

That said, I’d like to see the end of #141 (and make some tests with create_table = TRUE) to state anything else.

But I expect things to improve overall (except if I'm missing something).

[staticlibs]

GH actions are actually recovered, and 7ea8b08 with recent fixes going to be ready in a few minutes. Still I was observing unreasonably slow odbc_copy with Oracle on some input files - going to look deeper tomorrow (UTC+0). And can merge additional Firebird fixes tomorrow, if necessary.

[staticlibs]

@fdcastel

Just FYI, in case you be testing the performance:

Still I was observing unreasonably slow odbc_copy with Oracle on some input files

There is a bug in current version (added more than a week ago) that auto-commit is enabled back for all source batches after the first one. It tanks the performance badly, so current version is unsuitable for performance checks. I will file a fix shortly.

[fdcastel]

Yep! I did notice something strange yesterday. But since you’d already flagged something odd with Oracle, I decided to wait for your analysis before moving forward with the tests.

Thanks for keeping me in the loop 😉