RE: Parallel INSERT SELECT take 2

On Friday, August 7, 2026 8:44 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> I've spent a bit more time looking at the patches, and I have three issues to
> report - two seem fairly simple, more like omissions. The third one may be a
> more fundamental issue.

Thanks for reviewing!

>
> 1) GENERATED STORED columns not checked - those are evaluated ad insert
> time, just like triggers, so I think we need to check them too. I assume this is
> mostly just an oversight, because GENERATED STORED columns are a new
> feature, introduced after the original patch was written.

Right, I missed this new feature. Added in the V2 patch set.

>
>
> 2) set_attnotnull has this new block of code:
...
>
> But why? I don't think changing 'NOT NULL' can alter parallel safety.
> Maybe the block belongs somewhere else, and just got lost during a rebase, or
> something like that? It actually mentions detaching the partition, so maybe
> that's where it should be? But AFAIK that path already invalidates the flag in
> DetachPartitionFinalize.

Oh, this is a mis-rebased chunk, it should be removed. Sorry for the noise.

> 3) The more fundamental issue is that we can easily end up with stale value of
> the flag in relcache, if we get an invalidation while already calculating the
> flag.
>
> I speculated there may be such issues in my earlier message, but now I have
> an isolation test reproducing the issue - written with a bit of help from AI, but I
> believe the issue is legit.
>
> The root cause is that when RelationGetParallelDmlSafety() calculates the
> hazard (and walks partitions, indexes etc), it locks them. But locking a relation
> accepts invalidation messages. So this can happen:
>
> - something triggers an invalidation of rd_paralleldml
> - we start recalculating it, and the correct result is 's'
> - half-way through, another ALTER TABLE makes it parallel unsafe, but it's on
> one of the objects (partitions, ...) we already processed
> - we accept the invalidation, but that just resets rd_paralleldml
> - RelationGetParallelDmlSafety finishes, updates rd_paralleldml, but it sets it
> to the *stale* value, because it did not recheck the modified objects (from the
> second invalidation)
>
> I think this might be fixed by having a separate "isvalid" flag, reset by the
> invalidation, and repeat RelationGetParallelDmlSafety until it stays 'true'. So
> something like this in pseudocode:

I agree. I fixed it in a similar way. I noticed other cache implementations
(e.g., SyncingRelationsState and EventTriggerCacheStateType) use a tri-state
approach for such cases, so I followed their style to make the code clearer and
more consistent.

> Speaking of this, isn't it a bit weird that ExecInitPartitionInfo now checks
> RelationGetParallelDmlSafety at runtime? AFAIK it can cause failures in
> already-running DML, and it's the only place doing this. Why should it not just
> trust the rd_paralleldml value in relcache when planning the query?

This check was added as a safety guard to catch unsafe objects added
concurrently to a partition during execution. But I think it's OK to
remove it, as such cases would be caught by the existing parallel mode checks
(for example, IsInParallelMode() checks like the one below):

if (IsInParallelMode())
ereport(ERROR,
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
errmsg("cannot delete tuples during a parallel operation")));

I have not removed it in this version, but can do it later if required.

Best Regards,
Zhijie Hou