Обсуждение: BUG #15623: Inconsistent use of default for updatable view
The following bug has been logged on the website: Bug reference: 15623 Logged by: Roger Curley Email address: rocurley@gmail.com PostgreSQL version: 11.1 Operating system: Ubuntu 11.1 Description: Steps to reproduce (run in psql shell): ``` DROP TABLE IF EXISTS test CASCADE; CREATE TABLE test ( id int PRIMARY KEY, value int DEFAULT 0 ); CREATE VIEW test_view AS (SELECT * FROM test); INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT); INSERT INTO test VALUES (3, DEFAULT), (4, DEFAULT); INSERT INTO test_view VALUES (5, DEFAULT); SELECT * FROM test; ``` Result: ``` id | value ----+------- 1 | 2 | 3 | 0 4 | 0 5 | 0 ``` Expected Result: ``` id | value ----+------- 1 | 0 2 | 0 3 | 0 4 | 0 5 | 0 ``` In particular, it's surprising that inserting 1 row into an updatable view uses the table default, while inserting 2 uses null.
Hi, On 2019/02/08 6:42, PG Bug reporting form wrote: > The following bug has been logged on the website: > > Bug reference: 15623 > Logged by: Roger Curley > Email address: rocurley@gmail.com > PostgreSQL version: 11.1 > Operating system: Ubuntu 11.1 > Description: > > Steps to reproduce (run in psql shell): > ``` > DROP TABLE IF EXISTS test CASCADE; > CREATE TABLE test ( > id int PRIMARY KEY, > value int DEFAULT 0 > ); > CREATE VIEW test_view AS (SELECT * FROM test); > > INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT); > INSERT INTO test VALUES (3, DEFAULT), (4, DEFAULT); > INSERT INTO test_view VALUES (5, DEFAULT); > SELECT * FROM test; > ``` > > Result: > ``` > id | value > ----+------- > 1 | > 2 | > 3 | 0 > 4 | 0 > 5 | 0 > ``` > > Expected Result: > ``` > id | value > ----+------- > 1 | 0 > 2 | 0 > 3 | 0 > 4 | 0 > 5 | 0 > ``` > In particular, it's surprising that inserting 1 row into an updatable view > uses the table default, while inserting 2 uses null. Thanks for the report. Seems odd indeed. Looking into this, the reason it works when inserting just one row vs. more than one row is that those two cases are handled by nearby but different pieces of code. The code that handles multiple rows seems buggy as seen in the above example. Specifically, I think the bug is in rewriteValuesRTE() which is a function to replace the default placeholders in the input rows by the default values as defined for the target relation. It is called twice when inserting via the view -- first for the view relation and then again for the underlying table. This arrangement seems to work correctly if the view specifies its own defaults for columns (assuming that it's okay for the view's defaults to override the underlying base table's). If there are no view-specified defaults, then rewriteValuesRTE replaces the default placeholders in the input row by NULL constants when called for the first time with the view as target relation and the next invocation for the underlying table finds that it has no work to do, so its defaults are not filled. Attached find a patch that adjusts rewriteValuesRTE to not replace the default placeholder if the view has no default value for a given column. Also, adds a test in updatable_views.sql. Thanks, Amit
Вложения
On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > Thanks for the report. Seems odd indeed. Hmm, indeed. That seems to have been broken ever since updatable views were added. > Looking into this, the reason it works when inserting just one row vs. > more than one row is that those two cases are handled by nearby but > different pieces of code. The code that handles multiple rows seems buggy > as seen in the above example. Specifically, I think the bug is in > rewriteValuesRTE() which is a function to replace the default placeholders > in the input rows by the default values as defined for the target > relation. It is called twice when inserting via the view -- first for the > view relation and then again for the underlying table. Right, except when the view is trigger-updatable. In that case, we do have to explicitly set the column value to NULL when rewriteValuesRTE() is called for the view, because it won't be called again for the underlying table -- it is the trigger's responsibility to work how (or indeed if) to update the underlying table. IOW, you need to also use view_has_instead_trigger() to check the view, otherwise your patch breaks this case: DROP TABLE IF EXISTS test CASCADE; CREATE TABLE test ( id int PRIMARY KEY, value int DEFAULT 0 ); CREATE VIEW test_view AS (SELECT * FROM test); CREATE OR REPLACE FUNCTION test_view_ins() RETURNS trigger AS $$ BEGIN INSERT INTO test VALUES (NEW.id, NEW.value); RETURN NEW; END; $$ LANGUAGE plpgsql; CREATE TRIGGER test_view_trig INSTEAD OF INSERT ON test_view FOR EACH ROW EXECUTE FUNCTION test_view_ins(); INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT); ERROR: unrecognized node type: 142 While playing around with this, I noticed a related bug affecting the new identity columns feature. I've not investigated it fully, but It looks almost the same -- if the column is an identity column, and we're inserting a multi-row VALUES set containing DEFAULTS, they will get rewritten to NULLs which will then lead to an error if overriding the generated value isn't allowed: DROP TABLE IF EXISTS foo CASCADE; CREATE TABLE foo ( a int, b int GENERATED ALWAYS AS IDENTITY ); INSERT INTO foo VALUES (1,DEFAULT); -- OK INSERT INTO foo VALUES (2,DEFAULT),(3,DEFAULT); -- Fails I think fixing that should be tackled separately, because it may turn out to be subtly different, but it definitely looks like another bug. Regards, Dean
Thanks for looking at this. On Fri, Feb 8, 2019 at 8:01 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > Thanks for the report. Seems odd indeed. > > Hmm, indeed. That seems to have been broken ever since updatable views > were added. > > > Looking into this, the reason it works when inserting just one row vs. > > more than one row is that those two cases are handled by nearby but > > different pieces of code. The code that handles multiple rows seems buggy > > as seen in the above example. Specifically, I think the bug is in > > rewriteValuesRTE() which is a function to replace the default placeholders > > in the input rows by the default values as defined for the target > > relation. It is called twice when inserting via the view -- first for the > > view relation and then again for the underlying table. > > Right, except when the view is trigger-updatable. In that case, we do > have to explicitly set the column value to NULL when > rewriteValuesRTE() is called for the view, because it won't be called > again for the underlying table -- it is the trigger's responsibility > to work how (or indeed if) to update the underlying table. IOW, you > need to also use view_has_instead_trigger() to check the view, > otherwise your patch breaks this case: > > DROP TABLE IF EXISTS test CASCADE; > CREATE TABLE test ( > id int PRIMARY KEY, > value int DEFAULT 0 > ); > CREATE VIEW test_view AS (SELECT * FROM test); > > CREATE OR REPLACE FUNCTION test_view_ins() RETURNS trigger > AS > $$ > BEGIN > INSERT INTO test VALUES (NEW.id, NEW.value); > RETURN NEW; > END; > $$ > LANGUAGE plpgsql; > > CREATE TRIGGER test_view_trig INSTEAD OF INSERT ON test_view > FOR EACH ROW EXECUTE FUNCTION test_view_ins(); > > INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT); > > ERROR: unrecognized node type: 142 Oops, I missed this bit. Updated the patch per your suggestion and expanded the test case to exercise this. > While playing around with this, I noticed a related bug affecting the > new identity columns feature. I've not investigated it fully, but It > looks almost the same -- if the column is an identity column, and > we're inserting a multi-row VALUES set containing DEFAULTS, they will > get rewritten to NULLs which will then lead to an error if overriding > the generated value isn't allowed: > > DROP TABLE IF EXISTS foo CASCADE; > CREATE TABLE foo > ( > a int, > b int GENERATED ALWAYS AS IDENTITY > ); > > INSERT INTO foo VALUES (1,DEFAULT); -- OK > INSERT INTO foo VALUES (2,DEFAULT),(3,DEFAULT); -- Fails > > I think fixing that should be tackled separately, because it may turn > out to be subtly different, but it definitely looks like another bug. I haven't looked into the details of this, but agree about raising a thread on -hackers about it. Thanks, Amit
Вложения
On Sat, 9 Feb 2019 at 16:57, Amit Langote <amitlangote09@gmail.com> wrote: > On Fri, Feb 8, 2019 at 8:01 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > > Looking into this, the reason it works when inserting just one row vs. > > > more than one row is that those two cases are handled by nearby but > > > different pieces of code. The code that handles multiple rows seems buggy > > > as seen in the above example. Specifically, I think the bug is in > > > rewriteValuesRTE() which is a function to replace the default placeholders > > > in the input rows by the default values as defined for the target > > > relation. It is called twice when inserting via the view -- first for the > > > view relation and then again for the underlying table. > > > > Right, except when the view is trigger-updatable... > > Oops, I missed this bit. Updated the patch per your suggestion and > expanded the test case to exercise this. > Unfortunately, that's still not quite right because it makes the behaviour of single- and multi-row inserts inconsistent for rule-updatable views. Attached is an updated patch that fixes that. I adjusted the tests a bit to try to make it clearer which defaults get applied, and test all possibilities. However, this is still not the end of the story, because it doesn't fix the fact that, if the view has a DO ALSO rule on it, single-row inserts behave differently from multi-row inserts. In that case, each insert becomes 2 inserts, and defaults need to be treated differently in each of the 2 queries. That's going to need a little more thought. Regards, Dean
Вложения
On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > However, this is still not the end of the story, because it doesn't > fix the fact that, if the view has a DO ALSO rule on it, single-row > inserts behave differently from multi-row inserts. In that case, each > insert becomes 2 inserts, and defaults need to be treated differently > in each of the 2 queries. That's going to need a little more thought. > Here's an updated patch to handle that case. In case it's not obvious, I'm not intending to try to get this into next week's updates -- more time is needed to be sure of this fix, and more pairs of eyes would definitely be helpful, once those updates have been shipped. Regards, Dean
Вложения
On Sun, 10 Feb 2019 at 11:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > However, this is still not the end of the story, because it doesn't > > fix the fact that, if the view has a DO ALSO rule on it, single-row > > inserts behave differently from multi-row inserts. In that case, each > > insert becomes 2 inserts, and defaults need to be treated differently > > in each of the 2 queries. That's going to need a little more thought. > > > > Here's an updated patch to handle that case. > > In case it's not obvious, I'm not intending to try to get this into > next week's updates -- more time is needed to be sure of this fix. So I did some more testing of this and I'm reasonably happy that this now fixes the originally reported issue of inconsistent handling of DEFAULTS in multi-row VALUES lists vs single-row ones. I tested various other scenarios involving conditional/unconditional also/instead rules, and I didn't find any other surprises. Attached is an updated patch with improved comments, and a little less code duplication. Regards, Dean
Вложения
Hi Dean, On 2019/02/12 19:33, Dean Rasheed wrote: > On Sun, 10 Feb 2019 at 11:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >>> However, this is still not the end of the story, because it doesn't >>> fix the fact that, if the view has a DO ALSO rule on it, single-row >>> inserts behave differently from multi-row inserts. In that case, each >>> insert becomes 2 inserts, and defaults need to be treated differently >>> in each of the 2 queries. That's going to need a little more thought. >>> >> >> Here's an updated patch to handle that case. >> >> In case it's not obvious, I'm not intending to try to get this into >> next week's updates -- more time is needed to be sure of this fix. > > So I did some more testing of this and I'm reasonably happy that this > now fixes the originally reported issue of inconsistent handling of > DEFAULTS in multi-row VALUES lists vs single-row ones. I tested > various other scenarios involving conditional/unconditional > also/instead rules, and I didn't find any other surprises. Attached is > an updated patch with improved comments, and a little less code > duplication. Thanks for updating the patch. I can't really comment on all of the changes that that you made considering various cases, but became curious if the single-row and multi-row inserts cases could share the code that determines if the DEFAULT item be replaced by the target-relation-specified default or NULL? IOW, is there some reason why we can't avoid the special handling for the multi-row (RTE_VALUES) case? Thanks, Amit
On Wed, 13 Feb 2019 at 03:02, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > I can't really comment on all of the changes that that you made > considering various cases, but became curious if the single-row and > multi-row inserts cases could share the code that determines if the > DEFAULT item be replaced by the target-relation-specified default or NULL? > IOW, is there some reason why we can't avoid the special handling for the > multi-row (RTE_VALUES) case? > No, not as far as I can see. The tricky part for a multi-row insert is working out what to do when it sees a DEFAULT, and there is no column default on the target relation. For an auto-updatable view, it needs to leave the DEFAULT untouched, so that it can later apply the base relation's column default when it recurses. For all other kinds of relation, it needs to turn the DEFAULT into a NULL. For a single-row insert, that's all much easier. If it sees a DEFAULT, and there is no column default, it simply omits that entry from the targetlist. If it then recurses to the base relation, it will put the targetlist entry back in, if the base relation has a column default. So it doesn't need to know locally whether it's an auto-updatable view, and the logic is much simpler. The multi-row case can't easily do that (add and remove columns) because it's working with a fixed-width table structure. Actually, that's not quite the end of it. So far, this has only been considering INSERT's. I think there are more issues with UPDATE's, but that's a whole other can of worms. I think I'll commit this first, and start a thread on -hackers to discuss that. Regards, Dean
On 2019/02/13 18:04, Dean Rasheed wrote: > On Wed, 13 Feb 2019 at 03:02, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> >> I can't really comment on all of the changes that that you made >> considering various cases, but became curious if the single-row and >> multi-row inserts cases could share the code that determines if the >> DEFAULT item be replaced by the target-relation-specified default or NULL? >> IOW, is there some reason why we can't avoid the special handling for the >> multi-row (RTE_VALUES) case? >> > > No, not as far as I can see. > > The tricky part for a multi-row insert is working out what to do when > it sees a DEFAULT, and there is no column default on the target > relation. For an auto-updatable view, it needs to leave the DEFAULT > untouched, so that it can later apply the base relation's column > default when it recurses. For all other kinds of relation, it needs to > turn the DEFAULT into a NULL. > > For a single-row insert, that's all much easier. If it sees a DEFAULT, > and there is no column default, it simply omits that entry from the > targetlist. If it then recurses to the base relation, it will put the > targetlist entry back in, if the base relation has a column default. > So it doesn't need to know locally whether it's an auto-updatable > view, and the logic is much simpler. The multi-row case can't easily > do that (add and remove columns) because it's working with a > fixed-width table structure. Hmm yeah, column sets must be the same in in all value-lists. > Actually, that's not quite the end of it. So far, this has only been > considering INSERT's. I think there are more issues with UPDATE's, but > that's a whole other can of worms. I think I'll commit this first, and > start a thread on -hackers to discuss that. Sure, thanks for the explanation. Regards, Amit
On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Here's an updated patch ...
So I pushed that. However, ...
Playing around with it some more, I realised that whilst this appeared
to fix the reported problem, it exposes another issue which is down to
the interaction between rewriteTargetListIU() and rewriteValuesRTE()
--- for an INSERT with a VALUES RTE, rewriteTargetListIU() computes a
list of target attribute numbers (attrno_list) from the targetlist in
its original order, which rewriteValuesRTE() then relies on being the
same length (and in the same order) as each of the lists in the VALUES
RTE. That's OK for the initial invocation of those functions, but it
breaks down when they're recursively invoked for auto-updatable views.
For example, the following test-case, based on a slight variation of
the new regression tests:
create table foo (a int default 1, b int);
create view foo_v as select * from foo;
alter view foo_v alter column b set default 2;
insert into foo_v values (default), (default);
triggers the following Assert in rewriteValuesRTE():
/* Check list lengths (we can assume all the VALUES sublists are alike) */
Assert(list_length(attrnos) == list_length(linitial(rte->values_lists)));
What's happening is that the initial targetlist, which contains just 1
entry for the column a, gains another entry to set the default for
column b from the view. Then, when it recurses into
rewriteTargetListIU()/rewriteValuesRTE() for the base relation, the
size of the targetlist (now 2) no longer matches the sizes of the
VALUES RTE lists (1).
I think that that failed Assert was unreachable prior to 41531e42d3,
because the old version of rewriteValuesRTE() would always replace all
unresolved DEFAULT items with NULLs, so when it recursed into
rewriteValuesRTE() for the base relation, it would always bail out
early because there would be no DEFAULT items left, and so it would
fail to notice the list size mismatch.
My first thought was that this could be fixed by having
rewriteTargetListIU() compute attrno_list using only those targetlist
entries that refer to the VALUES RTE, and thus omit any new entries
added due to view defaults. That doesn't work though, because that's
not the only way that a list size mismatch can be triggered --- it's
also possible that rewriteTargetListIU() will merge together
targetlist entries, for example if they're array element assignments
referring to the same column, in which case the rewritten targetlist
could be shorter than the VALUES RTE lists, and attempting to compute
attrno_list from it correctly would be trickier.
So actually, I think the right way to fix this is to give up trying to
compute attrno_list in rewriteTargetListIU(), and have
rewriteValuesRTE() work out the attribute assignments itself for each
column of the VALUES RTE by examining the rewritten targetlist. That
looks to be quite straightforward, and results in a cleaner separation
of logic between the 2 functions, per the attached patch.
I think that rewriteValuesRTE() should only ever see DEFAULT items for
columns that are simple assignments to columns of the target relation,
so it only needs to work out the target attribute numbers for TLEs
that contain simple Vars referring to the VALUES RTE. Any DEFAULT seen
in a column not matching that would be an error, but actually I think
such a thing ought to be a "can't happen" error because of the prior
checks during parse analysis, so I've used elog() to report if this
does happen.
Incidentally, it looks like the part of rewriteValuesRTE()'s comment
about subscripted and field assignment has been wrong since
a3c7a993d5, so I've attempted to clarify that. That will need to look
different pre-9.6, I think.
Thoughts?
Regards,
Dean
Вложения
On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Here's an updated patch ...
So I pushed that. However, ...
Playing around with it some more, I realised that whilst this appeared
to fix the reported problem, it exposes another issue which is down to
the interaction between rewriteTargetListIU() and rewriteValuesRTE()
--- for an INSERT with a VALUES RTE, rewriteTargetListIU() computes a
list of target attribute numbers (attrno_list) from the targetlist in
its original order, which rewriteValuesRTE() then relies on being the
same length (and in the same order) as each of the lists in the VALUES
RTE. That's OK for the initial invocation of those functions, but it
breaks down when they're recursively invoked for auto-updatable views.
For example, the following test-case, based on a slight variation of
the new regression tests:
create table foo (a int default 1, b int);
create view foo_v as select * from foo;
alter view foo_v alter column b set default 2;
insert into foo_v values (default), (default);
triggers the following Assert in rewriteValuesRTE():
/* Check list lengths (we can assume all the VALUES sublists are alike) */
Assert(list_length(attrnos) == list_length(linitial(rte->values_lists)));
What's happening is that the initial targetlist, which contains just 1
entry for the column a, gains another entry to set the default for
column b from the view. Then, when it recurses into
rewriteTargetListIU()/rewriteValuesRTE() for the base relation, the
size of the targetlist (now 2) no longer matches the sizes of the
VALUES RTE lists (1).
I think that that failed Assert was unreachable prior to 41531e42d3,
because the old version of rewriteValuesRTE() would always replace all
unresolved DEFAULT items with NULLs, so when it recursed into
rewriteValuesRTE() for the base relation, it would always bail out
early because there would be no DEFAULT items left, and so it would
fail to notice the list size mismatch.
My first thought was that this could be fixed by having
rewriteTargetListIU() compute attrno_list using only those targetlist
entries that refer to the VALUES RTE, and thus omit any new entries
added due to view defaults. That doesn't work though, because that's
not the only way that a list size mismatch can be triggered --- it's
also possible that rewriteTargetListIU() will merge together
targetlist entries, for example if they're array element assignments
referring to the same column, in which case the rewritten targetlist
could be shorter than the VALUES RTE lists, and attempting to compute
attrno_list from it correctly would be trickier.
So actually, I think the right way to fix this is to give up trying to
compute attrno_list in rewriteTargetListIU(), and have
rewriteValuesRTE() work out the attribute assignments itself for each
column of the VALUES RTE by examining the rewritten targetlist. That
looks to be quite straightforward, and results in a cleaner separation
of logic between the 2 functions, per the attached patch.
I think that rewriteValuesRTE() should only ever see DEFAULT items for
columns that are simple assignments to columns of the target relation,
so it only needs to work out the target attribute numbers for TLEs
that contain simple Vars referring to the VALUES RTE. Any DEFAULT seen
in a column not matching that would be an error, but actually I think
such a thing ought to be a "can't happen" error because of the prior
checks during parse analysis, so I've used elog() to report if this
does happen.
Incidentally, it looks like the part of rewriteValuesRTE()'s comment
about subscripted and field assignment has been wrong since
a3c7a993d5, so I've attempted to clarify that. That will need to look
different pre-9.6, I think.
Thoughts?
Regards,
Dean
On 2019/02/27 18:37, Dean Rasheed wrote: > On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> Here's an updated patch ... > > So I pushed that. However, ... > > Playing around with it some more, I realised that whilst this appeared > to fix the reported problem, it exposes another issue which is down to > the interaction between rewriteTargetListIU() and rewriteValuesRTE() > --- for an INSERT with a VALUES RTE, rewriteTargetListIU() computes a > list of target attribute numbers (attrno_list) from the targetlist in > its original order, which rewriteValuesRTE() then relies on being the > same length (and in the same order) as each of the lists in the VALUES > RTE. That's OK for the initial invocation of those functions, but it > breaks down when they're recursively invoked for auto-updatable views. >> So actually, I think the right way to fix this is to give up trying to > compute attrno_list in rewriteTargetListIU(), and have > rewriteValuesRTE() work out the attribute assignments itself for each > column of the VALUES RTE by examining the rewritten targetlist. That > looks to be quite straightforward, and results in a cleaner separation > of logic between the 2 functions, per the attached patch. +1. Only rewriteValuesRTE needs to use that information, so it's better to teach it to figure it by itself. > I think that rewriteValuesRTE() should only ever see DEFAULT items for > columns that are simple assignments to columns of the target relation, > so it only needs to work out the target attribute numbers for TLEs > that contain simple Vars referring to the VALUES RTE. Any DEFAULT seen > in a column not matching that would be an error, but actually I think > such a thing ought to be a "can't happen" error because of the prior > checks during parse analysis, so I've used elog() to report if this > does happen. + if (attrno == 0) + elog(ERROR, "Cannot set value in column %d to DEFAULT", i); Maybe: s/Cannot/cannot/g + Assert(list_length(sublist) == numattrs); Isn't this Assert useless, because we're setting numattrs to list_length(<first-sublist>) and transformInsertStmt ensures that all sublists are same length? Thanks, Amit
On 2019/02/27 18:37, Dean Rasheed wrote: > On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> Here's an updated patch ... > > So I pushed that. However, ... > > Playing around with it some more, I realised that whilst this appeared > to fix the reported problem, it exposes another issue which is down to > the interaction between rewriteTargetListIU() and rewriteValuesRTE() > --- for an INSERT with a VALUES RTE, rewriteTargetListIU() computes a > list of target attribute numbers (attrno_list) from the targetlist in > its original order, which rewriteValuesRTE() then relies on being the > same length (and in the same order) as each of the lists in the VALUES > RTE. That's OK for the initial invocation of those functions, but it > breaks down when they're recursively invoked for auto-updatable views. >> So actually, I think the right way to fix this is to give up trying to > compute attrno_list in rewriteTargetListIU(), and have > rewriteValuesRTE() work out the attribute assignments itself for each > column of the VALUES RTE by examining the rewritten targetlist. That > looks to be quite straightforward, and results in a cleaner separation > of logic between the 2 functions, per the attached patch. +1. Only rewriteValuesRTE needs to use that information, so it's better to teach it to figure it by itself. > I think that rewriteValuesRTE() should only ever see DEFAULT items for > columns that are simple assignments to columns of the target relation, > so it only needs to work out the target attribute numbers for TLEs > that contain simple Vars referring to the VALUES RTE. Any DEFAULT seen > in a column not matching that would be an error, but actually I think > such a thing ought to be a "can't happen" error because of the prior > checks during parse analysis, so I've used elog() to report if this > does happen. + if (attrno == 0) + elog(ERROR, "Cannot set value in column %d to DEFAULT", i); Maybe: s/Cannot/cannot/g + Assert(list_length(sublist) == numattrs); Isn't this Assert useless, because we're setting numattrs to list_length(<first-sublist>) and transformInsertStmt ensures that all sublists are same length? Thanks, Amit
On Thu, 28 Feb 2019 at 07:47, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > + if (attrno == 0) > + elog(ERROR, "Cannot set value in column %d to > DEFAULT", i); > > Maybe: s/Cannot/cannot/g > Ah yes, you're right. That is the convention. > + Assert(list_length(sublist) == numattrs); > > Isn't this Assert useless, because we're setting numattrs to > list_length(<first-sublist>) and transformInsertStmt ensures that all > sublists are same length? > Well possibly I'm being over-paranoid, but given that it may have come via a previous invocation of rewriteValuesRTE() that may have completely rebuilt the lists, it seemed best to be sure that it hadn't done something unexpected. It's about to use that to read from the attrnos array, so it might read beyond the array bounds if any of the prior logic was faulty. Thanks for looking. Regards, Dean
On Thu, 28 Feb 2019 at 07:47, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > > + if (attrno == 0) > + elog(ERROR, "Cannot set value in column %d to > DEFAULT", i); > > Maybe: s/Cannot/cannot/g > Ah yes, you're right. That is the convention. > + Assert(list_length(sublist) == numattrs); > > Isn't this Assert useless, because we're setting numattrs to > list_length(<first-sublist>) and transformInsertStmt ensures that all > sublists are same length? > Well possibly I'm being over-paranoid, but given that it may have come via a previous invocation of rewriteValuesRTE() that may have completely rebuilt the lists, it seemed best to be sure that it hadn't done something unexpected. It's about to use that to read from the attrnos array, so it might read beyond the array bounds if any of the prior logic was faulty. Thanks for looking. Regards, Dean