Re: Expanding the use of FLEXIBLE_ARRAY_MEMBER for declarations like foo[1] - Mailing list pgsql-hackers

On Tue, Feb 17, 2015 at 9:02 AM, Andres Freund wrote:
> On 2015-02-17 05:51:22 +0900, Michael Paquier wrote:
>> -- inv_api.c uses bytea in an internal structure without putting it at
>> the end of the structure. For clarity I think that we should just use
>> a bytea pointer and do a sufficient allocation for the duration of the
>> lobj manipulation.
>
> Hm. I don't really see the problem tbh.
>
> There's actually is a reason the bytea is at the beginning - the other
> fields are *are* the data part of the bytea (which is just the varlena
> header).
>
>> -- Similarly, tuptoaster.c needed to be patched for toast_save_datum
>
> I'm not a big fan of these two changes. This adds some not that small
> memory allocations to a somewhat hot path. Without a big win in
> clarity. And it doesn't seem to have anything to do with with
> FLEXIBLE_ARRAY_MEMBER.

We could replace those palloc calls with a simple buffer that has a
predefined size, but this somewhat reduces the readability of the
code.

>> There are as well couple of things that are not changed on purpose:
>> - in namespace.h for FuncCandidateList. I tried manipulating it but it
>> resulted in allocation overflow in PortalHeapMemory
>
> Did you change the allocation in FuncnameGetCandidates()? Note the -
> sizeof(Oid) there.

Yeah. Missed it.

>> - I don't think that the t_bits fields in htup_details.h should be
>> updated either.
>
> Why not? Any not broken code should already use MinHeapTupleSize and
> similar macros.

Changing t_bits impacts HeapTupleHeaderData, ReorderBufferTupleBuf and
similarly a couple of redo routines in heapam.c using
HeapTupleHeaderData in a couple of structures not placing it at the
end (compiler complains). We could use for each of them a buffer that
has enough room with sizeof(HeapTupleHeaderData) + MaxHeapTupleSize,
but wouldn't it reduce the readability of the current code? Opinions
welcome.

>> diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
> [...]
> Generally the catalog changes are much less clearly a benefit imo.

OK, let's drop them then.

>> From ad8f54cdb5776146f17d1038bb295b5f13b549f1 Mon Sep 17 00:00:00 2001
>> From: Michael Paquier <michael@otacoo.com>
>> Date: Mon, 16 Feb 2015 03:53:38 +0900
>> Subject: [PATCH 3/3] Update varlena in c.h to use FLEXIBLE_ARRAY_MEMBER
>>
>> Places using a variable-length variable not at the end of a structure
>> are changed with workaround, without impacting what those features do.
>
> I vote for rejecting most of this, except a (corrected version) of the
> pg_authid change. Which doesn't really belong to the rest of the patch
> anyway ;)x

Changing bytea to use FLEXIBLE_ARRAY_MEMBER requires those changes, or
at least some changes in this area as something with
FLEXIBLE_ARRAY_MEMBER can only be placed at the end of a structure.
But I guess that we can do fine as well by replacing those structures
with some buffers with a pre-defined size. I'll draft an additional
patch on top of 0001 with all those less-trivial changes implemented.

>>  #define VARHDRSZ             ((int32) sizeof(int32))
>> diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
>> index e01e6aa..d8789a5 100644
>> --- a/src/include/catalog/pg_authid.h
>> +++ b/src/include/catalog/pg_authid.h
>> @@ -56,8 +56,10 @@ CATALOG(pg_authid,1260) BKI_SHARED_RELATION BKI_ROWTYPE_OID(2842) BKI_SCHEMA_MAC
>>       int32           rolconnlimit;   /* max connections allowed (-1=no limit) */
>>
>>       /* remaining fields may be null; use heap_getattr to read them! */
>> -     text            rolpassword;    /* password, if any */
>>       timestamptz rolvaliduntil;      /* password expiration time, if any */
>> +#ifdef CATALOG_VARLEN
>> +     text            rolpassword;    /* password, if any */
>> +#endif
>>  } FormData_pg_authid;
>
> That change IIRC is wrong, because it'll make rolvaliduntil until NOT
> NULL (any column that's fixed width and has only fixed with columns
> before it is marked as such).

This sounds better as a separate patch...
-- 
Michael