Re: [HACKERS] Arrays of domains
On 09/29/2017 01:10 PM, Tom Lane wrote: > Andrew Dunstan writes: >> On 09/28/2017 05:44 PM, Tom Lane wrote: >>> Assuming that that's going to happen for v11, I'm inclined to leave the >>> optimization problem until the dust settles around CaseTestExpr. >>> Does anyone feel that this can't be committed before that's addressed? >> I'm Ok with it as long as we don't forget to revisit this. > I decided to go ahead and build a quick optimization for this case, > as per the attached patch that applies on top of what we previously > discussed. It brings us back to almost par with HEAD: > > HEADPatch + 04.patch > > Q15453.235 ms 5440.876 ms 5407.965 ms > Q29340.670 ms 10191.194 ms9407.093 ms > Q319078.457 ms18967.279 ms19050.392 ms > Q448196.338 ms58547.531 ms48696.809 ms Nice. > > Unless Andres feels this is too ugly to live, I'm inclined to commit > the patch with this addition. If we don't get around to revisiting > CaseTestExpr, I think this is OK, and if we do, this will make sure > that we consider this case in the revisit. > > It's probably also worth pointing out that this test case is intentionally > chosen to be about the worst possible case for the patch. A less-trivial > coercion function would make the overhead proportionally less meaningful. > There's also the point that the old code sometimes applies two layers of > array coercion rather than one. As an example, coercing int4[] to > varchar(10)[] will do that. If I replace "x::int8[]" with > "x::varchar(10)[]" in Q2 and Q4 in this test, I get > > HEADPatch (+04) > > Q246929.728 ms20646.003 ms > Q4386200.286 ms 155917.572 ms > > Yeah, testing the worst case was the idea. This improvement in the non-worst case is pretty good. +1 for going ahead. cheers andrew -- Andrew Dunstanhttps://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
Re: [HACKERS] Arrays of domains
Andrew Dunstan writes: > On 09/28/2017 05:44 PM, Tom Lane wrote: >> Assuming that that's going to happen for v11, I'm inclined to leave the >> optimization problem until the dust settles around CaseTestExpr. >> Does anyone feel that this can't be committed before that's addressed? > I'm Ok with it as long as we don't forget to revisit this. I decided to go ahead and build a quick optimization for this case, as per the attached patch that applies on top of what we previously discussed. It brings us back to almost par with HEAD: HEADPatch + 04.patch Q1 5453.235 ms 5440.876 ms 5407.965 ms Q2 9340.670 ms 10191.194 ms9407.093 ms Q3 19078.457 ms18967.279 ms19050.392 ms Q4 48196.338 ms58547.531 ms48696.809 ms Unless Andres feels this is too ugly to live, I'm inclined to commit the patch with this addition. If we don't get around to revisiting CaseTestExpr, I think this is OK, and if we do, this will make sure that we consider this case in the revisit. It's probably also worth pointing out that this test case is intentionally chosen to be about the worst possible case for the patch. A less-trivial coercion function would make the overhead proportionally less meaningful. There's also the point that the old code sometimes applies two layers of array coercion rather than one. As an example, coercing int4[] to varchar(10)[] will do that. If I replace "x::int8[]" with "x::varchar(10)[]" in Q2 and Q4 in this test, I get HEADPatch (+04) Q2 46929.728 ms20646.003 ms Q4 386200.286 ms 155917.572 ms regards, tom lane diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index e0a8998..c5e97ef 100644 *** a/src/backend/executor/execExprInterp.c --- b/src/backend/executor/execExprInterp.c *** *** 34,43 * * For very simple instructions the overhead of the full interpreter * "startup", as minimal as it is, is noticeable. Therefore ! * ExecReadyInterpretedExpr will choose to implement simple scalar Var ! * and Const expressions using special fast-path routines (ExecJust*). ! * Benchmarking shows anything more complex than those may as well use the ! * "full interpreter". * * Complex or uncommon instructions are not implemented in-line in * ExecInterpExpr(), rather we call out to a helper function appearing later --- 34,41 * * For very simple instructions the overhead of the full interpreter * "startup", as minimal as it is, is noticeable. Therefore ! * ExecReadyInterpretedExpr will choose to implement certain simple ! * opcode patterns using special fast-path routines (ExecJust*). * * Complex or uncommon instructions are not implemented in-line in * ExecInterpExpr(), rather we call out to a helper function appearing later *** static Datum ExecJustConst(ExprState *st *** 149,154 --- 147,153 static Datum ExecJustAssignInnerVar(ExprState *state, ExprContext *econtext, bool *isnull); static Datum ExecJustAssignOuterVar(ExprState *state, ExprContext *econtext, bool *isnull); static Datum ExecJustAssignScanVar(ExprState *state, ExprContext *econtext, bool *isnull); + static Datum ExecJustApplyFuncToCase(ExprState *state, ExprContext *econtext, bool *isnull); /* *** ExecReadyInterpretedExpr(ExprState *stat *** 184,193 /* * Select fast-path evalfuncs for very simple expressions. "Starting up" ! * the full interpreter is a measurable overhead for these. Plain Vars ! * and Const seem to be the only ones where the intrinsic cost is small ! * enough that the overhead of ExecInterpExpr matters. For more complex ! * expressions it's cheaper to use ExecInterpExpr always. */ if (state->steps_len == 3) { --- 183,190 /* * Select fast-path evalfuncs for very simple expressions. "Starting up" ! * the full interpreter is a measurable overhead for these, and these ! * patterns occur often enough to be worth optimizing. */ if (state->steps_len == 3) { *** ExecReadyInterpretedExpr(ExprState *stat *** 230,235 --- 227,239 state->evalfunc = ExecJustAssignScanVar; return; } + else if (step0 == EEOP_CASE_TESTVAL && + step1 == EEOP_FUNCEXPR_STRICT && + state->steps[0].d.casetest.value) + { + state->evalfunc = ExecJustApplyFuncToCase; + return; + } } else if (state->steps_len == 2 && state->steps[0].opcode == EEOP_CONST) *** ExecJustAssignScanVar(ExprState *state, *** 1811,1816 --- 1815,1857 return 0; } + /* Evaluate CASE_TESTVAL and apply a strict function to it */ + static Datum + ExecJustApplyFuncToCase(ExprState *state, ExprContext *econtext, bool *isnull) + { + ExprEvalStep *op = &state->steps[0]; + FunctionCallInfo fcinfo; + bool *argnull; + int argno; + Datum d; + + /* + *
Re: [HACKERS] Arrays of domains
On 09/28/2017 05:44 PM, Tom Lane wrote: > > I get these query timings in a non-cassert build: > > HEADPatch > > Q15453.235 ms 5440.876 ms > Q29340.670 ms 10191.194 ms > Q319078.457 ms18967.279 ms > Q448196.338 ms58547.531 ms > > [ analysis elided] > > Assuming that that's going to happen for v11, I'm inclined to leave the > optimization problem until the dust settles around CaseTestExpr. > Does anyone feel that this can't be committed before that's addressed? > > I'm Ok with it as long as we don't forget to revisit this. cheers andrew -- Andrew Dunstanhttps://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
Re: [HACKERS] Arrays of domains
Andrew Dunstan writes: > On 09/28/2017 01:11 PM, Tom Lane wrote: >>> I wonder if we need to do any benchmarking to assure ourselves that the >>> changes to ArrayCoerceExpr don't have a significant performance impact? >> That would likely be a good idea, though I'm not very sure what or >> how to benchmark. > Some case where we only expect the current code to produce a single > ArrayCoerceExpr, I guess. say doing text[] -> int[] ? I spent some time looking into this. I settled on int4[] -> int8[] as the appropriate case to test, because int48() is about as cheap a cast function as we have. Q1 is the base case without a cast: select count(x) from (select array[i,i,i,i,i,i,i,i,i,i] as x from generate_series(1,1000) i) ss; Q2 is same with a cast added: select count(x::int8[]) from (select array[i,i,i,i,i,i,i,i,i,i] as x from generate_series(1,1000) i) ss; Q3 and Q4 are the same thing, but testing 100-element instead of 10-element arrays: select count(x) from (select array[ i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i ] as x from generate_series(1,1000) i) ss; select count(x::int8[]) from (select array[ i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i, i,i,i,i,i,i,i,i,i,i ] as x from generate_series(1,1000) i) ss; I get these query timings in a non-cassert build: HEADPatch Q1 5453.235 ms 5440.876 ms Q2 9340.670 ms 10191.194 ms Q3 19078.457 ms18967.279 ms Q4 48196.338 ms58547.531 ms (Timings are reproducible to a few percent.) So that's a bit disappointing; the per-element overhead is clearly noticeably more than before. However, poking into it with "perf" gives some grounds for optimism; Q4's hotspots with the patch are Children Self Samples Command Shared Object Symbol + 33.44%33.35% 81314 postmaster postgres [.] ExecInterpExpr + 21.88%21.83% 53223 postmaster postgres [.] array_map + 15.19%15.15% 36944 postmaster postgres [.] CopyArrayEls + 14.63%14.60% 35585 postmaster postgres [.] ArrayCastAndSet +6.07% 6.06% 14765 postmaster postgres [.] construct_md_array +1.80% 1.79% 4370 postmaster postgres [.] palloc0 +0.77% 0.77% 1883 postmaster postgres [.] AllocSetAlloc +0.75% 0.74% 1815 postmaster postgres [.] int48 +0.52% 0.52% 1276 postmaster postgres [.] advance_aggregates Surely we could get the amount of time spent in ExecInterpExpr down. One idea is to make a dedicated evalfunc for the case where the expression is just EEOP_CASE_TESTVAL + EEOP_FUNCEXPR[_STRICT], similar to the existing fast-path routines (ExecJust*). I've not actually tried to do that, but a reasonable guess is that it'd about halve that overhead, putting this case back on a par with the HEAD code. Also, I'd imagine that Andres' planned work on JIT-compiled expressions would put this back on par with HEAD, if not better, for installations using that. Also I believe that Andres has plans to revamp the CaseTestExpr mechanism, which might shave a few cycles as well. Right now there's an extra copy of each array datum + isnull value, because array_map sticks those into one pair of variables and then the EEOP_CASE_TESTVAL opcode just moves them somewhere else. It's reasonable to hope that we could redesign that so that array_map sticks the values straight into where they're needed, and then we need only the FUNCEXPR opcode, which'd be a great candidate for an ExecJust* fast-path. Assuming that that's going to happen for v11, I'm inclined to leave the optimization problem until the dust settles around CaseTestExpr. Does anyone feel that this can't be committed before that's addressed? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Arrays of domains
On 09/28/2017 01:11 PM, Tom Lane wrote: > >> I wonder if we need to do any benchmarking to assure ourselves that the >> changes to ArrayCoerceExpr don't have a significant performance impact? > That would likely be a good idea, though I'm not very sure what or > how to benchmark. > > Some case where we only expect the current code to produce a single ArrayCoerceExpr, I guess. say doing text[] -> int[] ? cheers andrew -- Andrew Dunstanhttps://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
Re: [HACKERS] Arrays of domains
Andrew Dunstan writes: > On 08/11/2017 01:17 PM, Tom Lane wrote: >> Attached is a patch series that allows us to create arrays of domain >> types. > 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 Thanks for reviewing! The DatumGetAnyArrayP thing is another artifact of 4bd199465 --- sorry for missing that. > 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. Yeah, perhaps. A lot of these argument lists are long enough that I'm not especially thrilled with the idea of making them one-arg-per-line; that seems like it would consume a lot of vertical space and make it harder to see context in a finite-size window. I think there's been some attempt at grouping the arguments into related groups on single lines, though I concede it's probably not very obvious nor 100% consistent. > I wonder if we need to do any benchmarking to assure ourselves that the > changes to ArrayCoerceExpr don't have a significant performance impact? That would likely be a good idea, though I'm not very sure what or how to benchmark. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 Dunstanhttps://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
Re: [HACKERS] Arrays of domains
I wrote: > Here's a rebased-up-to-HEAD version of this patch set. The only > actual change is removal of a no-longer-needed hunk in pl_exec.c. I see the patch tester is complaining that this broke, due to commit 4bd199465. The fix is trivial (s/GETARG_ANY_ARRAY/GETARG_ANY_ARRAY_P/) but for convenience here's an updated patch set. regards, tom lane diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index 9d75e86..d7db32e 100644 *** a/src/backend/optimizer/prep/preptlist.c --- b/src/backend/optimizer/prep/preptlist.c *** expand_targetlist(List *tlist, int comma *** 306,314 new_expr = coerce_to_domain(new_expr, InvalidOid, -1, atttype, COERCE_IMPLICIT_CAST, -1, - false, false); } else --- 306,314 new_expr = coerce_to_domain(new_expr, InvalidOid, -1, atttype, + COERCION_IMPLICIT, COERCE_IMPLICIT_CAST, -1, false); } else diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c index e79ad26..5a241bd 100644 *** a/src/backend/parser/parse_coerce.c --- b/src/backend/parser/parse_coerce.c *** *** 34,48 static Node *coerce_type_typmod(Node *node, Oid targetTypeId, int32 targetTypMod, ! CoercionForm cformat, int location, ! bool isExplicit, bool hideInputCoercion); static void hide_coercion_node(Node *node); static Node *build_coercion_expression(Node *node, CoercionPathType pathtype, Oid funcId, Oid targetTypeId, int32 targetTypMod, ! CoercionForm cformat, int location, ! bool isExplicit); static Node *coerce_record_to_complex(ParseState *pstate, Node *node, Oid targetTypeId, CoercionContext ccontext, --- 34,49 static Node *coerce_type_typmod(Node *node, Oid targetTypeId, int32 targetTypMod, ! CoercionContext ccontext, CoercionForm cformat, ! int location, ! bool hideInputCoercion); static void hide_coercion_node(Node *node); static Node *build_coercion_expression(Node *node, CoercionPathType pathtype, Oid funcId, Oid targetTypeId, int32 targetTypMod, ! CoercionContext ccontext, CoercionForm cformat, ! int location); static Node *coerce_record_to_complex(ParseState *pstate, Node *node, Oid targetTypeId, CoercionContext ccontext, *** coerce_to_target_type(ParseState *pstate *** 110,117 */ result = coerce_type_typmod(result, targettype, targettypmod, ! cformat, location, ! (cformat != COERCE_IMPLICIT_CAST), (result != expr && !IsA(result, Const))); if (expr != origexpr) --- 111,117 */ result = coerce_type_typmod(result, targettype, targettypmod, ! ccontext, cformat, location, (result != expr && !IsA(result, Const))); if (expr != origexpr) *** coerce_type(ParseState *pstate, Node *no *** 355,361 result = coerce_to_domain(result, baseTypeId, baseTypeMod, targetTypeId, ! cformat, location, false, false); ReleaseSysCache(baseType); --- 355,362 result = coerce_to_domain(result, baseTypeId, baseTypeMod, targetTypeId, ! ccontext, cformat, location, ! false); ReleaseSysCache(baseType); *** coerce_type(ParseState *pstate, Node *no *** 417,436 result = build_coercion_expression(node, pathtype, funcId, baseTypeId, baseTypeMod, ! cformat, location, ! (cformat != COERCE_IMPLICIT_CAST)); /* * If domain, coerce to the domain type and relabel with domain ! * type ID. We can skip the internal length-coercion step if the ! * selected coercion function was a type-and-length coercion. */ if (targetTypeId != baseTypeId) result = coerce_to_domain(result, baseTypeId, baseTypeMod, targetTypeId, ! cformat, location, true, ! exprIsLengthCoercion(result, ! NULL)); } else { --- 418,434 result = build_coercion_expression(node, pathtype, funcId, baseTypeId, baseTypeMod, ! ccontext, cformat, location); /* * If domain, coerce to the domain type and relabel with domain ! * type ID, hiding the previous coercion node. */ if (targetTypeId != baseTypeId) result = coerce_to_domain(result, baseTypeId, baseTypeMod, targetTypeId, ! ccontext, cformat, location, ! true); } else { *** coerce_type(ParseState *pstate, Node *no *** 444,450 * the
Re: [HACKERS] Arrays of domains
I wrote: > Attached is a patch series that allows us to create arrays of domain > types. Here's a rebased-up-to-HEAD version of this patch set. The only actual change is removal of a no-longer-needed hunk in pl_exec.c. regards, tom lane diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index 9d75e86..d7db32e 100644 *** a/src/backend/optimizer/prep/preptlist.c --- b/src/backend/optimizer/prep/preptlist.c *** expand_targetlist(List *tlist, int comma *** 306,314 new_expr = coerce_to_domain(new_expr, InvalidOid, -1, atttype, COERCE_IMPLICIT_CAST, -1, - false, false); } else --- 306,314 new_expr = coerce_to_domain(new_expr, InvalidOid, -1, atttype, + COERCION_IMPLICIT, COERCE_IMPLICIT_CAST, -1, false); } else diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c index e79ad26..5a241bd 100644 *** a/src/backend/parser/parse_coerce.c --- b/src/backend/parser/parse_coerce.c *** *** 34,48 static Node *coerce_type_typmod(Node *node, Oid targetTypeId, int32 targetTypMod, ! CoercionForm cformat, int location, ! bool isExplicit, bool hideInputCoercion); static void hide_coercion_node(Node *node); static Node *build_coercion_expression(Node *node, CoercionPathType pathtype, Oid funcId, Oid targetTypeId, int32 targetTypMod, ! CoercionForm cformat, int location, ! bool isExplicit); static Node *coerce_record_to_complex(ParseState *pstate, Node *node, Oid targetTypeId, CoercionContext ccontext, --- 34,49 static Node *coerce_type_typmod(Node *node, Oid targetTypeId, int32 targetTypMod, ! CoercionContext ccontext, CoercionForm cformat, ! int location, ! bool hideInputCoercion); static void hide_coercion_node(Node *node); static Node *build_coercion_expression(Node *node, CoercionPathType pathtype, Oid funcId, Oid targetTypeId, int32 targetTypMod, ! CoercionContext ccontext, CoercionForm cformat, ! int location); static Node *coerce_record_to_complex(ParseState *pstate, Node *node, Oid targetTypeId, CoercionContext ccontext, *** coerce_to_target_type(ParseState *pstate *** 110,117 */ result = coerce_type_typmod(result, targettype, targettypmod, ! cformat, location, ! (cformat != COERCE_IMPLICIT_CAST), (result != expr && !IsA(result, Const))); if (expr != origexpr) --- 111,117 */ result = coerce_type_typmod(result, targettype, targettypmod, ! ccontext, cformat, location, (result != expr && !IsA(result, Const))); if (expr != origexpr) *** coerce_type(ParseState *pstate, Node *no *** 355,361 result = coerce_to_domain(result, baseTypeId, baseTypeMod, targetTypeId, ! cformat, location, false, false); ReleaseSysCache(baseType); --- 355,362 result = coerce_to_domain(result, baseTypeId, baseTypeMod, targetTypeId, ! ccontext, cformat, location, ! false); ReleaseSysCache(baseType); *** coerce_type(ParseState *pstate, Node *no *** 417,436 result = build_coercion_expression(node, pathtype, funcId, baseTypeId, baseTypeMod, ! cformat, location, ! (cformat != COERCE_IMPLICIT_CAST)); /* * If domain, coerce to the domain type and relabel with domain ! * type ID. We can skip the internal length-coercion step if the ! * selected coercion function was a type-and-length coercion. */ if (targetTypeId != baseTypeId) result = coerce_to_domain(result, baseTypeId, baseTypeMod, targetTypeId, ! cformat, location, true, ! exprIsLengthCoercion(result, ! NULL)); } else { --- 418,434 result = build_coercion_expression(node, pathtype, funcId, baseTypeId, baseTypeMod, ! ccontext, cformat, location); /* * If domain, coerce to the domain type and relabel with domain ! * type ID, hiding the previous coercion node. */ if (targetTypeId != baseTypeId) result = coerce_to_domain(result, baseTypeId, baseTypeMod, targetTypeId, ! ccontext, cformat, location, ! true); } else { *** coerce_type(ParseState *pstate, Node *no *** 444,450 * then we won't need a RelabelType node. */ result = coerce_to_domain(node, InvalidOid, -1, targetTypeId, !
Re: [HACKERS] Arrays of domains
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. regards, tom lane diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index afc733f..277ca8b 100644 *** a/src/backend/optimizer/prep/preptlist.c --- b/src/backend/optimizer/prep/preptlist.c *** expand_targetlist(List *tlist, int comma *** 306,314 new_expr = coerce_to_domain(new_expr, InvalidOid, -1, atttype, COERCE_IMPLICIT_CAST, -1, - false, false); } else --- 306,314 new_expr = coerce_to_domain(new_expr, InvalidOid, -1, atttype, + COERCION_IMPLICIT, COERCE_IMPLICIT_CAST, -1, false); } else diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c index 0bc7dba..c406cea 100644 *** a/src/backend/parser/parse_coerce.c --- b/src/backend/parser/parse_coerce.c *** *** 34,48 static Node *coerce_type_typmod(Node *node, Oid targetTypeId, int32 targetTypMod, ! CoercionForm cformat, int location, ! bool isExplicit, bool hideInputCoercion); static void hide_coercion_node(Node *node); static Node *build_coercion_expression(Node *node, CoercionPathType pathtype, Oid funcId, Oid targetTypeId, int32 targetTypMod, ! CoercionForm cformat, int location, ! bool isExplicit); static Node *coerce_record_to_complex(ParseState *pstate, Node *node, Oid targetTypeId, CoercionContext ccontext, --- 34,49 static Node *coerce_type_typmod(Node *node, Oid targetTypeId, int32 targetTypMod, ! CoercionContext ccontext, CoercionForm cformat, ! int location, ! bool hideInputCoercion); static void hide_coercion_node(Node *node); static Node *build_coercion_ex
Re: [HACKERS] Arrays of domains
On 07/11/2017 12:44 PM, Tom Lane wrote: > > Can anyone think of a reason not to pursue that? > > I'm all for it. cheers andrew -- Andrew Dunstanhttps://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
Re: [HACKERS] Arrays of domains
On Tue, Jul 11, 2017 at 12:44:33PM -0400, Tom Lane wrote: > Over in > https://www.postgresql.org/message-id/877ezgyn60@metapensiero.it > there's a gripe about array_agg() not working for a domain type. > It fails because we don't create an array type over a domain type, > so the parser can't identify a suitable output type for the polymorphic > aggregate. > > We could imagine tweaking the polymorphic-function resolution rules > so that a domain matched to ANYELEMENT is smashed to its base type, > allowing ANYARRAY to be resolved as the base type's array type. > While that would be a pretty localized fix, it seems like a kluge > to me. > > 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. > I did find an argument here: > https://www.postgresql.org/message-id/3c98f7f6.29fe1...@redhat.com > that the SQL spec forbids domains over arrays, but that's the opposite > case (and a restriction we long since ignored, anyway). > > Can anyone think of a reason not to pursue that? +1 for pursuing it. When operations just compose, users get a more fun experience. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Arrays of domains
Over in https://www.postgresql.org/message-id/877ezgyn60@metapensiero.it there's a gripe about array_agg() not working for a domain type. It fails because we don't create an array type over a domain type, so the parser can't identify a suitable output type for the polymorphic aggregate. We could imagine tweaking the polymorphic-function resolution rules so that a domain matched to ANYELEMENT is smashed to its base type, allowing ANYARRAY to be resolved as the base type's array type. While that would be a pretty localized fix, it seems like a kluge to me. 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. I did find an argument here: https://www.postgresql.org/message-id/3c98f7f6.29fe1...@redhat.com that the SQL spec forbids domains over arrays, but that's the opposite case (and a restriction we long since ignored, anyway). Can anyone think of a reason not to pursue that? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers