Re: Supporting MERGE on updatable views

On Fri, 26 Jan 2024 at 16:48, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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 an INSTEAD 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 into the 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