Re: Eliminating SPI from RI triggers - take 2

On Thu, Aug 4, 2022 at 1:05 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Wed, Jul 13, 2022 at 8:59 PM Amit Langote <amitlangote09(at)gmail(dot)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