From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Strange coding in mvdistinct.c
Date: Mon, 15 Apr 2019 18:00:02 -0400
Message-ID: <29785.1555365602@sss.pgh.pa.us> (raw)
In the wake of the discussion at [1] I went looking for structs that
should be using FLEXIBLE_ARRAY_MEMBER and are not, by dint of grepping
for size calculations of the form "offsetof(struct,fld) + n * sizeof(...)"
and then seeing how "fld" is declared. I haven't yet found anything
like that that I want to change, but I did come across this bit in
mvdistinct.c's statext_ndistinct_serialize():
len = VARHDRSZ + SizeOfMVNDistinct +
ndistinct->nitems * (offsetof(MVNDistinctItem, attrs) + sizeof(int));
Given the way that the subsequent code looks, I would argue that
offsetof(MVNDistinctItem, attrs) has got basically nothing to do with
this calculation, and that the right way to phrase it is just
len = VARHDRSZ + SizeOfMVNDistinct +
ndistinct->nitems * (sizeof(double) + sizeof(int));
Consider if there happened to be alignment padding in MVNDistinctItem:
as the code stands it'd overestimate the space needed. (There won't be
padding on any machine we support, I believe, so this isn't a live bug ---
but it's overcomplicated code, and could become buggy if any
less-than-double-width fields get added to MVNDistinctItem.)
For largely the same reason, I do not think that SizeOfMVNDistinct is
a helpful way to compute the space needed for those fields --- any
alignment padding that might be included is irrelevant for this purpose.
In short I'd be inclined to phrase this just as
len = VARHDRSZ + 3 * sizeof(uint32) +
ndistinct->nitems * (sizeof(double) + sizeof(int));
It looks to me actually like all the uses of both SizeOfMVNDistinctItem
and SizeOfMVNDistinct are wrong, because the code using those symbols
is really thinking about the size of this serialized representation,
which is guaranteed not to have any inter-field padding, unlike the
structs.
Thoughts?
regards, tom lane
[1] https://postgr.es/m/a620f85a-42ab-e0f3-3337-b04b97e2e2f5@redhat.com
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, tomas.vondra@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Strange coding in mvdistinct.c
In-Reply-To: <29785.1555365602@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox