Re: Eliminating SPI from RI triggers - take 2

On Thu, Sep 29, 2022 at 6:09 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Sep 29, 2022 at 4:43 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > 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.
>
> Here's another version that hopefully fixes the crash reported by
> Cirrus CI [1] that is not reliably reproducible.

And cfbot #1, which failed a bit after the above one, is not happy
with my failing to include utils/snapshot.h in a partdesc.h to which I
added:

@@ -65,9 +66,11 @@ typedef struct PartitionDescData

extern PartitionDesc RelationGetPartitionDesc(Relation rel, bool
omit_detached);
+extern PartitionDesc RelationGetPartitionDescExt(Relation rel, bool
omit_detached,
+ Snapshot
omit_detached_snapshot);

extern PartitionDirectory CreatePartitionDirectory(MemoryContext
mcxt, bool omit_detached);
-extern PartitionDesc PartitionDirectoryLookup(PartitionDirectory, Relation);
+extern PartitionDesc PartitionDirectoryLookup(PartitionDirectory,
Relation, Snapshot);

So, here's a final revision for today. Sorry for the noise.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com