Thread: Re: Switch PgStat_HashKey.objoid from Oid to uint64

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)

Hi,

On Thu, Aug 29, 2024 at 08:56:59AM +0900, Michael Paquier wrote:
> On Mon, Aug 26, 2024 at 09:32:54AM +0300, Heikki Linnakangas wrote:
> > Currently, we rely on the fact that all the xl_xact_* structs require
> > sizeof(int) alignment. See comment above struct xl_xact_xinfo.
> 
> Thanks, I have missed this part.  So that explains the alignment I'd
> better use in the record.
> 
> > One idea is to store the uint64 as two uint32's.
> 
> Nice, we could just do that.  This idea makes me feel much better than
> sticking more aligment macros in the paths where the record is built.
> 
> Attached is an updated patch doing that.  ubsan is silent with that.

Thanks!

Yeah, indeed, with "COPT=-fsanitize=alignment -fno-sanitize-recover=all", then

"make -C src/test/modules/test_custom_rmgrs check":

- Is fine on master
- Fails with v1 applied due to things like:

xactdesc.c:91:28: runtime error: member access within misaligned address 0x5d29d22cc13c for type 'struct
xl_xact_stats_items',which requires 8 byte alignment
 
0x5d29d22cc13c: note: pointer points here
  7f 06 00 00 02 00 00 00  fe 7f 00 00 03 00 00 00  05 00 00 00 05 40 00 00  00 00 00 00 03 00 00 00

- Is fine with v2

So v2 does fix the alignment issue and I also think that's a nice way to fix it.

Lot of stuff that this patch does is mechanical changes:

- replace "objoid" by "objid" in *stats* files 
- change the related type from Oid to uint64
- make use of hash_bytes_extended() instead of hash_bytes when needed

and I don't see any issues here.

There is also some manipulation around the 2 new uint32 fields (objid_hi and
objid_lo) in the xactdesc.c and pgstat_xact.c files that look good to me.

But now we end up having functions that accept Oid as parameters to call
functions that accept uint64 as parameter (for the exact same parameter), for
example:

"
void
pgstat_create_function(Oid proid)
{
    pgstat_create_transactional(PGSTAT_KIND_FUNCTION,
                                MyDatabaseId,
                                proid);
}
"

as pgstat_create_transactional is now:

-pgstat_create_transactional(PgStat_Kind kind, Oid dboid, Oid objoid)
+pgstat_create_transactional(PgStat_Kind kind, Oid dboid, uint64 objid)

That's not an issue as both are unsigned and as we do those calls in that
order (Oid -> uint64).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Fri, Sep 13, 2024 at 07:34:21AM +0900, Michael Paquier wrote:
> On Thu, Sep 12, 2024 at 01:37:52PM +0000, Bertrand Drouvot wrote:
> > There is also some manipulation around the 2 new uint32 fields (objid_hi and
> > objid_lo) in the xactdesc.c and pgstat_xact.c files that look good to me.
> 
> Thanks for the reviews.  The high and low manipulations are still kind
> of OK to me as a solution for the record constructions.

Agree.

> > But now we end up having functions that accept Oid as parameters to call
> > functions that accept uint64 as parameter (for the exact same parameter), for
> > example:
> > 
> > "
> > void
> > pgstat_create_function(Oid proid)
> > {
> >     pgstat_create_transactional(PGSTAT_KIND_FUNCTION,
> >                                 MyDatabaseId,
> >                                 proid);
> > }
> > "
> > 
> > as pgstat_create_transactional is now:
> > 
> > -pgstat_create_transactional(PgStat_Kind kind, Oid dboid, Oid objoid)
> > +pgstat_create_transactional(PgStat_Kind kind, Oid dboid, uint64 objid)
> > 
> > That's not an issue as both are unsigned and as we do those calls in that
> > order (Oid -> uint64).
> 
> Yes, that's intentional.  All the pgstats routines associated to a
> particular object that depends on an OID should still use an OID, and
> anything that's generic enough to be used for all stats kinds had
> better use a uint64.

Yeah, that sounds good to me.

> I was wondering if it would be better hiding
> that behind a dedicated type, but decided to stick with uint64.

That makes sense to me.

Overall, the patch LGTM.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com