Thread: Re: Allow FDW extensions to support MERGE command via CustomScan
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
** https://www.postgresql.org/docs/current/custom-scan.html
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.
>
> 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