Amit Langote <amitlangot...@gmail.com> writes:
> [ v38 patchset ]
I spent a little bit of time looking through this, and concluded that
it's not something I will be wanting to push into v16 at this stage.
The patch doesn't seem very close to being committable on its own
terms, and even if it was now is not a great time in the dev cycle
to be making significant executor API changes. Too much risk of
having to thrash the API during beta, or even change it some more
in v17. I suggest that we push this forward to the next CF with the
hope of landing it early in v17.
A few concrete thoughts:
* I understand that your plan now is to acquire locks on all the
originally-named tables, then do permissions checks (which will
involve only those tables), then dynamically lock just inheritance and
partitioning child tables as we descend the plan tree. That seems
more or less okay to me, but it could be reflected better in the
structure of the patch perhaps.
* In particular I don't much like the "viewRelations" list, which
seems like a wart; those ought to be handled more nearly the same way
as other RTEs. (One concrete reason why is that this scheme is going
to result in locking views in a different order than they were locked
during original parsing, which perhaps could contribute to deadlocks.)
Maybe we should store an integer list of which RTIs need to be locked
in the early phase? Building that in the parser/rewriter would provide
a solid guide to the original locking order, so we'd be trivially sure
of duplicating that. (It might be close enough to follow the RT list
order, which is basically what AcquireExecutorLocks does today, but
this'd be more certain to do the right thing.) I'm less concerned
about lock order for child tables because those are just going to
follow the inheritance or partitioning structure.
* I don't understand the need for changes like this:
/* clean up tuple table */
- ExecClearTuple(node->ps.ps_ResultTupleSlot);
+ if (node->ps.ps_ResultTupleSlot)
+ ExecClearTuple(node->ps.ps_ResultTupleSlot);
ISTM that the process ought to involve taking a lock (if needed)
before we have built any execution state for a given plan node,
and if we find we have to fail, returning NULL instead of a
partially-valid planstate node. Otherwise, considerations of how
to handle partially-valid nodes are going to metastasize into all
sorts of places, almost certainly including EXPLAIN for instance.
I think we ought to be able to limit the damage to "parent nodes
might have NULL child links that you wouldn't have expected".
That wouldn't faze ExecEndNode at all, nor most other code.
* More attention is needed to comments. For example, in a couple of
places in plancache.c you have removed function header comments
defining API details and not replaced them with any info about the new
details, despite the fact that those details are more complex than the
old.
> It seems I hadn't noted in the ExecEndNode()'s comment that all node
> types' recursive subroutines need to handle the change made by this
> patch that the corresponding ExecInitNode() subroutine may now return
> early without having initialized all state struct fields.
> Also noted in the documentation for CustomScan and ForeignScan that
> the Begin*Scan callback may not have been called at all, so the
> End*Scan should handle that gracefully.
Yeah, I think we need to avoid adding such requirements. It's the
sort of thing that would far too easily get past developer testing
and only fail once in a blue moon in the field.
regards, tom lane
