Date: Sat, 2 Nov 2019 17:00:17 -0700
From: Andres Freund <andres@anarazel.de>
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Remove configure --disable-float4-byval and
 --disable-float8-byval
Message-ID: <20191103000017.msgpcspscrtg25se@alap3.anarazel.de>
References: <f3e1e576-2749-bbd7-2d57-3f9dcf75255a@2ndquadrant.com>
 <15316.1572528990@sss.pgh.pa.us>
 <CA+TgmoaCyw26S7wkTshAJKpjP5zfVnLiBSWYYvn5kT7rh3vrZg@mail.gmail.com>
 <b68834c1-2122-4b3d-11e4-b3938b83bab9@2ndquadrant.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b68834c1-2122-4b3d-11e4-b3938b83bab9@2ndquadrant.com>
Precedence: bulk

Hi,

On 2019-11-02 08:46:12 +0100, Peter Eisentraut wrote:
> On 2019-11-01 15:41, Robert Haas wrote:
> > On a related note, why do we store typbyval in the catalog anyway
> > instead of inferring it from typlen and maybe typalign? It seems like
> > a bad idea to record on disk the way we pass around values in memory,
> > because it means that a change to how values are passed around in
> > memory has ramifications for on-disk compatibility.
>
> This sounds interesting.  It would remove a pg_upgrade hazard (in the long
> run).
>
> There is some backward compatibility to be concerned about.  This change
> would require extension authors to change their code to insert #ifdef
> USE_FLOAT8_BYVAL or similar, where currently their code might only support
> one method or the other.

I think we really ought to remove the difference behind macros. That is,
for types that could be either, provide macros that fetch function
arguments into local memory, independent of whether the argument is a
byval or byref type.  I.e. instead of having separate #ifdef
USE_FLOAT8_BYVALs for DatumGetFloat8(), DatumGetInt64(), ... we should
provide that logic in one centralized set of macros.

The fact that USE_FLOAT8_BYVAL has to creep into various functions imo
is the reasons why people are unhappy about it.

One way to do this would be something roughly like this sketch:

/* allow to force types to be byref, for testing purposes */
#if USE_FLOAT8_BYVAL
#define DatumForTypeIsByval(type) (sizeof(type) <= SIZEOF_DATUM)
#else
#define DatumForTypeIsByval(type) (sizeof(type) <= sizeof(int))
#endif

/* yes, I dream of being C++ once grown up */
#define DefineSmallFixedWidthDatumTypeConversions(type, TypeToDatumName, DatumToTypeName) \
    static inline type \
    TypeToDatumName (type value) \
    { \
        if (DatumForTypeIsByval(type)) \
        { \
            Datum tmp; \
            \
            /* ensure correct conversion, consider e.g. float */ \
            memcpy(&tmp, &value, sizeof(type)); \
            \
            return tmp; \
        } \
        else \
        { \
             type *tmp = (type *) palloc(sizeof(datum)); \

             *tmp = value;

             return PointerGetDatum(tmp); \
        } \
    } \
    \
    static inline type \
    DatumToTypeName (Datum datum) \
    { \
        if (DatumForTypeIsByval(type)) \
        { \
            type tmp; \
            \
            /* ensure correct conversion */ \
            memcpy(&tmp, &datum, sizeof(type)); \
            \
            return tmp; \
        } \
        else \
             return *(type *) DatumGetPointer(type); \
    } \

And then have

DefineSmallFixedWidthDatumTypeConversions(
        float8,
        Float8GetDatum,
        DatumGetFloat8);

DefineSmallFixedWidthDatumTypeConversions(
        int64,
        Int64GetDatum,
        DatumGetInt64);

And now also

DefineSmallFixedWidthDatumTypeConversions(
        macaddr,
        Macaddr8GetDatum,
        DatumGetMacaddr8);

(there's probably plenty of bugs in the above, it's just a sketch)


We don't have to break types / extensions with inferring byval for fixed
width types. Instead we can change CREATE TYPE's PASSEDBYVALUE to accept
an optional parameter 'auto', allowing to opt in.


> > rhaas=# select typname, typlen, typbyval, typalign from pg_type where
> > typlen in (1,2,4,8) != typbyval;
>
> There are also typlen=6 types.  Who knew. ;-)

There's a recent thread about them :)


> >   typname  | typlen | typbyval | typalign
> > ----------+--------+----------+----------
> >   macaddr8 |      8 | f        | i
> > (1 row)
>
> This might be a case of the above issue: It's easier to just make it pass by
> reference always than deal with a bunch of #ifdefs.

Indeed. And that's a bad sign imo.

Greetings,

Andres Freund