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

On 2015-02-17 05:51:22 +0900, Michael Paquier wrote:
> - 0002 does the same for catalog tables
> - 0003 changes varlena in c.h. This is done as a separate patch
> because this needs some other modifications as variable-length things
> need to be placed at the end of structures, because of:
> -- rolpassword that should be placed as the last field of pg_authid
> (and shouldn't there be CATALOG_VARLEN here??)

Yes, there should.

> -- 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.

> 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.

> - 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.

Generally it's worthwhile to check the code you changed for
sizeof(changed_struct) and similar things. Because this very well could
add bugs. And not all of them will result in a outright visibile error
like the FuncnameGetCandidates() one.

> --- a/src/include/access/htup_details.h
> +++ b/src/include/access/htup_details.h
> @@ -150,7 +150,7 @@ struct HeapTupleHeaderData
>
> /* ^ - 23 bytes - ^ */
>
> - bits8 t_bits[1]; /* bitmap of NULLs -- VARIABLE LENGTH */
> + bits8 t_bits[1]; /* bitmap of NULLs */
>
> /* MORE DATA FOLLOWS AT END OF STRUCT */
> };
> @@ -579,7 +579,7 @@ struct MinimalTupleData
>
> /* ^ - 23 bytes - ^ */
>
> - bits8 t_bits[1]; /* bitmap of NULLs -- VARIABLE LENGTH */
> + bits8 t_bits[1]; /* bitmap of NULLs */
>
> /* MORE DATA FOLLOWS AT END OF STRUCT */
> };

This sees like overeager search/replace.

> diff --git a/src/include/nodes/params.h b/src/include/nodes/params.h
> index 5b096c5..0ad7ef0 100644
> --- a/src/include/nodes/params.h
> +++ b/src/include/nodes/params.h
> @@ -71,7 +71,7 @@ typedef struct ParamListInfoData
> ParserSetupHook parserSetup; /* parser setup hook */
> void *parserSetupArg;
> int numParams; /* number of ParamExternDatas following */
> - ParamExternData params[1]; /* VARIABLE LENGTH ARRAY */
> + ParamExternData params[1];
> } ParamListInfoData;
>

Eh?

> diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
> index 87a3462..73bcefe 100644
> --- a/src/include/catalog/pg_attribute.h
> +++ b/src/include/catalog/pg_attribute.h
> @@ -157,13 +157,13 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS BKI_ROWTYPE_OID(75) BK
> /* NOTE: The following fields are not present in tuple descriptors. */
>
> /* Column-level access permissions */
> - aclitem attacl[1];
> + aclitem attacl[FLEXIBLE_ARRAY_MEMBER];
>
> /* Column-level options */
> - text attoptions[1];
> + text attoptions[FLEXIBLE_ARRAY_MEMBER];
>
> /* Column-level FDW options */
> - text attfdwoptions[1];
> + text attfdwoptions[FLEXIBLE_ARRAY_MEMBER];
> #endif
> } FormData_pg_attribute;

Ugh. This really isn't C at all anymore - these structs wouldn't compile
if you removed the #ifdef. Maybe we should instead a 'textarray' type
for genbki's benefit?

Generally the catalog changes are much less clearly a benefit imo.

> From ad8f54cdb5776146f17d1038bb295b5f13b549f1 Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael(at)otacoo(dot)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

> #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).

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services