Hi,
Ilmar asked in July whether he was reading the ownership mismatch
correctly, and the entry has been waiting since. He is, and the tree
says so in four places. On v13 applied to master:
prepare.c:198 ExecuteQuery
GetCachedPlan(entry->plansource, paramLI, NULL, NULL)
postgres.c:2055 exec_bind_message
GetCachedPlan(psrc, params, NULL, NULL)
portalmem.c PortalReleaseCachedPlan
ReleaseCachedPlan(portal->cplan, NULL)
pquery.c:1867 PortalLockCachedPlan (new in 0001)
ReleaseCachedPlan(portal->cplan, portal->resowner)
The portal's reference is acquired with a NULL owner on both entry
paths and released with a NULL owner at the end, so the retry path
releasing it against portal->resowner is releasing a reference that
resource owner never owned. That is the error the CFBot prints.
There is a second half to it that has not been mentioned: three lines
below, the replacement plan is acquired with
portal->cplan = GetCachedPlan(..., portal->resowner, ...)
and that reference is the one PortalReleaseCachedPlan() will later
release with NULL. So even if the first call did not error, the new
reference is registered with a resource owner that will not release
it, and released against an owner that does not hold it. The two calls
are wrong in opposite directions.
Attached is the one-line-each change: use NULL for both, leaving
ExecutorPrepAndLock() with portal->resowner, since the locks really do
belong to it.
I could not reproduce the failure on Linux either, and I think I can
say why. I put an elog at the top of that replan branch and ran:
- the test the CFBot fails on, test_plan_advice/001_replan_regress
- make check in full
- a deliberate setup: a prepared statement on a partitioned table at
its generic plan, a second session holding ACCESS EXCLUSIVE on the
partition the plan needs, the EXECUTE blocking on that lock, and
the other session doing ALTER TABLE on it before committing, so
that the invalidation lands while the portal is already open
The branch was not reached once, in any of them. So on Linux nothing
in the suite exercises that path, which also means the new replan
branch has no test coverage here. Whatever gets there on MinGW, it is
not something the tree currently runs.
With the change, make check, test_plan_advice and the isolation suite
all pass, though given the above that says less than I would like: it
shows nothing regressed, not that the path now works. If you have a
way to reach that branch I am happy to run it.
Regards,
Manu
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index a81a45700cc..fd7a9c14ba7 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1863,13 +1863,24 @@ PortalLockCachedPlan(Portal portal, bool do_prep,
else if (AcquireExecutorLocks(portal->cplan))
return false;
- /* Replan. Locks will be taken freshly. */
- ReleaseCachedPlan(portal->cplan, portal->resowner);
+ /*
+ * Replan. Locks will be taken freshly.
+ *
+ * The portal's reference to its cached plan is acquired with a NULL
+ * owner, both by ExecuteQuery() and by exec_bind_message(), and
+ * PortalReleaseCachedPlan() releases it the same way. Keep the
+ * replacement reference on that same footing: handing portal->resowner
+ * here would release a reference that resource owner never owned, and
+ * register the new one where nothing will release it. The locks taken
+ * below do belong to portal->resowner, which is why
ExecutorPrepAndLock()
+ * above still gets it.
+ */
+ ReleaseCachedPlan(portal->cplan, NULL);
portal->cplan = NULL;
portal->stmts = NIL;
portal->cplan = GetCachedPlan(portal->plansource,
portal->portalParams,
-
portal->resowner,
+ NULL,
portal->queryEnv);
portal->stmts = portal->cplan->stmt_list;
portal->strategy = ChoosePortalStrategy(portal->stmts);
