Re: [HACKERS] Arrays of domains

On 08/11/2017 01:17 PM, Tom Lane wrote:
> I wrote:
>> Probably a better answer is to start supporting arrays over domain
>> types.  That was left unimplemented in the original domains patch,
>> but AFAICS not for any better reason than lack of round tuits.
> Attached is a patch series that allows us to create arrays of domain
> types.  I haven't tested this in great detail, so there might be some
> additional corners of the system that need work, but it passes basic
> sanity checks.  I believe it's independent of the other patch I have
> in the commitfest for domains over composites, but I haven't tested
> for interactions there either.
>
> 01-rationalize-coercion-APIs.patch cleans up the APIs of
> coerce_to_domain() and some internal functions in parse_coerce.c so that
> we consistently pass around a CoercionContext along with CoercionForm.
> Previously, we sometimes passed an "isExplicit" boolean flag instead,
> which is strictly less information; and coerce_to_domain() didn't even get
> that, but instead had to reverse-engineer isExplicit from CoercionForm.
> That's contrary to the documentation in primnodes.h that says that
> CoercionForm only affects display and not semantics.  I don't think this
> change fixes any live bugs, but it makes things more consistent.  The
> main reason for doing it though is that now build_coercion_expression()
> receives ccontext, which it needs in order to be able to recursively
> invoke coerce_to_target_type(), as required by the next patch.
>
> 02-reimplement-ArrayCoerceExpr.patch is the core of the patch.  It changes
> ArrayCoerceExpr so that the node does not directly know any details of
> what has to be done to the individual array elements while performing the
> array coercion.  Instead, the per-element processing is represented by
> a sub-expression whose input is a source array element and whose output
> is a target array element.  This simplifies life in parse_coerce.c,
> because it can build that sub-expression by a recursive invocation of
> coerce_to_target_type(), and it allows the executor to handle the
> per-element processing as a compiled expression instead of hard-wired
> code.  This is probably about a wash or a small loss performance-wise
> for the simplest case where we just need to invoke one coercion function
> for each element.  However, there are many cases where the existing code
> ends up generating two nested ArrayCoerceExprs, one to do the type
> conversion and one to apply a typmod (length) coercion function.  In the
> new code there will be just one ArrayCoerceExpr, saving one deconstruction
> and reconstruction of the array.  If I hadn't done it like this, adding
> domains into the mix could have produced as many as three
> ArrayCoerceExprs, where the top one would have only checked domain
> constraints; that did not sound nice performance-wise, and it would have
> required a lot of code duplication as well.
>
> Finally, 03-support-arrays-of-domains.patch simply turns on the spigot
> by creating an array type in DefineDomain(), and adds some test cases.
> Given the new method of handling ArrayCoerceExpr, everything Just Works.
>
> I'll add this to the next commitfest.
>
>             
>


I've reviewed and tested the updated versions of these patches. The
patches apply but there's an apparent typo in arrayfuncs.c -
DatumGetAnyArray instead of DatumGetAnyArrayP

Some of the line breaking in argument lists for some of the code
affected by these patches is a bit bizarre. It hasn't been made worse by
these patches but it hasn't been made better either. That's especially
true of patch 1.

Patch 1 is fairly straightforward, as is patch 3. Patch 2 is fairly
complex, but it still does the one thing stated above - there's just a
lot of housekeeping that goes along with that. I couldn't see any
obvious problems with the implementation.

I wonder if we need to do any benchmarking to assure ourselves that the
changes to ArrayCoerceExpr don't have a significant performance impact?

Apart from those concerns I think this is ready to be committed.

cheers

andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers