test: SERIALIZABLE refuses a write skew on a columnar table, and the docs say so (#1341 item 2)

CHANGELOG.md

@@ -401,6 +401,23 @@ true until the next version shipped.
 
 ### Fixed
 
+- **A concurrent `UPDATE` of the same row now fails with 40001 on a table with a
+  unique index, as documented, not 23505 (#1421).** Two sessions updating one row of
+  a table with a PRIMARY KEY or UNIQUE index: the second used to wait on the first's
+  new key and then fail with `unique_violation`, naming a duplicate the user never
+  wrote, so an application retrying on `serialization_failure` did not retry. The
+  same happened to a row two sessions moved between partitions of a parent with a
+  key. The update path now serializes the row before its new version reaches the
+  unique check. On such tables each row is serialized as the `UPDATE` reaches it, so
+  two multi-row `UPDATE`s reaching shared rows in opposite orders can now deadlock
+  (40P01), as a heap can. Each row's check reads its row group's committed delete
+  marks. On PostgreSQL 17, an `UPDATE` of a keyed table took about 25% to 50%
+  longer: 100,000 rows updated where half the table was deleted, and 200,000 where
+  nothing was (three runs each, by both of us). A table without a unique index is
+  unchanged. `test/update_conc.sh` and the new
+  `test/partition_rows.sh`, with `test_partition_rows.py`, which also holds such an
+  `UPDATE` to bounded memory.
+
 - **The Iceberg wrapper no longer reads server files while planning for a role the scan
   would refuse.** `EXPLAIN` on an Iceberg foreign table read its `metadata_path` for any
   role holding SELECT on it. That showed a role without `pg_read_server_files` the

docs/limitations.md

@@ -601,14 +601,27 @@ same way as a `CREATE INDEX` that is not concurrent. Turn projection scans off w
   is lost. This holds at every isolation level and needs no unique index. Retry
   the transaction on this error, as you retry one at `REPEATABLE READ`. The
   behavior is stricter than a heap at `READ COMMITTED`. A heap re-applies both
-  updates transparently there; pgColumnar makes the loser retry. The GUC
+  updates transparently there; pgColumnar makes the loser retry. The same holds
+  for a row an `UPDATE` moves to another partition. On a table with a unique index,
+  an `UPDATE` serializes each row as it reaches it, not at the end of the statement.
+  Two multi-row `UPDATE`s that reach shared rows in opposite orders can therefore
+  deadlock. One of them then gets `deadlock_detected` (40P01), which is also
+  retryable. The GUC
   `pgcolumnar.enable_row_update_lock` (default on) controls this serialization.
   The GUC `pgcolumnar.row_lock_buckets` (default 1024) bounds the held locks per
   storage, so a bulk update cannot exhaust the lock table. Unrelated rows may
   share a bucket, which only makes them wait when they need not. For the same
   reason, two multi-statement transactions can rarely deadlock, even on disjoint
   rows. This happens when their bucketed locks collide in opposite statement order.
   Retry resolves it, as with any serialization failure.
+- `SERIALIZABLE` works on columnar tables: a write skew between two serializable
+  transactions fails one of them with `serialization_failure` (40001), as on a
+  heap. The access method takes no predicate locks of its own. The protection comes
+  from PostgreSQL's serializable isolation on pgColumnar's own catalog tables, which
+  every read of a columnar table consults and every write changes.
+  `test/serializable.sh` covers two shapes. In one, each transaction updates a row,
+  in the same row group or in different ones. In the other, each inserts a row after
+  a scan of the whole table or an index scan.
 - Concurrent inserts of the same unique key go in sequence. The server therefore
   always finds the conflict. Before a new row reaches the uniqueness check, the
   access method takes an advisory lock with the scope of the transaction. The key
@@ -657,7 +670,8 @@ other than where the feature is used:
   `ON CONFLICT DO NOTHING` takes no row lock and works, with or without a conflict
   target.
 
-`CREATE TABLE` refuses an unlogged columnar table for the same reason. Both cases
+`CREATE TABLE` refuses an unlogged columnar table for the same reason, and so does
+`ALTER TABLE ... SET UNLOGGED`. Both cases
 follow one rule. This access method refuses a configuration that it cannot
 support at the point where you choose it. It does not refuse it at each later
 use.
@@ -783,7 +797,11 @@ therefore still be cheaper as a scan.
   of a deleted row only while a transaction that began before the delete is still
   running, as it does on heap.
 - `CREATE INDEX CONCURRENTLY` (the concurrent validate path) and partial
-  block-range index builds are not supported.
+  block-range index builds are not supported. A refused `CREATE INDEX
+  CONCURRENTLY` leaves its index behind, marked invalid, as PostgreSQL does for any
+  concurrent build that fails. Drop that index before you build it again.
+- `ALTER TABLE ... SET TABLESPACE` is not supported on a columnar table. It is
+  refused, and the table stays where it was.
 
 ## Constraints on the import path
 

src/columnar.h

@@ -1065,6 +1065,11 @@ extern bool PgColumnarRowWriteConflict(Relation rel, ItemPointer otid,
 									   TM_FailureData *tmfd, TM_Result *result);
 extern void PgColumnarSerializeFlushRows(uint64 storageId, const uint64 *rows,
 										 int nrows);
+extern void PgColumnarSerializeRowNow(Relation rel, uint64 rowNumber);
+extern bool PgColumnarHasUniqueIndex(Relation rel);
+extern NativeRowGroupMetadata *PgColumnarLookupRowGroup(uint64 storageId,
+														 Snapshot metaSnapshot,
+														 uint64 rowNumber);
 
 /* -------------------------------------------------------------------------
  * planner integration (pgcolumnar_customscan.c, spec 8.3, 9)

src/columnar_reader.c

@@ -4472,6 +4472,18 @@ pgcolumnar_lookup_row_group(uint64 storageId, Snapshot metaSnapshot,
 	return pgcolumnar_group_memo_walk(m, rowNumber);
 }
 
+/*
+ * PgColumnarLookupRowGroup
+ *		The row group covering rowNumber under metaSnapshot, or NULL, through the
+ *		same memo the fetch path uses. For the update path's single-row conflict
+ *		check (#1421), which needs one group, not the whole list.
+ */
+NativeRowGroupMetadata *
+PgColumnarLookupRowGroup(uint64 storageId, Snapshot metaSnapshot, uint64 rowNumber)
+{
+	return pgcolumnar_lookup_row_group(storageId, metaSnapshot, rowNumber);
+}
+
 /*
  * Find the entry for this group in this command, or prepare an empty one.
  *

src/columnar_row_lock.c

@@ -343,3 +343,98 @@ PgColumnarSerializeFlushRows(uint64 storageId, const uint64 *rows, int nrows)
 				 errmsg("columnar: could not serialize access due to concurrent update or delete of the same row"),
 				 errhint("Retry the transaction.")));
 }
+
+/*
+ * PgColumnarSerializeRowNow
+ *		Serialize one row on its identity NOW, during the UPDATE, rather than at
+ *		the flush (#1421). Take the row's bucket lock, waiting for a concurrent
+ *		writer of the row to finish, then raise the same retryable
+ *		serialization_failure if that writer committed the row as deleted.
+ *
+ *		WHY NOT WAIT FOR THE FLUSH. An UPDATE of a table with a unique index takes
+ *		the new version's unique-key lock before the flush. When a concurrent
+ *		transaction updated the same row, its new version holds that key: the
+ *		loser waited on it, and when the winner committed the executor's unique
+ *		check raised unique_violation (23505), a duplicate the user never wrote,
+ *		before the flush that raises 40001 was reached. docs/limitations.md
+ *		promises 40001 and says the serialization "needs no unique index".
+ *
+ *		Called only for a relation with a unique index, so a table without one
+ *		keeps the batched, sorted acquisition at flush. The flush still runs and
+ *		re-takes this lock, which the lock manager makes a no-op. Taking locks in
+ *		row order during the statement can deadlock two multi-row UPDATEs that
+ *		reach shared rows in opposite orders (40P01, also retryable), as heap can.
+ *
+ *		ONE GROUP, NOT THE LIST. The batch reads every group's committed mask; per
+ *		row that would be O(groups). The covering group comes through the fetch
+ *		path's memo, and only its delete vector is read.
+ */
+void
+PgColumnarSerializeRowNow(Relation rel, uint64 rowNumber)
+{
+	uint64		storageId;
+	uint32		numBuckets;
+	LOCKTAG		tag;
+	Snapshot	meta;
+	NativeRowGroupMetadata *rg;
+	bool		deleted = false;
+	MemoryContext tmp;
+	MemoryContext old;
+
+	if (!pgcolumnar_enable_row_update_lock)
+		return;
+
+	storageId = PgColumnarStorageId(rel);
+	numBuckets = (uint32) Max(1, pgcolumnar_row_lock_buckets);
+	SET_LOCKTAG_ADVISORY(tag, MyDatabaseId, (uint32) storageId,
+						 (uint32) (row_identity_lock_key(storageId, rowNumber) % numBuckets),
+						 PGCOLUMNAR_LOCKCLASS_ROW_IDENTITY);
+	(void) LockAcquire(&tag, ExclusiveLock, false /* transaction lock */ ,
+					   false /* wait */ );
+
+	/*
+	 * With the lock held, the latest committed state: a delete committed after
+	 * this statement's snapshot is visible here. This transaction's own deletes
+	 * are in its buffer, not in this committed view, so they are not conflicts.
+	 *
+	 * The reads go in a context of their own, deleted before returning. This runs
+	 * once per updated row, and in the caller's context each call would keep a copy
+	 * of the group's delete bitmap until the statement ends: 402 MB for 25,000 rows
+	 * of a 50,000-row group (OffgridwithJD, review of #1422).
+	 */
+	tmp = AllocSetContextCreate(CurrentMemoryContext,
+								"pgcolumnar row serialization check",
+								ALLOCSET_SMALL_SIZES);
+	old = MemoryContextSwitchTo(tmp);
+	PushActiveSnapshot(GetLatestSnapshot());
+	meta = PgColumnarCatalogSnapshot(GetActiveSnapshot());
+	rg = PgColumnarLookupRowGroup(storageId, meta, rowNumber);
+	if (rg != NULL)
+	{
+		List	   *maskList = PgColumnarReadDeleteVectorList(storageId,
+															  rg->groupNumber, meta);
+		uint64		bit = rowNumber - rg->firstRowNumber;
+		ListCell   *mc;
+
+		foreach(mc, maskList)
+		{
+			DeleteVectorMetadata *rm = (DeleteVectorMetadata *) lfirst(mc);
+
+			if (rm->bitmap != NULL && (bit >> 3) < rm->bitmapLen &&
+				(rm->bitmap[bit >> 3] & (1 << (bit & 7))) != 0)
+			{
+				deleted = true;
+				break;
+			}
+		}
+	}
+	PopActiveSnapshot();
+	MemoryContextSwitchTo(old);
+	MemoryContextDelete(tmp);
+
+	if (deleted)
+		ereport(ERROR,
+				(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+				 errmsg("columnar: could not serialize access due to concurrent update or delete of the same row"),
+				 errhint("Retry the transaction.")));
+}

src/columnar_tableam.c

@@ -2236,6 +2236,19 @@ pgcolumnar_tuple_delete(COLUMNAR_TUPLE_DELETE_ARGS)
 	if (PgColumnarRowWriteConflict(rel, tid, cid, wait, tmfd, &conflict))
 		return conflict;
 
+	/*
+	 * A row moving to another partition is deleted here and inserted there, and
+	 * the insert reaches the destination's unique check before this delete is
+	 * flushed. Serialize it now, as the update path does, so a concurrent mover
+	 * of the same row loses with 40001 rather than 23505 (#1421).
+	 */
+#if PG_VERSION_NUM >= 190000
+	if ((options & TABLE_DELETE_CHANGING_PARTITION) != 0)
+#else
+	if (changingPart)
+#endif
+		PgColumnarSerializeRowNow(rel, rowNumber);
+
 	PgColumnarMarkRowDeleted(rel, rowNumber);
 	return TM_Ok;
 }
@@ -2270,6 +2283,14 @@ pgcolumnar_tuple_update(COLUMNAR_TUPLE_UPDATE_ARGS)
 		return conflict;
 	}
 
+	/*
+	 * #1421: with a unique index, the new version's unique-key lock and check
+	 * come before the flush that serializes the old row, so a loser waited on
+	 * the winner's key and failed with 23505. Serialize the old row first.
+	 */
+	if (PgColumnarHasUniqueIndex(rel))
+		PgColumnarSerializeRowNow(rel, oldRowNumber);
+
 	PgColumnarMarkRowDeleted(rel, oldRowNumber);
 
 	writeState = PgColumnarGetWriteState(rel);

src/columnar_unique.c

@@ -326,6 +326,21 @@ pgcolumnar_acquire_key_lock(Oid indexOid, uint32 bucket)
 					   false /* wait */ );
 }
 
+/*
+ * PgColumnarHasUniqueIndex
+ *		Does rel have a unique index that a new row version is checked against?
+ *		The same set PgColumnarLockUniqueKeys locks for (unique, immediate, valid,
+ *		ready), read from the same cache. The update path asks it to decide
+ *		whether a row must be serialized before its new version is inserted
+ *		(#1421). Independent of enable_unique_insert_lock: without that lock the
+ *		btree's own uniqueness check still waits on the winner's key.
+ */
+bool
+PgColumnarHasUniqueIndex(Relation rel)
+{
+	return pgcolumnar_unique_lookup(rel)->nIndexes > 0;
+}
+
 /*
  * PgColumnarLockUniqueKeys
  *		For a row about to be inserted into rel (through the table AM), take a

test/advisory_lock_class.sh

@@ -245,4 +245,126 @@ check "row_lock_buckets is fixed at server start, so a session cannot change it
 	"$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At -v VERBOSITY=sqlstate -c "SET pgcolumnar.row_lock_buckets = 7;" 2>&1 | sed -n 's/^.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\).*$/\1/p')|$(q "SELECT context || '|' || setting FROM pg_settings WHERE name = 'pgcolumnar.row_lock_buckets'")" \
 	"55P02|postmaster|1024"
 
+# The documented bound (docs/configuration.md): row_lock_buckets "bounds the row locks
+# a transaction holds, so a bulk update cannot exhaust the lock table". One transaction
+# updates 200000 rows and then counts its own class-104 locks (one -c string is one
+# transaction): one per bucket, not one per row. A row lock per row would not fit the
+# default lock table at all.
+psql_run "CREATE TABLE rlb (id int, v int) USING pgcolumnar;
+          INSERT INTO rlb SELECT g, g FROM generate_series(1, 200000) g;" >/dev/null
+check "a bulk UPDATE of 200000 rows holds one row-identity lock per bucket, row_lock_buckets of them" \
+	"$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At \
+		-c "UPDATE rlb SET v = v + 1;
+		    SELECT count(*) || '|' || current_setting('pgcolumnar.row_lock_buckets') FROM pg_locks
+		     WHERE pid = pg_backend_pid() AND locktype = 'advisory' AND objsubid = 104;" 2>&1 |
+		grep -E '^[0-9]+[|]|ERROR' | tail -1)" "1024|1024"
+check "and the UPDATE committed every row" "$(q "SELECT count(*) || '|' || sum(v - id) FROM rlb")" "200000|200000"
+
+# ---- the row-identity lock cannot be reached from SQL (#1341 item 56) ------------
+# The INSERT arms above ask this of the unique-key and storage-row locks; nothing
+# asked it of class 104. An UPDATE is held open and every advisory lock it took is
+# read. None may be in field 1 or 2, the only classes SQL can produce. Then a second
+# session takes each of those exact tags with pg_advisory_xact_lock(int4, int4), and
+# a columnar UPDATE of the same row must not wait on it. An oid above the int4 range
+# is addressed by its negative int4, the same 32 bits lockfuncs.c puts in the tag.
+rl_wait_for() {	# rl_wait_for APPLICATION_NAME -> its pid, once it has named itself
+	local i pid=""
+	for i in $(seq 1 100); do
+		pid="$(q "SELECT pid FROM pg_stat_activity WHERE application_name = '$1'")"
+		[ -n "$pid" ] && break
+		sleep 0.1
+	done
+	echo "$pid"
+}
+rl_end() {	# rl_end PID BGPID: end a holder's backend and its psql
+	q "SELECT pg_terminate_backend($1)" >/dev/null 2>&1
+	kill "$2" 2>/dev/null; wait "$2" 2>/dev/null
+	local i
+	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
+}
+env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -Atq \
+	-c "BEGIN; UPDATE rl SET v = v + 1 WHERE id = 7; SET application_name = 'rl_tagger'; SELECT pg_sleep(30); COMMIT;" \
+	>/dev/null 2>&1 &
+rl_bg=$!
+rl_pid="$(rl_wait_for rl_tagger)"
+RL_TAGS="$(q "SELECT CASE WHEN classid::bigint > 2147483647 THEN classid::bigint - 4294967296 ELSE classid::bigint END
+	              || ' ' || CASE WHEN objid::bigint > 2147483647 THEN objid::bigint - 4294967296 ELSE objid::bigint END
+	              || ' ' || objsubid
+	         FROM pg_locks WHERE locktype = 'advisory' AND granted AND pid = ${rl_pid:-0}")"
+rl_end "${rl_pid:-0}" "$rl_bg"
+RL_N="$(printf '%s\n' "$RL_TAGS" | grep -c '[0-9]')"
+rl_reach=""
+while read -r _c _o _f; do
+	case "${_f:-}" in 1|2) rl_reach="$rl_reach ($_c,$_o,field4=$_f)" ;; esac
+done <<<"$RL_TAGS"
+check "the locks an UPDATE takes are not in a SQL-reachable class" \
+	"$(if [ "$RL_N" -eq 0 ]; then echo "unknown (no advisory lock seen; holder pid ${rl_pid:-none})"
+	   elif [ -n "$rl_reach" ]; then echo "reachable:$rl_reach"
+	   else echo unreachable; fi)" "unreachable"
+rl_grab="BEGIN;"
+while read -r _c _o _f; do
+	[ -z "${_f:-}" ] && continue
+	rl_grab="$rl_grab SELECT pg_advisory_xact_lock($_c::int, $_o::int);"
+done <<<"$RL_TAGS"
+env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -Atq \
+	-c "$rl_grab SET application_name = 'rl_user'; SELECT pg_sleep(30); COMMIT;" >/dev/null 2>&1 &
+rl_bg=$!
+rl_pid="$(rl_wait_for rl_user)"
+check "premise: the other session holds every one of those tags through the SQL functions" \
+	"$(q "SELECT count(*) FROM pg_locks WHERE locktype = 'advisory' AND granted AND objsubid = 2
+	         AND pid = ${rl_pid:-0}")" "$RL_N"
+rl_upd="$(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 "UPDATE rl SET v = v + 1 WHERE id = 7;" 2>&1)"
+case "$rl_upd" in
+	*timeout*|*canceling*) rl_verdict="BLOCKED by the user lock" ;;
+	*ERROR*)               rl_verdict="ERROR: $(head -1 <<<"$rl_upd")" ;;
+	*)                     rl_verdict=ok ;;
+esac
+check "a user advisory lock on those tags does not block a columnar UPDATE" "$rl_verdict" "ok"
+rl_end "${rl_pid:-0}" "$rl_bg"
+
+# ---- bulk_parallel_writer set by hand still serialises a first write (#1341 item 55) --
+# The setting lets parallel_copy's loaders skip the storage-row creation lock (class
+# 102). The reference says setting it by hand is safe, because the skip fires only
+# once the storage row is committed. Two sessions with it on each make the first
+# write to a committed, empty table: the second must wait for the first and then
+# land its row. A skip that did not wait for the row to be committed lets both
+# create it, and the second fails on storage_pkey.
+#
+# The first writer commits only once the suite has looked for the second one waiting
+# and then inserted a committed flag row, not after a fixed sleep: a slow second
+# writer would otherwise start after the first had committed, and the premise below
+# would fail on timing (OffgridwithJD, review of #1415). The pytest twin does the same.
+psql_run "CREATE TABLE bw (id int) USING pgcolumnar; CREATE TABLE bw_go (go int) USING heap;"
+env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -Atq \
+	-c "SET statement_timeout = '60s'; BEGIN; SET LOCAL pgcolumnar.bulk_parallel_writer = on; INSERT INTO bw VALUES (1); SET application_name = 'bw_first';
+	    DO \$\$ BEGIN WHILE NOT EXISTS (SELECT 1 FROM bw_go) LOOP PERFORM pg_sleep(0.1); END LOOP; END \$\$; COMMIT;" \
+	>/dev/null 2>&1 &
+bw_bg=$!
+bw_pid="$(rl_wait_for bw_first)"
+env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -At \
+	-c "SET pgcolumnar.bulk_parallel_writer = on; SET statement_timeout = '30s';" -c "INSERT INTO bw VALUES (2);" \
+	>"$PGC_WORKDIR/bw_second.out" 2>&1 &
+bw_bg2=$!
+# The second write must be waiting on the first before the first commits, or the arm
+# measures two writes in sequence.
+bw_waited=no
+for _i in $(seq 1 100); do
+	[ "$(q "SELECT count(*) FROM pg_stat_activity WHERE wait_event_type = 'Lock' AND query LIKE 'INSERT INTO bw%'")" -ge 1 ] && { bw_waited=yes; break; }
+	sleep 0.1
+done
+psql_run "INSERT INTO bw_go VALUES (1);"	# release the first writer, whatever the poll saw
+check "premise: the second first write waited on the first" "$bw_waited" "yes"
+wait "$bw_bg2"; wait "$bw_bg"
+bw_out="$(cat "$PGC_WORKDIR/bw_second.out")"
+case "$bw_out" in
+	*ERROR*) bw_verdict="ERROR: $(grep -m1 ERROR <<<"$bw_out")" ;;
+	*)       bw_verdict=ok ;;
+esac
+check "with bulk_parallel_writer set by hand, a second first write waits for the first and both rows land" \
+	"$([ -n "$bw_pid" ] && echo held || echo "no holder")|$bw_verdict|$(q "SELECT count(*) FROM bw")" "held|ok|2"
+
 pgc_summary