On Mon, Apr 4, 2022 at 3:26 PM Noah Misch <n...@leadboat.com> wrote: > > On Mon, Apr 04, 2022 at 08:20:08AM +0530, Amit Kapila wrote: > > On Mon, Apr 4, 2022 at 8:01 AM Noah Misch <n...@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 <n...@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.kapil...@gmail.com> wrote: > > > > > > > On Sat, Apr 2, 2022 at 1:43 PM Noah Misch <n...@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? > > I'd write it like this, though I'm not sure it's an improvement on your words: > > When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on > some of the C types that correspond to TYPALIGN_DOUBLE SQL types. To ensure > catalog C struct layout matches catalog tuple layout, arrange for the tuple > offset of each fixed-width, attalign='d' catalog column to be divisible by 8 > unconditionally. Keep such columns before the first NameData column of the > catalog, since packagers can override NAMEDATALEN to an odd number.
Thanks! > > The best place for such a comment would be in one of > src/test/regress/sql/*sanity*.sql, next to a test written to detect new > violations. Agreed. IIUC in the new test, we would need a new SQL function to calculate the offset of catalog columns including padding, is that right? Or do you have an idea to do that by using existing functionality? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
