On Sat, 4 Jan 2020 at 17:13, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Dean Rasheed <dean.a.rash...@gmail.com> writes: > > That included a change to rewriteTargetListIU() to prevent it from > > adding dummy targetlist entries for unassigned-to attributes for > > auto-updatable views, in case they are no longer simple references to > > the underlying relation. Instead, that is left to expand_targetlist(), > > as for a normal table. However, in this case (an UPDATE on a view with > > a conditional rule), the target relation of the original query isn't > > rewritten (we leave it to the executor to report the error), and so > > expand_targetlist() ends up adding a new targetlist entry that > > references the target relation, which is still the original view. > > So why did we leave it to the executor to throw an error? I have > a feeling it was either because the rewriter didn't have (easy?) > access to the info, or it seemed like it'd be duplicating code. >
Perhaps it was more to do with history and not wanting to duplicate code. Before we had auto-updatable views, it was always the executor that threw this error. With the addition of auto-updatable views, we also throw the error from rewriteTargetView() if there are no rules or triggers. But there is a difference -- rewriteTargetView() has more detailed information about why the view isn't auto-updatable, which it includes in the error detail. I think that the required information is easily available in the rewriter though. Currently RewriteQuery() is doing this: if ( !instead // No unconditional INSTEAD rules && qual_product == NULL // No conditional INSTEAD rules either && relkind == VIEW && !view_has_instead_trigger() ) { // Attempt auto-update, throwing an error if not possible rewriteTargetView(...) ... } So if that were to become something like: if ( !instead // No unconditional INSTEAD rules && relkind == VIEW && !view_has_instead_trigger() ) { if (qual_product != NULL) { // Conditional INSTEAD rules exist, but no unconditional INSTEAD rules // or INSTEAD OF triggers, so throw an error ... } // Attempt auto-update, throwing an error if not possible rewriteTargetView(...) ... } then in theory I think the error condition in the executor should never be triggered. That will lead to a few lines of duplicated code because the error-throwing code block includes a switch on command type. However, it also gives us an opportunity to be a more specific in the new error, with detail for this specific case. Regards, Dean