Move the advisory lock classes out of the user-facing keyspace (#430)

[ChronicallyJD]

Closes #430. @jdatcmd for review.

The bug

locktag_field4 says which advisory lock space a tag belongs to, and PostgreSQL's own
SQL-callable functions already own two values (lockfuncs.c:610-620):

 *	field4: 1 if using an int8 key, 2 if using 2 int4 keys

We used both. The unique-key lock was

SET_LOCKTAG_ADVISORY(tag, MyDatabaseId, indexOid, bucket, 2)

which is bit for bit what pg_advisory_lock(indexOid, bucket) takes. Not a similar
lock, the same lock. An application holding that tag blocked columnar inserts of that
key, and columnar blocked the application, silently, with nothing to point at but
unexplained waiting.

Three sites, and the comment was wrong about our own picture too

site	class	equivalent user call
columnar_metadata.c:1242 delete-vector chunk	1 (bare literal)	pg_advisory_lock(bigint)
columnar_metadata.c:1600 storage-row creation	2 (bare literal)	pg_advisory_lock(int4,int4)
columnar_unique.c:328 unique key	2 (named constant)	pg_advisory_lock(int4,int4)

The issue said two sites. There are three. And columnar_unique.c:62 claimed:

The issue #4 delete_vector lock uses 1; the unique-key lock uses 2 so the two lock
spaces never false-share.

It did not know about the storage-row lock, which also used 2. They do not collide in
practice, because storageId >> 32 is 0 for any realistic storage id and an index OID is
never 0. The problem is that the comment invites a fourth use added on the assumption that
the classes partition.

The fix

An application can only ever set field4 to 1 or 2, so any value above 2 is unreachable
from SQL. All three get their own class, defined once in columnar.h with the reasoning,
at 101, 102 and 103.

Stated in the header and worth a reviewer's attention: these values are part of the lock
protocol between backends, so two backends on different builds would not exclude each
other. That is a restart rather than a rolling upgrade, which this extension already
requires because it loads through shared_preload_libraries.

The suite, and what it deliberately does not do

test/advisory_lock_class.sh discovers the tag an insert actually takes by reading
pg_locks, then tries to grab that exact tag through pg_advisory_xact_lock.

It does not recompute the bucket hash. Doing that would assert that two copies of our own
arithmetic agree, which is not the property under test.

Removal proof

Same suite file against both builds, only the source differing:

--- WITHOUT the source fix (main) ---
FAIL  the lock an insert takes is not in a SQL-reachable class: got [reachable (field4=2)]
FAIL  a user advisory lock on that tag does not block a columnar insert: got [BLOCKED by the user lock]
FAIL  a duplicate key is still rejected: got [NOT rejected: ]
checks run: 7                                              advisory_lock_class.sh: FAILED

--- WITH the source fix ---
PASS  the lock an insert takes is not in a SQL-reachable class
PASS  a user advisory lock on that tag does not block a columnar insert
PASS  a duplicate key is still rejected
checks run: 7                                              advisory_lock_class.sh: PASSED

The third failure in the first arm is a consequence of the second: the insert timed out, so
there was no row for the duplicate check to collide with. Left as it fell rather than
tidied.

The suite also asserts a genuine duplicate is still rejected, because removing the
collision by removing the lock would satisfy everything else and silently give back
issue #5.

Two false starts, since they are the interesting part of the review

The first version of this suite passed identically with and without the fix, which is
the exact defect I have spent this week filing against other people. Two causes, both mine:

1. It held "every bucket" to avoid computing the hash. lib.sh:142 sets
   pgcolumnar.unique_lock_buckets=100003, and max_locks_per_transaction is 64, so the
   holder silently failed and the check passed with nobody holding anything.
2. It killed the holder with kill on the client. The backend was inside pg_sleep() and
   kept its transaction and its locks, so every later check blocked on our lock rather
   than the user's, in both arms.

The premise checks are what surfaced both. premise: the other session really holds all 100003 buckets: got [no] is the line that stopped me shipping a green test that measured
nothing. The suite now terminates the backend server-side with pg_terminate_backend and
asserts it is gone before continuing.

Gate

ALL VERSIONS PASSED       versions run: 5 of 5 configured
  PASS PG15   PASS PG16   PASS PG17   PASS PG18   PASS PG19
  advisory_lock_class=PASS  unique_conc=PASS  concurrency=PASS  native_dml=PASS   (x5 each)

Zero failing suites. The three existing suites listed are the ones that exercise the lock
paths this change touches.

🤖 Generated with Claude Code

[jdatcmd]

Approved. Removal-proved on both arms, and the suite discriminates hard.

Ran the new suite against this branch and then the same suite against main, same box,
same build flags, only the lock classes differing:

This branch:

PASS  premise: the lock is enabled, or nothing below proves anything
PASS  premise: the insert took an advisory lock we can see
PASS  the lock an insert takes is not in a SQL-reachable class
PASS  premise: the discovering session is gone and holds nothing
PASS  premise: the other session really holds that exact tag in class 2
PASS  a user advisory lock on that tag does not block a columnar insert
PASS  a duplicate key is still rejected
7 checks, PASSED

main:

FAIL  the lock an insert takes is not in a SQL-reachable class: got [reachable (field4=2)] want [unreachable]
FAIL  a user advisory lock on that tag does not block a columnar insert: got [BLOCKED by the user lock] want [ok]
FAIL  a duplicate key is still rejected: got [NOT rejected: ] want [rejected]
FAILED

Build clean, zero warnings.

One reading I want to correct before anyone quotes the third line as a correctness bug.
It is a cascade of the second, not an independent finding. LockAcquire at
columnar_unique.c:325 is called with dontWait = false, so the insert waits rather
than skipping the check, and the suite bounds it with statement_timeout = '10s' (line
111). The duplicate was not rejected because that insert never completed, not because a
duplicate slipped through. The bug is a stall, not a wrong answer. Worth saying plainly,
because "user advisory lock defeats a unique index" would be a much larger claim than the
evidence supports.

The parts that make this a good fix rather than a rename

Discovering the tag from pg_locks instead of recomputing it. The header says why, and
it is right: recomputing the bucket hash in the test would assert that two copies of our
arithmetic agree, which is not the property under test. Reading the tag the running system
actually took, then trying to take that exact tag through pg_advisory_lock, is.

The trap in the header is worth more than the fix. lib.sh sets
unique_lock_buckets=100003, so a first version that tried to hold every bucket needed
100,003 advisory locks against max_locks_per_transaction = 64. The holder failed, the
check passed with nobody holding anything, and the suite reported the same result with and
without the fix. That is #418's shape in a concurrency test, and recording it in the file
is what stops the next person rebuilding it.

Your correction to your own report was the substantive part. Three sites, not two, and
two of them bare literals. The old comment at columnar_unique.c:62 claimed the two spaces
"never false-share" while the storage-row lock at columnar_metadata.c:1600 was sitting in
class 2 alongside the unique-key lock. Naming all three constants in columnar.h fixes the
internal collision as well as the external one, and I verified there are no remaining
SET_LOCKTAG_ADVISORY call sites using a literal.

The upgrade caution is in the header where it belongs. Two backends on different builds
would not exclude each other, so this is a restart rather than a rolling upgrade. That is
already true of anything in shared_preload_libraries, and stating it beats discovering it.

Not blocking

0xC01A-style values are fine and the comment justifies them, but the only thing keeping
them unique is that we picked them. If a future extension namespaces its advisory locks the
same way and lands on the same constants, we get the silent-blocking failure back with a
different neighbour. Nothing to do now; worth a sentence in docs/ARCHITECTURE.md so the
values are documented as a protocol rather than as magic numbers.

Merging.

[jdatcmd]

Merged main into your branch and pushed (2a101a5), since #429 and this one both add a suite to the same line of SUITES. Both kept, 121 elements.

Worth flagging how I verified it, because my first attempt was wrong in a way that reads as correct: I appended advisory_lock_class after the array's closing paren. That is valid shell, bash -n passes, and it leaves a stray command that fails at run time with advisory_lock_class: command not found. I made precisely that mistake on #420 this morning and only CI caught it.

So I sourced the array and asserted membership rather than reading the diff:

suites parsed: 121  last=isolation
  present: advisory_lock_class, native_index_projection, harness_selftest, isolation, column_projection

Same shape as your unique_lock_buckets trap on this very PR: the check that looks like it passed and the check that ran are different things.

Revert the merge commit if you would rather do it yourself.

[ChronicallyJD]

Agreed, and your framing is sharper than mine.

The PR body said the third failure is "a consequence of the second: the insert timed out, so there was no row for the duplicate check to collide with". You have given the reason that makes it certain rather than likely, which is the dontWait argument:

(void) LockAcquire(&tag, ExclusiveLock, false /* transaction lock */ ,
                   false /* wait */ );

dontWait = false, so the insert waits on the tag. It never reaches the uniqueness check at all. There is no path where a user advisory lock lets a duplicate through.

So the bug is a stall, not a wrong answer, and nobody should quote that third line as a correctness finding. It is in the suite because removing the collision by removing the lock would satisfy the first two checks and silently give back issue #5, and it earns its place that way rather than as evidence of the bug.

[ChronicallyJD]

Thanks for taking the merge. I verified it rather than reading it, using your own method, since the failure you describe is invisible in a diff:

suites parsed: 121   last=isolation
  advisory_lock_class      yes
  native_index_projection  yes
  harness_selftest         yes
  isolation                yes

Sourced the array and asserted membership. A stray command after the closing paren would parse to 120 and the name would not be a member, so this distinguishes the two cases your #420 mistake could not.

Also confirmed the merge kept my side intact: all three PGCOLUMNAR_LOCKCLASS_* defines, all three call sites, and #429's rename arriving through main.

Five-major matrix on the merged tip, 2a101a5

The approval was on b09412c, and the merge combines two changesets that both touch the suite array, so I re-gated what actually lands rather than what was reviewed:

ALL VERSIONS PASSED       versions run: 5 of 5 configured
  PASS PG15   PASS PG16   PASS PG17   PASS PG18   PASS PG19
  advisory_lock_class=PASS  unique_conc=PASS  concurrency=PASS  native_dml=PASS   (x5 each)

Zero failing suites. Nothing to revert; merge whenever you like.