Re: Expanding the use of FLEXIBLE_ARRAY_MEMBER for declarations like foo[1]

On Thu, Feb 19, 2015 at 11:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> On Thu, Feb 19, 2015 at 7:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Moreover, if we have any code that is assuming such cases are okay, it
>>> probably needs a second look. Isn't this situation effectively assuming
>>> that a variable-length array is fixed-length?
>
>> AFAIK, switching a bunch of things to use FLEXIBLE_ARRAY_MEMBER has
>> put a couple of things in light that could be revisited:
>> 1) tuptoaster.c, with this declaration of varlena:
>> struct
>> ...
>> } chunk_data;
>
> I'm pretty sure that thing ought to be a union, not a struct.
>
>> 2) inv_api.c with this thing:
>> ...
> And probably this too.

Sounds good to me, but with an additional VARHDRSZ to give enough room
IMO (or toast_save_datum explodes).

>> 3) heapam.c in three places with HeapTupleHeaderData:
>> struct
>> {
>> HeapTupleHeaderData hdr;
>> char data[MaxHeapTupleSize];
>> } tbuf;
>
> And this, though I'm not sure if we'd have to change the size of the
> padding data[] member.

Here I think that we should add sizeof(HeapTupleHeaderData) to ensure
that there is enough room

>> 5) reorderbuffer.h with its use of HeapTupleHeaderData:
>
> Hmm. Andres will have to answer for that one ;-)

Surely. This impacts decode.c and reorder.c at quick glance.

So, attached are a new set of patches:
1) 0001 is more or less the same as upthread, changing trivial places
with foo[1]. I have checked as well calls to sizeof for the structures
impacted:
1-1) In dumputils.c, I guess that the call of sizeof with
SimpleStringListCell should be changed as follows:
cell = (SimpleStringListCell *)
- pg_malloc(sizeof(SimpleStringListCell) + strlen(val));
+ pg_malloc(sizeof(SimpleStringListCell));
1-2) sizeof(ParamListInfoData) is present in a couple of places,
assuming that sizeof(ParamListInfoData) has the equivalent of 1
parameter, like prepare.c, functions.c, spi.c and postgres.c:
- /* sizeof(ParamListInfoData) includes the first array element */
paramLI = (ParamListInfo)
palloc(sizeof(ParamListInfoData) +
- (num_params - 1) * sizeof(ParamExternData));
+ num_params * sizeof(ParamExternData));
1-3) FuncCandidateList in namespace.c (thanks Andres!):
newResult = (FuncCandidateList)
- palloc(sizeof(struct _FuncCandidateList) - sizeof(Oid)
- + effective_nargs * sizeof(Oid));
+ palloc(sizeof(struct _FuncCandidateList) +
+ effective_nargs * sizeof(Oid));
I imagine that we do not want for those palloc calls to use ifdef
FLEXIBLE_ARRAY_MEMBER to save some memory for code readability even if
compiler does not support flexible-array length, right?
2) 0002 fixes pg_authid to use CATALOG_VARLEN, this is needed for 0003.
3) 0003 switches varlena in c.h to use FLEXIBLE_ARRAY_MEMBER, with
necessary tweaks added for tuptoaster.c and inv_api.c
4) 0004 is some preparatory work before switching HeapTupleHeaderData
and MinimalTupleData, changing the struct declarations to union in
heapam.c with enough room ensured for processing.

Regards,
--
Michael