Re: Eliminating SPI from RI triggers - take 2

On Thu, Sep 29, 2022 at 1:46 PM Amit Langote <amitlangote09(at)gmail(dot)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