Re: Text-any concatenation volatility acting as optimization barrier

Hi list,

Andrew Dunstan reported an awkward-seeming case on IRC where shifting
around a concatenation expression in a view made the planner choose a
good or a bad execution plan.

Simplified, it boils down to this:

db=# create table foo(i int);
db=# explain verbose select i from (select i, i::text || 'x' as asd
from foo) as subq;
Seq Scan on public.foo (cost=0.00..34.00 rows=2400 width=4)
Output: foo.i

db=# explain verbose select i from (select i, i || 'x'::text as asd
from foo) as subq;
Subquery Scan on subq (cost=0.00..76.00 rows=2400 width=4)
Output: subq.i
-> Seq Scan on public.foo (cost=0.00..52.00 rows=2400 width=4)
Output: foo.i, ((foo.i)::text || 'x'::text)

Case #1 uses the normal textcat(text, text) operator by automatically
coercing 'x' as text.
However, case #2 uses the anytextcat(anynonarray, text), which is
marked as volatile thus acts as an optimization barrier. Later, the
anytextcat SQL function is inlined and the EXPLAIN VERBOSE output has
no trace of what happened.

Is this something we can, or want, to fix?

One way would be doing preprocess_expression() before
pull_up_subqueries() so function inlining happens earlier, but I can't
imagine what unintended consequences that might have.

Another option would be creating explicit immutable text || foo
operators for common types, but that sounds pretty hacky.

Regards,
Marti

On 02/07/2012 03:18 PM, Marti Raudsepp wrote:
> Hi list,
>
> Andrew Dunstan reported an awkward-seeming case on IRC where shifting
> around a concatenation expression in a view made the planner choose a
> good or a bad execution plan.
>
> Simplified, it boils down to this:
>
> db=# create table foo(i int);
> db=# explain verbose select i from (select i, i::text || 'x' as asd
> from foo) as subq;
> Seq Scan on public.foo (cost=0.00..34.00 rows=2400 width=4)
> Output: foo.i
>
> db=# explain verbose select i from (select i, i || 'x'::text as asd
> from foo) as subq;
> Subquery Scan on subq (cost=0.00..76.00 rows=2400 width=4)
> Output: subq.i
> -> Seq Scan on public.foo (cost=0.00..52.00 rows=2400 width=4)
> Output: foo.i, ((foo.i)::text || 'x'::text)
>
> Case #1 uses the normal textcat(text, text) operator by automatically
> coercing 'x' as text.
> However, case #2 uses the anytextcat(anynonarray, text), which is
> marked as volatile thus acts as an optimization barrier. Later, the
> anytextcat SQL function is inlined and the EXPLAIN VERBOSE output has
> no trace of what happened.
>
> Is this something we can, or want, to fix?
>
> One way would be doing preprocess_expression() before
> pull_up_subqueries() so function inlining happens earlier, but I can't
> imagine what unintended consequences that might have.
>
> Another option would be creating explicit immutable text || foo
> operators for common types, but that sounds pretty hacky.
>

It gets worse if you replace the expression with a call to a (non-sql)
function returning text, which was in fact the original use case. Then
you're pretty much hosed.

cheers

andrew

On Tue, Feb 7, 2012 at 22:31, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> It gets worse if you replace the expression with a call to a (non-sql)
> function returning text, which was in fact the original use case. Then
> you're pretty  much hosed.

Oh, if it's a non-SQL function then marking it as STABLE/IMMUTABLE
should solve it -- did you try that?

Regards,
Marti

On 02/07/2012 03:36 PM, Marti Raudsepp wrote:
> On Tue, Feb 7, 2012 at 22:31, Andrew Dunstan<andrew(at)dunslane(dot)net> wrote:
>> It gets worse if you replace the expression with a call to a (non-sql)
>> function returning text, which was in fact the original use case. Then
>> you're pretty much hosed.
> Oh, if it's a non-SQL function then marking it as STABLE/IMMUTABLE
> should solve it -- did you try that?
>

Hmm, your're right. I could have sworn I tried that. That still leaves
the oddity you reported, though.

cheers

andrew

Marti Raudsepp <marti(at)juffo(dot)org> writes:
> Case #1 uses the normal textcat(text, text) operator by automatically
> coercing 'x' as text.
> However, case #2 uses the anytextcat(anynonarray, text), which is
> marked as volatile thus acts as an optimization barrier.

Hmm ... since those operators were invented (in 8.3), we have adopted a
policy that I/O functions are presumed to be no worse than stable:
http://archives.postgresql.org/pgsql-committers/2010-07/msg00307.php
ISTM that would justify relabeling anytextcat/textanycat as stable,
which should fix this.

> One way would be doing preprocess_expression() before
> pull_up_subqueries() so function inlining happens earlier, but I can't
> imagine what unintended consequences that might have.

I'm pretty sure that would be a bad idea.

regards, tom lane

On Wed, Feb 8, 2012 at 06:21, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marti Raudsepp <marti(at)juffo(dot)org> writes:
>> Case #1 uses the normal textcat(text, text) operator by automatically
>> coercing 'x' as text.
>> However, case #2 uses the anytextcat(anynonarray, text), which is
>> marked as volatile thus acts as an optimization barrier.
>
> Hmm ... since those operators were invented (in 8.3), we have adopted a
> policy that I/O functions are presumed to be no worse than stable:
> http://archives.postgresql.org/pgsql-committers/2010-07/msg00307.php
> ISTM that would justify relabeling anytextcat/textanycat as stable,
> which should fix this.

Yes, we should definitely take advantage of that.

I scanned through all of pg_proc, there are 4 functions like this that
can be changed: textanycat, anytextcat, quote_literal and
quote_nullable. All of these have SQL wrappers to cast their argument
to ::text.

quote_literal | select pg_catalog.quote_literal($1::pg_catalog.text)
quote_nullable | select pg_catalog.quote_nullable($1::pg_catalog.text)
textanycat | select $1 || $2::pg_catalog.text
anytextcat | select $1::pg_catalog.text || $2

Patch attached (in git am format). Passes all regression tests (except
'json' which fails on my machine even on git master).

No documentation changes necessary AFAICT.

Regards,
Marti

On Wed, Feb 8, 2012 at 4:53 AM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
> Patch attached (in git am format). Passes all regression tests (except
> 'json' which fails on my machine even on git master).

Can you provide the diffs from the json test on your machine? I don't
see any build-farm failures off-hand...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Feb 8, 2012 at 4:53 AM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
>> Patch attached (in git am format). Passes all regression tests (except
>> 'json' which fails on my machine even on git master).

> Can you provide the diffs from the json test on your machine? I don't
> see any build-farm failures off-hand...

I'm seeing diffs too after applying Marti's patch: instead of "z", "b",
etc, the field labels in the json values look like "f1", "f2", etc in
the output of queries such as

SELECT row_to_json(q)
FROM (SELECT $$a$$ || x AS b,
y AS c,
ARRAY[ROW(x.*,ARRAY[1,2,3]),
ROW(y.*,ARRAY[4,5,6])] AS z
FROM generate_series(1,2) x,
generate_series(4,5) y) q;

I believe what is happening is that now that the planner can flatten the
sub-select, the RowExprs are getting expanded differently, and that ties
into the "when do we lose column names" business that Andrew has been
on about.

However, I was not seeing that before applying the patch, so maybe Marti
has another issue too.

I am going to go ahead and commit the patch with the altered json
results, because IMO it is mere accident that these regression cases
were coming out with "nice" field labels anyway. When and if Andrew
gets the RowExpr cases fixed properly, that will show up as these cases
going back to nicer-looking output.

regards, tom lane

On Wed, Feb 8, 2012 at 18:20, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Feb 8, 2012 at 4:53 AM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
>>> Patch attached (in git am format). Passes all regression tests (except
>>> 'json' which fails on my machine even on git master).
>
>> Can you provide the diffs from the json test on your machine?  I don't
>> see any build-farm failures off-hand...
>
> I'm seeing diffs too after applying Marti's patch: instead of "z", "b",
> etc, the field labels in the json values look like "f1", "f2", etc in
> the output of queries such as

Sorry, my bad. I guess I got the two versions (patched and unpatched)
mixed up. Indeed this difference disappears when I remove my patch.
I'll have to be more careful when checking regression diffs in the
future. :)

Regards,
Marti

On 02/08/2012 11:20 AM, Tom Lane wrote:
> I am going to go ahead and commit the patch with the altered json
> results, because IMO it is mere accident that these regression cases
> were coming out with "nice" field labels anyway. When and if Andrew
> gets the RowExpr cases fixed properly, that will show up as these
> cases going back to nicer-looking output.

I take it this is your way of making me hurry up with that work :-)

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 02/08/2012 11:20 AM, Tom Lane wrote:
>> I am going to go ahead and commit the patch with the altered json
>> results, because IMO it is mere accident that these regression cases
>> were coming out with "nice" field labels anyway. When and if Andrew
>> gets the RowExpr cases fixed properly, that will show up as these
>> cases going back to nicer-looking output.

> I take it this is your way of making me hurry up with that work :-)

Well, right now the behavior is more consistent than it was before;
we would surely have gotten questions about it as it was. Whether
you find time to improve it or not isn't my concern at the moment.

BTW, the order of the items in the json output doesn't match the column
order of the sub-select, with or without the patch. This seems ... odd.
Is it intentional?

regards, tom lane