Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
  pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Refactor construct_array() and deconstruct_array() for built-in
 types
References: <2914356f-9e5f-8c59-2995-5997fc48bcba@enterprisedb.com>
	<1844517.1651502932@sss.pgh.pa.us>
	<c030a2ef-02d6-ccdb-8ebe-bc97df69be59@enterprisedb.com>
Date: Fri, 01 Jul 2022 11:43:21 +0100
In-Reply-To: <c030a2ef-02d6-ccdb-8ebe-bc97df69be59@enterprisedb.com> (Peter
	Eisentraut's message of "Fri, 1 Jul 2022 11:41:37 +0200")
Message-ID: <871qv5kt9y.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Archived-At: 
 <https://www.postgresql.org/message-id/871qv5kt9y.fsf%40wibble.ilmari.org>
Precedence: bulk

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

> On 02.05.22 16:48, Tom Lane wrote:
>> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>>> There are many calls to construct_array() and deconstruct_array() for
>>> built-in types, for example, when dealing with system catalog columns.
>>> These all hardcode the type attributes necessary to pass to these functions.
>> 
>>> To simplify this a bit, add construct_array_builtin(),
>>> deconstruct_array_builtin() as wrappers that centralize this hardcoded
>>> knowledge.  This simplifies many call sites and reduces the amount of
>>> hardcoded stuff that is spread around.
>> 
>>> I also considered having genbki.pl generate lookup tables for these
>>> hardcoded values, similar to schemapg.h, but that ultimately seemed
>>> excessive.
>> +1 --- the added overhead of the switch statements is probably a
>> reasonable price to pay for the notational simplification and
>> bug-proofing.
>> One minor coding gripe is that compilers that don't know that
>> elog(ERROR)
>> doesn't return will certainly generate "use of possibly-uninitialized
>> variable" complaints.  Suggest inserting "return NULL;" or similar into
>> the default: cases.  I'd also use more specific error wording to help
>> people find where they need to add code when they make use of a new type;
>> maybe like "type %u not supported by construct_array_builtin".
>
> I have pushed this with the improvements you had suggested.

I dind't pay much attention to this thread earlier, but I was struck by
the duplication of the switch statement determining the elemlen,
elembyval, and elemalign values between the construct and deconstruct
functions.  How about a common function they can both call?  Something
like:

static void builtin_type_details(Oid elemtype,
                                 int *elemlen,
                                 bool *elembyval,
                                 char *elemalign);

- ilmari