Обсуждение: 'update returning *' returns 0 columns instead of empty row with 2columns when (i) no rows updated and (ii) when applied to a partitioned tablewith sub-partition

Hello,

The following code snippet demonstrates the problem: the first select
passes and the second [select * from testf(FALSE)] fails. I would expect
that select * from testf(...); works without errors in both cases.

begin;

create table test (id integer, data char(1)) partition by list (id)
tablespace pg_default;
create table test_1 partition of test for values in (1) partition by
list (data);
create table test_1_a partition of test_1 for values in ('a');
create function testf(p boolean) returns setof test language 'plpgsql'
as $body$ begin return query update test set id=id where p returning *;
end; $body$;
insert into test (id, data) values (1, 'a');
select * from testf(TRUE);
select * from testf(FALSE);

rollback;   

The result:

ERROR: structure of query does not match function result type

SQL state: 42804

Detail: Number of returned columns (0) does not match expected column
count (2).

Context: PL/pgSQL function testf(boolean) line 1 at RETURN QUERY


I'm on  PostgreSQL 11.1 on x86_64-pc-linux-gnu, compiled by gcc (GCC)
4.8.5 20150623 (Red Hat 4.8.5-28), 64-bit on Centos 7

uname -a gives   Linux 3.10.0-862.11.6.el7.x86_64 #1 SMP Tue Aug 14
21:49:04 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

Hi,

On 2019/02/01 23:32, Petr Fedorov wrote:
> Hello,
> 
> The following code snippet demonstrates the problem: the first select
> passes and the second [select * from testf(FALSE)] fails. I would expect
> that select * from testf(...); works without errors in both cases.
> 
> begin;
> 
> create table test (id integer, data char(1)) partition by list (id)
> tablespace pg_default;
> create table test_1 partition of test for values in (1) partition by
> list (data);
> create table test_1_a partition of test_1 for values in ('a');
> create function testf(p boolean) returns setof test language 'plpgsql'
> as $body$ begin return query update test set id=id where p returning *;
> end; $body$;
> insert into test (id, data) values (1, 'a');
> select * from testf(TRUE);
> select * from testf(FALSE);
> 
> rollback;   
> 
> The result:
> 
> ERROR: structure of query does not match function result type
> 
> SQL state: 42804
> 
> Detail: Number of returned columns (0) does not match expected column
> count (2).
> 
> Context: PL/pgSQL function testf(boolean) line 1 at RETURN QUERY

Thanks for the report.  There indeed appears to be a bug here.

The problem seems to be with how planner handles an empty plan (due to
constant-FALSE qual) when the target table is an inheritance tree.  OP's
example contains a partitioned table, but I could reproduce it with
regular inheritance:

create table parent (id int);
create table child () inherits (parent);
create or replace function testf(p boolean) returns setof parent
  language 'plpgsql' as $body$
    begin
      return query update parent set id = id where p returning *;
    end;
$body$;

select * from testf(true);
 id
────
(0 rows)

select * from testf(false);
ERROR:  structure of query does not match function result type
DETAIL:  Number of returned columns (0) does not match expected column
count (1).
CONTEXT:  PL/pgSQL function testf(boolean) line 1 at RETURN QUERY

No problem when there is no inheritance:

drop function testf;
create table foo (like parent);
create or replace function testf(p boolean) returns setof foo
  language 'plpgsql' as $body$
    begin
      return query update foo set id = id where p returning *;
    end;
$body$;
select * from testf(false);
 id
────
(0 rows)


Mismatch between the query result type and the function result type occurs
in the inheritance case, because the targetlist of the plan for the UPDATE
query in testf's body is empty, whereas the function execution code
(pl_exec.c) expects it match the function's result type (set of parent).
It's empty because inheritance_planner sets an empty Result path when it
finds that all the children are excluded, but hasn't generated enough
state in the path's RelOptInfo and PlannerInfo such that the correct
targetlist could be set in the empty Result plan that's eventually created.

Attached patch seems to fix it.  It also adds a test in inherit.sql.

Thoughts?

Thanks,
Amit

On 2019/02/06 16:35, Amit Langote wrote:
> Hi,
> 
> On 2019/02/01 23:32, Petr Fedorov wrote:
>> Hello,
>>
>> The following code snippet demonstrates the problem: the first select
>> passes and the second [select * from testf(FALSE)] fails. I would expect
>> that select * from testf(...); works without errors in both cases.
>>
>> begin;
>>
>> create table test (id integer, data char(1)) partition by list (id)
>> tablespace pg_default;
>> create table test_1 partition of test for values in (1) partition by
>> list (data);
>> create table test_1_a partition of test_1 for values in ('a');
>> create function testf(p boolean) returns setof test language 'plpgsql'
>> as $body$ begin return query update test set id=id where p returning *;
>> end; $body$;
>> insert into test (id, data) values (1, 'a');
>> select * from testf(TRUE);
>> select * from testf(FALSE);
>>
>> rollback;   
>>
>> The result:
>>
>> ERROR: structure of query does not match function result type
>>
>> SQL state: 42804
>>
>> Detail: Number of returned columns (0) does not match expected column
>> count (2).
>>
>> Context: PL/pgSQL function testf(boolean) line 1 at RETURN QUERY
> 
> Thanks for the report.  There indeed appears to be a bug here.
> 
> The problem seems to be with how planner handles an empty plan (due to
> constant-FALSE qual) when the target table is an inheritance tree.  OP's
> example contains a partitioned table, but I could reproduce it with
> regular inheritance:
> 
> create table parent (id int);
> create table child () inherits (parent);
> create or replace function testf(p boolean) returns setof parent
>   language 'plpgsql' as $body$
>     begin
>       return query update parent set id = id where p returning *;
>     end;
> $body$;
> 
> select * from testf(true);
>  id
> ────
> (0 rows)
> 
> select * from testf(false);
> ERROR:  structure of query does not match function result type
> DETAIL:  Number of returned columns (0) does not match expected column
> count (1).
> CONTEXT:  PL/pgSQL function testf(boolean) line 1 at RETURN QUERY
> 
> No problem when there is no inheritance:
> 
> drop function testf;
> create table foo (like parent);
> create or replace function testf(p boolean) returns setof foo
>   language 'plpgsql' as $body$
>     begin
>       return query update foo set id = id where p returning *;
>     end;
> $body$;
> select * from testf(false);
>  id
> ────
> (0 rows)
> 
> 
> Mismatch between the query result type and the function result type occurs
> in the inheritance case, because the targetlist of the plan for the UPDATE
> query in testf's body is empty, whereas the function execution code
> (pl_exec.c) expects it match the function's result type (set of parent).
> It's empty because inheritance_planner sets an empty Result path when it
> finds that all the children are excluded, but hasn't generated enough
> state in the path's RelOptInfo and PlannerInfo such that the correct
> targetlist could be set in the empty Result plan that's eventually created.
> 
> Attached patch seems to fix it.  It also adds a test in inherit.sql.
> 
> Thoughts?

Will add this to next CF.

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2019/02/01 23:32, Petr Fedorov wrote:
>> ERROR: structure of query does not match function result type

> Thanks for the report.  There indeed appears to be a bug here.

Yup, for sure.  You don't actually need a function at all to see
that there's a problem: if you just execute
    UPDATE ... WHERE false RETURNING some-columns;
you will notice that the emitted resultset has zero columns.

> Attached patch seems to fix it.  It also adds a test in inherit.sql.

This isn't quite right, because what we actually need to return is the
RETURNING column set.  If you only check "RETURNING *" then you might not
notice the difference, but with anything else it's obviously wrong.
I propose the attached modification instead.

            regards, tom lane

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 5579dfa..93d9448 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1585,10 +1585,19 @@ inheritance_planner(PlannerInfo *root)

     /*
      * If we managed to exclude every child rel, return a dummy plan; it
-     * doesn't even need a ModifyTable node.
+     * doesn't even need a ModifyTable node.  But, if the query has RETURNING,
+     * we need to cons up a suitable pathtarget, else the finished plan will
+     * appear to return the wrong column set.
      */
     if (subpaths == NIL)
     {
+        if (parse->returningList)
+        {
+            final_rel->reltarget = create_pathtarget(root,
+                                                     parse->returningList);
+            /* hack to keep createplan.c from breaking things: */
+            root->processed_tlist = parse->returningList;
+        }
         set_dummy_rel_pathlist(final_rel);
         return;
     }
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index f259d07..9d610b8 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -539,6 +539,41 @@ CREATE TEMP TABLE z (b TEXT, PRIMARY KEY(aa, b)) inherits (a);
 INSERT INTO z VALUES (NULL, 'text'); -- should fail
 ERROR:  null value in column "aa" violates not-null constraint
 DETAIL:  Failing row contains (null, text).
+-- Check inherited UPDATE with all children excluded
+create table some_tab (a int, b int);
+create table some_tab_child () inherits (some_tab);
+insert into some_tab_child values(1,2);
+explain (verbose, costs off)
+update some_tab set a = a + 1 where false;
+        QUERY PLAN
+--------------------------
+ Result
+   One-Time Filter: false
+(2 rows)
+
+update some_tab set a = a + 1 where false;
+explain (verbose, costs off)
+update some_tab set a = a + 1 where false returning b, a;
+            QUERY PLAN
+----------------------------------
+ Result
+   Output: some_tab.b, some_tab.a
+   One-Time Filter: false
+(3 rows)
+
+update some_tab set a = a + 1 where false returning b, a;
+ b | a
+---+---
+(0 rows)
+
+table some_tab;
+ a | b
+---+---
+ 1 | 2
+(1 row)
+
+drop table some_tab cascade;
+NOTICE:  drop cascades to table some_tab_child
 -- Check UPDATE with inherited target and an inherited source table
 create temp table foo(f1 int, f2 int);
 create temp table foo2(f3 int) inherits (foo);
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 425052c..5480fe7 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -97,6 +97,21 @@ SELECT relname, d.* FROM ONLY d, pg_class where d.tableoid = pg_class.oid;
 CREATE TEMP TABLE z (b TEXT, PRIMARY KEY(aa, b)) inherits (a);
 INSERT INTO z VALUES (NULL, 'text'); -- should fail

+-- Check inherited UPDATE with all children excluded
+create table some_tab (a int, b int);
+create table some_tab_child () inherits (some_tab);
+insert into some_tab_child values(1,2);
+
+explain (verbose, costs off)
+update some_tab set a = a + 1 where false;
+update some_tab set a = a + 1 where false;
+explain (verbose, costs off)
+update some_tab set a = a + 1 where false returning b, a;
+update some_tab set a = a + 1 where false returning b, a;
+table some_tab;
+
+drop table some_tab cascade;
+
 -- Check UPDATE with inherited target and an inherited source table
 create temp table foo(f1 int, f2 int);
 create temp table foo2(f3 int) inherits (foo);

On 2019/02/22 7:18, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> On 2019/02/01 23:32, Petr Fedorov wrote:
>>> ERROR: structure of query does not match function result type
> 
>> Thanks for the report.  There indeed appears to be a bug here.
> 
> Yup, for sure.  You don't actually need a function at all to see
> that there's a problem: if you just execute
>     UPDATE ... WHERE false RETURNING some-columns;
> you will notice that the emitted resultset has zero columns.

Ah, indeed.

>> Attached patch seems to fix it.  It also adds a test in inherit.sql.
> 
> This isn't quite right, because what we actually need to return is the
> RETURNING column set.  If you only check "RETURNING *" then you might not
> notice the difference, but with anything else it's obviously wrong.
>
> I propose the attached modification instead.

Looks good.

I know this code may not be with us forever, but I wonder why the plan
shape looks different for an empty update on a regular table vs
inheritance tree.  For regular table, it's ModifyTable on top of a dummy
Result node, whereas it's just dummy Result node for the latter.  If the
plan shape for the two cases had matched, we wouldn't have this bug at all
or we'd have it for both cases (in slightly different form for the regular
table case).  To check I patched grouping_planner() to not add a
ModifyTable on top of a dummy result path for a regular table and the
resulting target list is not quite right (what you get with my patch
upthread):

create table foo (a int);
update foo set a = a where false returning a+1 as b;
 a
───
(0 rows)

explain verbose update foo set a = a where false returning a+1 as b;
                QUERY PLAN
───────────────────────────────────────────
 Result  (cost=0.00..0.00 rows=0 width=10)
   Output: a, ctid
   One-Time Filter: false
(3 rows)

Also, I noticed regression test failure having to do with statement
triggers not firing, which makes sense, as there's no ModifyTable to
invoke them.

That means we have a bug (?) today that statement triggers of inheritance
parent tables don't fire when it's an empty update/delete.

create table parent (a int, b int);
create table child () inherits (parent);
create or replace function before_stmt_notice() returns trigger as $$
begin raise notice 'updating %', TG_TABLE_NAME; return null; end; $$
language plpgsql;
create trigger before_stmt_trigger before update on parent execute
function before_stmt_notice();

-- trigger doesn't fire
update parent set a = a where false returning a+1 as b;
──
(0 rows)

It does fire for an empty update on a regular table (with HEAD I mean)

create trigger before_stmt_trigger before update on foo execute function
before_stmt_notice();
update foo set a = a where false returning a+1 as b;
NOTICE:  updating foo
 b
───
(0 rows)


Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> Also, I noticed regression test failure having to do with statement
> triggers not firing, which makes sense, as there's no ModifyTable to
> invoke them.

Oh!  You are right, that's a separate bug.  So really we can't have
this fast-path exit at all, we should produce a ModifyTable node in
every case.

I'm too tired to work on that anymore today, do you want to run
with it?

            regards, tom lane

On 2019/02/22 13:43, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> Also, I noticed regression test failure having to do with statement
>> triggers not firing, which makes sense, as there's no ModifyTable to
>> invoke them.
> 
> Oh!  You are right, that's a separate bug.  So really we can't have
> this fast-path exit at all, we should produce a ModifyTable node in
> every case.
> 
> I'm too tired to work on that anymore today, do you want to run
> with it?

Sure, see attached a patch.

To fix the trigger bug, we'll be putting a minimally valid-looking
ModifyTable node on top of a dummy plan.  So, the bug reported on this
thread is taken care of automatically, because ModifyTable plan's
targetlist is already set correctly.

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2019/02/22 13:43, Tom Lane wrote:
>> I'm too tired to work on that anymore today, do you want to run
>> with it?

> Sure, see attached a patch.

OK, I prettified this a bit and pushed it.

            regards, tom lane

On Sat, Feb 23, 2019 at 2:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> > On 2019/02/22 13:43, Tom Lane wrote:
> >> I'm too tired to work on that anymore today, do you want to run
> >> with it?
>
> > Sure, see attached a patch.
>
> OK, I prettified this a bit and pushed it.

Thank you.

Regards,
Amit