Fix error message for MERGE foreign tables

On Fri, Oct 14, 2022 at 10:59 AM bt22nakamorit <
bt22nakamorit@oss.nttdata.com> wrote:

Hi,

MERGE command does not accept foreign tables as targets.
When a foreign table is specified as a target, it shows error messages
like this:

-- ERROR: cannot execute MERGE on relation "child1"
-- DETAIL: This operation is not supported for foreign tables.

However, when a partitioned table includes foreign tables as partitions
and MERGE is executed on the partitioned table, following error message
shows.

-- ERROR: unexpected operation: 5

The latter error message is unclear, and should be the same as the
former one.
The attached patch adds the code to display error the former error
messages in the latter case.
Any thoughts?

+1. The new message is an improvement to the default one.

I wonder if we can provide more details in the error message, such as
foreign table name.

Thanks
Richard

On Fri, Oct 14, 2022 at 12:07 PM Richard Guo <guofenglinux@gmail.com> wrote:

On Fri, Oct 14, 2022 at 10:59 AM bt22nakamorit <
bt22nakamorit@oss.nttdata.com> wrote:

Hi,

MERGE command does not accept foreign tables as targets.
When a foreign table is specified as a target, it shows error messages
like this:

-- ERROR: cannot execute MERGE on relation "child1"
-- DETAIL: This operation is not supported for foreign tables.

However, when a partitioned table includes foreign tables as partitions
and MERGE is executed on the partitioned table, following error message
shows.

-- ERROR: unexpected operation: 5

The latter error message is unclear, and should be the same as the
former one.
The attached patch adds the code to display error the former error
messages in the latter case.
Any thoughts?

+1. The new message is an improvement to the default one.

I wonder if we can provide more details in the error message, such as
foreign table name.

Maybe something like below, so that we keep it consistent with the case
of a foreign table being specified as a target.

--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1872,6 +1872,13 @@ postgresPlanForeignModify(PlannerInfo *root,
                             returningList,
                             &retrieved_attrs);
            break;
+       case CMD_MERGE:
+           ereport(ERROR,
+                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                    errmsg("cannot execute MERGE on relation \"%s\"",
+                           RelationGetRelationName(rel)),
+
 errdetail_relkind_not_supported(rel->rd_rel->relkind)));
+           break;

Thanks
Richard

On Fri, Oct 14, 2022 at 12:26:19PM +0800, Richard Guo wrote:

Maybe something like below, so that we keep it consistent with the case
of a foreign table being specified as a target.

--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1872,6 +1872,13 @@ postgresPlanForeignModify(PlannerInfo *root,
returningList,
&retrieved_attrs);
break;
+       case CMD_MERGE:
+           ereport(ERROR,
+                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                    errmsg("cannot execute MERGE on relation \"%s\"",
+                           RelationGetRelationName(rel)),
+
errdetail_relkind_not_supported(rel->rd_rel->relkind)));
+           break;

Yeah, you should not use an elog(ERROR) for cases that would be faced
directly by users.
--
Michael

On 2022-Oct-14, Michael Paquier wrote:

On Fri, Oct 14, 2022 at 12:26:19PM +0800, Richard Guo wrote:

Maybe something like below, so that we keep it consistent with the case
of a foreign table being specified as a target.

--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1872,6 +1872,13 @@ postgresPlanForeignModify(PlannerInfo *root,
returningList,
&retrieved_attrs);
break;
+       case CMD_MERGE:
+           ereport(ERROR,
+                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                    errmsg("cannot execute MERGE on relation \"%s\"",
+                           RelationGetRelationName(rel)),
+
errdetail_relkind_not_supported(rel->rd_rel->relkind)));
+           break;

Yeah, you should not use an elog(ERROR) for cases that would be faced
directly by users.

Yeah, I think this just flies undetected until it hits code that doesn't
support the case. I'll add a test and push as Richard suggests, thanks.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)

Actually, I hadn't realized that the originally submitted patch had the
test in postgres_fdw only, but we really want it to catch any FDW, so it
needs to be somewhere more general. The best place I found to put this
test is in make_modifytable ... I searched for some earlier place in the
planner to do it, but couldn't find anything.

So what do people think about this?

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La grandeza es una experiencia transitoria. Nunca es consistente.
Depende en gran parte de la imaginación humana creadora de mitos"
(Irulan)

On Fri, Oct 14, 2022 at 5:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Actually, I hadn't realized that the originally submitted patch had the
test in postgres_fdw only, but we really want it to catch any FDW, so it
needs to be somewhere more general. The best place I found to put this
test is in make_modifytable ... I searched for some earlier place in the
planner to do it, but couldn't find anything.

So what do people think about this?

Good point. I agree that the test should be in a more general place.

I wonder if we can make it earlier in grouping_planner() just before we
add ModifyTablePath.

--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1772,6 +1772,17 @@ grouping_planner(PlannerInfo *root, double
tuple_fraction)
         /* Build per-target-rel lists needed by ModifyTable */
         resultRelations = lappend_int(resultRelations,
                                       resultRelation);
+        if (parse->commandType == CMD_MERGE &&
+            this_result_rel->fdwroutine != NULL)
+        {
+            RangeTblEntry *rte = root->simple_rte_array[resultRelation];
+
+            ereport(ERROR,
+                    errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                    errmsg("cannot execute MERGE on relation \"%s\"",
+                           get_rel_name(rte->relid)),
+                    errdetail_relkind_not_supported(rte->relkind));
+        }

Thanks
Richard

On Fri, Oct 14, 2022 at 7:19 PM Richard Guo <guofenglinux@gmail.com> wrote:

On Fri, Oct 14, 2022 at 5:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Actually, I hadn't realized that the originally submitted patch had the
test in postgres_fdw only, but we really want it to catch any FDW, so it
needs to be somewhere more general. The best place I found to put this
test is in make_modifytable ... I searched for some earlier place in the
planner to do it, but couldn't find anything.

So what do people think about this?

Good point. I agree that the test should be in a more general place.

I wonder if we can make it earlier in grouping_planner() just before we
add ModifyTablePath.

Or maybe we can make it even earlier, when we expand an RTE for a
partitioned table and add result tables to leaf_result_relids.

--- a/src/backend/optimizer/util/inherit.c
+++ b/src/backend/optimizer/util/inherit.c
@@ -627,6 +627,16 @@ expand_single_inheritance_child(PlannerInfo *root,
RangeTblEntry *parentrte,
      root->leaf_result_relids = bms_add_member(root->leaf_result_relids,
                                                childRTindex);

+     if (parse->commandType == CMD_MERGE &&
+         childrte->relkind == RELKIND_FOREIGN_TABLE)
+     {
+         ereport(ERROR,
+                 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                  errmsg("cannot execute MERGE on relation \"%s\"",
+                         RelationGetRelationName(childrel)),
+                  errdetail_relkind_not_supported(childrte->relkind)));
+     }

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:

Or maybe we can make it even earlier, when we expand an RTE for a
partitioned table and add result tables to leaf_result_relids.

I'm not really on board with injecting command-type-specific logic into
completely unrelated places just so that we can throw an error a bit
earlier. Alvaro's suggestion of make_modifytable seemed plausible,
not least because it avoids spending any effort when the command
couldn't be MERGE at all.

regards, tom lane

On Fri, Oct 14, 2022 at 10:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Richard Guo <guofenglinux@gmail.com> writes:

Or maybe we can make it even earlier, when we expand an RTE for a
partitioned table and add result tables to leaf_result_relids.

I'm not really on board with injecting command-type-specific logic into
completely unrelated places just so that we can throw an error a bit
earlier. Alvaro's suggestion of make_modifytable seemed plausible,
not least because it avoids spending any effort when the command
couldn't be MERGE at all.

Yeah, that makes sense. Putting this check in inherit.c does look some
weird as there is no other commandType related code in that file.

Agree that Alvaro's suggestion is more reasonable.

Thanks
Richard