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 ... |
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?
There was a problem hiding this comment.
So both comments here were on code that I'd brought in near the last second — previously, I'd had the drivers lazily check whether a database is Yugabyte by doing it on first insert, and the idea here was to do an aggressive database check on client start. As uncovered by these issues though, doing so leads to added complexity and a number of possible races, so I think the whole scheme was more trouble than it was worth, and have now removed it.
| 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.
a098ddf to
2b9a430
Compare
2b9a430 to
e32e5dc
Compare
|
@bgentry Pushed a new version of this. Mind taking a look? |
|
The reporter is using Yugabyte 2025.2.1, where Could we make Yugabyte use polling when notification support is unavailable, and add coverage for queue-control changes? If support requires a Yugabyte version and flags that enable |
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Leaving just a single bot-written finding that I didn't have time to fully review before heading to dinner, but otherwise LGTM and ship it when ready ✌️
853ecdc to
afd40a7
Compare
|
Yugabyte supports https://docs.yugabyte.com/stable/api/ysql/the-sql-language/statements/cmd_listen_notify/ |
|
Nice. Okay, I modified this to also check for listen/notify and fall back to poll only where not available.
Yep, good catch. My implementation here will check for the listen/notify setting specifically: coalesce(current_setting('yb_enable_listen_notify', true), 'off')::boolean |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afd40a7d84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ID: params.ID, | ||
| CancelAttemptedAt: cancelledAt, | ||
| ControlTopic: params.ControlTopic, | ||
| Notify: capabilities.SupportsListenNotify, |
There was a problem hiding this comment.
When YugabyteDB reports LISTEN/NOTIFY disabled, this sets Notify to false, so cancelling a running job only records cancel_attempted_at while deliberately leaving the job in running. For a job executing in another process, the only prompt path to producer.maybeCancelJob is the control notification, while poll mode only polls queue settings; consequently JobCancel returns success but the worker context is never cancelled and may continue indefinitely. Add a cancellation polling or equivalent cross-process fallback, including coverage in the shared driver behavior suite.
AGENTS.md reference: AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
afd40a7 to
7c0c1ed
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c0c1ed27e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !capabilities.SupportsListenNotify { | ||
| return nil |
There was a problem hiding this comment.
When Yugabyte reports LISTEN/NOTIFY unavailable, both PostgreSQL executors return success here without delivering leadership messages. After QueueMaintainerLeader.tryStart exhausts its three startup attempts, it relies exclusively on ClientNotifyBundle.RequestResign; that call funnels through this no-op, so the elector never receives pendingRequestResign, continues renewing its term, and maintenance such as scheduling, rescue, periodic enqueueing, and cleanup can remain stopped indefinitely instead of another client taking over. Add a pollable or direct local resignation path for this recovery flow, with shared driver coverage.
AGENTS.md reference: AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
7c0c1ed to
f0247c2
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
f0247c2 to
43f2ec6
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
43f2ec6 to
b182275
Compare
|
Thx! Adding the listen/notify stuff definitely led Codex to find some knock on bugs, but they should be all fixed up now. |
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