Re: generic plans and "initial" pruning

On Mon, Jul 6, 2026 at 5:15 PM Ilmar Yunusov <tanswis42@gmail.com> 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