On 2019/02/27 18:37, Dean Rasheed wrote: > On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rash...@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
