Re: 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