Thread: Cleanup gcc trick with varattrib_1b_e in VARATT_EXTERNAL_GET_POINTER()

Hi,

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.

varattrib_1b_e should never be referenced directly in the code, so it
would be nice to clean up things like in the attached.  The CI is OK
with that, which is not the buildfarm but it's a start.

Comments or opinions?
--
Michael

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].

> 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] https://postgr.es/m/27632.1191182717%40sss.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] https://postgr.es/m/27632.1191182717%40sss.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