Subject: Strange assertion in procarray.c

From: Michail Nikolaev <[email-masked]>
Date: Mon, 25 Nov 2024 20:38:00 +0100

Hello, everyone!

While working on stabilization of tests for [0] I noticed a strange assert
happens in procarray.c [1].

It looks like this:

        [562801][client backend] [isolation/two-ids/s2][50/131:536136]
ERROR:  could not serialize access due to read/write dependencies among
transactions
        [562801][client backend] [isolation/two-ids/s2][50/131:536136]
DETAIL:  Reason code: Canceled on identification as a pivot, during commit
attempt.
        [562801][client backend] [isolation/two-ids/s2][50/131:536136]
HINT:  The transaction might succeed if retried.
        [562801][client backend] [isolation/two-ids/s2][50/131:536136]
STATEMENT:  COMMIT;
        [562801][client backend] [isolation/two-ids/s2][50/0:536136] ERROR:
 ResourceOwnerEnlarge called after release started
        [562801][client backend] [isolation/two-ids/s2][50/0:536136]
WARNING:  AbortTransaction while in ABORT state
        TRAP: failed Assert("TransactionIdIsValid(proc->xid)"), File:
"../src/backend/storage/ipc/procarray.c", Line: 677, PID: 562801
        [562819][client backend] [pg_regress/test_parser][:0] LOG:
 disconnection: session time: 0:00:00.011 user=someone
database=regression_test_parser host=[local]
        postgres: someone isolation_regression [local]
COMMIT(ExceptionalCondition+0xbe)[0x55f2a101f185]
        postgres: someone isolation_regression [local]
COMMIT(ProcArrayEndTransaction+0x46)[0x55f2a0ddf7b3]
        postgres: someone isolation_regression [local]
COMMIT(+0x1e29b1)[0x55f2a09e59b1]
        postgres: someone isolation_regression [local]
COMMIT(+0x1e347b)[0x55f2a09e647b]
        postgres: someone isolation_regression [local]
COMMIT(AbortCurrentTransaction+0xe)[0x55f2a09e63a3]
        postgres: someone isolation_regression [local]
COMMIT(PostgresMain+0x538)[0x55f2a0e20ff1]
        postgres: someone isolation_regression [local]
COMMIT(+0x61457b)[0x55f2a0e1757b]
        postgres: someone isolation_regression [local]
COMMIT(postmaster_child_launch+0x137)[0x55f2a0d295bf]
        postgres: someone isolation_regression [local]
COMMIT(+0x52cff5)[0x55f2a0d2fff5]
        postgres: someone isolation_regression [local]
COMMIT(+0x52a6cd)[0x55f2a0d2d6cd]
        postgres: someone isolation_regression [local]
COMMIT(PostmasterMain+0x1629)[0x55f2a0d2cfae]
        postgres: someone isolation_regression [local]
COMMIT(+0x404ba2)[0x55f2a0c07ba2]
        /lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x7f6afbe7e1ca]

/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x7f6afbe7e28b]
        postgres: someone isolation_regression [local]
COMMIT(_start+0x25)[0x55f2a08e3ab5]

I made a reproducer for that. Ignore index_concurrently_upsert - it should
fail. Also, some source files are changed - but it is only injection points.
But in several cases of "meson test --print-errorlogs --num-processes=8
--setup running" build backend crashes. I was unable to reproduce it during
"non-running" tests.

A full backend log for the crash run is attached.

There are some helpful commands to reproduce locally:

        cd build
        ninja && meson test --suite setup
        cd ../
        export
LD_LIBRARY_PATH="$(pwd)/build/tmp_install/usr/local/pgsql/lib/x86_64-linux-gnu:$LD_LIBRARY_PATH"
        build/tmp_install/usr/local/pgsql/bin/initdb -N build/runningcheck
--no-instructions -A trust
        echo "include '$(pwd)/src/tools/ci/pg_ci_base.conf'" >>
build/runningcheck/postgresql.conf

        build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off'
-D build/runningcheck -l build/testrun/runningcheck.log start
        cd build
        meson test --print-errorlogs --num-processes=8 --setup running

Best regards,
Mikhail.

[0]: https://commitfest.postgresql.org/50/5160/
[1]:
https://github.com/postgres/postgres/blob/478846e7688c9ab73d2695a66822e9ae0574b551/src/backend/storage/ipc/procarray.c#L677
From 7ed9e2409d6b0fc4e4a915ba45232a10326c778b Mon Sep 17 00:00:00 2001
From: nkey <[email-masked]>
Date: Mon, 25 Nov 2024 19:13:40 +0100
Subject: [PATCH v1] meson test --print-errorlogs --num-processes=8 --setup
 running

reproducer for strange assert:

2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] ERROR:  could not serialize access due to read/write dependencies among transactions
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] DETAIL:  Reason code: Canceled on identification as a pivot, during commit attempt.
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] HINT:  The transaction might succeed if retried.
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] STATEMENT:  COMMIT;
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/0:536136] ERROR:  ResourceOwnerEnlarge called after release started
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/0:536136] WARNING:  AbortTransaction while in ABORT state
TRAP: failed Assert("TransactionIdIsValid(proc->xid)"), File: "../src/backend/storage/ipc/procarray.c", Line: 677, PID: 562801
2024-11-25 19:08:12.064 CET [562819][client backend] [pg_regress/test_parser][:0] LOG:  disconnection: session time: 0:00:00.011 user=nkey database=regression_test_parser host=[local]
postgres: nkey isolation_regression [local] COMMIT(ExceptionalCondition+0xbe)[0x55f2a101f185]
postgres: nkey isolation_regression [local] COMMIT(ProcArrayEndTransaction+0x46)[0x55f2a0ddf7b3]
postgres: nkey isolation_regression [local] COMMIT(+0x1e29b1)[0x55f2a09e59b1]
postgres: nkey isolation_regression [local] COMMIT(+0x1e347b)[0x55f2a09e647b]
postgres: nkey isolation_regression [local] COMMIT(AbortCurrentTransaction+0xe)[0x55f2a09e63a3]
postgres: nkey isolation_regression [local] COMMIT(PostgresMain+0x538)[0x55f2a0e20ff1]
postgres: nkey isolation_regression [local] COMMIT(+0x61457b)[0x55f2a0e1757b]
postgres: nkey isolation_regression [local] COMMIT(postmaster_child_launch+0x137)[0x55f2a0d295bf]
postgres: nkey isolation_regression [local] COMMIT(+0x52cff5)[0x55f2a0d2fff5]
postgres: nkey isolation_regression [local] COMMIT(+0x52a6cd)[0x55f2a0d2d6cd]
postgres: nkey isolation_regression [local] COMMIT(PostmasterMain+0x1629)[0x55f2a0d2cfae]
postgres: nkey isolation_regression [local] COMMIT(+0x404ba2)[0x55f2a0c07ba2]
/lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x7f6afbe7e1ca]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x7f6afbe7e28b]
postgres: nkey isolation_regression [local] COMMIT(_start+0x25)[0x55f2a08e3ab5]
---
 src/backend/commands/indexcmds.c              |  4 +-
 src/backend/executor/execIndexing.c           |  3 +
 src/backend/executor/nodeModifyTable.c        |  2 +
 src/backend/utils/time/snapmgr.c              |  2 +
 src/test/modules/injection_points/Makefile    |  2 +-
 .../expected/index_concurrently_upsert.out    | 80 +++++++++++++++++++
 src/test/modules/injection_points/meson.build |  1 +
 .../specs/index_concurrently_upsert.spec      | 68 ++++++++++++++++
 8 files changed, 160 insertions(+), 2 deletions(-)
 create mode 100644 src/test/modules/injection_points/expected/index_concurrently_upsert.out
 create mode 100644 src/test/modules/injection_points/specs/index_concurrently_upsert.spec

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d1134733c17..8f48f14eddd 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1766,6 +1766,7 @@ DefineIndex(Oid tableId,
 	 * before the reference snap was taken, we have to wait out any
 	 * transactions that might have older snapshots.
 	 */
+	INJECTION_POINT("define_index_before_set_valid");
 	pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 								 PROGRESS_CREATEIDX_PHASE_WAIT_3);
 	WaitForOlderSnapshots(limitXmin, true);
@@ -4206,7 +4207,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 	 * the same time to make sure we only get constraint violations from the
 	 * indexes with the correct names.
 	 */
-
+	INJECTION_POINT("reindex_relation_concurrently_before_swap");
 	StartTransactionCommand();
 
 	/*
@@ -4285,6 +4286,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 	 * index_drop() for more details.
 	 */
 
+	INJECTION_POINT("reindex_relation_concurrently_before_set_dead");
 	pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 								 PROGRESS_CREATEIDX_PHASE_WAIT_4);
 	WaitForLockersMultiple(lockTags, AccessExclusiveLock, true);
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index f9a2fac79e4..5d04f189340 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -117,6 +117,7 @@
 #include "utils/multirangetypes.h"
 #include "utils/rangetypes.h"
 #include "utils/snapmgr.h"
+#include "utils/injection_point.h"
 
 /* waitMode argument to check_exclusion_or_unique_constraint() */
 typedef enum
@@ -936,6 +937,8 @@ retry:
 	econtext->ecxt_scantuple = save_scantuple;
 
 	ExecDropSingleTupleTableSlot(existing_slot);
+	if (!conflict)
+		INJECTION_POINT("check_exclusion_or_unique_constraint_no_conflict");
 
 	return !conflict;
 }
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 1161520f76b..23cf4c6b540 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -69,6 +69,7 @@
 #include "utils/datum.h"
 #include "utils/rel.h"
 #include "utils/snapmgr.h"
+#include "utils/injection_point.h"

 typedef struct MTTargetRelLookup
@@ -1087,6 +1088,7 @@ ExecInsert(ModifyTableContext *context,
 					return NULL;
 				}
 			}
+			INJECTION_POINT("exec_insert_before_insert_speculative");
 
 			/*
 			 * Before we start insertion proper, acquire our "speculative
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 7d2b34d4f20..3a7357a050d 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -64,6 +64,7 @@
 #include "utils/resowner.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
+#include "utils/injection_point.h"

 /*
@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
 		pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
 		CatalogSnapshot = NULL;
 		SnapshotResetXmin();
+		INJECTION_POINT("invalidate_catalog_snapshot_end");
 	}
 }
 
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 0753a9df58c..3bde3e67503 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,7 @@ PGFILEDESC = "injection_points - facility for injection points"
 REGRESS = injection_points reindex_conc
 REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
 
-ISOLATION = basic inplace
+ISOLATION = basic inplace index_concurrently_upsert
 
 TAP_TESTS = 1
 
diff --git a/src/test/modules/injection_points/expected/index_concurrently_upsert.out b/src/test/modules/injection_points/expected/index_concurrently_upsert.out
new file mode 100644
index 00000000000..42dd059d985
--- /dev/null
+++ b/src/test/modules/injection_points/expected/index_concurrently_upsert.out
@@ -0,0 +1,80 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s3_start_create_index s1_start_upsert s4_wakeup_define_index_before_set_valid s2_start_upsert s4_wakeup_s1_from_invalidate_catalog_snapshot s4_wakeup_s2 s4_wakeup_s1
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+step s3_start_create_index: CREATE UNIQUE INDEX CONCURRENTLY tbl_pkey_duplicate ON test.tbl(i); <waiting ...>
+step s1_start_upsert: INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); <waiting ...>
+step s4_wakeup_define_index_before_set_valid: 
+	SELECT injection_points_detach('define_index_before_set_valid');
+	SELECT injection_points_wakeup('define_index_before_set_valid');
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+step s2_start_upsert: INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); <waiting ...>
+step s4_wakeup_s1_from_invalidate_catalog_snapshot: 
+	SELECT injection_points_detach('invalidate_catalog_snapshot_end');
+	SELECT injection_points_wakeup('invalidate_catalog_snapshot_end');
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+step s4_wakeup_s2: 
+	SELECT injection_points_detach('exec_insert_before_insert_speculative');
+	SELECT injection_points_wakeup('exec_insert_before_insert_speculative');
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+step s2_start_upsert: <... completed>
+step s4_wakeup_s1: 
+	SELECT injection_points_detach('check_exclusion_or_unique_constraint_no_conflict');
+	SELECT injection_points_wakeup('check_exclusion_or_unique_constraint_no_conflict');
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+step s3_start_create_index: <... completed>
+step s1_start_upsert: <... completed>
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f19001157..b50140bcf4a 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -44,6 +44,7 @@ tests += {
     'specs': [
       'basic',
       'inplace',
+      'index_concurrently_upsert',
     ],
   },
   'tap': {
diff --git a/src/test/modules/injection_points/specs/index_concurrently_upsert.spec b/src/test/modules/injection_points/specs/index_concurrently_upsert.spec
new file mode 100644
index 00000000000..075450935b6
--- /dev/null
+++ b/src/test/modules/injection_points/specs/index_concurrently_upsert.spec
@@ -0,0 +1,68 @@
+# Test race conditions involving:
+# - s1: UPSERT a tuple
+# - s2: UPSERT the same tuple
+# - s3: CREATE UNIQUE INDEX CONCURRENTLY
+# - s4: operations with injection points
+
+setup
+{
+	CREATE EXTENSION injection_points;
+	CREATE SCHEMA test;
+	CREATE UNLOGGED TABLE test.tbl(i int primary key, updated_at timestamp);
+	ALTER TABLE test.tbl SET (parallel_workers=0);
+}
+
+teardown
+{
+	DROP SCHEMA test CASCADE;
+	DROP EXTENSION injection_points;
+}
+
+session s1
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('check_exclusion_or_unique_constraint_no_conflict', 'wait');
+	SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait');
+}
+step s1_start_upsert	{ INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); }
+
+session s2
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('exec_insert_before_insert_speculative', 'wait');
+}
+step s2_start_upsert	{ INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); }
+
+session s3
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('define_index_before_set_valid', 'wait');
+}
+step s3_start_create_index		{ CREATE UNIQUE INDEX CONCURRENTLY tbl_pkey_duplicate ON test.tbl(i); }
+
+session s4
+step s4_wakeup_s1		{
+	SELECT injection_points_detach('check_exclusion_or_unique_constraint_no_conflict');
+	SELECT injection_points_wakeup('check_exclusion_or_unique_constraint_no_conflict');
+}
+step s4_wakeup_s1_from_invalidate_catalog_snapshot	{
+	SELECT injection_points_detach('invalidate_catalog_snapshot_end');
+	SELECT injection_points_wakeup('invalidate_catalog_snapshot_end');
+}
+step s4_wakeup_s2		{
+	SELECT injection_points_detach('exec_insert_before_insert_speculative');
+	SELECT injection_points_wakeup('exec_insert_before_insert_speculative');
+}
+step s4_wakeup_define_index_before_set_valid	{
+	SELECT injection_points_detach('define_index_before_set_valid');
+	SELECT injection_points_wakeup('define_index_before_set_valid');
+}
+
+permutation
+	s3_start_create_index
+	s1_start_upsert
+	s4_wakeup_define_index_before_set_valid
+	s2_start_upsert
+	s4_wakeup_s1_from_invalidate_catalog_snapshot
+	s4_wakeup_s2
+	s4_wakeup_s1
\ No newline at end of file
-- 
2.43.0

---

From: Michail Nikolaev <[email-masked]>
Date: Wed, 27 Nov 2024 03:27:00 +0100

Hello, Nathan and Michael!

I believe I’ve identified the root cause of the issue. It appears to be
related to the GetNamedDSMSegment and injection_points module.

Here’s how it happens:

* A backend attaches (even locally!) to an injection point, which might get
triggered during resource release (ResourceOwnerRelease).
* Another backend attempts to release the same resource (e.g., by aborting
a transaction) and triggers the injection point.
* This leads to a call to GetNamedDSMSegment, as it’s the first time this
backend interacts with injection points.
* Consequently, an assertion failure occurs in ResourceOwnerEnlarge because
the backend is in the process of releasing all resources.

I’ve attached a reproducer—this version is much simpler for debugging.

So, it looks like we need to provide some option to guarantee
GetNamedDSMSegment called for injection points module.

Any other ideas?

Best regards,
Mikhail.

[0]:
https://github.com/postgres/postgres/commit/8b2bcf3f287c79eaebf724cba57e5ff664b01e06
From e56b6de29aea01916d35d22e9a59241a04202b37 Mon Sep 17 00:00:00 2001
From: nkey <[email-masked]>
Date: Wed, 27 Nov 2024 02:10:17 +0100
Subject: [PATCH v2] Test to reproduce issue with crash caused by passing throw
 injection points during transaction aborting (caused by call to
 injection_init_shmem).

---
 src/backend/utils/time/snapmgr.c              |  2 +
 src/test/modules/injection_points/Makefile    |  2 +-
 .../injection_points/expected/crash.out       | 26 ++++++++++
 src/test/modules/injection_points/meson.build |  1 +
 .../modules/injection_points/specs/crash.spec | 47 +++++++++++++++++++
 5 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 src/test/modules/injection_points/expected/crash.out
 create mode 100644 src/test/modules/injection_points/specs/crash.spec

 /*
 
index 0753a9df58c..da8930ea49f 100644
 
-ISOLATION = basic inplace
+ISOLATION = basic crash inplace
 
 TAP_TESTS = 1
 
diff --git a/src/test/modules/injection_points/expected/crash.out b/src/test/modules/injection_points/expected/crash.out
new file mode 100644
index 00000000000..7d7f298c786
--- /dev/null
+++ b/src/test/modules/injection_points/expected/crash.out
@@ -0,0 +1,26 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s4_attach_locally wx1 rxwy2 c1 ry3 c2 c3
+injection_points_set_local
+--------------------------
+                          
+(1 row)
+
+step s4_attach_locally: SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait');
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+step wx1: update D1 set id = id + 1;
+step rxwy2: update D2 set id = (select id+1 from D1);
+step c1: COMMIT;
+step ry3: select id from D2;
+id
+--
+ 1
+(1 row)
+
+step c2: COMMIT;
+ERROR:  could not serialize access due to read/write dependencies among transactions
+step c3: COMMIT;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f19001157..6079187dd57 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -43,6 +43,7 @@ tests += {
   'isolation': {
     'specs': [
       'basic',
+      'crash',
       'inplace',
     ],
   },
diff --git a/src/test/modules/injection_points/specs/crash.spec b/src/test/modules/injection_points/specs/crash.spec
new file mode 100644
index 00000000000..55e6a5434ab
--- /dev/null
+++ b/src/test/modules/injection_points/specs/crash.spec
@@ -0,0 +1,47 @@
+setup
+{
+ create table D1 (id int primary key not null);
+ create table D2 (id int primary key not null);
+ insert into D1 values (1);
+ insert into D2 values (1);
+
+ CREATE EXTENSION injection_points;
+}
+
+teardown
+{
+ DROP TABLE D1, D2;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup		{ BEGIN ISOLATION LEVEL SERIALIZABLE;}
+step wx1	{ update D1 set id = id + 1; }
+step c1		{ COMMIT; }
+
+session s2
+setup		{
+	BEGIN ISOLATION LEVEL SERIALIZABLE;
+}
+step rxwy2	{ update D2 set id = (select id+1 from D1); }
+step c2		{ COMMIT; }
+
+session s3
+setup		{ BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step ry3	{ select id from D2; }
+step c3		{ COMMIT; }
+
+session s4
+setup	{
+	SELECT injection_points_set_local();
+}
+step s4_attach_locally	{ SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait'); }
+
+permutation
+	s4_attach_locally
+	wx1
+	rxwy2
+	c1
+	ry3
+	c2
+	c3
\ No newline at end of file
-- 
2.43.0

---

From: Michail Nikolaev <[email-masked]>
Date: Wed, 27 Nov 2024 10:50:15 +0100

Hello, again.

> Another backend attempts to release the same resource (e.g., by aborting
a transaction) and triggers the injection point.
Oh, all that GPT-like correctors required to be carefully checked :)

Correct version: Another backend attempts to release some resource (e.g.,
by aborting a transaction) and triggers the injection point.

Best regards,
Mikhail.

---

From: Michail Nikolaev <[email-masked]>
Date: Thu, 28 Nov 2024 21:04:36 +0100

Hello, Heikki, Nathan and Michael!

Oh, please excuse my impudence in bringing you all here, but I finally
found what almost the same issue was fixed by Heikki already [0].

I discovered that a similar issue was previously addressed by Heikki in
commit [0], where installcheck was disabled for injection point tests.
However, in the meson build configuration, this was only applied to
regression tests - the isolation and TAP tests are still running during
installcheck.

As demonstrated in the previously shared reproducer [1], even *local*
injection points can cause backend crashes through unexpected side effects.
Therefore, I propose extending the installcheck disable to cover both TAP
and isolation tests as well.

I've attached a patch implementing these changes.

A patch with such change is attached.

Best regards,
Mikhail.

[0]:
https://github.com/postgres/postgres/commit/e2e3b8ae9ed73fcd3096c5ca93971891a7767388
[1]:
https://www.postgresql.org/message-id/flat/CANtu0ojbx6%3DesP8euQgzD1CN6tigTQvDmupwEmLTHZT%3D6_yx_A%40mail.gmail.com#18544d553544da67b4fc1ef764df3c3d

---

 /*

 
 TAP_TESTS = 1

---

From: Michael Paquier <[email-masked]>
Date: Fri, 29 Nov 2024 14:06:36 +0900

> regression tests - the isolation and TAP tests are still running during

I fail to see how this is related?  The original issue was that this
was impossible to run safely concurrently, but now we have the
facilities able to do so.  There are a few cases where using a wait
point has limits, for example outside a transaction context for some
of the processes, but that has not really been an issue up to now.

@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
         pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
         CatalogSnapshot = NULL;
         SnapshotResetXmin();
+        INJECTION_POINT("invalidate_catalog_snapshot_end");
[...]
+step s4_attach_locally	{ SELECT
injection_points_attach('invalidate_catalog_snapshot_end', 'wait'); }
[...]
#4  0x0000563f09d22f39 in ExceptionalCondition
(conditionName=0x563f0a072d00 
"TransactionIdIsValid(proc->xid)", fileName=0x563f0a072a4a
"procarray.c", lineNumber=677)     at assert.c:66
#5  0x0000563f096f0684 in ProcArrayEndTransaction
(proc=0x7fbd4d083ac0, latestXid=750) at procarray.c:677
#6  0x0000563f088c54f3 in AbortTransaction () at xact.c:2946
#7  0x0000563f088c758d in AbortCurrentTransactionInternal () at
#xact.c:3531
#8  0x0000563f088c72a6 in AbortCurrentTransaction () at xact.c:3449
#9  0x0000563f0979c0f7 in PostgresMain (dbname=0x563f0f128100
"isolation_regression", username=0x563f0f1280e8 "ioltas") at
postgres.c:4524
#10 0x0000563f0978a5e5 in BackendMain (startup_data=0x7ffdcf50cfa8 "",
startup_data_len=4) at backend_startup.c:107
#11 0x0000563f094d8613 in postmaster_child_launch
(child_type=B_BACKEND, child_slot=1,
startup_data=0x7ffdcf50cfa8 "", startup_data_len=4,
client_sock=0x7ffdcf50cfe0) 

Isn't that pointing to an actual bug with serializable transactions?

What you are telling here is that there is a race condition where it
is possible to trigger an assertion failure when finishing a session
while another one is waiting on an invalidation, if there's in the mix
a read/write dependency error.  Disabling the test hides the problem,
it does not fix it.  And we should do the latter, not the former.
--
Michael