Thread: BUG #15471: psql 11 array concatenation in CASE takes on values fromthe CASE expression when using enum_range

The following bug has been logged on the website:

Bug reference:      15471
Logged by:          Matt Williams
Email address:      pg@mattyw.net
PostgreSQL version: 11.0
Operating system:   Confirmed on MacOs and Alpine Linux
Description:

Below is an example .sql file that replicates the problem. Put simply, when
we array concat with enum_range in the result of a CASE statement the
concatenation takes the expression from the CASE statement, not the enum
range.

if you run the below .sql file against postgres 10 and 11 you'll see ex1 and
ex2 return the same array. However ex3,4,5 all show arrays concatenated with
the value in the CASE condition (TRUE, 'true' and 1). They all return the
expected array under postgres 9.6 and 10. The change only appears in
postgres 11.
    
The last example (ex6) shows the CASE rewritten to remove the expression,
ex6 works the same under postgres 10 and 11.

These results for ex 3, 4 and 5 were surprising, I couldn't find anything in
the docs that alludes to this change, and I've been unable to find any
online discussion regarding this.

--- Start of sql file ---
SELECT version();

DROP TYPE IF EXISTS myenum;

CREATE TYPE myenum AS ENUM ('e', 'f', 'g');

SELECT enum_range(NULL::myenum);

SELECT ARRAY['a', 'b', 'c', 'd'] || enum_range(NULL::myenum)::text[] as
ex1;

SELECT
    CASE TRUE
        WHEN TRUE THEN ARRAY['a', 'b', 'c', 'd'] || ARRAY['e', 'f', 'g']
        WHEN FALSE THEN ARRAY['a', 'b', 'c', 'd']
END as ex2;

-- All of the above works as expected

SELECT
    CASE TRUE
        WHEN TRUE THEN ARRAY['a', 'b', 'c', 'd'] ||
enum_range(NULL::myenum)::text[]
        WHEN FALSE THEN ARRAY['a', 'b', 'c', 'd']
END as ex3;

-- In the above case statement we'd expected the output: {a,b,c,d,e,f,g}
-- However under postres 11 we get the following output: {a,b,c,d,t,t,t}


SELECT
    CASE 'true'
        WHEN 'true' THEN ARRAY['a', 'b', 'c', 'd'] ||
enum_range(NULL::myenum)::text[]
        WHEN 'false' THEN ARRAY['a', 'b', 'c', 'd']
END as ex4;

-- Postgres 10: {a,b,c,d,e,f,g}
-- Postgres 11: {a,b,c,d,true,true,true}


SELECT
    CASE 1
        WHEN 1 THEN ARRAY['a', 'b', 'c', 'd'] ||
enum_range(NULL::myenum)::text[]
        WHEN 2 THEN ARRAY['a', 'b', 'c', 'd']
END as ex5;

-- Postgres 10: {a,b,c,d,e,f,g}
-- Postgres 11: {a,b,c,d,1,1,1}

SELECT
    CASE
        WHEN TRUE THEN ARRAY['a', 'b', 'c', 'd'] ||
enum_range(NULL::myenum)::text[]
        ELSE ARRAY['a', 'b', 'c', 'd']
END as ex6;

-- In this form we get the same answer for both postgres 10 and 11:
{a,b,c,d,e,f,g}

=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> Below is an example .sql file that replicates the problem. Put simply, when
> we array concat with enum_range in the result of a CASE statement the
> concatenation takes the expression from the CASE statement, not the enum
> range.

Wow, that's ... bizarre.  I'm thinking that we probably did something
silly in the big expression-execution rewrite, but it's not clear exactly
where.  Anyway, will look into it if Andres doesn't beat me to it.

Some poking at the examples finds that it seems to be necessary to have
the coercion to text[] to show the problem, eg this seems fine:

SELECT
  CASE 1
    WHEN 1 THEN ARRAY['e', 'f']::myenum[] || enum_range(NULL::myenum)
    WHEN 2 THEN ARRAY['f', 'g']::myenum[]
END;

What's really strange is that EXPLAIN VERBOSE shows that the planner has
correctly elided the CASE altogether in each case, eg for ex5:

explain verbose SELECT
  CASE 1
    WHEN 1 THEN ARRAY['a', 'b', 'c', 'd'] || enum_range(NULL::myenum)::text[]
    WHEN 2 THEN ARRAY['a', 'b', 'c', 'd']
END as ex5;
                              QUERY PLAN                               
-----------------------------------------------------------------------
 Result  (cost=0.00..0.07 rows=1 width=32)
   Output: ('{a,b,c,d}'::text[] || (enum_range(NULL::myenum))::text[])
(2 rows)

So how come it's still affecting the result of the array coercion?

Looking at the generated plan tree with debug_print_plan narrows
things down quite a bit: the array coercion looks like, eg,

               {ARRAYCOERCEEXPR 
               :arg 
                  {FUNCEXPR 
                  :funcid 3531 
                  :funcresulttype 197116 
                  :funcretset false 
                  :funcvariadic false 
                  :funcformat 0 
                  :funccollid 0 
                  :inputcollid 0 
                  :args (
                     {CONST 
                     :consttype 197117 
                     :consttypmod -1 
                     :constcollid 0 
                     :constlen 4 
                     :constbyval true 
                     :constisnull true 
                     :location 72 
                     :constvalue <>
                     }
                  )
                  :location 61
                  }
               :elemexpr 
                  {COERCEVIAIO 
                  :arg 
                     {CONST 
                     :consttype 23 
                     :consttypmod -1 
                     :constcollid 0 
                     :constlen 4 
                     :constbyval true 
                     :constisnull false 
                     :location 14 
                     :constvalue 4 [ 1 0 0 0 0 0 0 0 ]
                     }
                  :resulttype 25 
                  :resultcollid 100 
                  :coerceformat 1 
                  :location 85
                  }
               :resulttype 1009 
               :resulttypmod -1 
               :resultcollid 100 
               :coerceformat 1 
               :location 85
               }

So somehow the planner is messing up and inserting an outer CaseTestExpr
value as the source of the elemexpr's coercion expression.  IIRC,
ruleutils doesn't print the elemexpr at all, which is how come we're not
seeing the mistake in the EXPLAIN output.

However, ArrayCoerceExpr has abused CaseTestExpr that way for awhile, so
it's still not very clear why it broke in v11 and not before ...

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> Below is an example .sql file that replicates the problem. Put
 >> simply, when we array concat with enum_range in the result of a CASE
 >> statement the concatenation takes the expression from the CASE
 >> statement, not the enum range.

 Tom> Wow, that's ... bizarre. I'm thinking that we probably did
 Tom> something silly in the big expression-execution rewrite, but it's
 Tom> not clear exactly where. Anyway, will look into it if Andres
 Tom> doesn't beat me to it.

I took a look, and what I'm seeing suggests that commit 3decd150a2d
might possibly be relevant here (at least to explain why it breaks in 11
but not 10).

What's going on in eval_const_expressions_mutator is that
context->case_val is set when recursing into the elemexpr in the
ArrayCoerceExpr case, so when that sees a CaseTestExpr inside that, it
replaces it (incorrectly).

-- 
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> Wow, that's ... bizarre. I'm thinking that we probably did
>  Tom> something silly in the big expression-execution rewrite, but it's
>  Tom> not clear exactly where. Anyway, will look into it if Andres
>  Tom> doesn't beat me to it.

> I took a look, and what I'm seeing suggests that commit 3decd150a2d
> might possibly be relevant here (at least to explain why it breaks in 11
> but not 10).

Good guess but I don't think that changed anything.  It looks to me
like the culprit is commit c12d570fa, so it's my bug not Andres' :-(.
Before that, we weren't abusing CaseTestExpr as part of ArrayCoerceExpr.

Probably, the fix is to have eval_const_expressions be sure to
save/clear/restore context->case_val when dealing with ArrayCoerceExpr's
elemexpr.  That possibly does mean that we have to undo the generic
processing of ArrayCoerceExpr, but it was already broken before that
transformation.

This is all some more fuel for the idea that we need a less messy
substitute for CaseTestExpr.  As it happens, I was just fooling with
that yesterday, and hope to have something to post soon.  But it'll
be too invasive to back-patch.  I'll try to fix this particular
problem more locally.

            regards, tom lane

Hi,

On 2018-10-30 07:59:35 -0400, Tom Lane wrote:
> =?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> > Below is an example .sql file that replicates the problem. Put simply, when
> > we array concat with enum_range in the result of a CASE statement the
> > concatenation takes the expression from the CASE statement, not the enum
> > range.
> 
> Wow, that's ... bizarre.  I'm thinking that we probably did something
> silly in the big expression-execution rewrite, but it's not clear exactly
> where.

It can't be "the big one", because then it'd be in 10 too, right?

> So somehow the planner is messing up and inserting an outer CaseTestExpr
> value as the source of the elemexpr's coercion expression.  IIRC,
> ruleutils doesn't print the elemexpr at all, which is how come we're not
> seeing the mistake in the EXPLAIN output.
> 
> However, ArrayCoerceExpr has abused CaseTestExpr that way for awhile, so
> it's still not very clear why it broke in v11 and not before ...

Without further analysis, the only commit that looks midly relevant is

commit c12d570fa147d0ec273df53de3a2802925d551ba
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   2017-09-30 13:40:56 -0400

    Support arrays over domains.

Greetings,

Andres Freund

Hi,

On 2018-10-30 09:30:54 -0400, Tom Lane wrote:
> Good guess but I don't think that changed anything.  It looks to me
> like the culprit is commit c12d570fa, so it's my bug not Andres' :-(.

:)



> This is all some more fuel for the idea that we need a less messy
> substitute for CaseTestExpr.  As it happens, I was just fooling with
> that yesterday, and hope to have something to post soon.  But it'll
> be too invasive to back-patch.  I'll try to fix this particular
> problem more locally.

Yea, I agree we need something better there.  I happened to also play
around with that (more from the angle that we'd want similar
infrastructure for the SQL function inlining case we talked about) on
the long flights back from .eu yesterday, but didn't get that far. Will
wait till you have posted something.

Greetings,

Andres Freund