Re: Supporting MERGE on updatable views

On Fri, 26 Jan 2024 at 16:48, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Thanks for working on this.  The patch looks well finished.  I didn't
> try to run it, though.  I gave it a read and found nothing to complain
> about, just these two pretty minor comments:
>

Thanks for reviewing.

> > -CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
> > +CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation,
> > +                                     List *mergeActions)
> >  {
>
> I suggest to add commentary explaining the new argument of this function.
>

OK.

> > @@ -1080,6 +1083,51 @@ CheckValidResultRel(ResultRelInfo *resul
> >                                                                               RelationGetRelationName(resultRel)),
> >                                                                errhint("To enable deleting from the view, provide
anINSTEAD OF DELETE trigger or an unconditional ON DELETE DO INSTEAD rule.")));
 
> >                                       break;
> > +                             case CMD_MERGE:
> > +
> > +                                     /*
> > +                                      * Must have a suitable INSTEAD OF trigger for each MERGE
> > +                                      * action.  Note that the error hints here differ from
> > +                                      * above, since MERGE doesn't support rules.
> > +                                      */
> > +                                     foreach(lc, mergeActions)
> > +                                     {
> > +                                             MergeAction *action = (MergeAction *) lfirst(lc);
> > +
> > +                                             switch (action->commandType)
> > +                                             {
> > +                                                     case CMD_INSERT:
> > +                                                             if (!trigDesc || !trigDesc->trig_insert_instead_row)
> > +                                                                     ereport(ERROR,
> > +
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> > +                                                                                      errmsg("cannot insert into
view\"%s\"",
 
> > +
RelationGetRelationName(resultRel)),
> > +                                                                                      errhint("To enable inserting
intothe view using MERGE, provide an INSTEAD OF INSERT trigger.")));
 
>
> Looking at the comments in ereport_view_not_updatable and the code
> coverate reports, it appears that these checks here are dead code?  If
> so, maybe it would be better to turn them into elog() calls?  I don't
> mean to touch the existing code, just that I don't see that it makes
> sense to introduce the new ones as ereport().
>

Yeah, for all practical purposes, that check in CheckValidResultRel()
has been dead code since d751ba5235, but I think it's still worth
doing, and if we're going to do it, we should do it properly. I don't
like using elog() in some cases and ereport() in others, but I also
don't like having that much duplicated code between this and the
rewriter (and this patch doubles the size of that block).

A neater solution is to export the rewriter functions and use them in
CheckValidResultRel(). All these checks can then be reduced to

    if (!view_has_instead_trigger(...))
        error_view_not_updatable(...)

which eliminates a lot of duplicated code and means that we now have
just one place that throws these errors.

Regards,
Dean