From: Tom Lane <tgl@sss.pgh.pa.us>
To: jesper.pedersen@redhat.com
cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>, sulamul@gmail.com,
        PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
        Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Subject: Re: COLLATE: Hash partition vs UPDATE
In-reply-to: <826e4d52-234a-4d35-73b2-7c6b0a92220e@redhat.com>
References: <a620f85a-42ab-e0f3-3337-b04b97e2e2f5@redhat.com>
 <b462ff0c-a846-dce6-b0a7-ab1397e73b98@lab.ntt.co.jp>
 <826e4d52-234a-4d35-73b2-7c6b0a92220e@redhat.com>
Comments: In-reply-to Jesper Pedersen <jesper.pedersen@redhat.com>
	message dated "Tue, 09 Apr 2019 08:43:57 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <30550.1555275007.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 14 Apr 2019 16:50:07 -0400
Message-ID: <30551.1555275007@sss.pgh.pa.us>
Precedence: bulk

Jesper Pedersen <jesper.pedersen@redhat.com> writes:
> Yeah, that works here - apart from an issue with the test case; fixed in=
 =

> the attached.

Couple issues spotted in an eyeball review of that:

* There is code that supposes that partsupfunc[] is the last
field of ColumnsHashData, eg

            fcinfo->flinfo->fn_extra =3D
                MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
                                       offsetof(ColumnsHashData, partsupfu=
nc) +
                                       sizeof(FmgrInfo) * nargs);

I'm a bit surprised that this patch manages to run without crashing,
because this would certainly not allocate space for partcollid[].

I think we would likely be well advised to do

-		FmgrInfo	partsupfunc[PARTITION_MAX_KEYS];
+		FmgrInfo	partsupfunc[FLEXIBLE_ARRAY_MEMBER];

to make it more obvious that that has to be the last field.  Or else
drop the cuteness with variable-size allocations of ColumnsHashData.
FmgrInfo is only 48 bytes, I'm not really sure that it's worth the
risk of bugs to "optimize" this.

* I see collation-less calls of the partsupfunc at both partbounds.c:2931
and partbounds.c:2970, but this patch touches only the first one.  How
can that be right?

			regards, tom lane