Feedback-ID: i131946ab:Fastmail
Message-ID: <c030a2ef-02d6-ccdb-8ebe-bc97df69be59@enterprisedb.com>
Date: Fri, 1 Jul 2022 11:41:37 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.10.0
Subject: Re: Refactor construct_array() and deconstruct_array() for built-in
 types
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <2914356f-9e5f-8c59-2995-5997fc48bcba@enterprisedb.com>
 <1844517.1651502932@sss.pgh.pa.us>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <1844517.1651502932@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/c030a2ef-02d6-ccdb-8ebe-bc97df69be59%40enterprisedb.com>
Precedence: bulk

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.