From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Vincent Lachenal <vincent(dot)lachenal(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-bugs(at)postgresql(dot)org> |
Subject: | Re: [BUGS] BUG #14897: Segfault on statitics SQL request |
Date: | 2017-11-14 15:53:46 |
Message-ID: | 18815.1510674826@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Tue, Nov 14, 2017 at 12:31 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | justin.lu | 2017-11-14 21:20:06 | BUG #14907: missing postgis extension files |
Previous Message | Michael Paquier | 2017-11-14 06:25:24 | Re: [BUGS] BUG #14897: Segfault on statitics SQL request |