From 2abdbf2d3cecf193f9f6dbb154a1bf0c36afae04 Mon Sep 17 00:00:00 2001
From: amitlan <amitlangote09@gmail.com>
Date: Wed, 28 Sep 2022 16:37:55 +0900
Subject: [PATCH v7 4/4] Teach ri_LookupKeyInPkRel() to pass
 omit_detached_snapshot

Now that the RI triggers that need to look up PK rows in a
partitioned table can manipulate partitions directly through
ExecGetLeafPartitionForKey(), the snapshot being passed to omit or
include detach-pending partitions can also now be passed explicitly,
rather than using ActiveSnapshot for that purpose.

For the detach-pending partitions to be correctly omitted or included
from the consideration of PK row lookup, the PartitionDesc machinery
needs to see the latest snapshot.  Pushing the latest snapshot to be
the ActiveSnapshot as is done presently meant that even the scans that
should NOT be using the latest snapshot also end up using one to
time-qualify table/partition rows.  That led to incorrect results of
PK lookups over partitioned tables running under REPEATABLE READ
isolation; 00cb86e75d added a test that demonstrates this bug.

To fix, do not force-push the latest snapshot in the cases of PK
lookup over partitioned tables (as was being done by passing
detectNewRows=true to ri_PerformCheck()), but rather make
ri_LookupKeyInPkRel() pass the latest snapshot directly to
PartitionDirectoryLookup() through its new omit_detached_snapshot
parameter.

The buggy output in src/test/isolation/expected/fk-snapshot.out
of the relevant test case that was added by 00cb86e75d has been
changed to the correct output.
---
 src/backend/utils/adt/ri_triggers.c         | 18 ++++++------------
 src/test/isolation/expected/fk-snapshot.out |  4 ++--
 src/test/isolation/specs/fk-snapshot.spec   |  5 +----
 3 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index d7fa2f36ce..eb00125657 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -434,17 +434,11 @@ RI_FKey_check(TriggerData *trigdata)
 							 &qkey, fk_rel, pk_rel);
 	}
 
-	/*
-	 * Now check that foreign key exists in PK table
-	 *
-	 * XXX detectNewRows must be true when a partitioned table is on the
-	 * referenced side.  The reason is that our snapshot must be fresh in
-	 * order for the hack in find_inheritance_children() to work.
-	 */
+	/* Now check that foreign key exists in PK table */
 	ri_PerformCheck(riinfo, &qkey, qplan,
 					fk_rel, pk_rel,
 					NULL, newslot,
-					pk_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE,
+					false,
 					CMD_SELECT);
 
 	table_close(pk_rel, RowShareLock);
@@ -2715,16 +2709,16 @@ ri_LookupKeyInPkRel(struct RI_Plan *plan,
 		PartitionDirectory partdir;
 
 		/*
-		 * Note that this relies on the latest snapshot having been pushed by
-		 * the caller to be the ActiveSnapshot.  The PartitionDesc machinery
+		 * Pass the latest snapshot for omit_detached_snapshot so that any
+		 * detach-pending partitions are correctly omitted or included from
+		 * the considerations of this lookup.  The PartitionDesc machinery
 		 * that runs as part of this will need to use the snapshot to determine
 		 * whether to omit or include any detach-pending partition based on the
 		 * whether the pg_inherits row that marks it as detach-pending is
 		 * is visible to it or not, respectively.
 		 */
-		Assert(ActiveSnapshotSet());
 		partdir = CreatePartitionDirectory(CurrentMemoryContext,
-										   GetActiveSnapshot());
+										   GetLatestSnapshot());
 		leaf_pk_rel = ExecGetLeafPartitionForKey(partdir,
 												 pk_rel, riinfo->nkeys,
 												 riinfo->pk_attnums,
diff --git a/src/test/isolation/expected/fk-snapshot.out b/src/test/isolation/expected/fk-snapshot.out
index 5faf80d6ce..22752cc742 100644
--- a/src/test/isolation/expected/fk-snapshot.out
+++ b/src/test/isolation/expected/fk-snapshot.out
@@ -47,12 +47,12 @@ a
 
 step s2ifn2: INSERT INTO fk_noparted VALUES (2);
 step s2c: COMMIT;
+ERROR:  insert or update on table "fk_noparted" violates foreign key constraint "fk_noparted_a_fkey"
 step s2sfn: SELECT * FROM fk_noparted;
 a
 -
 1
-2
-(2 rows)
+(1 row)
 
 
 starting permutation: s1brc s2brc s2ip2 s1sp s2c s1sp s1ifp2 s2brc s2sfp s1c s1sfp s2ifn2 s2c s2sfn
diff --git a/src/test/isolation/specs/fk-snapshot.spec b/src/test/isolation/specs/fk-snapshot.spec
index 378507fbc3..64d27f29c3 100644
--- a/src/test/isolation/specs/fk-snapshot.spec
+++ b/src/test/isolation/specs/fk-snapshot.spec
@@ -46,10 +46,7 @@ step s2sfn	{ SELECT * FROM fk_noparted; }
 # inserting into referencing tables in transaction-snapshot mode
 # PK table is non-partitioned
 permutation s1brr s2brc s2ip2 s1sp s2c s1sp s1ifp2 s1c s1sfp
-# PK table is partitioned: buggy, because s2's serialization transaction can
-# see the uncommitted row thanks to the latest snapshot taken for
-# partition lookup to work correctly also ends up getting used by the PK index
-# scan
+# PK table is partitioned
 permutation s2ip2 s2brr s1brc s1ifp2 s2sfp s1c s2sfp s2ifn2 s2c s2sfn
 
 # inserting into referencing tables in up-to-date snapshot mode
-- 
2.35.3

