Re: [HACKERS] Arrays of domains - Mailing list pgsql-hackers
From | Andrew Dunstan |
---|---|
Subject | Re: [HACKERS] Arrays of domains |
Date | |
Msg-id | 9b89fa47-83f7-dbd4-22bd-3886110a2f00@2ndQuadrant.com Whole thread Raw |
In response to | Re: [HACKERS] Arrays of domains (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: [HACKERS] Arrays of domains
|
List | pgsql-hackers |
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
pgsql-hackers by date: