-
Notifications
You must be signed in to change notification settings - Fork 178
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach originally comes from SQLite, but extracted out to here to be reusable.
8359011 to
a524293
Compare
brandur
commented
Aug 11, 2026
@bgentry Thoughts on this?
I'm kind of thinking that we wouldn't be able to officially support Yugabyte since I really don't want to be testing against it, but we could have soft support that works as long as they commit to their stated PG 15 contract with a few very minor deviations (like the xmax thing which admittedly is a little bit abusive of Postgres anyway).
jqueuniet
commented
Aug 12, 2026
I tested this branch in our test environment and our service managed to both enqueue asynchronous tasks and run scheduled ones without any other error.
brandur
commented
Aug 12, 2026
@jqueuniet Excellent! Thanks for checking. Next we'll see if it can stand the stress of a DB-based job queue ...
79e78e8 to
e166e64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 via Codex: This changes the startup health check into a one-time capability initialization. Once uniqueInsertMode has been cached, normally by initialPing, Ping returns successfully without performing any database I/O.
A later Start or restart can therefore succeed while PostgreSQL is unavailable, which is the exact case this block is intended to reject, especially for poll-only and database/sql clients.
Could we keep a real SELECT 1 or pool ping on every Start and initialize the insert mode separately, or make Ping always reach the database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 via Codex: Holding this non-context-aware mutex across PGGetProductAndVersion can make inserts wait for the full 10-second initialPing timeout even after their own context is canceled.
There is also a MaxConns=1 cycle: an existing transaction holds the sole connection, initialPing takes this mutex and waits for the pool, and InsertTx uses the existing transaction but blocks on this mutex. Progress resumes only when initialPing times out. The new test pauses before the real Ping takes this lock, so it does not cover the cycle.
Could we avoid background initialization and resolve lazily, or permit redundant detection queries or context-aware waiting instead of holding a mutex across database I/O? The database/sql implementation has the same issue.
e166e64 to
a098ddf
Compare
This one's aimed at #1346, in which it might be possible for us to support Yugabyte as a database target without a hugely inordinate amount of work. Yugabyte is currently targeting compatibility against Postgres 15 [1]. It doesn't support `xmax` which is what #1346 is about, but somewhat surprisingly, we only use `xmax` in one place and don't use any other Postgres 16+ features (as Postgres 15 is still a valid target in the CI matrix). The `xmax` trick to determine whether an upserted row is new or existing is a little outdated anyway because Postgres 18 added the capability to detect an existing row with `OLD.id IS NOT NULL` [2]. Long run, we should switch to that for everything. Shorter term, Postgres 18 is still quite new, so I propose we do something like this: * If on Postgres 18+ (we should be getting Postgres 19 soon), use `OLD.id IS NOT NULL`. * If on Yugabyte, fall back to the same trick we use in SQLite by upserting rows with a unique nonce and checking whether the nonce was the one we inserted or not. * Otherwise, use the existing approach with `xmax`. We do have to check which database we're on, but only once, after which we can cache that information forever, so it shouldn't have any impact on performance. Fixes #1346. [1] https://docs.yugabyte.com/stable/faq/compatibility/#what-is-the-extent-of-compatibility-with-postgresql [2] https://www.crunchydata.com/blog/postgres-18-old-and-new-in-the-returning-clause
a098ddf to
2b9a430
Compare
This one's aimed at #1346, in which it might be possible for us to
support Yugabyte as a database target without a hugely inordinate amount
of work.
Yugabyte is currently targeting compatibility against Postgres 15 [1].
It doesn't support
xmaxwhich is what #1346 is about, but somewhatsurprisingly, we only use
xmaxin one place and don't use any otherPostgres 16+ features (as Postgres 15 is still a valid target in the
CI matrix).
The
xmaxtrick to determine whether an upserted row is new or existingis a little outdated anyway because Postgres 18 added the capability to
detect an existing row with
OLD.id IS NOT NULL[2].Long run, we should switch to that for everything. Shorter term,
Postgres 18 is still quite new, so I propose we do something like this:
If on Postgres 18+ (we should be getting Postgres 19 soon), use
OLD.id IS NOT NULL.If on Yugabyte, fall back to the same trick we use in SQLite by
upserting rows with a unique nonce and checking whether the nonce was
the one we inserted or not.
Otherwise, use the existing approach with
xmax.We do have to check which database we're on, but only once, after which
we can cache that information forever, so it shouldn't have any impact
on performance.
Fixes #1346.
[1] https://docs.yugabyte.com/stable/faq/compatibility/#what-is-the-extent-of-compatibility-with-postgresql
[2] https://www.crunchydata.com/blog/postgres-18-old-and-new-in-the-returning-clause