Обсуждение: Shouldn't construct_array_builtin and deconstruct_array_builtin agree on types?

Hi,

I noticed that these two function, introduced in d746021de18b, disagree
on what types they support. For construct_array_builtin, we support
these types:

 - CHAROID:
 - CSTRINGOID:
 - FLOAT4OID:
 - INT2OID:
 - INT4OID:
 - INT8OID:
 - NAMEOID:
 - OIDOID:
 - REGTYPEOID:
 - TEXTOID:
 - TIDOID:

while deconstruct_array_builtin supports just a subset:

 - CHAROID:
 - CSTRINGOID:
 - FLOAT8OID:
 - INT2OID:
 - OIDOID:
 - TEXTOID:
 - TIDOID:

I ran into this for INT4OID. Sure, I can just lookup the stuff and use
the regualr deconstruct_array, but the asymmetry seems a bit strange. Is
that intentional?

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12.06.23 22:38, Tomas Vondra wrote:
> I noticed that these two function, introduced in d746021de18b, disagree
> on what types they support.

> I ran into this for INT4OID. Sure, I can just lookup the stuff and use
> the regualr deconstruct_array, but the asymmetry seems a bit strange. Is
> that intentional?

They only support the types that they were actually being used with.  If 
you need another type, feel free to add it.

On Mon, Jun 12, 2023 at 11:06:18PM +0200, Peter Eisentraut wrote:
> They only support the types that they were actually being used with.  If you
> need another type, feel free to add it.

FWIW, I agree with Tomas that this is an oversight that should be
fixed in v16, saving from the need to have a copy of
deconstruct_array_builtin() in extensions.  Same argument here:
couldn't it be possible that an extension does multiple array
constructs and deconstructs in a single code path?  We have patterns
like that in core as well.  For instance, see
extension_config_remove().
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Jun 12, 2023 at 11:06:18PM +0200, Peter Eisentraut wrote:
>> They only support the types that they were actually being used with.  If you
>> need another type, feel free to add it.

> FWIW, I agree with Tomas that this is an oversight that should be
> fixed in v16, saving from the need to have a copy of
> deconstruct_array_builtin() in extensions.

We don't want to bloat these functions indefinitely, so I understand
Peter's approach of only adding the cases actually being used.
At the same time, it's reasonable to take some thought for extensions
that might want slightly more functionality than the core code
happens to need at any given moment.

The idea of making both functions support the same set of types
does seem like a reasonable compromise.

            regards, tom lane