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

src/columnar.h

@@ -804,6 +804,36 @@ extern char *PgColumnarDecompressValueStream(const char *comp, uint32 compLen,
 										   MemoryContext targetContext);
 
 
+/* -------------------------------------------------------------------------
+ * advisory-lock classes (issue #430)
+ *
+ * locktag_field4 discriminates advisory lock spaces, and PostgreSQL's own
+ * SQL-callable functions already own two values. From lockfuncs.c:
+ *
+ *     field4: 1 if using an int8 key, 2 if using 2 int4 keys
+ *
+ * so pg_advisory_lock(bigint) is class 1 and pg_advisory_lock(int4, int4) is
+ * class 2, and NOTHING ELSE reachable from SQL sets this field. We used to use
+ * 1 and 2, which made our internal locks bit-identical to a user's: the
+ * unique-key lock was exactly pg_advisory_lock(indexOid, bucket). An
+ * application holding that tag blocked our inserts, and we blocked it, silently
+ * and with no bad query to point at.
+ *
+ * Any value above 2 is unreachable from SQL, so these three cannot be taken by
+ * an application at all. They are distinct from each other as well, which is
+ * what the previous comment in pgcolumnar_unique.c wanted and did not achieve:
+ * there were three uses across two classes, and the storage-row lock shared
+ * class 2 with the unique-key lock.
+ *
+ * These values are part of the on-the-wire lock protocol between backends, so
+ * two backends running 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.
+ * ------------------------------------------------------------------------- */
+#define PGCOLUMNAR_LOCKCLASS_DELETE_VECTOR	101
+#define PGCOLUMNAR_LOCKCLASS_STORAGE_ROW	102
+#define PGCOLUMNAR_LOCKCLASS_UNIQUE_KEY		103
+
 /* -------------------------------------------------------------------------
  * concurrent unique-key insert serialization (pgcolumnar_unique.c, issue #5)
  *

src/columnar_metadata.c

@@ -1240,7 +1240,8 @@ delete_vector_lock_chunk_group(uint64 storageId, uint64 stripeId, int chunkId)
 	uint64		key = delete_vector_chunk_lock_key(storageId, stripeId, chunkId);
 
 	SET_LOCKTAG_ADVISORY(tag, MyDatabaseId,
-						 (uint32) (key >> 32), (uint32) (key & 0xFFFFFFFF), 1);
+						 (uint32) (key >> 32), (uint32) (key & 0xFFFFFFFF),
+						 PGCOLUMNAR_LOCKCLASS_DELETE_VECTOR);
 
 	(void) LockAcquire(&tag, ExclusiveLock, false /* transaction lock */ ,
 					   false /* wait */ );
@@ -1599,7 +1600,8 @@ PgColumnarInsertNativeStorageRow(const NativeStorageMetadata *s)
 
 	SET_LOCKTAG_ADVISORY(tag, MyDatabaseId,
 						 (uint32) (s->storageId >> 32),
-						 (uint32) (s->storageId & 0xFFFFFFFF), 2);
+						 (uint32) (s->storageId & 0xFFFFFFFF),
+						 PGCOLUMNAR_LOCKCLASS_STORAGE_ROW);
 	(void) LockAcquire(&tag, ExclusiveLock, false /* transaction lock */ ,
 					   false /* wait */ );
 

src/columnar_unique.c

@@ -58,12 +58,6 @@
 bool		pgcolumnar_enable_unique_lock = true;
 int			pgcolumnar_unique_lock_buckets = 128;
 
-/*
- * 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
-
 /* 64-bit FNV-1a basis/prime, matching delete_vector_chunk_lock_key's mixer */
 #define COLUMNAR_FNV_OFFSET UINT64CONST(1469598103934665603)
 #define COLUMNAR_FNV_PRIME  UINT64CONST(1099511628211)
@@ -326,7 +320,7 @@ pgcolumnar_acquire_key_lock(Oid indexOid, uint32 bucket)
 	LOCKTAG		tag;
 
 	SET_LOCKTAG_ADVISORY(tag, MyDatabaseId, (uint32) indexOid, bucket,
-						 COLUMNAR_UNIQUE_LOCK_CLASS);
+						 PGCOLUMNAR_LOCKCLASS_UNIQUE_KEY);
 
 	(void) LockAcquire(&tag, ExclusiveLock, false /* transaction lock */ ,
 					   false /* wait */ );

test/advisory_lock_class.sh

@@ -0,0 +1,141 @@
+#!/usr/bin/env bash
+#
+# pgColumnar's internal advisory locks must not be reachable from SQL (#430).
+#
+# locktag_field4 says which advisory lock space a tag belongs to, and PostgreSQL's
+# own functions own exactly two values. From lockfuncs.c:
+#
+#     field4: 1 if using an int8 key, 2 if using 2 int4 keys
+#
+# We used both. The unique-key lock was SET_LOCKTAG_ADVISORY(db, indexOid, bucket, 2),
+# which is bit for bit what pg_advisory_lock(indexOid, bucket) takes. So an
+# application holding that tag blocked columnar inserts of that key, and columnar
+# blocked the application, with nothing to point at but unexplained waiting.
+#
+# The lock is DISCOVERED from pg_locks rather than recomputed here. Reimplementing
+# the bucket hash in the test would assert that two copies of our arithmetic agree,
+# which is not the property. Reading the tag the running system actually took, then
+# trying to grab that exact tag through the SQL function, is.
+#
+# It also avoids a trap the first version of this file walked into: lib.sh sets
+# pgcolumnar.unique_lock_buckets=100003, so "hold every bucket" needs 100,003
+# advisory locks against a max_locks_per_transaction of 64. The holder failed, the
+# check passed with nobody holding anything, and the suite reported the same result
+# with and without the fix.
+#
+# Usage:  test/advisory_lock_class.sh [PG_CONFIG]
+# Written fresh for pgColumnar.
+set -uo pipefail
+. "$(dirname "${BASH_SOURCE[0]}")/lib.sh"
+pgc_setup "${1:-/usr/local/pg17/bin/pg_config}"
+
+PSQL_BG() {  # run SQL in a background session that stays open
+	env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \
+		-d "$PGC_DB" -At -c "$1" >"$2" 2>&1 &
+	echo $!
+}
+
+psql_run "CREATE TABLE u (k int, v text) USING pgcolumnar;
+          CREATE UNIQUE INDEX u_k ON u (k);
+          INSERT INTO u SELECT g, 'v'||g FROM generate_series(1,100) g;" >/dev/null
+
+check "premise: the lock is enabled, or nothing below proves anything" \
+	"$(q "SHOW pgcolumnar.enable_unique_insert_lock")" "on"
+
+# ---------------------------------------------------------------------------
+# 1. Discover the advisory lock an insert actually takes.
+# ---------------------------------------------------------------------------
+HOLD1="$PGC_WORKDIR/discover.out"
+PID1=$(PSQL_BG "BEGIN; INSERT INTO u VALUES (900001, 'probe'); SELECT pg_sleep(30);" "$HOLD1")
+
+lockrow=""
+for _i in $(seq 1 60); do
+	lockrow=$(q "SELECT classid || ' ' || objid || ' ' || objsubid
+	               FROM pg_locks
+	              WHERE locktype = 'advisory' AND granted
+	                AND pid <> pg_backend_pid()
+	              ORDER BY objsubid DESC LIMIT 1")
+	[ -n "$lockrow" ] && break
+	sleep 0.2
+done
+set -- $lockrow
+LK_CLASSID="${1:-}"; LK_OBJID="${2:-}"; LK_SUBID="${3:-}"
+
+check "premise: the insert took an advisory lock we can see" \
+	"$([ -n "$LK_SUBID" ] && echo yes || echo "no (pg_locks showed nothing)")" "yes"
+echo "      the lock it took: classid=$LK_CLASSID objid=$LK_OBJID field4=$LK_SUBID"
+
+# The assertion, read straight off the tag. 1 and 2 are the only values an
+# application can produce, so anything else is unreachable from SQL.
+check "the lock an insert takes is not in a SQL-reachable class" \
+	"$(case "$LK_SUBID" in 1|2) echo "reachable (field4=$LK_SUBID)" ;; "") echo unknown ;; *) echo unreachable ;; esac)" \
+	"unreachable"
+
+# Terminate the BACKEND, not just psql. Killing the client leaves the server
+# inside pg_sleep() holding its transaction, and every check below then blocks on
+# our own lock rather than on the user's, in both arms, which is how the first
+# version of this file reported the same result with and without the fix.
+q "SELECT pg_terminate_backend(pid) FROM pg_stat_activity
+    WHERE backend_type = 'client backend' AND pid <> pg_backend_pid()
+      AND state IN ('idle in transaction', 'active')" >/dev/null
+kill "$PID1" 2>/dev/null; wait "$PID1" 2>/dev/null
+gone=no
+for _i in $(seq 1 60); do
+	if [ "$(q "SELECT count(*) FROM pg_locks WHERE locktype='advisory' AND pid<>pg_backend_pid()")" = "0" ]; then
+		gone=yes; break
+	fi
+	sleep 0.2
+done
+check "premise: the discovering session is gone and holds nothing" "$gone" "yes"
+
+# ---------------------------------------------------------------------------
+# 2. A user taking that exact tag must not block the insert.
+# ---------------------------------------------------------------------------
+# pg_advisory_xact_lock(int4,int4) produces field4 = 2. Before the fix our lock
+# was also field4 = 2, so this took the same tag and the insert waited forever.
+if [ -n "$LK_CLASSID" ] && [ "$LK_CLASSID" -le 2147483647 ] && [ "$LK_OBJID" -le 2147483647 ]; then
+	HOLD2="$PGC_WORKDIR/holder.out"
+	PID2=$(PSQL_BG "BEGIN;
+	                SELECT pg_advisory_xact_lock($LK_CLASSID::int, $LK_OBJID::int);
+	                SELECT pg_sleep(30);" "$HOLD2")
+	held=no
+	for _i in $(seq 1 60); do
+		n=$(q "SELECT count(*) FROM pg_locks WHERE locktype='advisory' AND granted
+		         AND objsubid = 2 AND classid = $LK_CLASSID AND objid = $LK_OBJID")
+		[ "${n:-0}" -ge 1 ] && { held=yes; break; }
+		sleep 0.2
+	done
+	check "premise: the other session really holds that exact tag in class 2" "$held" "yes"
+
+	ins=$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At \
+		-c "SET statement_timeout = '10s';" -c "INSERT INTO u VALUES (900001, 'new');" 2>&1)
+	case "$ins" in
+		*timeout*|*canceling*) verdict="BLOCKED by the user lock" ;;
+		*ERROR*)               verdict="ERROR: $(head -1 <<<"$ins")" ;;
+		*)                     verdict=ok ;;
+	esac
+	check "a user advisory lock on that tag does not block a columnar insert" "$verdict" "ok"
+
+	q "SELECT pg_terminate_backend(pid) FROM pg_stat_activity
+	    WHERE backend_type = 'client backend' AND pid <> pg_backend_pid()
+	      AND state IN ('idle in transaction', 'active')" >/dev/null
+	kill "$PID2" 2>/dev/null; wait "$PID2" 2>/dev/null
+	for _i in $(seq 1 60); do
+		[ "$(q "SELECT count(*) FROM pg_locks WHERE locktype='advisory' AND pid<>pg_backend_pid()")" = "0" ] && break
+		sleep 0.2
+	done
+else
+	echo "SKIP  classid $LK_CLASSID or objid $LK_OBJID exceeds int4, so the SQL form cannot address it"
+fi
+
+# ---------------------------------------------------------------------------
+# 3. The internal lock still does its job.
+# ---------------------------------------------------------------------------
+# Removing the collision by removing the lock would satisfy everything above and
+# silently give back issue #5.
+dup=$(psql_run "INSERT INTO u VALUES (900001, 'dup');" 2>&1)
+check "a duplicate key is still rejected" \
+	"$(grep -qiE 'duplicate key|unique constraint' <<<"$dup" && echo rejected || echo "NOT rejected: $(head -1 <<<"$dup")")" \
+	"rejected"
+
+pgc_summary

test/run_all_versions.sh

@@ -208,7 +208,7 @@ SRCDIR="${PGC_RUN_SRCDIR:-$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)}"
 SUITES=(harness_selftest docs_style smoke phase2 phase3 phase4 phase5 phase6 audit concurrency unique_conc \
 	differential recovery replication native_backend_crash fuzz fuzz_parquet fuzz_arrow hardening concurrent_diff parallel sorted_projection \
 	arrow_export parquet_export read_stream corruption \
-	generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_setting bloom_lazy native_vecskip native_index native_index_projection native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster pg19_vacuum_options native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection isolation)
+	generated_columns temporal arrow_import index_only projections arrow_nested parquet_import parquet_nested arrow_nested_import parquet_nested_import native_writer native_roundtrip native_encoding native_fastdecode native_zonemap write_minmax_fastpath write_fsst_compressed fsst_margin encode_invariants encode_effort native_skip pushdown_report native_agg native_agg_deletes native_agg_addcolumn native_groupagg ungrouped_vector_agg parallel_vector_agg native_bloom bloom_setting bloom_lazy native_vecskip native_index native_index_projection native_fetch_position native_dml alter_column_type native_ios native_projection native_cluster pg19_vacuum_options native_repack native_compact native_recluster recluster_extent native_vacuum_race native_sort_by sort_status native_reclaim native_ownership drop_cleanup pg_dump_roundtrip native_reclaim_cycles native_reclaim_frag native_reclaim_reconcile native_gap native_format native_truncate native_rewrite native_rewrite_conc rewrite_group_scan native_parquet_schema native_read_parquet native_parquet_fdw native_parquet_pushdown native_parquet_hardening server_file_privilege native_parquet_stack native_parquet_units native_parquet_flba native_parquet_codecs native_parquet_projection native_parquet_multifile native_parquet_streaming native_parquet_partition native_cancel cancel_decode wal_envelope decode_interrupts import_exclusion import_deferred parallel_copy parallel_export_parquet fk_referencing row_triggers native_lazy_slot native_ctas native_fetch_cache native_fetch_interrupt analyze_stats analyze_reltuples native_fetch_projection column_projection advisory_lock_class isolation)
 
 # Default matrix: one assert-enabled pg_config per major, 15 through 19.
 DEFAULT_CONFIGS=(