[PATCH v1 1/1] Avoid carrying self lock-only xmax to updated tuple

heap_update() normally carries pre-existing tuple lockers from the old
tuple version to the new one. That behavior is required when the old
tuple is locked by another transaction, in particular for FOR KEY SHARE
locks taken by referential-integrity checks.

However, when the old tuple's xmax is a single-XID, lock-only xmax, and
that XID is the same XID that is now updating the tuple, there is no
independent locker to carry forward.

In the single-XID case, heap_update() carries the old xmax to the new
tuple only as HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY, with no
HEAP_KEYS_UPDATED bit. That is useful for preserving a remote key-share
locker, but it is redundant for a self-owned lock.

The new tuple's xmin is the updating XID, so other transactions cannot
treat the new tuple as a visible committed tuple while the update is in
progress, and the self-owned lock ends when that same transaction ends.

Treat such a same-XID lock-only xmax as having no remaining locker to
carry to the new tuple. Remote lockers, different subtransactions, and
MultiXacts keep the existing behavior.

Add pageinspect coverage for a repeated subtransaction pattern that locks
and updates the same tuple.

Discussion: https://postgr.es/m/20190724232439.lpxzjw2jg3ukgcqn@alap3.anarazel.de
---
contrib/pageinspect/expected/page.out | 62 ++++++++++++++++++++++++++
contrib/pageinspect/sql/page.sql | 57 +++++++++++++++++++++++
src/backend/access/heap/README.tuplock | 38 ++++++++++++++++
src/backend/access/heap/heapam.c | 13 ++++++
4 files changed, 170 insertions(+)

diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index fcf19c5ca5a..266810774d5 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -161,6 +161,68 @@ SELECT unnest(combined_flags)
HEAP_XMIN_FROZEN
(3 rows)

+-- Repeated subtransactions need not carry a lock held only by the updating
+-- XID forward to each new tuple version.
+CREATE TABLE test_lock_carry_counter (
+ tenant_id text NOT NULL,
+ counter_name text NOT NULL,
+ counter_value bigint,
+ PRIMARY KEY (tenant_id, counter_name)
+) WITH (autovacuum_enabled = off);
+INSERT INTO test_lock_carry_counter VALUES ('t1', 'main', 1);
+CREATE FUNCTION test_lock_carry_counter_bump(n integer) RETURNS void
+LANGUAGE plpgsql AS $$
+DECLARE
+ current_value bigint;
+BEGIN
+ FOR i IN 1..n LOOP
+ BEGIN
+ SELECT counter_value INTO current_value
+ FROM test_lock_carry_counter
+ WHERE tenant_id = 't1' AND counter_name = 'main'
+ FOR UPDATE;
+ UPDATE test_lock_carry_counter
+ SET counter_value = counter_value + 1
+ WHERE tenant_id = 't1' AND counter_name = 'main';
+ EXCEPTION WHEN OTHERS THEN
+ NULL;
+ END;
+ END LOOP;
+END;
+$$;
+BEGIN;
+SELECT test_lock_carry_counter_bump(2);
+ test_lock_carry_counter_bump
+------------------------------
+
+(1 row)
+
+WITH items AS (
+ SELECT lp, t_xmax, raw_flags
+ FROM heap_page_items(get_raw_page('test_lock_carry_counter', 0)),
+ LATERAL heap_tuple_infomask_flags(t_infomask, t_infomask2)
+ WHERE lp_flags = 1
+),
+tail AS (
+ SELECT t_xmax, raw_flags
+ FROM items
+ ORDER BY lp DESC
+ LIMIT 1
+)
+SELECT (SELECT count(*) FROM items) AS tuple_versions,
+ (SELECT bool_or(array_position(raw_flags, 'HEAP_XMAX_IS_MULTI') IS NOT NULL)
+ FROM items) AS any_multi,
+ t_xmax = '0'::xid AS tail_xmax_invalid,
+ array_position(raw_flags, 'HEAP_XMAX_INVALID') IS NOT NULL AS tail_has_invalid
+ FROM tail;
+ tuple_versions | any_multi | tail_xmax_invalid | tail_has_invalid
+----------------+-----------+-------------------+------------------
+ 3 | f | t | t
+(1 row)
+
+ROLLBACK;
+DROP FUNCTION test_lock_carry_counter_bump(integer);
+DROP TABLE test_lock_carry_counter;
-- no flags at all
SELECT * FROM heap_tuple_infomask_flags(0, 0);
raw_flags | combined_flags
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index c75fe1147f6..4e92488f9b2 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -57,6 +57,63 @@ SELECT unnest(raw_flags)
SELECT unnest(combined_flags)
FROM heap_tuple_infomask_flags(x'FFFF'::int, x'FFFF'::int) ORDER BY 1;

+-- Repeated subtransactions need not carry a lock held only by the updating
+-- XID forward to each new tuple version.
+CREATE TABLE test_lock_carry_counter (
+ tenant_id text NOT NULL,
+ counter_name text NOT NULL,
+ counter_value bigint,
+ PRIMARY KEY (tenant_id, counter_name)
+) WITH (autovacuum_enabled = off);
+
+INSERT INTO test_lock_carry_counter VALUES ('t1', 'main', 1);
+
+CREATE FUNCTION test_lock_carry_counter_bump(n integer) RETURNS void
+LANGUAGE plpgsql AS $$
+DECLARE
+ current_value bigint;
+BEGIN
+ FOR i IN 1..n LOOP
+ BEGIN
+ SELECT counter_value INTO current_value
+ FROM test_lock_carry_counter
+ WHERE tenant_id = 't1' AND counter_name = 'main'
+ FOR UPDATE;
+ UPDATE test_lock_carry_counter
+ SET counter_value = counter_value + 1
+ WHERE tenant_id = 't1' AND counter_name = 'main';
+ EXCEPTION WHEN OTHERS THEN
+ NULL;
+ END;
+ END LOOP;
+END;
+$$;
+
+BEGIN;
+SELECT test_lock_carry_counter_bump(2);
+WITH items AS (
+ SELECT lp, t_xmax, raw_flags
+ FROM heap_page_items(get_raw_page('test_lock_carry_counter', 0)),
+ LATERAL heap_tuple_infomask_flags(t_infomask, t_infomask2)
+ WHERE lp_flags = 1
+),
+tail AS (
+ SELECT t_xmax, raw_flags
+ FROM items
+ ORDER BY lp DESC
+ LIMIT 1
+)
+SELECT (SELECT count(*) FROM items) AS tuple_versions,
+ (SELECT bool_or(array_position(raw_flags, 'HEAP_XMAX_IS_MULTI') IS NOT NULL)
+ FROM items) AS any_multi,
+ t_xmax = '0'::xid AS tail_xmax_invalid,
+ array_position(raw_flags, 'HEAP_XMAX_INVALID') IS NOT NULL AS tail_has_invalid
+ FROM tail;
+ROLLBACK;
+
+DROP FUNCTION test_lock_carry_counter_bump(integer);
+DROP TABLE test_lock_carry_counter;
+
-- no flags at all
SELECT * FROM heap_tuple_infomask_flags(0, 0);
-- no combined flags
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 16f7d78b7d2..6d14d9649b5 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -73,6 +73,44 @@ in the tuple itself. We do this by storing the locker's Xid in XMAX, and
setting infomask bits specifying the locking strength. See "Infomask Bits"
below for details on the bit patterns we use.

+Carrying locks across UPDATE
+----------------------------
+
+When UPDATE creates a new tuple version, it normally carries pre-existing
+lockers from the old tuple to the new tuple. That is necessary when another
+transaction has locked the old tuple, since the lock must continue to protect
+the updated version. This is essential for referential-integrity checks that
+use FOR KEY SHARE locks.
+
+There is a narrow exception when the old tuple's XMAX is a single XID that is
+LOCK_ONLY, and that XID is the same XID that is now updating the tuple. In
+that case, there is no independent locker to carry forward.
+
+For a single-XID XMAX carried to the new tuple, heap_update() does not preserve
+the old tuple lock's original strength. The carried state is encoded as
+HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY, with no HEAP_KEYS_UPDATED bit.
+That representation is enough for the case that needs carrying: a different
+transaction's FOR KEY SHARE lock, such as one taken by a referential-integrity
+check. It is not a mechanism for preserving a self-held FOR UPDATE or FOR NO
+KEY UPDATE lock on the new tuple version.
+
+The same-XID case is redundant for a simpler reason. The new tuple's XMIN is
+the updating XID, so other transactions cannot treat the new version as a
+visible committed tuple while the update is in progress. Once the updating
+transaction ends, any self-owned tuple lock ends with it. A self-owned
+KEYSHR_LOCK | LOCK_ONLY marker carried to the new tuple therefore has no
+remaining concurrency or referential-integrity role.
+
+The old tuple still records the update in its XMAX. If the old locker and the
+updater are the same XID, compute_new_xmax_infomask() can fold those states
+while preparing the old tuple's XMAX, but the safety of dropping the carried
+new-tuple XMAX does not depend on comparing the old lock strength with the
+UPDATE lock strength.
+
+This exception does not apply when the old locker is another transaction, a
+different subtransaction of the current transaction, or a MultiXact. Those
+cases still need to preserve the lock information on the new tuple.
+
MultiXacts
----------

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 72d6541734c..135569eaa63 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3628,6 +3628,19 @@ l2:
TransactionIdDidAbort(update_xact))
can_continue = true;
}
+ else if (HEAP_XMAX_IS_LOCKED_ONLY(infomask) &&
+ TransactionIdEquals(xwait, xid))
+ {
+ /*
+ * At this point Xmax is known not to be a MultiXactId. The tuple
+ * is locked only by the same XID that is now updating it, so
+ * there is no independent locker to carry forward to the new
+ * tuple. The update itself is recorded in the old tuple's Xmax.
+ */
+ checked_lockers = true;
+ locker_remains = false;
+ can_continue = true;
+ }
else if (TransactionIdIsCurrentTransactionId(xwait))
{
/*
--
2.43.0