Nathan Bossart <nathandboss...@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