Re: Patch to avoid orphaned dependencies
Hi, On Wed, Mar 23, 2022 at 12:49:06PM -0400, Tom Lane wrote: > 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). Thanks for the idea (and sorry for the delay replying to it)! I had a look at it and just created a new thread [1] based on your proposal. [1]: https://www.postgresql.org/message-id/flat/ZiYjn0eVc7pxVY45%40ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Patch to avoid orphaned dependencies
This entry has been waiting on author input for a while (our current threshold is roughly two weeks), so I've marked it Returned with Feedback. Once you think the patchset is ready for review again, you (or any interested party) can resurrect the patch entry by visiting https://commitfest.postgresql.org/38/3106/ and changing the status to "Needs Review", and then changing the status again to "Move to next CF". (Don't forget the second step; hopefully we will have streamlined this in the near future!) Thanks, --Jacob
Re: Patch to avoid orphaned dependencies
On Wed, Mar 23, 2022 at 12:49:06PM -0400, Tom Lane wrote: > Nathan Bossart 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. I marked the commitfest entry as waiting-on-author, set the target version to v16, and moved it to the next commitfest. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Patch to avoid orphaned dependencies
Nathan Bossart 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 DROPSession 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 DROPSession 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
Re: Patch to avoid orphaned dependencies
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? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Patch to avoid orphaned dependencies
Hi, For genam.c: + UseDirtyCatalogSnapshot = dirtysnap; + Does the old value of UseDirtyCatalogSnapshot need to be restored at the end of the func ? +systable_recheck_tuple(SysScanDesc sysscan, HeapTuple tup, bool dirtysnap) Considering that parameter dirtysnap is a bool, I think it should be named isdirtysnap so that its meaning can be distinguished from: + Snapshot dirtySnapshot; + UseDirtyCatalogSnapshot = true; + + dirtySnapshot = GetCatalogSnapshot(RelationGetRelid(*depRel)); I tend to think that passing usedirtysnap (bool parameter) to GetCatalogSnapshot() would be more flexible than setting global variable. Cheers
Re: Patch to avoid orphaned dependencies
Hi, On 2021-12-17 14:19:18 +0100, Drouvot, Bertrand wrote: > Please find enclosed v1-0003-orphaned-dependencies.patch, that contains: > > - a mandatory rebase > > - a few isolation tests added in src/test/modules/test_dependencies (but I'm > not sure at all that's the right place to add them, is it?) This fails on windows w/ msvc: https://cirrus-ci.com/task/5368174125252608?logs=configure#L102 https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.157904#L12 Greetings, Andres Freund
Re: Patch to avoid orphaned dependencies
> On 20 Sep 2021, at 12:50, Drouvot, Bertrand wrote: > > Hi Ronan, > > On 9/17/21 10:09 AM, Ronan Dunklau wrote: >> Hello Bertrand, >> >> Le mardi 4 mai 2021, 11:55:43 CEST Drouvot, Bertrand a écrit : >>> Implementation overview: >>> >>> * A new catalog snapshot is added: DirtyCatalogSnapshot. >>> * This catalog snapshot is a dirty one to be able to look for >>> in-flight dependencies. >>> * Its usage is controlled by a new UseDirtyCatalogSnapshot variable. >>> * Any time this variable is being set to true, then the next call to >>> GetNonHistoricCatalogSnapshot() is returning the dirty snapshot. >>> * This snapshot is being used to check for in-flight dependencies and >>> also to get the objects description to generate the error messages. >>> >> I quickly tested the patch, it behaves as advertised, and passes tests. > > Thanks for looking at it! > >> >> Isolation tests should be added to demonstrate the issues it is solving. > > Good point. I'll have a look. > >> >> However, I am bit wary of this behaviour of setting the DirtyCatalogSnapshot >> global variable which is then reset after each snapshot acquisition: I'm >> having trouble understanding all the implications of that, if it would be >> possible to introduce an unforeseen snapshot before the one we actually want >> to be dirty. > > I don't think that could be possible as long as: > > - this is a per backend variable > > - we pay attention where we set it to true > > But i might be missing something. > > Do you have any corner cases in mind? > >> I don't want to derail this thread, but couldn't predicate locks on the >> pg_depend index pages corresponding to the dropped object / referenced >> objects >> work as a different approach ? > > I'm fine to have a look at another approach if needed, but does it mean we > are not happy with the current approach proposal? This patch fails to apply as a whole, with the parts applying showing quite large offsets. Have you had the chance to look at the isolation test asked for above? -- Daniel Gustafsson https://vmware.com/
Re: Patch to avoid orphaned dependencies
Hello Bertrand, Le mardi 4 mai 2021, 11:55:43 CEST Drouvot, Bertrand a écrit : > > Implementation overview: > > * A new catalog snapshot is added: DirtyCatalogSnapshot. > * This catalog snapshot is a dirty one to be able to look for > in-flight dependencies. > * Its usage is controlled by a new UseDirtyCatalogSnapshot variable. > * Any time this variable is being set to true, then the next call to > GetNonHistoricCatalogSnapshot() is returning the dirty snapshot. > * This snapshot is being used to check for in-flight dependencies and > also to get the objects description to generate the error messages. > I quickly tested the patch, it behaves as advertised, and passes tests. Isolation tests should be added to demonstrate the issues it is solving. However, I am bit wary of this behaviour of setting the DirtyCatalogSnapshot global variable which is then reset after each snapshot acquisition: I'm having trouble understanding all the implications of that, if it would be possible to introduce an unforeseen snapshot before the one we actually want to be dirty. I don't want to derail this thread, but couldn't predicate locks on the pg_depend index pages corresponding to the dropped object / referenced objects work as a different approach ? I'm not familiar enough with them so maybe there is some fundamental misunderstanding on my end. -- Ronan Dunklau
