Internal advisory locks share PostgreSQL's user-facing advisory lock keyspace: pg_advisory_lock(indexOid, bucket) takes the same lock

[ChronicallyJD]

Found while auditing shared namespaces for #428. Different bug, different fix, so it is
filed on its own.

pgColumnar's internal advisory locks are in the user-facing advisory lock keyspace

pg_advisory_lock() and friends encode which form was called in locktag_field4, and
only two values exist. src/backend/utils/adt/lockfuncs.c:610-620:

 *	field4: 1 if using an int8 key, 2 if using 2 int4 keys
 */
#define SET_LOCKTAG_INT64(tag, key64) \
	SET_LOCKTAG_ADVISORY(tag, MyDatabaseId, (uint32)((key64) >> 32), (uint32)(key64), 1)
#define SET_LOCKTAG_INT32(tag, key1, key2) \
	SET_LOCKTAG_ADVISORY(tag, MyDatabaseId, key1, key2, 2)

We use both of those values. src/columnar_unique.c:62-65:

/*
 * Advisory-lock discriminator in locktag_field4. The issue #4 delete_vector lock
 * uses 1; the unique-key lock uses 2 so the two lock spaces never false-share.
 */
#define COLUMNAR_UNIQUE_LOCK_CLASS 2

and columnar_unique.c:328:

SET_LOCKTAG_ADVISORY(tag, MyDatabaseId, (uint32) indexOid, bucket,
                     COLUMNAR_UNIQUE_LOCK_CLASS);

That is bit-identical to a user calling:

SELECT pg_advisory_lock(<indexOid>, <bucket>);

Same database, same two keys, same class 2. It is the same lock, not a similar one.

The delete-vector lock at columnar_metadata.c:1242,1600 uses class 1, which is
pg_advisory_lock(bigint).

The comment reasons carefully about our two internal classes not false-sharing with each
other, and does not consider that both values are already spoken for by PostgreSQL's own
SQL-callable functions.

Consequences

An application that uses two-integer advisory locks can take, or block on, a lock
pgColumnar needs:

• an app holding pg_advisory_lock(k1, k2) where k1 happens to equal one of our index
  OIDs and k2 is under pgcolumnar.unique_lock_buckets (default 128) blocks inserts of
  that key;
• conversely our insert path can block the application;
• across transactions this is a deadlock or a stall, and it is silent: it looks like
  unexplained blocking with no bad query to point at.

It needs a coincidence, but not an unlikely one. OIDs are enumerable, and applications
commonly pass an object identifier as the first key.

The fix is small and complete

User advisory locks can only ever set field4 to 1 or 2. So any other value is
unreachable from SQL, and one line per site removes the shared keyspace entirely:

#define COLUMNAR_DELETE_VECTOR_LOCK_CLASS  0xC01A
#define COLUMNAR_UNIQUE_LOCK_CLASS         0xC01B

The two remain distinct from each other, which is what the existing comment wanted, and
neither is reachable by pg_advisory_lock.

One caution for whoever takes it. Changing a lock class changes the locktag, so two
backends running different builds would not mutually exclude. That is a restart, not a
rolling upgrade, which pgColumnar already requires because it is in
shared_preload_libraries. Worth stating in the commit rather than discovering.

Coverage

test/unique_conc.sh exercises the unique lock, so a regression test can assert the
property directly: take pg_advisory_lock(indexOid, bucket) from a second session and
show that a concurrent insert of that key is not blocked by it. That check fails on
today's code and passes after the change, which is the removal proof.

Happy to take it.

[ChronicallyJD]

Taking this, starting now.

One correction to my own report while I am here: there are three advisory-lock sites, not two, and two of them use a bare literal rather than the named constant.

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

So the comment at columnar_unique.c:62 is wrong about our own internal picture too:

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

There are three uses across two classes, and the storage-row lock shares class 2 with the unique lock. They do not collide in practice, because storageId >> 32 is 0 for any realistic storage id and an index OID is never 0, but the comment invites a fourth use added on the assumption that the classes partition. All three get their own class.