From b09412c4574928ebef6225eb902eb19a07e7f0f8 Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Wed, 5 Aug 2026 14:11:31 -0600 Subject: [PATCH] fix: move the advisory lock classes out of the user-facing keyspace (#430) locktag_field4 discriminates advisory lock spaces, 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, not the two the issue first said, and two used a bare literal: columnar_metadata.c:1242 delete-vector chunk class 1 pg_advisory_lock(bigint) columnar_metadata.c:1600 storage-row creation class 2 pg_advisory_lock(int4,int4) columnar_unique.c:328 unique key class 2 pg_advisory_lock(int4,int4) So the comment at columnar_unique.c:62 was wrong about our own internal picture too. It said the delete-vector lock uses 1 and the unique lock uses 2 "so the two lock spaces never false-share", unaware that the storage-row lock 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, but it invited a fourth use added on the assumption that the classes partition. An application can only ever set field4 to 1 or 2, so any value above 2 is unreachable from SQL. All three now have their own class, defined once in columnar.h with the reason, at 101, 102 and 103. 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. test/advisory_lock_class.sh discovers the tag an insert actually takes from pg_locks rather than recomputing the bucket hash, since reimplementing it here would only assert that two copies of our arithmetic agree. Removal proof, same suite file against both builds: without the fix FAIL the lock is in a SQL-reachable class (field4=2) FAIL a user advisory lock on that tag blocks the insert FAIL a duplicate key is still rejected (the insert timed out) with the fix 7 of 7 PASS The suite also asserts that a genuine duplicate is still rejected, because removing the collision by removing the lock would satisfy everything else and silently give back issue #5. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UqprqkCXuH8SegiZejE1Tw --- src/columnar.h | 30 ++++++++ src/columnar_metadata.c | 6 +- src/columnar_unique.c | 8 +- test/advisory_lock_class.sh | 141 ++++++++++++++++++++++++++++++++++++ test/run_all_versions.sh | 2 +- 5 files changed, 177 insertions(+), 10 deletions(-) create mode 100755 test/advisory_lock_class.sh diff --git a/src/columnar.h b/src/columnar.h index 6741e6c..9ce4593 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -788,6 +788,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) * diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index b3e52e8..40c3d6d 100644 --- a/src/columnar_metadata.c +++ b/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 */ ); diff --git a/src/columnar_unique.c b/src/columnar_unique.c index 5367659..70b0d0d 100644 --- a/src/columnar_unique.c +++ b/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 */ ); diff --git a/test/advisory_lock_class.sh b/test/advisory_lock_class.sh new file mode 100755 index 0000000..b0c0595 --- /dev/null +++ b/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 diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 811c6df..cada206 100755 --- a/test/run_all_versions.sh +++ b/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_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_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=(