Re: Parallel INSERT SELECT take 2

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.

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.

2) set_attnotnull has this new block of code:

/*
* The parallel DML safety hazard levels cached in relcache for the
* parent's ancestors, if any, were computed including the detached
* partition, so they must be invalidated as well. (The parent's own
* cached value is already discarded by the relcache invalidation above,
* so no message is needed for it.)
*/
CacheInvalidateParallelDmlSafetyForAncestors(RelationGetRelid(rel));

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.

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:

invalidation:

rd_paralleldml_isvalid = false

RelationGetParallelDmlSafety:

while (!rd_paralleldml_isvalid)
{
rd_paralleldml_isvalid = true;
... recalculate the rd_paralleldml ...
}

return rd_paralleldml;

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?

regards

--
Tomas Vondra