Thread: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml
Hi,
I noticed that the following note about direct modification via
GetForeignUpperPaths in fdwhandler.sgml is a bit confusing. We have
another approach using PlanDirectModify, so that should be reflected in
the note as well. Please find attached a patch.
<function>PlanForeignModify</> and the other callbacks described in
<xref linkend="fdw-callbacks-update"> are designed around the
assumption
that the foreign relation will be scanned in the usual way and then
individual row updates will be driven by a local
<literal>ModifyTable</>
plan node. This approach is necessary for the general case where an
update requires reading local tables as well as foreign tables.
However, if the operation could be executed entirely by the foreign
server, the FDW could generate a path representing that and insert it
into the <literal>UPPERREL_FINAL</> upper relation, where it would
compete against the <literal>ModifyTable</> approach.
Best regards,
Etsuro Fujita
Attachment
On 2016/08/01 21:14, Etsuro Fujita wrote: > I noticed that the following note about direct modification via > GetForeignUpperPaths in fdwhandler.sgml is a bit confusing. We have > another approach using PlanDirectModify, so that should be reflected in > the note as well. Please find attached a patch. > > <function>PlanForeignModify</> and the other callbacks described in > <xref linkend="fdw-callbacks-update"> are designed around the > assumption > that the foreign relation will be scanned in the usual way and then > individual row updates will be driven by a local > <literal>ModifyTable</> > plan node. This approach is necessary for the general case where an > update requires reading local tables as well as foreign tables. > However, if the operation could be executed entirely by the foreign > server, the FDW could generate a path representing that and insert it > into the <literal>UPPERREL_FINAL</> upper relation, where it would > compete against the <literal>ModifyTable</> approach. I'll add this to the upcoming CF. Best regards, Etsuro Fujita
On Mon, Aug 1, 2016 at 5:44 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote: > I noticed that the following note about direct modification via > GetForeignUpperPaths in fdwhandler.sgml is a bit confusing. We have another > approach using PlanDirectModify, so that should be reflected in the note as > well. Please find attached a patch. > > <function>PlanForeignModify</> and the other callbacks described in > <xref linkend="fdw-callbacks-update"> are designed around the > assumption > that the foreign relation will be scanned in the usual way and then > individual row updates will be driven by a local > <literal>ModifyTable</> > plan node. This approach is necessary for the general case where an > update requires reading local tables as well as foreign tables. > However, if the operation could be executed entirely by the foreign > server, the FDW could generate a path representing that and insert it > into the <literal>UPPERREL_FINAL</> upper relation, where it would > compete against the <literal>ModifyTable</> approach. I suppose this is factually correct, but I don't think it's very illuminating. I think that if we're going to document both approaches, there should be some discussion of the pros and cons of PlanDirectModify vs. PlanForeignModify. Of course either should be better than an iterative ModifyTable, but how should the FDW author decide between the two of them? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Robert,
Thanks for the comments!
On 2016/09/02 11:55, Robert Haas wrote:
> On Mon, Aug 1, 2016 at 5:44 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> I noticed that the following note about direct modification via
>> GetForeignUpperPaths in fdwhandler.sgml is a bit confusing. We have another
>> approach using PlanDirectModify, so that should be reflected in the note as
>> well. Please find attached a patch.
>>
>> <function>PlanForeignModify</> and the other callbacks described in
>> <xref linkend="fdw-callbacks-update"> are designed around the
>> assumption
>> that the foreign relation will be scanned in the usual way and then
>> individual row updates will be driven by a local
>> <literal>ModifyTable</>
>> plan node. This approach is necessary for the general case where an
>> update requires reading local tables as well as foreign tables.
>> However, if the operation could be executed entirely by the foreign
>> server, the FDW could generate a path representing that and insert it
>> into the <literal>UPPERREL_FINAL</> upper relation, where it would
>> compete against the <literal>ModifyTable</> approach.
> I suppose this is factually correct, but I don't think it's very
> illuminating. I think that if we're going to document both
> approaches, there should be some discussion of the pros and cons of
> PlanDirectModify vs. PlanForeignModify.
PlanDirectModify vs. GetForeignUpperPaths for an UPPERREL_FINAL upper
relation?
> Of course either should be
> better than an iterative ModifyTable, but how should the FDW author
> decide between the two of them?
That would apply to row locking. We have two approaches for that too:
GetForeignRowMarkType and GetForeignUpperPaths, which is documented in
the same paragraph following the above documentation:
This approach could also be used to implement remote <literal>SELECT FOR UPDATE</>, rather than using the
rowlocking callbacks described in <xref linkend="fdw-callbacks-row-locking">. Keep in mind that a path
The point of the patch is just to let the FDW author know that there is
another approach for implementing direct modification (ie,
PlanDirectModify) just as for implementing row locking.
I agree that the documentation about how the FDW author should decide
between the two would be helpful, but I'd like to leave that for future
work.
Best regards,
Etsuro Fujita
On Fri, Sep 2, 2016 at 5:00 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
Considering the primary object of this patch is just to let the FDW author know
Hi Robert,
Thanks for the comments!
On 2016/09/02 11:55, Robert Haas wrote:On Mon, Aug 1, 2016 at 5:44 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:I noticed that the following note about direct modification via
GetForeignUpperPaths in fdwhandler.sgml is a bit confusing. We have another
approach using PlanDirectModify, so that should be reflected in the note as
well. Please find attached a patch.
<function>PlanForeignModify</> and the other callbacks described in
<xref linkend="fdw-callbacks-update"> are designed around the
assumption
that the foreign relation will be scanned in the usual way and then
individual row updates will be driven by a local
<literal>ModifyTable</>
plan node. This approach is necessary for the general case where an
update requires reading local tables as well as foreign tables.
However, if the operation could be executed entirely by the foreign
server, the FDW could generate a path representing that and insert it
into the <literal>UPPERREL_FINAL</> upper relation, where it would
compete against the <literal>ModifyTable</> approach.I suppose this is factually correct, but I don't think it's very
illuminating. I think that if we're going to document both
approaches, there should be some discussion of the pros and cons of
PlanDirectModify vs. PlanForeignModify.
PlanDirectModify vs. GetForeignUpperPaths for an UPPERREL_FINAL upper relation?Of course either should be
better than an iterative ModifyTable, but how should the FDW author
decide between the two of them?
That would apply to row locking. We have two approaches for that too: GetForeignRowMarkType and GetForeignUpperPaths, which is documented in the same paragraph following the above documentation:
This approach
could also be used to implement remote <literal>SELECT FOR UPDATE</>,
rather than using the row locking callbacks described in
<xref linkend="fdw-callbacks-row-locking">. Keep in mind that a path
The point of the patch is just to let the FDW author know that there is another approach for implementing direct modification (ie, PlanDirectModify) just as for implementing row locking.
that there is another approach for implementing direct modification, I like the
idea of modifying the document.
I agree that the documentation about how the FDW author should decide between the two would be helpful, but I'd like to leave that for future work.
I performed basic test with patch,
a) patch get applied cleanly on latest source,
b) able to build documentation cleanly.
Marking this as ready for committer.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Rushabh Lathia
On Thu, Sep 22, 2016 at 7:48 PM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote: > I performed basic test with patch, > > a) patch get applied cleanly on latest source, > b) able to build documentation cleanly. > > Marking this as ready for committer. Oops, incorrect patch... I am moving it to next CF. Discussion can continue there. -- Michael
On Sun, Oct 2, 2016 at 10:03 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Sep 22, 2016 at 7:48 PM, Rushabh Lathia > <rushabh.lathia@gmail.com> wrote: >> I performed basic test with patch, >> >> a) patch get applied cleanly on latest source, >> b) able to build documentation cleanly. >> >> Marking this as ready for committer. > > Oops, incorrect patch... I am moving it to next CF. Discussion can > continue there. I'm not interested in committing this patch. I don't believe it is an improvement on what we've got today. Tom, any chance you could offer an opinion? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I'm not interested in committing this patch. I don't believe it is an
> improvement on what we've got today.
> Tom, any chance you could offer an opinion?
I have no objection to this patch as such, but I think that the docs
around FDW direct modify need significantly more work than this.
I've had a to-do item for awhile to work on that, but it hasn't gotten
to the top of the list.
A larger issue is that I think the API itself is poorly designed, as
I stated awhile ago (<31706.1457547166@sss.pgh.pa.us>) and was told it
was too late to object. So that's kind of discouraged me from bothering.
regards, tom lane
On Wed, Oct 26, 2016 at 3:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm not interested in committing this patch. I don't believe it is an >> improvement on what we've got today. >> Tom, any chance you could offer an opinion? > > I have no objection to this patch as such, but I think that the docs > around FDW direct modify need significantly more work than this. > I've had a to-do item for awhile to work on that, but it hasn't gotten > to the top of the list. Well, the question is whether you or someone else is willing to commit it. I am not. If no one else is either, then let's call it Rejected and move on. > A larger issue is that I think the API itself is poorly designed, as > I stated awhile ago (<31706.1457547166@sss.pgh.pa.us>) and was told it > was too late to object. So that's kind of discouraged me from bothering. Apparently, you were already fairly discouraged, because you were weighing in about once per release cycle to say "nope, still not right". I'd really be quite happy to see you take a more active hand in the FDW discussions; clearly, you've got a lot of understanding of that area that is pretty much unique to you. I'd even support an effort to rewrite the work that has already been done in a form more to your liking, but I think you'd need to actually pay attention to the threads on a somewhat regular basis in order that to be practical. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2016/11/03 23:39, Robert Haas wrote: > On Wed, Oct 26, 2016 at 3:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> A larger issue is that I think the API itself is poorly designed, as >> I stated awhile ago (<31706.1457547166@sss.pgh.pa.us>) I agree on that point. I plan to rewrite direct modify using upper planner path-ification; I think we would no longer need the planner API PlanDirectModify, but I expect the executor/explain APIs (ie, BeginDirectModify, IterateDirectModify, EndDirectModify, and ExplainDirectModify) would be still useful after that rewrite. Before that, however, I'd like to work on extend postgres_fdw so as to handle more cases such as UPDATE/DELETE on a join and INSERT, with the existing API. > I'd really be quite happy to see you take a more active hand > in the FDW discussions; clearly, you've got a lot of understanding of > that area that is pretty much unique to you. I'd even support an > effort to rewrite the work that has already been done in a form more > to your liking, but I think you'd need to actually pay attention to > the threads on a somewhat regular basis in order that to be practical. +1 Best regards, Etsuro Fujita