Thread: fill_extraUpdatedCols is done in completely the wrong place

I happened to notice $subject while working on the release notes.
AFAICS, it is 100% inappropriate for the parser to compute the
set of generated columns affected by an UPDATE, because that set
could change before execution.  It would be really easy to break
this for an UPDATE in a stored rule, for example.

I think that that processing should be done by the planner, instead.
I don't object too much to keeping the data in RTEs ... but there had
better be a bold annotation that the set is not valid till after
planning.

An alternative solution is to keep the set in some executor data structure
and compute it during executor startup; perhaps near to where the
expressions are prepared for execution, so as to save extra stringToNode
calls.

            regards, tom lane

On 2020-05-08 21:05, Tom Lane wrote:
> I happened to notice $subject while working on the release notes.
> AFAICS, it is 100% inappropriate for the parser to compute the
> set of generated columns affected by an UPDATE, because that set
> could change before execution.  It would be really easy to break
> this for an UPDATE in a stored rule, for example.

Do you have a specific situation in mind?  How would a rule change the 
set of columns updated by a query?  Something involving CTEs?  Having a 
test case would be good.

> I think that that processing should be done by the planner, instead.
> I don't object too much to keeping the data in RTEs ... but there had
> better be a bold annotation that the set is not valid till after
> planning.
> 
> An alternative solution is to keep the set in some executor data structure
> and compute it during executor startup; perhaps near to where the
> expressions are prepared for execution, so as to save extra stringToNode
> calls.

Yeah, really only the executor ended up needing this, so perhaps it 
should be handled in the executor.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-05-08 21:05, Tom Lane wrote:
>> I happened to notice $subject while working on the release notes.
>> AFAICS, it is 100% inappropriate for the parser to compute the
>> set of generated columns affected by an UPDATE, because that set
>> could change before execution.  It would be really easy to break
>> this for an UPDATE in a stored rule, for example.

> Do you have a specific situation in mind?  How would a rule change the 
> set of columns updated by a query?  Something involving CTEs?  Having a 
> test case would be good.

broken-update-rule.sql, attached, shows the scenario I had in mind:
the rule UPDATE query knows nothing of the generated column that
gets added after the rule is stored, so the UPDATE fails to update it.

However, on the way to preparing that test case I discovered that
auto-updatable views have the same disease even when the generated column
exists from the get-go; see broken-updatable-views.sql.  In the context
of the existing design, I suppose this means that there needs to be
a fill_extraUpdatedCols call somewhere in the code path that constructs
an auto-update query.  But if we moved the whole thing to the executor
then the problem would go away.

I observe also that the executor doesn't seem to need this bitmap at all
unless (a) there are triggers or (b) there are generated columns.
So in a lot of simpler cases, the cost of doing fill_extraUpdatedCols
at either parse or plan time would be quite wasted.  That might be a good
argument for moving it to executor start, even though we'd then have
to re-do it when re-using a prepared plan.

            regards, tom lane

drop view v1;
drop table tab1;

create table tab1 (f1 int, id serial);

insert into tab1 values (42);

table tab1;

create view v1 as select f1, id from tab1;

create rule r1 as on update to v1 do instead
update tab1 set f1 = new.f1 where tab1.id = new.id;

update v1 set f1 = 11;

table tab1;

alter table tab1 add column f2 int generated always as (f1 * 2) stored;

table tab1;

update v1 set f1 = 12;

table tab1;
drop view v1;
drop table tab1;

create table tab1 (f1 int, f2 int generated always as (f1 * 2) stored);

insert into tab1 values (42);

table tab1;

create view v1 as select * from tab1;

update v1 set f1 = 11;

table tab1;