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()?
Regards,
Ilmar Yunusov
