'update returning *' returns 0 columns instead of empty row with 2 columns when (i) no rows updated and (ii) when applied to a partitioned table with sub-partition

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

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