On Thu, Aug 4, 2022 at 1:05 PM Amit Langote <amitlangot...@gmail.com> wrote: > On Wed, Jul 13, 2022 at 8:59 PM Amit Langote <amitlangot...@gmail.com> wrote: > > That bit came in to make DETACH CONCURRENTLY produce sane answers for > > RI queries in some cases. > > > > I guess my comment should really have said something like: > > > > HACK: find_inheritance_children_extended() has a hack that assumes > > that the queries originating in this module push the latest snapshot > > in transaction-snapshot mode. > > Posting a new version with this bit fixed; cfbot complained that 0002 > needed a rebase over 3592e0ff98. > > I will try to come up with a patch to enhance the PartitionDirectory > interface to allow passing the snapshot to use when scanning > pg_inherits explicitly, so we won't need the above "hack".
Sorry about the delay. So I came up with such a patch that is attached as 0003. The main problem I want to fix with it is the need for RI_FKey_check() to "force"-push the latest snapshot that the PartitionDesc code wants to use to correctly include or omit a detach-pending partition from the view of that function's RI query. Scribbling on ActiveSnapshot that way means that *all* scans involved in the execution of that query now see a snapshot that they shouldn't likely be seeing; a bug resulting from this has been demonstrated in a test case added by the commit 00cb86e75d. The fix is to make RI_FKey_check(), or really its RI_Plan's execution function ri_LookupKeyInPkRel() added by patch 0002, pass the latest snapshot explicitly as a parameter of PartitionDirectoryLookup(), which passes it down to the PartitionDesc code. No need to manipulate ActiveSnapshot. The actual fix is in patch 0004, which I extracted out of 0002 to keep the latter a mere refactoring patch without any semantic changes (though a bit more on that below). BTW, I don't know of a way to back-patch a fix like this for the bug, because there is no way other than ActiveSnapshot to pass the desired snapshot to the PartitionDesc code if the only way we get to that code is by executing an SQL query plan. 0003 moves the relevant logic out of find_inheritance_children_extended() into its callers. The logic of deciding which snapshot to use to determine if a detach-pending partition should indeed be omitted from the consideration of a caller based on the result of checking the visibility of the corresponding pg_inherits row with the snapshot; it just uses ActiveSnapshot now. Given the problems with using ActiveSnapshot mentioned above, I think it is better to make the callers decide the snapshot and pass it using a parameter named omit_detached_snapshot. Only PartitionDesc code actually cares about sending anything but the parent query's ActiveSnapshot, so the PartitionDesc and PartitionDirectory interface has been changed to add the same omit_detached_snapshot parameter. find_inheritance_children(), the other caller used in many sites that look at a table's partitions, defaults to using ActiveSnapshot, which does not seem problematic. Furthermore, only RI_FKey_check() needs to pass anything other than ActiveSnapshot, so other users of PartitionDesc, like user queries, still default to using the ActiveSnapshot, which doesn't have any known problems either. 0001 and 0002 are mostly unchanged in this version, except I took out the visibility bug-fix from 0002 into 0004 described above, which looks better using the interface added by 0003 anyway. I need to address the main concern that it's still hard to be sure that the patch in its current form doesn't break any user-level semantics of these RI check triggers and other concerns about the implementation that Robert expressed in [1]. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/message-id/CA%2BTgmoaiTNj4DgQy42OT9JmTTP1NWcMV%2Bke0i%3D%2Ba7%3DVgnzqGXw%40mail.gmail.com
v4-0002-Avoid-using-an-SQL-query-for-some-RI-checks.patch
Description: Binary data
v4-0004-Teach-ri_LookupKeyInPkRel-to-pass-omit_detached_s.patch
Description: Binary data
v4-0003-Make-omit_detached-logic-independent-of-ActiveSna.patch
Description: Binary data
v4-0001-Avoid-using-SPI-in-RI-trigger-functions.patch
Description: Binary data
