Thread: Fix error message for MERGE foreign tables

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?

Best,
Tatsuhiro Nakamori

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)

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