Re: Skipping logical replication transactions on subscriber side

On Mon, Apr 4, 2022 at 8:01 AM Noah Misch <noah@leadboat.com> wrote:
>
> On Mon, Apr 04, 2022 at 10:28:30AM +0900, Masahiko Sawada wrote:
> > On Sun, Apr 3, 2022 at 9:45 AM Noah Misch <noah@leadboat.com> wrote:
> > > On Sat, Apr 02, 2022 at 08:44:45PM +0900, Masahiko Sawada wrote:
> > > > On Sat, Apr 2, 2022 at 7:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > > On Sat, Apr 2, 2022 at 1:43 PM Noah Misch <noah@leadboat.com> wrote:
> > > > > > Some options:
> > > > > > - Move subskiplsn after subdbid, so it's always aligned anyway.  I've
> > > > > >   confirmed that this lets the test pass, in 44s.
>
> > --- a/src/include/catalog/pg_subscription.h
> > +++ b/src/include/catalog/pg_subscription.h
> > @@ -54,6 +54,17 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId) BKI_SHARED_RELATION BKI_ROW
> >
> >       Oid                     subdbid BKI_LOOKUP(pg_database);        /* Database the
> >                                                                                                        * subscription is in. */
> > +
> > +     /*
> > +      * All changes finished at this LSN are skipped.
> > +      *
> > +      * Note that XLogRecPtr, pg_lsn in the catalog, is 8-byte alignment
> > +      * (TYPALIGN_DOUBLE) and it does not match the alignment on some platforms
> > +      * such as AIX.  Therefore subskiplsn needs to be placed here so it is
> > +      * always aligned.
>
> I'm reading this comment as saying that TYPALIGN_DOUBLE is always 8 bytes, but
> the problem arises precisely because TYPALIGN_DOUBLE==4 on AIX.
>

How about a comment like: "It has to be kept at 8-byte alignment
boundary so as to be accessed directly via C struct as it uses
TYPALIGN_DOUBLE for storage which has 4-byte alignment on platforms
like AIX."? Can you please suggest a better comment if you don't like
this one?

> > +      */
> > +     XLogRecPtr      subskiplsn;
> > +
> >       NameData        subname;                /* Name of the subscription */
> >
> >       Oid                     subowner BKI_LOOKUP(pg_authid); /* Owner of the subscription */
> > @@ -71,9 +82,6 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId) BKI_SHARED_RELATION BKI_ROW
> >       bool            subdisableonerr;        /* True if a worker error should cause the
> >                                                                        * subscription to be disabled */
> >
> > -     XLogRecPtr      subskiplsn;             /* All changes finished at this LSN are
> > -                                                              * skipped */
>
> Some code sites list pg_subscription fields in field order.  Please update
> them so they continue to list fields in field order.  CreateSubscription() is
> one example.
>

Another minor point is that I think it is better to use DatumGetLSN to
read this in GetSubscription as we use LSNGetDatum while storing it. I
am not sure if there is any direct problem due to this but that looks
consistent to me.

-- 
With Regards,
Amit Kapila.