On Mon, Jul 6, 2026 at 5:15 PM Ilmar Yunusov <[email protected]> wrote: > The following review has been posted through the commitfest application: > make installcheck-world: not tested > Implements feature: tested, failed > Spec compliant: not tested > Documentation: not tested > > Hi, > > Thanks for clarifying the prepared UPDATE behavior. My DML check now makes > sense to me: in v13 the reduced locking applies to the SELECT portal path, and > actual UPDATE/DELETE execution is still outside that path. > > I looked separately at the current CFBot Windows MinGW failure for v13. > > The failure is in test_plan_advice/001_replan_regress, while running the core > graph_table_rls test under pg_plan_advice. The diff shows: > > ERROR: plancache reference 000002ef2f2d77b0 is not owned by resource owner > Portal > > I could not reproduce that locally on Linux. I used the same v13 series on > origin/master at e994f956e4864f424320f5243b9af11e173ad398. The series applied > cleanly with git am, and git diff --check reported no issues. > > This passed: > > ./configure --prefix="$PWD/pg-install" --without-readline --without-zlib > --without-icu --enable-tap-tests > make -s -j$(nproc) > make -s install > make -C src/test/regress check > make -C src/test/modules/test_plan_advice check > > I also repeated test_plan_advice with a cassert/debug build: > > ./configure --prefix="$PWD/pg-install" --without-readline --without-zlib > --without-icu --enable-tap-tests --enable-cassert --enable-debug CFLAGS="-O0 > -g" > make -s clean > make -s -j$(nproc) > make -s install > make -C src/test/modules/test_plan_advice check > > That passed too. > > Even though I cannot reproduce the MinGW failure locally, the error looks like > it may point at a real ownership mismatch in the new portal replan path. > > For a SQL EXECUTE portal, ExecuteQuery() gets the plan with: > > GetCachedPlan(entry->plansource, paramLI, NULL, NULL) > > That matches the portal ownership model: PortalDefineQuery() stores the cplan, > and PortalReleaseCachedPlan() later releases the portal reference with: > > ReleaseCachedPlan(portal->cplan, NULL) > > exec_bind_message() appears to follow the same pattern. > > But the invalidation/retry path in PortalLockCachedPlan() does: > > ReleaseCachedPlan(portal->cplan, portal->resowner) > > and then reacquires the replacement plan with: > > GetCachedPlan(portal->plansource, > portal->portalParams, > portal->resowner, > portal->queryEnv) > > That looks inconsistent with the way the portal cached-plan reference was > originally acquired. If the plan was acquired with owner = NULL, then > releasing > it with portal->resowner seems like it would explain the CFBot error: the > Portal resource owner does not own that plancache reference. > > Am I reading that correctly? If so, should PortalLockCachedPlan() keep the > stored portal->cplan reference as a portal-owned reference in the retry path > too, by releasing and reacquiring it with owner = NULL, while still using > portal->resowner for the locks acquired during > ExecutorPrepAndLock()/AcquireExecutorLocks()?
Thanks for checking. I think you're right about the ownership mismatch in PortalLockCachedPlan(). v14 follows your suggestion: the portal's plancache reference is released and reacquired with owner = NULL, matching the original acquisition, while portal->resowner is used for the executor locks as before. I'll post v14 in this thread later this week. Could you hold the re-review until then? -- Thanks, Amit Langote
