Support Yugabyte as a Postgres target - #1347
Conversation
|
|
||
| // UniqueInsertMetadataWithNonce returns metadata with nonce set under | ||
| // UniqueInsertMetadataKey. | ||
| func UniqueInsertMetadataWithNonce(metadata []byte, nonce string) ([]byte, error) { |
There was a problem hiding this comment.
This approach originally comes from SQLite, but extracted out to here to be reusable.
8359011 to
a524293
Compare
|
@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 |
|
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. |
|
@jqueuniet Excellent! Thanks for checking. Next we'll see if it can stand the stress of a DB-based job queue ... |
6ce2728 to
79e78e8
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
79e78e8 to
e166e64
Compare
| // non-functional. Here we try to make an initial assessment of health and | ||
| // return quickly in case of an apparent problem. | ||
| if err := c.driver.GetExecutor().Exec(fetchCtx, "SELECT 1"); err != nil { | ||
| if err := c.driver.GetExecutor().Ping(fetchCtx); err != nil { |
There was a problem hiding this comment.
🤖 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?
| return mode, nil | ||
| } | ||
|
|
||
| e.driver.uniqueInsertModeInitMu.Lock() |
There was a problem hiding this comment.
🤖 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.
3 participants
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