Repository navigation
Conversation
944343d to
d867319
Compare
levkk
left a comment
There was a problem hiding this comment.
The advisory lock query will return an error if the lock failed to get acquired. It's much easier for us (and we do this already) to inspect the error we receive and remove the lock from our optimistic data structure.
d867319 to
5e544cb
Compare
Backport pgdogdev#1579 onto v0.1.56 without the 69 unrelated commits currently on upstream main.
|
Thanks. I may be misunderstanding the suggested path, but PostgreSQL does not return an error when pg_try_advisory_lock* cannot acquire a contended lock - it returns a normal boolean false result. Inspecting errors would cover actual execution failures, but not lock contention. Would you prefer that PgDog inspect the returned false value instead of reconciling through pg_locks? I used reconciliation because it remains correct for binary results, multiple result columns, nested lock calls, and reentrant locks, but I’m happy to align with the preferred result-inspection design. |
|
Nope, it's me who completely misunderstood 🤣 Sorry, getting late in the week (albeit it's only Thursday..). Yes, I think you're right, we need to inspect the result (we support parsing Running an extra query is too expensive, inspecting the result is cheap. Do make sure to do that if and only if we are running an advisory lock query (maybe flag it in the context) so we don't have to inspect all results we receive. |
5e544cb to
b23ec6d
Compare
|
Thanks — updated in ec10acb. PgDog now records the directly returned The |
b23ec6d to
361497e
Compare
361497e to
ec10acb
Compare
fix: avoid pinning clients on failed advisory try locks (pgdogdev#1579 on v0.1.60)
ec10acb to
3c1d121
Compare
pg_try_advisory_lock returns false instead of an error when the lock is held by someone else. Check the result of SELECT pg_try_advisory_lock(...) before tracking the lock, so polling clients don't get pinned. Closes pgdogdev#1578
3c1d121 to
2e01c23
Compare
Summary
pg_try_advisory_lock*result rows before merging optimistic lock bookkeepingfalsePgDog previously merged advisory-lock intent from the parsed statement at
ReadyForQuery, regardless of whether a try-lock succeeded. The parser now records the result columns for directly returned session try-lock calls, and the query engine inspects only thoseDataRowfields. It accounts for text and binary results, multiple result columns, and repeated rows without adding another backend query. Missing or undecodable results remain conservatively pinned.Fixes #1578.
Validation
cargo test -p pgdog frontend::client::query_engine::test::advisory_lock -- --test-threads=1(15 passed)cargo test -p integration_tests_rust --test mod integration::advisory_lock::test_failed_try_advisory_lock_does_not_pin_client --no-runcargo clippy -p integration_tests_rust --test mod --no-deps -- -D warningscargo check -p pgdogcargo build -p pgdoggit diff --checkNotes