I've added the new xtdb-jdbc module to next.jdbc's testing setup and I'm updating the tests to work with a required _id column (instead of an auto-generated id column), and I'm slowly making progress:
Ran 360 tests containing 3651 assertions.
124 failures, 9 errors.
which is a lot better than where I started a couple of hours ago...This is what it has takenThanks again for your efforts and for sharing that. I guess an update-able CREATE VIEW with an alias might have helped? π€
Not sure what that means or what it would add?
The main changes in the test suite are:
β’ _id is the primary key in all tables rather than (by convention) id or some other column
β’ insert ... returning * doesn't return anything useful -- all other databases can return the newly-inserted row, or at least the newly-generated primary key
β’ since .getTableName() doesn't return the table name associated with a column in a query result set, the "traditional" qualified column names -- :table/column -- are not possible like they are with other databases
For the latter, Oracle also behaves like XTDB in that respect -- but I don't test against it -- and MS SQL Server requires specific parameters set on the PreparedStatement for it to be able to return table names (which I provide in the tests that matter). PostgreSQL provides those table names, but calling .getTableName() causes the driver to issue a metadata query behind the scenes to fetch that data.
For the most part, that third bullet was already handled in most of the test suite, via the column function which also handled the natural upper/lower case variations across databases, but the first two caused extensive changes.
ah sorry, I meant something like:
CREATE VIEW fruit AS SELECT _id AS id, * FROM actual_fruit;
then you could stick with your convention-based approach:
select * from fruit order by id
Ah, interesting. I haven't worked with views in databases for a very long time...
> .getTableName() doesn't return the table name associated with a column in a query result set
thanks, this sounds like something we might want to add - not sure I've seen that mentioned before
I've mentioned it several times π
(in the context of qualified column names and the general HoneySQL/`next.jdbc` approach to :table/column and not having / in column names π )
ahh, apologies, my JDBC experience is very basic and my eyes glaze over π
Heh... like me and Datalog, I suspect...
I saw your comment about / round-tripping in PG, BTW. It really isn't something I want to support in HoneySQL or next.jdbc (and no PG users have ever asked for it) -- and in fact I just ripped out all the experimental support for it this weekend, after confirming that XTDB doesn't allow / in general... and I was pleased to see none of the examples in the docs seemed to use this any more...
completely understand yep, I just want to make sure all perspectives are considered - e.g. the (potential, hypothetical) need for 'expanded' URI keys in JSON-LD handling
just opened https://github.com/xtdb/xtdb/issues/3906 π (thanks for your persistence mentioning it!)
When I get my energy back after all that XTDB-related work over the weekend on HoneySQL and next.jdbc, I'll go through the changes in the test suite and see if there are any other "obvious" issues I should create for XTDB... I did create one for the "unexpected error on tx submit" I encountered.
thanks @seancorfield - much appreciated, as always π
Down to this now, so I'll pick it up again tomorrow:
Ran 360 tests containing 3647 assertions.
78 failures, 1 errors.great news, thanks @seancorfield βΊοΈ
12 failures, 1 errors.Success!
Ran 360 tests containing 3637 assertions.
0 failures, 0 errors.This is what it has taken to get to no failures: https://github.com/seancorfield/next-jdbc/compare/develop...xtdb-testing?w=1
Q: Is .setMaxRows implemented on Statement for XTDB? I'm setting this to 2 but getting back more than two rows so I'm guessing it isn't?
> I do recall seeing a TODO for it in the codebase https://github.com/xtdb/xtdb/blob/52ec4d4a6a6790de43814577f370fdce028ad1f0/core/src/main/clojure/xtdb/pgwire.clj#L1540?
not yet, no, although I do recall seeing a TODO for it in the codebase π
Should I create a GH issue for it, for tracking purposes? Once I'm done with this next.jdbc testing pass, I may have several "issues" to raise π
As I'm working on getting all the next.jdbc tests passing on XTDB (or at least disabling tests that cannot work... yet), I'm finding that if I run my failing test suite multiple times, after a few runs I get this error:
Execution error (PSQLException) at org.postgresql.core.v3.QueryExecutorImpl/receiveErrorResponse (QueryExecutorImpl.java:2733).
ERROR: unexpected error on tx submit (report as a bug)
(stacktrace in π§΅ )I have a PR open against next.jdbc to enable automated XTDB testing going forward -- https://github.com/seancorfield/next-jdbc/pull/290 (so you can also see all the changes required to get the test to pass with XTDB -- mostly that _id is a fixed primary key (instead of the convention of id -- not all DBs allow columns to begin with _!) and that insert doesn't (can't) return the inserted PK.
I won't merge it until #3894 is addressed, but at least I can easily test any future changes to next.jdbc against XTDB automatically!
It happens in different places on different runs, but once XTDB gets into this state, the only solution is to shutdown the Docker instance and restart it (and sometimes I need to rm the container and recreate it).
This branch has the current state of play, if you want to try for yourselves: https://github.com/seancorfield/next-jdbc/tree/xtdb-testing
Clone the repo, checkout that branch, run docker compose up, and then run NEXT_JDBC_TEST_XTDB=yes clojure -X:test a few times.
And here's the log output from the XTDB Docker instance when it fails like that:
Note: even having reached 0 failures, 0 errors, if you run the test command several times, you'll still hit this "unexpected error"...
thanks @seancorfield - yep, we shouldn't be trying to do that. if you get there before me, could you raise an issue for this one?
I used Slack to create an issue from this Slack thread. Not sure how clear/useful that will be but I think it links back to this thread...