Cleanup gcc trick with varattrib_1b_e in VARATT_EXTERNAL_GET_POINTER()

On Mon, Jun 09, 2025 at 05:21:19PM +0900, Michael Paquier wrote:

Back in b89e151054a0, the following macro has been introduced to
retrieve the varatt_external of an on-disk external TOAST Datum, stuff
now in detoast.h:
/*
* Macro to fetch the possibly-unaligned contents of an EXTERNAL datum
* into a local "struct varatt_external" toast pointer. This should be
* just a memcpy, but some versions of gcc seem to produce broken code
* that assumes the datum contents are aligned. Introducing an explicit
* intermediate "varattrib_1b_e *" variable seems to fix it.
*/
#define VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr) \
do { \
varattrib_1b_e *attre = (varattrib_1b_e *) (attr); \
Assert(VARATT_IS_EXTERNAL(attre)); \
Assert(VARSIZE_EXTERNAL(attre) == sizeof(toast_pointer) + VARHDRSZ_EXTERNAL); \
memcpy(&(toast_pointer), VARDATA_EXTERNAL(attre), sizeof(toast_pointer)); \
} while (0)

I think that was actually added in commit 27b8922 [0]/messages/by-id/27632.1191182717@sss.pgh.pa.us.

I vaguely recall that this has been mentioned during the unconference
session dedicated to TOAST, or perhaps not. Anyway, I've just bumped
into that again while working on this area, and I am wondering if this
is relevant these days.

The risk/reward ratio might not be favorable on this one. Presumably we'd
need some level of confidence that this is no longer an issue, and in the
end the patch only saves a few lines of code.

[0]: /messages/by-id/27632.1191182717@sss.pgh.pa.us

--
nathan

On Mon, Jun 09, 2025 at 04:17:25PM -0500, Nathan Bossart wrote:

The risk/reward ratio might not be favorable on this one. Presumably we'd
need some level of confidence that this is no longer an issue, and in the
end the patch only saves a few lines of code.

[0] /messages/by-id/27632.1191182717@sss.pgh.pa.us

Ah, thanks. Well, then it's a good thing now that HPPA is entirely
gone (edadeb0710e8), for a tweak added 17 years ago.
--
Michael

On 09.06.25 10:21, Michael Paquier wrote:

Back in b89e151054a0, the following macro has been introduced to
retrieve the varatt_external of an on-disk external TOAST Datum, stuff
now in detoast.h:
/*
* Macro to fetch the possibly-unaligned contents of an EXTERNAL datum
* into a local "struct varatt_external" toast pointer. This should be
* just a memcpy, but some versions of gcc seem to produce broken code
* that assumes the datum contents are aligned. Introducing an explicit
* intermediate "varattrib_1b_e *" variable seems to fix it.
*/
#define VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr) \
do { \
varattrib_1b_e *attre = (varattrib_1b_e *) (attr); \
Assert(VARATT_IS_EXTERNAL(attre)); \
Assert(VARSIZE_EXTERNAL(attre) == sizeof(toast_pointer) + VARHDRSZ_EXTERNAL); \
memcpy(&(toast_pointer), VARDATA_EXTERNAL(attre), sizeof(toast_pointer)); \
} while (0)

I vaguely recall that this has been mentioned during the unconference
session dedicated to TOAST, or perhaps not. Anyway, I've just bumped
into that again while working on this area, and I am wondering if this
is relevant these days.

I'm not sure that the original reason applies anymore.

If attr in the above code is of type Datum, then I think the original
problem still exists. The compiler can assume that values of type Datum
have alignment fitting for Datum. But all the callers in the current
code have type struct varlena *, and the cast target behind
VARDATA_EXTERNAL() is varattrib_1b_e, both of which AFAICT have no
higher alignment requirement.

I can see how this might have been different historically. I have
noticed that there are some areas of code where Datum and struct varlena
* or similar are used interchangeably. Macros tend to hide that kind of
confusion. But some of this has been cleaned up with changing some
macros to inline functions. Maybe doing the same would help here, too.

On Tue, Jun 10, 2025 at 06:30:33PM +0200, Peter Eisentraut wrote:

I can see how this might have been different historically. I have noticed
that there are some areas of code where Datum and struct varlena * or
similar are used interchangeably. Macros tend to hide that kind of
confusion. But some of this has been cleaned up with changing some macros
to inline functions. Maybe doing the same would help here, too.

Yeah, I've noticed that some code paths tend to be a bit confused
about both concepts, sticking to one while silently assuming the
other. Perhaps these could be made cleaner.
--
Michael

Peter Eisentraut <peter@eisentraut.org> writes:

On 09.06.25 10:21, Michael Paquier wrote:

now in detoast.h:
/*
* Macro to fetch the possibly-unaligned contents of an EXTERNAL datum
* into a local "struct varatt_external" toast pointer. This should be
* just a memcpy, but some versions of gcc seem to produce broken code
* that assumes the datum contents are aligned. Introducing an explicit
* intermediate "varattrib_1b_e *" variable seems to fix it.
*/

I'm not sure that the original reason applies anymore.

I don't see why it wouldn't; if anything, compilers have gotten more
eager to optimize on the strength of dubious assumptions. Perhaps
we are less likely to notice now that there are so few machines that
will throw a bus error for misaligned accesses --- but I believe that
those are still quite expensive on a lot of platforms, so we'd do
well to avoid them.

If attr in the above code is of type Datum, then I think the original
problem still exists. The compiler can assume that values of type Datum
have alignment fitting for Datum.

I think this argument is confusing the alignment of the pointer datum
with that of the struct to which it points.

I can see how this might have been different historically. I have
noticed that there are some areas of code where Datum and struct varlena
* or similar are used interchangeably. Macros tend to hide that kind of
confusion. But some of this has been cleaned up with changing some
macros to inline functions. Maybe doing the same would help here, too.

I don't have any particular beef with changing this code to use
an inline function, if we can figure out how to make the sizeof()
bits work. Right now it's agnostic about how big toast_pointer is,
which seems like a good property to preserve if we are thinking
about having multiple kinds of toast pointer.

regards, tom lane