Nathan Bossart <nathandbossart@gmail.com> writes:
> Bertand, do you think this has any chance of making it into v15? If not,
> are you alright with adjusting the commitfest entry to v16 and moving it to
> the next commitfest?
I looked this over briefly, and IMO it should have no chance of being
committed in anything like this form.
The lesser problem is that (as already noted) the reliance on a global
variable that changes catalog lookup semantics is just unbelievably
scary. An example of the possible consequences here is that a syscache
entry could get made while that's set, containing a row that we should
not be able to see yet, and indeed might never get committed at all.
Perhaps that could be addressed by abandoning the patch's ambition to tell
you the details of an uncommitted object (which would have race conditions
anyway), so that *only* reads of pg_[sh]depend itself need be dirty.
The bigger problem is that it fails to close the race condition that
it's intending to solve. This patch will catch a race like this:
Session doing DROP Session doing CREATE
DROP begins
CREATE makes a dependent catalog entry
DROP scans for dependent objects
CREATE commits
DROP removes catalog entry
DROP commits
However, it will not catch this slightly different timing:
Session doing DROP Session doing CREATE
DROP begins
DROP scans for dependent objects
CREATE makes a dependent catalog entry
CREATE commits
DROP removes catalog entry
DROP commits
So I don't see that we've moved the goalposts very far at all.
Realistically, if we want to prevent this type of problem, then all
creation DDL will have to take a lock on each referenced object that'd
conflict with a lock taken by DROP. This might not be out of reach:
I think we do already take such locks while dropping objects. The
reference-side lock could be taken by the recordDependency mechanism
itself, ensuring that we don't miss anything; and that would also
allow us to not bother taking such a lock on pinned objects, which'd
greatly cut the cost (though not to zero).
regards, tom lane
