Problems with "pg.dropped" column after upgrade 9.5 to 9.6

2016-11-01 22:56 GMT+01:00 Pavel Hanák <hanak@is-it.eu>:

Hi,

I've got very the similar problem as described in the message
with the subject "got some errors after upgrade poestgresql from 9.5 to
9.6".

I'll try to describe this very strange behaviour.

I have a table A which has some "pg.dropped" attribute in
pg_attribute table. It looks like:

select attname, attnum
from pg_attribute
where attrelid = A::regclass and attisdropped;

attname | attnum
-------------------------------+--------
........pg.dropped.57........ | 57
(1 row)

Now, I create SQL function doing only update on this table
when the boolean parameter of the function is True:

CREATE OR REPLACE FUNCTION _test_sql_update(in do_update boolean)
RETURNS VOID LANGUAGE sql VOLATILE AS $$
update A
set col = NULL
where do_update;
$$;

Now running:

select _test_sql_update(False);

returns this error:

ERROR: table row type and query-specified row type do not match
DETAIL: Query provides a value for a dropped column at ordinal position
57.
CONTEXT: SQL function "_test_sql_update" statement 1

This is runtime error - this check is evaluated, when function returns one
or more rows

If I don't use the parameter in "where" and instead I use the constant
False directly, everything works:

CREATE OR REPLACE FUNCTION _test_sql_update(in do_update boolean)
RETURNS VOID LANGUAGE sql VOLATILE AS $$
update A
set col = NULL
where False;
$$;

select _test_sql_update(False);

SQL=# _test_sql
-----------

(1 row)

in this case, the check is not evaluated because there is not any row on
result

If I define the function as plpgsql, everything is also working:

CREATE OR REPLACE FUNCTION _test_plpgsql_update(in do_update boolean)
RETURNS VOID LANGUAGE plpgsql VOLATILE AS $$
BEGIN
update A
set col = NULL
where do_update;
END;
$$;

My conclusion is:

The problem occurs only under these circumstances:

- Postgresql 9.6 (no problem in 9.5)

- SQL function doing update

- There is a boolean parameter of the fucntion used in the update command
and the table which is updated has some attisdropped attributes

Can anybody explain what is the problem?

The pipeline of SQL and PLpgSQL functions is pretty different - this is new
regression in 9.6 code.

Regards

Pavel

2016-11-01 22:56 GMT+01:00 Pavel Hanák <hanak@is-it.eu>:

Hi,

I've got very the similar problem as described in the message
with the subject "got some errors after upgrade poestgresql from 9.5 to
9.6".

I'll try to describe this very strange behaviour.

I have a table A which has some "pg.dropped" attribute in
pg_attribute table. It looks like:

select attname, attnum
from pg_attribute
where attrelid = A::regclass and attisdropped;

attname | attnum
-------------------------------+--------
........pg.dropped.57........ | 57
(1 row)

Now, I create SQL function doing only update on this table
when the boolean parameter of the function is True:

CREATE OR REPLACE FUNCTION _test_sql_update(in do_update boolean)
RETURNS VOID LANGUAGE sql VOLATILE AS $$
update A
set col = NULL
where do_update;
$$;

Now running:

select _test_sql_update(False);

returns this error:

ERROR: table row type and query-specified row type do not match
DETAIL: Query provides a value for a dropped column at ordinal position
57.
CONTEXT: SQL function "_test_sql_update" statement 1

If I don't use the parameter in "where" and instead I use the constant
False directly, everything works:

CREATE OR REPLACE FUNCTION _test_sql_update(in do_update boolean)
RETURNS VOID LANGUAGE sql VOLATILE AS $$
update A
set col = NULL
where False;
$$;

select _test_sql_update(False);

SQL=# _test_sql
-----------

(1 row)

If I define the function as plpgsql, everything is also working:

CREATE OR REPLACE FUNCTION _test_plpgsql_update(in do_update boolean)
RETURNS VOID LANGUAGE plpgsql VOLATILE AS $$
BEGIN
update A
set col = NULL
where do_update;
END;
$$;

My conclusion is:

The problem occurs only under these circumstances:

- Postgresql 9.6 (no problem in 9.5)

- SQL function doing update

- There is a boolean parameter of the fucntion used in the update command
and the table which is updated has some attisdropped attributes

Can anybody explain what is the problem?

please, can you send test case - I cannot to reproduce this bug on master.

Regards

Pavel

On Wed, Nov 2, 2016 at 3:32 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2016-11-01 22:56 GMT+01:00 Pavel Hanák <hanak@is-it.eu>:

Can anybody explain what is the problem?

please, can you send test case - I cannot to reproduce this bug on master.

I can reproduce the regression on master and 9.6:
DROP TABLE aa;
CREATE TABLE aa (a int, b int, c int);
CREATE OR REPLACE FUNCTION _test_sql_update(in do_update boolean)
RETURNS VOID LANGUAGE sql VOLATILE AS $$
update aa
set a = NULL
where do_update;
$$;
INSERT INTO aa VALUES (generate_series(1,1,1));
INSERT INTO aa VALUES (generate_series(2,1,1));
INSERT INTO aa VALUES (generate_series(3,1,1));
ALTER TABLE aa DROP COLUMN b;
SELECT _test_sql_update(false);
SELECT _test_sql_update(true);

I am just digging into it... An interesting part is that removing the
WHERE clause makes the regression go away.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Wed, Nov 2, 2016 at 3:54 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

I am just digging into it... An interesting part is that removing the
WHERE clause makes the regression go away.

And one bisect later:
commit: 3fc6e2d7f5b652b417fa6937c34de2438d60fa9f
author: Tom Lane <tgl@sss.pgh.pa.us>
date: Mon, 7 Mar 2016 15:58:22 -0500
Make the upper part of the planner work by generating and comparing Paths

In short, that's going to be fun to create a patch...
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Michael Paquier <michael.paquier@gmail.com> writes:

And one bisect later:
commit: 3fc6e2d7f5b652b417fa6937c34de2438d60fa9f
author: Tom Lane <tgl@sss.pgh.pa.us>
date: Mon, 7 Mar 2016 15:58:22 -0500
Make the upper part of the planner work by generating and comparing Paths

Yeah. The reimplementation I did in createplan.c causes the planner to
generate a targetlist that seems valid in practice, but doesn't pass the
rather simplistic test in ExecCheckPlanOutput. (The latter is expecting
a simple null Const corresponding to a dropped column, but what it's
getting is a Var that references a null Const one plan level down.)

Not sure whether it's reasonable to make the planner jump through extra
hoops here, or complicate the null-for-dropped-cols test in
ExecCheckPlanOutput, or just delete that as not really needed.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Michael Paquier <michael.paquier@gmail.com> writes:

I can reproduce the regression on master and 9.6:
DROP TABLE aa;
CREATE TABLE aa (a int, b int, c int);
CREATE OR REPLACE FUNCTION _test_sql_update(in do_update boolean)
RETURNS VOID LANGUAGE sql VOLATILE AS $$
update aa
set a = NULL
where do_update;
$$;
ALTER TABLE aa DROP COLUMN b;

OK, I looked into this enough to identify exactly where it broke.
The targetlist as set up by preptlist.c looks like

null Const -- value being assigned to a
null Const -- placeholder for dropped b column
Var for c -- to copy updated row's previous c value forward
Var for ctid -- since ModifyTable will need the row TID (resjunk)

The plan tree needs to contain a SeqScan on aa, returning at least values
for c and ctid, plus a Result node that will gate execution based on the
"do_update" parameter, and then a ModifyTable on top. For safety's sake,
the ModifyTable wants to check that the output of its child node matches
the table's current actual rowtype; in particular it wants to see that
the value being put into b is null, since it knows that column is dropped.

In previous releases, the plan tree returned by create_plan() was just
the SeqScan and Result, and the tlists that those output contained just
"c" and "ctid", ie only the required Vars. Then grouping_planner pasted
the final tlist onto just the top plan node (the Result).

As of 9.6, create_plan is aware that it ought to return the final tlist,
and it sets up the SeqScan to do that --- then, more or less as an
afterthought (see create_gating_plan), it pastes a Result on top
producing the same tlist. So at this point we have Const,Const,Var,Var
in both plan nodes, and you'd think that should still be fine. But it
isn't, because setrefs.c processes the Result's tlist to replace Vars
in it with references to the outputs of the child SeqScan. And it will
also notice that the Consts match expressions in the child, and replace
them with Vars. So now the Result's tlist contains four Vars referencing
the child output columns, and that makes ExecCheckPlanOutput unhappy.

The fact that this didn't happen before seems to hinge entirely on the
fact that in prior versions create_plan always returned tlists containing
only Vars, so that there was no opportunity for setrefs.c to match a Const
to lower-level output unless there was more than one level of Plan node
stuck on top of that result within planner.c --- and in an INSERT or
UPDATE, there would be nothing stuck on top except the ModifyTable.

So I'm pretty tempted to fix it as per attached, that is, teach setrefs.c
that it's silly to try to convert a Const into a Var. It is sane to
replace more-complicated expressions with Vars, because that means we save
re-evaluating those expressions. But a quick look at ExecEvalConst and
ExecEvalVar shows that a Const is strictly cheaper to execute than a Var,
so doing such a conversion is actually a pessimization.

This doesn't seem to make for any changes in the core regression tests,
but postgres_fdw notices, eg here:

***************
*** 2450,2456 ****
     Output: (count(c2)), c2, (5), (7.0), (9)
     Sort Key: ft1.c2
     ->  Foreign Scan
!          Output: (count(c2)), c2, (5), (7.0), (9)
           Relations: Aggregate on (public.ft1)
           Remote SQL: SELECT count(c2), c2, 5, 7.0, 9 FROM "S 1"."T 1" GROUP BY
 c2, 5::integer, 9::integer
  (7 rows)
--- 2450,2456 ----
     Output: (count(c2)), c2, (5), (7.0), (9)
     Sort Key: ft1.c2
     ->  Foreign Scan
!          Output: (count(c2)), c2, 5, 7.0, 9
           Relations: Aggregate on (public.ft1)
           Remote SQL: SELECT count(c2), c2, 5, 7.0, 9 FROM "S 1"."T 1" GROUP BY
 c2, 5::integer, 9::integer
  (7 rows)

Losing the indirection doesn't seem like a problem, but I wonder if
anyone's concerned about it.

regards, tom lane

I wrote:

So I'm pretty tempted to fix it as per attached, that is, teach setrefs.c
that it's silly to try to convert a Const into a Var. It is sane to
replace more-complicated expressions with Vars, because that means we save
re-evaluating those expressions. But a quick look at ExecEvalConst and
ExecEvalVar shows that a Const is strictly cheaper to execute than a Var,
so doing such a conversion is actually a pessimization.

After further poking around, I concluded that it'd be a good idea to make
a similar change in set_dummy_tlist_references(), to prevent a failure
if the top plan node below ModifyTable chances to be a Sort or other
non-projecting plan node. I'm not sure such a case can arise today, but
I'm not sure it can't either.

With that, there are a couple more changes in regression test EXPLAIN
output. A full patch that passes "make check-world" is attached.

I'm not sure if it's worth memorializing the specific test case discussed
in this thread as a regression test. It depends enough on the behavior
of SQL function planning that I wouldn't have much confidence in it
continuing to test what we meant it to test going forward.

regards, tom lane

On Thu, Nov 3, 2016 at 2:08 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

After further poking around, I concluded that it'd be a good idea to make
a similar change in set_dummy_tlist_references(), to prevent a failure
if the top plan node below ModifyTable chances to be a Sort or other
non-projecting plan node. I'm not sure such a case can arise today, but
I'm not sure it can't either.

[ ... Back on this thread ... ]
Thanks for the input! It would have taken waaay longer to me to figure
out a clean patch. I am not very familiar with this code :)

I'm not sure if it's worth memorializing the specific test case discussed
in this thread as a regression test. It depends enough on the behavior
of SQL function planning that I wouldn't have much confidence in it
continuing to test what we meant it to test going forward.

Definitely fine to not include it for me, the regression tests
modified by your patch and the git history are enough to understand
the story of the fix.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs