On Thu, Sep 29, 2022 at 1:46 PM Amit Langote <amitlangot...@gmail.com> wrote: > 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].
Oops, I apparently posted the wrong 0004, containing a bug that crashes `make check`. Fixed version attached. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
v5-0002-Avoid-using-an-SQL-query-for-some-RI-checks.patch
Description: Binary data
v5-0003-Make-omit_detached-logic-independent-of-ActiveSna.patch
Description: Binary data
v5-0004-Teach-ri_LookupKeyInPkRel-to-pass-omit_detached_s.patch
Description: Binary data
v5-0001-Avoid-using-SPI-in-RI-trigger-functions.patch
Description: Binary data
