Re: Errors when update a view with conditional-INSTEAD rules

On Sat, 4 Jan 2020 at 17:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Dean Rasheed <dean.a.rasheed@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