Arrays of domains

On Tue, Jul 11, 2017 at 12:44:33PM -0400, Tom Lane wrote:

Over in
/messages/by-id/877ezgyn60.fsf@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:
/messages/by-id/3C98F7F6.29FE1248@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 <david(at)fetter(dot)org> 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

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

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

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

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

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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,10000000) 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,10000000) 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,10000000) 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,10000000) i) ss;

I get these query timings in a non-cassert build:

HEAD Patch

Q1 5453.235 ms 5440.876 ms
Q2 9340.670 ms 10191.194 ms
Q3 19078.457 ms 18967.279 ms
Q4 48196.338 ms 58547.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

On 09/28/2017 05:44 PM, Tom Lane wrote:

I get these query timings in a non-cassert build:

HEAD Patch

Q1 5453.235 ms 5440.876 ms
Q2 9340.670 ms 10191.194 ms
Q3 19078.457 ms 18967.279 ms
Q4 48196.338 ms 58547.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 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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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:

HEAD Patch + 04.patch

Q1 5453.235 ms 5440.876 ms 5407.965 ms
Q2 9340.670 ms 10191.194 ms 9407.093 ms
Q3 19078.457 ms 18967.279 ms 19050.392 ms
Q4 48196.338 ms 58547.531 ms 48696.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

HEAD Patch (+04)

Q2 46929.728 ms 20646.003 ms
Q4 386200.286 ms 155917.572 ms

regards, tom lane

On 09/29/2017 01:10 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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:

HEAD Patch + 04.patch

Q1 5453.235 ms 5440.876 ms 5407.965 ms
Q2 9340.670 ms 10191.194 ms 9407.093 ms
Q3 19078.457 ms 18967.279 ms 19050.392 ms
Q4 48196.338 ms 58547.531 ms 48696.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

HEAD Patch (+04)

Q2 46929.728 ms 20646.003 ms
Q4 386200.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 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

Hi,

On 2017-09-29 13:10:35 -0400, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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:

HEAD Patch + 04.patch

Q1 5453.235 ms 5440.876 ms 5407.965 ms
Q2 9340.670 ms 10191.194 ms 9407.093 ms
Q3 19078.457 ms 18967.279 ms 19050.392 ms
Q4 48196.338 ms 58547.531 ms 48696.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.

I didn't see this at the time, unfortunately. I'm architecturally
bothered by recursively invoking expression evaluation, but not really
by using CaseTestExpr. I've spent a lot of energy making expression
evaluation non-recursive, and it's also a requirement for a number of
further improvements.

On a read of the thread I didn't find anything along those lines, but
did you consider not using a separate expression state for the
per-element conversion? Something like

EEOP_ARRAYCOERCE_UNPACK
... conversion operations
... including
EEOP_CASE_TESTVAL
... and other things
EEOP_ARRAYCOERCE_PACK

where _UNPACK would set up the ArrayMapState, newly including an
array_iter, and stage the "source" array element for the CaseTest. _PACK
would put processed element into the values array. If _PACK sees there's
further elements, it sets up the new value for the TESTVAL, and jumps to
the step after UNPACK. Otherwise it builds the array and continues.

While that means we'd introduce backward jumps, it'd avoid needing an
expression eval startup for each element, which is a quite substantial
win. It also avoids needing memory from two different expression
contexts, which is what I'd like to avoid right now.

It seems to me that we practically can be certain that the
EEOP_CASE_TESTVAL will be the first step after the
EEOP_ARRAYCOERCE_UNPACK, and that we therefore actually wouldn't ever
need it. The only reason to have the CaseTestExpr really is that it's
otherwise hard to represent the source of the conversion expression in
the expression tree form. At least I don't immediately see a good way to
do so without it. I wonder if it's worth to just optimize it away
during expression "compilation", it's actually easier to understand that
way.

Greetings,

Andres Freund