Hi,
I would like to ask for comments on a possible plan-cache race involving
ATTACH PARTITION on a non-topmost partitioned table. This is an analysis
only; I do not yet have a deterministic reproducer or patch.
The scenario is on current master, and seems related to the behavior
discussed by commit 898e5e3290a ("Allow ATTACH PARTITION with only
ShareUpdateExclusiveLock").
Consider this hierarchy:
root
`- mid (partition of root, itself partitioned)
|- m1
`- m2 (attached concurrently)
Root is queried by a prepared statement. While that statement is
building a generic plan, another session attaches m2 to mid.
Example setup:
CREATE TABLE root (a int) PARTITION BY RANGE (a);
CREATE TABLE mid (a int) PARTITION BY RANGE (a);
ALTER TABLE root ATTACH PARTITION mid
FOR VALUES FROM (1) TO (100);
CREATE TABLE m1 (a int);
ALTER TABLE mid ATTACH PARTITION m1
FOR VALUES FROM (1) TO (50);
CREATE TABLE m2 (a int);
INSERT INTO m2 VALUES (75);
PREPARE p AS SELECT count(*) FROM root;
Now suppose session A starts:
EXECUTE p;
and, while the planner is expanding root's hierarchy, session B commits:
ALTER TABLE mid ATTACH PARTITION m2
FOR VALUES FROM (50) TO (100);
ATTACH takes ShareUpdateExclusiveLock on mid and AccessExclusiveLock on
m2. Those locks do not conflict with the AccessShareLocks already held
by session A on root, mid, and m1. Therefore the ATTACH can commit
inside the planning interval.
The possible interleaving is:
1. Session A's planner expands root and obtains a partition descriptor
for mid that contains m1 but not m2.
2. Session B commits the ATTACH. StorePartitionBound() sends a relcache
invalidation for mid, the direct parent.
3. Session A's planner continues and processes that invalidation while
doing subsequent catalog or relation accesses.
4. At that moment, the replacement CachedPlan is still being built inside
BuildCachedPlan(). In particular, it has not yet been assigned to
plansource->gplan.
5. PlanCacheRelCallback() has two relevant checks:
- The source-level dependency list, plansource->relationOids, comes
from the rewritten query tree. For this prepared statement it
contains root, but not mid.
- The plan-level dependency check examines
plansource->gplan->stmt_list and its PlannedStmt->relationOids.
The new plan would indeed have mid among its dependencies, because
relation RTEs created during partition expansion are retained for
plan dependency tracking. However, plansource->gplan is still NULL
while the new generic plan is being built, so the callback cannot
mark that plan invalid.
Consequently, the invalidation for mid can be consumed without marking
either the plansource or the new plan invalid. Once BuildCachedPlan()
returns, GetCachedPlan() links the plan as the generic plan. I do not
see a post-build recheck for invalidations that arrived during plan
construction.
The resulting generic plan may therefore contain only m1, be linked with
is_valid = true, and continue to be reused by subsequent EXECUTEs of the
same prepared statement. Those executions would miss rows in m2 without
an error.
Normally, CheckCachedPlan() would acquire executor locks and process any
pending invalidations before accepting the generic plan. In this race,
however, the mid invalidation has already been processed during the
original build, so the later lock-and-recheck has nothing to observe.
Without another invalidation affecting root, mid, or m1, the stale plan
may remain valid. Operations on m2 alone do not help, because the stale
plan does not know that it depends on m2.
This appears to be a more severe variant of the behavior mentioned in
the commit message for 898e5e3290a:
invalidation messages will trigger replanning at some point.
Normally, this happens by the time the very next command is
executed, but if the next command acquires no locks and executes a
prepared query, it can manage not to notice until a new transaction
is started. We might want to tighten that up, but it's material for
a separate patch.
The commit also says a query that starts just before an ATTACH commits
may fail to see the new partition, which I understand to be an accepted
one-time visibility wart. The case I am worried about is different: the
invalidation arrives while the replacement plan is not yet reachable
from the plan cache, so the invalidation signal is lost and the stale
generic plan can survive beyond the first execution.
Relevant code paths are:
- StorePartitionBound() invalidates the direct parent only.
- PartitionDirectory ensures that one planning invocation sees a stable
partition descriptor, but it does not by itself make the eventual
CachedPlan invalid when a concurrent ATTACH commits during planning.
- setrefs.c records relation OIDs from the final range table, including
relation RTEs created during partition expansion. Thus the new plan
can know that it depends on mid.
- PlanCacheRelCallback() distinguishes source-level dependencies from
generic-plan dependencies.
- GetCachedPlan() links the newly built generic plan only after
BuildCachedPlan() returns.
One obvious mitigation would be to invalidate every ancestor of the
attached partition, up to and including root. That would make the
source-level dependencies of queries referencing any ancestor match, and
would force a stale generic plan to be dropped on the next execution.
It would not fix the first execution's old view. It would also turn an
ATTACH at any lower level into a broad invalidation of cached plans
referencing every ancestor, which may be a heavy false-positive cost.
Therefore, I am not sure it is the right solution.
My questions:
1. Does the plan-cache analysis above match the intended behavior, or is
there another protection that should make the newly built generic
plan invalid?
2. Is the possible loss of an invalidation during generic plan
construction considered an acceptable wart here, or is this something
the plan cache should handle more explicitly?
3. What would be the preferred design direction for closing this class
of race without reintroducing a parent-level AccessExclusiveLock for
ATTACH PARTITION?
Thanks,
nahaitao
