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 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().

-- 
Á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)