On 26/08/2024 03:58, Michael Paquier wrote:
> An interesting thing is that I have seen ubsan complain about this
> patch, due to the way WAL records xl_xact_commit are built with
> XACT_XINFO_HAS_DROPPED_STATS and parsed as xl_xact_stats_item requires
> an 8-byte alignment now (see pg_waldump TAP reports when using the
> attached), but we don't enforce anything as the data of such WAL
> records is added with a simple XLogRegisterData(), like:
> # xactdesc.c:91:28: runtime error: member access within misaligned
> address 0x5651e996b86c for type 'struct xl_xact_stats_items', which
> requires 8 byte alignment # 0x5651e996b86c: note: pointer points here
>
> TBH, I've looked at that for quite a bit, thinking about the addition
> of some "dummy" member to some of the parsed structures to force some
> padding, or play with the alignment macros, or for some alignment when
> inserting the record, or looked at pg_attribute_aligned().
>
> First I'm surprised that it did not show up as an issue yet in this
> area. Second, I could not get down to something "nice", but perhaps
> there are preferred approaches when it comes to that and somebody has
> a fancier idea? Or perhaps the problem is bigger than that due to
> the way the record is designed and built? It also feels that I'm
> missing something obvious, not sure what TBH. Still I'm OK to paint
> some more MAXALIGN()s to make sure that all these deparsing pointers
> have a correct alignment with some more TYPEALIGN()s or similar,
> because this deparsing stuff is about that, but I'm also wondering if
> there is an argument for forcing that for the record itself? I'll
> think more about that next week or so.
Currently, we rely on the fact that all the xl_xact_* structs require
sizeof(int) alignment. See comment above struct xl_xact_xinfo.
One idea is to store the uint64 as two uint32's.
--
Heikki Linnakangas
Neon (https://neon.tech)
