On Tue, Apr 5, 2022 at 9:21 AM Noah Misch <n...@leadboat.com> wrote: > > On Mon, Apr 04, 2022 at 06:55:45PM +0900, Masahiko Sawada wrote: > > 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: > > > > 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? > > Something like this: > > select > attrelid::regclass, > attname, > array(select typname > from pg_type t join pg_attribute pa on t.oid = pa.atttypid > where pa.attrelid = a.attrelid and pa.attnum > 0 and pa.attnum < > a.attnum order by pa.attnum) AS types_before, > (select sum(attlen) > from pg_type t join pg_attribute pa on t.oid = pa.atttypid > where pa.attrelid = a.attrelid and pa.attnum > 0 and pa.attnum < a.attnum) > AS len_before > from pg_attribute a > join pg_class c on c.oid = attrelid > where attalign = 'd' and relkind = 'r' and attnotnull and attlen <> -1 > order by attrelid::regclass::text, attnum; > attrelid │ attname │ types_before > │ len_before > ─────────────────┼──────────────┼─────────────────────────────────────────────┼──────────── > pg_sequence │ seqstart │ {oid,oid} > │ 8 > pg_sequence │ seqincrement │ {oid,oid,int8} > │ 16 > pg_sequence │ seqmax │ {oid,oid,int8,int8} > │ 24 > pg_sequence │ seqmin │ {oid,oid,int8,int8,int8} > │ 32 > pg_sequence │ seqcache │ {oid,oid,int8,int8,int8,int8} > │ 40 > pg_subscription │ subskiplsn │ {oid,oid,name,oid,bool,bool,bool,char,bool} > │ 81 > (6 rows) > > That doesn't count padding, but hazardous column changes will cause a diff in > the output.
Yes, in this case, we can detect the violated column order even without considering padding. On the other hand, I think this calculation could not detect some patterns of order. For instance, suppose the column order is {oid, bool, bool, oid, bool, bool, oid, int8}, the len_before is 16 but offset of int8 column including padding is 20 on ALIGNOF_DOUBLE==4 environment. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/