Allow FDW extensions to support MERGE command via CustomScan
Hi hackers,
Currently, it is not possible for any fdw extension to support Merge
command, as that's prohibited in the parser.
In this proposal, we allow extensions to support Merge command via
`CustomScan` node by moving the Merge support check from parser to planner.
For existing fdw, they don't have to change anything. I changed
postgres_fdw mostly for documentation purposes.
See the attached patch.
Thanks,
Onder KALACI
Attachments:
0001-Allow-FDW-extensions-to-support-MERGE-command-via-Cu.patchapplication/octet-stream; name=0001-Allow-FDW-extensions-to-support-MERGE-command-via-Cu.patchDownload
From 80d4e853fe6197228612e4312d690f66c1f818c4 Mon Sep 17 00:00:00 2001
From: Onder KALACI <onderkalaci@gmail.com>
Date: Fri, 6 Dec 2024 11:07:26 +0300
Subject: [PATCH] Allow FDW extensions to support MERGE command via CustomScan
Currently, it is not possible for any fdw extension
to support Merge command, as that's prohibited in the
parser.
In this commit, we allow extensions to support Merge command
via `CustomScan` node by moving the Merge support check
from parser to planner.
For existing fdw, they don't have to change anyting. We change
postgres_fdw mostly for documentation purposes.
---
contrib/postgres_fdw/postgres_fdw.c | 16 ++++++++++++++++
doc/src/sgml/fdwhandler.sgml | 25 +++++++++++++++++++++++++
src/backend/optimizer/plan/createplan.c | 16 ++++++++++------
src/backend/parser/parse_merge.c | 12 ++++++++++--
src/include/foreign/fdwapi.h | 5 +++++
src/tools/pgindent/typedefs.list | 1 +
6 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index c0810fbd7c..541a57575c 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -376,6 +376,7 @@ static void postgresBeginForeignInsert(ModifyTableState *mtstate,
static void postgresEndForeignInsert(EState *estate,
ResultRelInfo *resultRelInfo);
static int postgresIsForeignRelUpdatable(Relation rel);
+static bool postgresIsForeignServerMergeCapable(void);
static bool postgresPlanDirectModify(PlannerInfo *root,
ModifyTable *plan,
Index resultRelation,
@@ -574,6 +575,7 @@ postgres_fdw_handler(PG_FUNCTION_ARGS)
routine->BeginForeignInsert = postgresBeginForeignInsert;
routine->EndForeignInsert = postgresEndForeignInsert;
routine->IsForeignRelUpdatable = postgresIsForeignRelUpdatable;
+ routine->IsForeignServerMergeCapable = postgresIsForeignServerMergeCapable;
routine->PlanDirectModify = postgresPlanDirectModify;
routine->BeginDirectModify = postgresBeginDirectModify;
routine->IterateDirectModify = postgresIterateDirectModify;
@@ -2348,6 +2350,20 @@ postgresIsForeignRelUpdatable(Relation rel)
(1 << CMD_INSERT) | (1 << CMD_UPDATE) | (1 << CMD_DELETE) : 0;
}
+
+/*
+ * postgresIsForeignServerMergeCapable
+ * Determine whether the foreign server is capable of merge. Core code (ExecMerge())
+ * doesn't support merge on foreign tables, so we always return false. Some FDWs
+ * may support merge via CustomScan nodes, in which case they should return true.
+ */
+static bool
+postgresIsForeignServerMergeCapable(void)
+{
+ /* postgres_fdw does not support CMD_MERGE */
+ return false;
+}
+
/*
* postgresRecheckForeignScan
* Execute a local join execution plan for a foreign join
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index b80320504d..818ae93e7e 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1288,6 +1288,31 @@ RecheckForeignScan(ForeignScanState *node,
</para>
</sect2>
+ <sect2 id="fdw-callbacks-merge">
+ <title>FDW Routines for <command>MERGE</command></title>
+
+ <para>
+<programlisting>
+bool
+IsForeignServerMergeCapable();
+</programlisting>
+
+ Postgres doesn't support <command>MERGE</command> on foreign tables,
+ see <function>ExecMerge</function>. Still, extensions may provide
+ custom scan nodes to support <command>MERGE</command> on foreign
+ tables. If your extension provides such custom scan node, this
+ function should return true.
+ </para>
+
+ <para>
+ If the <function>IsForeignServerMergeCapable</function> pointer is set to
+ <literal>NULL</literal> or returns <literal>false</literal>,
+ <command>MERGE</command> fails on the planning phase with a proper
+ error message.
+ </para>
+
+ </sect2>
+
<sect2 id="fdw-callbacks-explain">
<title>FDW Routines for <command>EXPLAIN</command></title>
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 178c572b02..de6418ac0b 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -7242,13 +7242,17 @@ make_modifytable(PlannerInfo *root, Plan *subplan,
*/
if (operation == CMD_MERGE && fdwroutine != NULL)
{
- RangeTblEntry *rte = planner_rt_fetch(rti, root);
+ /* Check if the foreign table is mergeable */
+ if (!fdwroutine->IsForeignServerMergeCapable || !fdwroutine->IsForeignServerMergeCapable())
+ {
+ RangeTblEntry *rte = planner_rt_fetch(rti, root);
- 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));
+ 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));
+ }
}
/*
diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c
index 87df79027d..97db9f11f4 100644
--- a/src/backend/parser/parse_merge.c
+++ b/src/backend/parser/parse_merge.c
@@ -194,10 +194,18 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
false, targetPerms);
qry->mergeTargetRelation = qry->resultRelation;
- /* The target relation must be a table or a view */
+ /*
+ * The target relation must be a table or a view.
+ *
+ * Although the Merge command on foreign tables are allowed in the
+ * grammar, it is not natively supported for foreign tables. We allow the
+ * parser so that we give extensions a chance to support it via custom
+ * scan nodes.
+ */
if (pstate->p_target_relation->rd_rel->relkind != RELKIND_RELATION &&
pstate->p_target_relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE &&
- pstate->p_target_relation->rd_rel->relkind != RELKIND_VIEW)
+ pstate->p_target_relation->rd_rel->relkind != RELKIND_VIEW &&
+ pstate->p_target_relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot execute MERGE on relation \"%s\"",
diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h
index fcde3876b2..13a4fc0e25 100644
--- a/src/include/foreign/fdwapi.h
+++ b/src/include/foreign/fdwapi.h
@@ -115,6 +115,8 @@ typedef void (*EndForeignInsert_function) (EState *estate,
typedef int (*IsForeignRelUpdatable_function) (Relation rel);
+typedef bool (*IsForeignServerMergeCapable_function) (void);
+
typedef bool (*PlanDirectModify_function) (PlannerInfo *root,
ModifyTable *plan,
Index resultRelation,
@@ -248,6 +250,9 @@ typedef struct FdwRoutine
RefetchForeignRow_function RefetchForeignRow;
RecheckForeignScan_function RecheckForeignScan;
+ /* Support functions for MERGE */
+ IsForeignServerMergeCapable_function IsForeignServerMergeCapable;
+
/* Support functions for EXPLAIN */
ExplainForeignScan_function ExplainForeignScan;
ExplainForeignModify_function ExplainForeignModify;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ce33e55bf1..3c38e9f98c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1268,6 +1268,7 @@ IpcSemaphoreId
IpcSemaphoreKey
IsForeignPathAsyncCapable_function
IsForeignRelUpdatable_function
+IsForeignServerMergeCapable_function
IsForeignScanParallelSafe_function
IsoConnInfo
IspellDict
--
2.43.0
Hi,
Just some thoughts on documentation part.
+</programlisting> + + Postgres doesn't support <command>MERGE</command> on foreign tables, + see <function>ExecMerge</function>. Still, extensions may provide + custom scan nodes to support <command>MERGE</command> on foreign + tables. If your extension provides such custom scan node, this + function should return true. + </para>
What's the point about mentioning the ExecMerge function? I didn't
find any relevant documentation about why it not support foreign
tables, maybe its because it should not?
I didn't understand what is a "custom scan node" on the fdw context at
first place (I don't know if it is an already know word on this
context), but from what I've understood so far, to a fdw extension
support MERGE it should implements on PlanForeignModify right? If
that's the case maybe it's worth updating the PlanForeignModify
documentation as well? It only mention insert, update, or delete
operations. Also, I don't know if would be good to link the
PlanForeignModify on this part of the documentation, WYT?
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
HI Matheus, all
+</programlisting> + + Postgres doesn't support <command>MERGE</command> on foreign tables, + see <function>ExecMerge</function>. Still, extensions may provide + custom scan nodes to support <command>MERGE</command> on foreign + tables. If your extension provides such custom scan node, this + function should return true. + </para>What's the point about mentioning the ExecMerge function? I didn't
find any relevant documentation about why it not support foreign
tables, maybe its because it should not?
Exec{Insert|Update|Delete} functions, they all have the common pattern
of code flow, where they call the fdw registered functions via
ExecForeign{Insert|Update|Delete}. And, that's where each fdw
implementation decides what to do on {Insert|Update|Delete} such as
postgres_fdw does in postgresExecForeignInsert.
At this time, ExecMerge() ignores fdws as Merge command is prohibited
for foreign tables in the parser. So, it is guaranteed that
ExecMerge() won't run on a foreign table.
Overall, it would probably be an improvement to mention on ExecMerge()
function comment that foreign tables are not supported.
I didn't understand what is a "custom scan node" on the fdw context at
first place (I don't know if it is an already know word on this
context), but from what I've understood so far, to a fdw extension
support MERGE it should implements on PlanForeignModify right?
In the long term, I think that's a good plan. First, the core code in
ExecMerge() should be aware of foreign tables, then each foreign table
should handle Merge command planning on its own PlanForeignModify.
That'd be great, because the execution of Merge command is pretty
complex, and in essence Postgres would be providing the solid
infrastructure for all foreign tables.
However, I expect that to be a non-trivial patch. Instead, the goal of
this patch is to at least let extensions to completely override the
planning & execution via CustomScan, not confined to Postgres' foreign
table planning & execution. Then, it is completely the responsibility
of the extension developer to handle the Merge command. Today, that's
not possible, and CustomScan's are the recommended** way to implement.
To recap the goal of this patch: Do not change anything for the
existing fdws, but at least give the willing fdw extensions to have a
way to implement Merge command via CustomScan.
Thanks,
Onder
On 2024-Dec-13, Önder Kalacı wrote:
Matheus Alcantara <matheusssilv97@gmail.com> wrote:
I didn't understand what is a "custom scan node" on the fdw context at
first place (I don't know if it is an already know word on this
context), but from what I've understood so far, to a fdw extension
support MERGE it should implements on PlanForeignModify right?In the long term, I think that's a good plan. First, the core code in
ExecMerge() should be aware of foreign tables, then each foreign table
should handle Merge command planning on its own PlanForeignModify.
That'd be great, because the execution of Merge command is pretty
complex, and in essence Postgres would be providing the solid
infrastructure for all foreign tables.However, I expect that to be a non-trivial patch. Instead, the goal of
this patch is to at least let extensions to completely override the
planning & execution via CustomScan, not confined to Postgres' foreign
table planning & execution.
IMO this is a bad plan. It'll become _the_ way to run MERGE on foreign
tables, which will become a selling point for proprietary FDWs, and
nobody will be motivated to write the code to implement the long-term
plan you were describing.
In short, -1 from me.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Hi Alvaro, all
IMO this is a bad plan. It'll become _the_ way to run MERGE on foreign
tables, which will become a selling point for proprietary FDWs, and
nobody will be motivated to write the code to implement the long-term
plan you were describing.In short, -1 from me.
I see your point, but this seems like an artificial limitation in Postgres.
The parser usually doesn’t impose such restrictions, so it’s hard to
understand why FDWs should be treated differently. If someone really wanted
to work around this today, they could hack Postgres and avoid the
limitation anyway.
Our goal here is to follow the spirit of custom scans: enable
experimentation and see what works. This approach doesn’t close the door to
a more complete, native implementation later—it just creates a more natural
path forward in the meantime.
Thanks,
Onder
On 12/13/24 16:03, Önder Kalacı wrote:
Hi Alvaro, all
IMO this is a bad plan. It'll become _the_ way to run MERGE on foreign
tables, which will become a selling point for proprietary FDWs, and
nobody will be motivated to write the code to implement the long-term
plan you were describing.In short, -1 from me.
I see your point, but this seems like an artificial limitation in
Postgres. The parser usually doesn’t impose such restrictions, so it’s
hard to understand why FDWs should be treated differently. If someone
really wanted to work around this today, they could hack Postgres and
avoid the limitation anyway.Our goal here is to follow the spirit of custom scans: enable
experimentation and see what works. This approach doesn’t close the door
to a more complete, native implementation later—it just creates a more
natural path forward in the meantime.
If you want to experiment during development, you can easily do that on
a local fork. I think that's fine.
But if we're expected to support such change, we'd need a way to test
it, which most likely means it'd need postgres_fdw to support it. And
I'd guess adding that would be roughly comparable to actually adding the
"proper" MERGE planning into PlanForeignModify.
So in short, I agree with Álvaro.
regards
--
Tomas Vondra