diff --git a/src/columnar.h b/src/columnar.h index 7aeada7..eea5044 100644 --- a/src/columnar.h +++ b/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) * 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 2416ac2..4db8cc1 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_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=(