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

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