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

On 2015-02-16 21:34:57 -0500, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2015-02-17 05:51:22 +0900, Michael Paquier wrote:
> >> 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).
> 
> You were muttering about a BKI_FORCE_NOT_NULL option ... for symmetry,
> maybe we could add BKI_FORCE_NULL as well, and then use that for cases
> like this?

Yea, I guess it'd not be too hard.

> Also, if we want to insist that these fields be accessed
> through heap_getattr, I'd be inclined to put them inside the "#ifdef
> CATALOG_VARLEN" to enforce that.

That we definitely should do. It's imo just a small bug that it was
omitted here. I'll fix it, but not backpatch unless you prefer?

> I'm generally -1 on reordering any catalog columns as part of this patch.
> There should be zero user-visible change from it IMO.  However, if we
> stick both those columns inside the ifdef, we don't need to reorder.

Agreed.

Greetings,

Andres Freund

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