Re: Error "initial slot snapshot too large" in create replication slot

At Mon, 12 Sep 2022 14:51:56 -0700, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> Thanks for working on this!
> 
> 
> I think this should include a test that fails without this change and succeeds
> with it...
> 
> 
> On 2022-07-19 11:55:06 +0900, Kyotaro Horiguchi wrote:
> > From abcf0a0e0b3e2de9927d8943a3e3c145ab189508 Mon Sep 17 00:00:00 2001
> > From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> > Date: Tue, 19 Jul 2022 11:50:29 +0900
> > Subject: [PATCH v6] Create correct snapshot during CREATE_REPLICATION_SLOT
> 
> This sees a tad misleading - the previous snapshot wasn't borken, right?

I saw it kind of broken that ->xip contains sub transactions.  But I
didn't meant it's broken by "correct". Is "proper" suitable there?


> > +/*
> > + * ReorderBufferXidIsKnownSubXact
> > + *        Returns true if the xid is a known subtransaction.
> > + */
> > +bool
> > +ReorderBufferXidIsKnownSubXact(ReorderBuffer *rb, TransactionId xid)
> > +{
> > +    ReorderBufferTXN *txn;
> > +
> > +    txn = ReorderBufferTXNByXid(rb, xid, false,
> > +                                NULL, InvalidXLogRecPtr, false);
> > +
> > +    /* a known subtxn? */
> > +    if (txn && rbtxn_is_known_subxact(txn))
> > +        return true;
> > +
> > +    return false;
> > +}
> 
> The comments here just seem to restate the code....

Yeah, it is pulled from the existing code but result looks like so..

> It's not obvious to me that it's the right design (or even correct) to ask
> reorderbuffer about an xid being a subxid. Maybe I'm missing something, but
> why would reorderbuffer even be guaranteed to know about all these subxids?

I think you're missing that the code is visited only after the reorder
buffer's state becomes SNAPBUILD_CONSISTENT. I think
rbtxn_is_known_subxact() is reliable at that stage.

> > @@ -568,9 +571,17 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
> >
> >      MyProc->xmin = snap->xmin;
> >
> > -    /* allocate in transaction context */
> > +    /*
> > +     * Allocate in transaction context.
> > +     *
> > +     * We could use only subxip to store all xids (takenduringrecovery
> > +     * snapshot) but that causes useless visibility checks later so we hasle to
> > +     * create a normal snapshot.
> > +     */
> 
> I can't really parse this comment at this point, and I seriously doubt I could
> later on.

Mmm. The "takenduringrecovery" is relly perplexing (it has been
somehow lower-cased..), but after removing the parenthesized part, it
looks like this. And it had a misspelling but I removed that word.  Is
this still unreadable?

We could use only subxip to store all xids but that causes useless
visibility checks later so we create a normal snapshot.


> > @@ -591,12 +605,24 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
> >
> >          if (test == NULL)
> >          {
> > -            if (newxcnt >= GetMaxSnapshotXidCount())
> > -                ereport(ERROR,
> > -                        (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
> > -                         errmsg("initial slot snapshot too large")));
> > -
> > -            newxip[newxcnt++] = xid;
> > +            /* Store the xid to the appropriate xid array */
> > +            if (ReorderBufferXidIsKnownSubXact(builder->reorder, xid))
> > +            {
> > +                if (!overflowed)
> > +                {
> > +                    if (newsubxcnt >= GetMaxSnapshotSubxidCount())
> > +                        overflowed = true;
> > +                    else
> > +                        newsubxip[newsubxcnt++] = xid;
> > +                }
> > +            }
> > +            else
> > +            {
> > +                if (newxcnt >= GetMaxSnapshotXidCount())
> > +                    elog(ERROR,
> > +                         "too many transactions while creating snapshot");
> > +                newxip[newxcnt++] = xid;
> > +            }
> >          }
> 
> Hm, this is starting to be pretty deeply nested...

Yeah, at least one if() is removable.

> I wonder if a better fix here wouldn't be to allow importing a snapshot with a
> larger ->xid array. Yes, we can't do that in CurrentSnapshotData, but IIRC we
> need to be in a transactional snapshot anyway, which is copied anyway?

The other reason for oversized xip array is it causes visibility check
when it is used. AFAICS XidInMVCCSnapshot has additional path for
takenDuringRecovery snapshots that contains a linear search (currently
it is replaced by pg_lfind32()). This saves us from doing this for
that snapshot.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center