Re: [HACKERS] Arrays of domains

Hi,

On 2017-09-29 13:10:35 -0400, Tom Lane wrote:
> Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)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