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
