Re: FailedAssertion("pd_idx == pinfo->nparts", File: "execPartition.c", Line: 1689)

Andy Fan <zhihui.fan1213@gmail.com> writes:
> Attached is the v2 patch.

Forgot to mention that I'd envisioned adding this as a src/test/modules/
module; contrib/ is for things that we intend to expose to users, which
I think this isn't.

I played around with this and got the isolation test I'd experimented
with yesterday to work with it.  If you revert 7a980dfc6 then the
attached patch will expose that bug.  Interestingly, I had to add an
explicit AcceptInvalidationMessages() call to reproduce the bug; so
apparently we do none of those between planning and execution in the
ordinary query code path.  Arguably, that means we're testing a scenario
somewhat different from what can happen in live databases, but I think
it's OK.  Amit's recipe for reproducing the bug works because there are
other relation lock acquisitions (and hence AcceptInvalidationMessages
calls) later in planning than where he asked us to wait.  So this
effectively tests a scenario where a very late A.I.M. call within the
planner detects an inval event for some already-planned relation, and
that seems like a valid-enough scenario.

Anyway, attached find a reviewed version of your patch plus a test
scenario contributed by me (I was too lazy to split it into two
patches).  Barring objections, I'll push this tomorrow or so.

(BTW, I checked and found that this test does *not* expose the problems
with Amit's original patch.  Not sure if it's worth trying to finagle
it so it does.)

            regards, tom lane

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 29de73c060..1428529b04 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global
 SUBDIRS = \
           brin \
           commit_ts \
+          delay_execution \
           dummy_index_am \
           dummy_seclabel \
           snapshot_too_old \
diff --git a/src/test/modules/delay_execution/.gitignore b/src/test/modules/delay_execution/.gitignore
new file mode 100644
index 0000000000..ba2160b66c
--- /dev/null
+++ b/src/test/modules/delay_execution/.gitignore
@@ -0,0 +1,3 @@
+# Generated subdirectories
+/output_iso/
+/tmp_check_iso/
diff --git a/src/test/modules/delay_execution/Makefile b/src/test/modules/delay_execution/Makefile
new file mode 100644
index 0000000000..f270aebf3a
--- /dev/null
+++ b/src/test/modules/delay_execution/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/delay_execution/Makefile
+
+PGFILEDESC = "delay_execution - allow delay between parsing and execution"
+
+MODULE_big = delay_execution
+OBJS = \
+    $(WIN32RES) \
+    delay_execution.o
+
+ISOLATION = partition-addition
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/delay_execution
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c
new file mode 100644
index 0000000000..03ea23d0f2
--- /dev/null
+++ b/src/test/modules/delay_execution/delay_execution.c
@@ -0,0 +1,104 @@
+/*-------------------------------------------------------------------------
+ *
+ * delay_execution.c
+ *        Test module to allow delay between parsing and execution of a query.
+ *
+ * The delay is implemented by taking and immediately releasing a specified
+ * advisory lock.  If another process has previously taken that lock, the
+ * current process will be blocked until the lock is released; otherwise,
+ * there's no effect.  This allows an isolationtester script to reliably
+ * test behaviors where some specified action happens in another backend
+ * between parsing and execution of any desired query.
+ *
+ * Copyright (c) 2020, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *      src/test/modules/delay_execution/delay_execution.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include <limits.h>
+
+#include "optimizer/planner.h"
+#include "utils/builtins.h"
+#include "utils/guc.h"
+#include "utils/inval.h"
+
+
+PG_MODULE_MAGIC;
+
+/* GUC: advisory lock ID to use.  Zero disables the feature. */
+static int    post_planning_lock_id = 0;
+
+/* Save previous planner hook user to be a good citizen */
+static planner_hook_type prev_planner_hook = NULL;
+
+/* Module load/unload functions */
+void        _PG_init(void);
+void        _PG_fini(void);
+
+
+/* planner_hook function to provide the desired delay */
+static PlannedStmt *
+delay_execution_planner(Query *parse, const char *query_string,
+                        int cursorOptions, ParamListInfo boundParams)
+{
+    PlannedStmt *result;
+
+    /* Invoke the planner, possibly via a previous hook user */
+    if (prev_planner_hook)
+        result = prev_planner_hook(parse, query_string, cursorOptions,
+                                   boundParams);
+    else
+        result = standard_planner(parse, query_string, cursorOptions,
+                                  boundParams);
+
+    /* If enabled, delay by taking and releasing the specified lock */
+    if (post_planning_lock_id != 0)
+    {
+        DirectFunctionCall1(pg_advisory_lock_int8,
+                            Int64GetDatum((int64) post_planning_lock_id));
+        DirectFunctionCall1(pg_advisory_unlock_int8,
+                            Int64GetDatum((int64) post_planning_lock_id));
+
+        /*
+         * Ensure that we notice any pending invalidations, since the advisory
+         * lock functions don't do this.
+         */
+        AcceptInvalidationMessages();
+    }
+
+    return result;
+}
+
+/* Module load function */
+void
+_PG_init(void)
+{
+    /* Set up the GUC to control which lock is used */
+    DefineCustomIntVariable("delay_execution.post_planning_lock_id",
+                            "Sets the advisory lock ID to be locked/unlocked after planning.",
+                            "Zero disables the delay.",
+                            &post_planning_lock_id,
+                            0,
+                            0, INT_MAX,
+                            PGC_USERSET,
+                            0,
+                            NULL,
+                            NULL,
+                            NULL);
+
+    /* Install our hook */
+    prev_planner_hook = planner_hook;
+    planner_hook = delay_execution_planner;
+}
+
+/* Module unload function (pro forma, not used currently) */
+void
+_PG_fini(void)
+{
+    planner_hook = prev_planner_hook;
+}
diff --git a/src/test/modules/delay_execution/expected/partition-addition.out
b/src/test/modules/delay_execution/expected/partition-addition.out
new file mode 100644
index 0000000000..7c91090eef
--- /dev/null
+++ b/src/test/modules/delay_execution/expected/partition-addition.out
@@ -0,0 +1,21 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2lock s1exec s2addp s2unlock
+step s2lock: SELECT pg_advisory_lock(12345);
+pg_advisory_lock
+
+
+step s1exec: LOAD 'delay_execution';
+          SET delay_execution.post_planning_lock_id = 12345;
+          SELECT * FROM foo WHERE a <> 1 AND a <> (SELECT 3); <waiting ...>
+step s2addp: CREATE TABLE foo2 (LIKE foo);
+          ALTER TABLE foo ATTACH PARTITION foo2 FOR VALUES IN (2);
+          INSERT INTO foo VALUES (2, 'ADD2');
+step s2unlock: SELECT pg_advisory_unlock(12345);
+pg_advisory_unlock
+
+t
+step s1exec: <... completed>
+a              b
+
+4              GHI
diff --git a/src/test/modules/delay_execution/specs/partition-addition.spec
b/src/test/modules/delay_execution/specs/partition-addition.spec
new file mode 100644
index 0000000000..2a0948247e
--- /dev/null
+++ b/src/test/modules/delay_execution/specs/partition-addition.spec
@@ -0,0 +1,38 @@
+# Test addition of a partition with less-than-exclusive locking.
+
+setup
+{
+  CREATE TABLE foo (a int, b text) PARTITION BY LIST(a);
+  CREATE TABLE foo1 PARTITION OF foo FOR VALUES IN (1);
+  CREATE TABLE foo3 PARTITION OF foo FOR VALUES IN (3);
+  CREATE TABLE foo4 PARTITION OF foo FOR VALUES IN (4);
+  INSERT INTO foo VALUES (1, 'ABC');
+  INSERT INTO foo VALUES (3, 'DEF');
+  INSERT INTO foo VALUES (4, 'GHI');
+}
+
+teardown
+{
+  DROP TABLE foo;
+}
+
+# The SELECT will be planned with just the three partitions shown above,
+# of which we expect foo1 to be pruned at planning and foo3 at execution.
+# Then we'll block, and by the time the query is actually executed,
+# partition foo2 will also exist.  We expect that not to be scanned.
+# This test is specifically designed to check ExecCreatePartitionPruneState's
+# code for matching up the partition lists in such cases.
+
+session "s1"
+step "s1exec"    { LOAD 'delay_execution';
+          SET delay_execution.post_planning_lock_id = 12345;
+          SELECT * FROM foo WHERE a <> 1 AND a <> (SELECT 3); }
+
+session "s2"
+step "s2lock"    { SELECT pg_advisory_lock(12345); }
+step "s2unlock"    { SELECT pg_advisory_unlock(12345); }
+step "s2addp"    { CREATE TABLE foo2 (LIKE foo);
+          ALTER TABLE foo ATTACH PARTITION foo2 FOR VALUES IN (2);
+          INSERT INTO foo VALUES (2, 'ADD2'); }
+
+permutation "s2lock" "s1exec" "s2addp" "s2unlock"