Re: generic plans and "initial" pruning

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