Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

Started by Etsuro Fujitaover 9 years ago10 messages

fujita.etsuro@lab.ntt.co.jp

over 9 years ago

1 attachment(s)

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

Attachments:

fdw-query-planning.patchbinary/octet-stream; name=fdw-query-planning.patchDownload

*** a/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
***************
*** 1548,1554 **** GetForeignServerByName(const char *name, bool missing_ok);
      </para>
  
      <para>
!      <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</>
--- 1548,1554 ----
      </para>
  
      <para>
!      <function>PlanForeignModify</> and related 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</>
***************
*** 1557,1563 **** GetForeignServerByName(const char *name, bool missing_ok);
       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.  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
--- 1557,1565 ----
       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, rather than
!      using <function>PlanDirectModify</> and related callbacks for direct
!      modification described in the same section.  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

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 9 years ago

In reply to: Etsuro Fujita (#1)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

over 9 years ago

In reply to: Etsuro Fujita (#1)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 9 years ago

In reply to: Robert Haas (#3)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

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.

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

--
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

rushabh.lathia@gmail.com

over 9 years ago

In reply to: Etsuro Fujita (#4)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

On Fri, Sep 2, 2016 at 5:00 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

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.

Considering the primary object of this patch is just to let the FDW author
know
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

Michael Paquier

michael.paquier@gmail.com

over 9 years ago

In reply to: Rushabh Lathia (#5)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

about 9 years ago

In reply to: Michael Paquier (#6)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

about 9 years ago

In reply to: Robert Haas (#7)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

about 9 years ago

In reply to: Tom Lane (#8)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

about 9 years ago

In reply to: Robert Haas (#9)

Re: Confusing docs about GetForeignUpperPaths in fdwhandler.sgml

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.

Best regards,
Etsuro Fujita

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers