Re: generic plans and "initial" pruning
Ilmar Y <tanswis42@gmail.com>
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
Commits
-
Stamp 19beta1.
- 4b0bf0788b06 19 (unreleased) cited
-
Revert "Don't lock partitions pruned by initial pruning"
- 1722d5eb05d8 18.0 landed
-
Ensure first ModifyTable rel initialized if all are pruned
- 28317de723b6 18.0 cited
-
Fix bug in cbc127917 to handle nested Append correctly
- cbb9086c9ef6 18.0 landed
-
Remove unstable test suite added by 525392d57
- 4f1b6e5bb4fe 18.0 landed
-
Don't lock partitions pruned by initial pruning
- 525392d5727f 18.0 landed
-
Fix an oversight in cbc127917 to handle MERGE correctly
- 75dfde13639a 18.0 landed
-
Track unpruned relids to avoid processing pruned relations
- cbc127917e04 18.0 landed
-
Perform runtime initial pruning outside ExecInitNode()
- d47cbf474ecb 18.0 landed
-
Move PartitionPruneInfo out of plan nodes into PlannedStmt
- bb3ec16e14de 18.0 landed
-
Fix setrefs.c's failure to do expression processing on prune steps.
- bf826ea06297 18.0 cited
-
Remove obsolete executor cleanup code
- d060e921ea5a 17.0 landed
-
Revert "Move PartitionPruneInfo out of plan nodes into PlannedStmt"
- 5472743d9e85 16.0 landed
-
Move PartitioPruneInfo out of plan nodes into PlannedStmt
- ec386948948c 16.0 landed
-
Refactor and cleanup runtime partition prune code a little
- 297daa9d4353 15.0 landed
-
Remove some unnecessary fields from Plan trees.
- 52ed730d511b 12.0 cited
-
Remove more redundant relation locking during executor startup.
- f2343653f5b2 12.0 cited
-
Shut down Gather's children before shutting down Gather itself.
- acf555bc53ac 10.0 cited