From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael.paquier@gmail.com>
cc: Dmitry Dolgov <9erthalion6@gmail.com>, Andres Freund <andres@anarazel.de>,
        Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
        Vincent Lachenal <vincent.lachenal@gmail.com>,
        PostgreSQL mailing lists <pgsql-bugs@postgresql.org>
Subject: Re: [BUGS] BUG #14897: Segfault on statitics SQL request
In-reply-to: 
 <CAB7nPqQ7+rnOsGNDrtVC9uxb6erog2s4faM0FvmY_s-7uUmaNw@mail.gmail.com>
References: <20171110185747.31519.28038@wrigleys.postgresql.org>
 <1415.1510340891@sss.pgh.pa.us>
 <CAA2tJ7o-VK+ghsxifDAszFN5fikmG6U3sWHCEvc9nAVOypfjAw@mail.gmail.com>
 <5652.1510344738@sss.pgh.pa.us>
 <CAA2tJ7qsopt0hHbsm397H62LMsaE4hjRwaUu3FQdNgo5AYAGXA@mail.gmail.com>
 <7710.1510346813@sss.pgh.pa.us>
 <CAA2tJ7odG4KB=wvu_+5FfssxckbLTnEuR9C_TXRyeU_35z+CSA@mail.gmail.com>
 <10197.1510349123@sss.pgh.pa.us>
 <CAA2tJ7p72wbuHEzOA_g2V4YcmeDPHYKggenTR4dXG=kSpCiO8Q@mail.gmail.com>
 <14075.1510352167@sss.pgh.pa.us>
 <CA+q6zcXe9aV5NV88u9Y=vMp+=fP7QOXgTyFQhzuvmCvTBvb_kQ@mail.gmail.com>
 <17617.1510355215@sss.pgh.pa.us>
 <CAB7nPqR7mWvtnALZF93oez_v03NFCJ8dXP9CNicaUDv3KjDbYw@mail.gmail.com>
 <3192.1510508352@sss.pgh.pa.us>
 <CAB7nPqReMmn3Pg942we4aK73SFSVamdM0SAUO0BZHcvzA71h+A@mail.gmail.com>
 <CA+q6zcWnWYX0ArC6QYC3YeRk3WnsyuexV1FzqBuBpGqODEK3fQ@mail.gmail.com>
 <10396.1510621709@sss.pgh.pa.us> <17615.1510630283@sss.pgh.pa.us>
 <CAB7nPqQ7+rnOsGNDrtVC9uxb6erog2s4faM0FvmY_s-7uUmaNw@mail.gmail.com>
Comments: In-reply-to Michael Paquier <michael.paquier@gmail.com>
	message dated "Tue, 14 Nov 2017 14:20:25 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <18814.1510674826.1@sss.pgh.pa.us>
Date: Tue, 14 Nov 2017 10:53:46 -0500
Message-ID: <18815.1510674826@sss.pgh.pa.us>
Precedence: bulk

Michael Paquier <michael.paquier@gmail.com> writes:
> On Tue, Nov 14, 2017 at 12:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As I said before, I don't like moving the int128 typedefs into a section
>> where they don't belong, but that's just cosmetic --- this is good enough
>> for testing.

> Section 3 could be moved after the section 4 listing the alignment
> macros. It seems that it won't hurt to back-patch the refactoring as
> well.

After some further consideration of what's where in c.h, I propose that
we redefine section 1 (hacks to cope with non-ANSI C compilers) as
"compiler characteristics", and then move all of these items into it:

"#ifdef PG_FORCE_DISABLE_INLINE" stanza

all the pg_attribute_xxx() macros, also pg_noinline

PG_USED_FOR_ASSERTS_ONLY

maybe pg_unreachable(), although it's not insane to keep it in section 7

ditto for likely()/unlikely()

If anyone's really hot to keep the "non-ANSI hacks" segregated from those,
we could invent a "section 1.1" for that ... but I'd say that at least one
of the stanzas that are in there now is unrelated to ANSI compatibility
already.

Having gotten pg_attribute_aligned in front of the int128 stanza,
my vision for a full fix for the bug is:

* Explicitly document somewhere that MAXALIGN ignores int128.

* Have configure test for and define ALIGNOF_PG_INT128_TYPE
if it defines PG_INT128_TYPE.

* Set up the int128 stanza like this:

#if defined(PG_INT128_TYPE)
#if defined(pg_attribute_aligned) || ALIGNOF_PG_INT128_TYPE <= MAXIMUM_ALIGNOF
#define HAVE_INT128 1
typedef PG_INT128_TYPE int128
#if defined(pg_attribute_aligned)
pg_attribute_aligned(MAXIMUM_ALIGNOF)
#endif
;
typedef unsigned PG_INT128_TYPE uint128
#if defined(pg_attribute_aligned)
pg_attribute_aligned(MAXIMUM_ALIGNOF)
#endif
;
#endif
#endif

That is, even if configure finds an int128 type, we'll only use it if
it fits or can be made to fit into our system-wide MAXALIGN assumption.

* It'd be kind of nice to insert a
	StaticAssertStmt(alignof(int128) <= MAXIMUM_ALIGNOF)
someplace to cross-check this, but as far as I can find we aren't
relying on alignof() to exist, so I don't see a good way to do it.


BTW, looking at the existing uses of pg_attribute_aligned
along with this example, I can't help but think that
this decision:

/*
 * NB: aligned and packed are not given default definitions because they
 * affect code functionality; they *must* be implemented by the compiler
 * if they are to be used.
 */

was just useless pedantry.  If we're going to ifdef around it everywhere
the macro is of use, why not just #define it to empty?  That would
certainly make the above a lot easier to read.  We could add a
HAVE_PG_ATTRIBUTE_ALIGNED symbol for use in #if tests.

Barring objections, I'll get on with making this happen.

			regards, tom lane