Re: Supporting MERGE on updatable views

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:

On 2024-Jan-26, Dean Rasheed wrote:

> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
> new file mode 100644
> index 13a9b7d..5a99e4a
> --- a/src/backend/executor/execMain.c
> +++ b/src/backend/executor/execMain.c
> @@ -1021,11 +1021,13 @@ InitPlan(QueryDesc *queryDesc, int eflag
>   * CheckValidRowMarkRel.
>   */
>  void
> -CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
> +CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation,
> +                    List *mergeActions)
>  {

I suggest to add commentary explaining the new argument of this function.

> @@ -1080,6 +1083,51 @@ CheckValidResultRel(ResultRelInfo *resul
>                                          RelationGetRelationName(resultRel)),
>                                   errhint("To enable deleting from the view, provide an INSTEAD OF DELETE trigger or
anunconditional 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
INSTEADOF 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().

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
 en duras y empinadas sillas    / desprovistas, por cierto
 de blandos atenuantes"                          (Patricio Vogel)