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@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Thu, Feb 19, 2015 at 7:29 AM, Tom Lane <tgl@sss.pgh.pa.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