Re: Eliminating SPI from RI triggers - take 2

On Sat, Jul 9, 2022 at 1:15 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jul 1, 2022 at 2:23 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > So, I hacked together a patch (attached 0001) that invents an "RI
> > plan" construct (struct RIPlan) to replace the use of an "SPI plan"
> > (struct _SPI_plan).
> >
> > With that in place, I decided to rebase my previous patch [1] to use
> > this new interface and the result is attached 0002.
>

Thanks for taking a look at this.  I'll try to respond to other points
in a separate email, but I wanted to clarify something about below:

> I find my ego slightly wounded by the comment that "the partition
> descriptor machinery has a hack that assumes that the queries
> originating in this module push the latest snapshot in the
> transaction-snapshot mode." It's true that the partition descriptor
> machinery gives different answers depending on the active snapshot,
> but, err, is that a hack, or just a perfectly reasonable design
> decision?

I think my calling it a hack of "partition descriptor machinery" is
not entirely fair (sorry), because it's talking about the following
comment in find_inheritance_children_extended(), which describes it as
being a hack, so I mentioned the word "hack" in my comment too:

        /*
         * Cope with partitions concurrently being detached.  When we see a
         * partition marked "detach pending", we omit it from the returned set
         * of visible partitions if caller requested that and the tuple's xmin
         * does not appear in progress to the active snapshot.  (If there's no
         * active snapshot set, that means we're not running a user query, so
         * it's OK to always include detached partitions in that case; if the
         * xmin is still running to the active snapshot, then the partition
         * has not been detached yet and so we include it.)
         *
         * The reason for this hack is that we want to avoid seeing the
         * partition as alive in RI queries during REPEATABLE READ or
         * SERIALIZABLE transactions: such queries use a different snapshot
         * than the one used by regular (user) queries.
         */

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.

> An alternative might be for PartitionDirectoryLookup to take
> a snapshot as an explicit argument rather than relying on the global
> variable to get that information from context. I generally feel that
> we rely too much on global variables where we should be passing around
> explicit parameters, so if you're just arguing that explicit
> parameters would be better here, then I agree and just didn't think of
> it. If you're arguing that making the answer depend on the snapshot is
> itself a bad idea, I don't agree with that.

No, I'm not arguing that using a snapshot there is wrong and haven't
really thought hard about an alternative.

I tend to agree passing a snapshot explicitly might be better than
using ActiveSnapshot stuff for this.

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