[PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi,
As per the following code, t1 is a remote table through postgres_fdw:
test=# BEGIN;
BEGIN
test=# SELECT * FROM t1;
...
test=# PREPARE TRANSACTION 'gxid1';
ERROR: cannot prepare a transaction that modified remote tables
I have attached a patch to the documentation that adds remote tables to
the list of objects where any operation prevent using a prepared
transaction, currently it is just notified "operations involving
temporary tables or the session's temporary namespace".
The second patch modify the message returned by postgres_fdw as per the
SELECT statement above the message should be more comprehensible with:
ERROR: cannot PREPARE a transaction that has operated on remote tables
like for temporary objects:
ERROR: cannot PREPARE a transaction that has operated on temporary
objects
Best regards,
--
Gilles
--
Gilles Darold
http://www.darold.net/
Attachments:
fix_prepare_transaction_doc-v1.difftext/x-patch; name=fix_prepare_transaction_doc-v1.diffDownload
diff --git a/doc/src/sgml/ref/prepare_transaction.sgml b/doc/src/sgml/ref/prepare_transaction.sgml
index 5016ca287e..443caf131e 100644
--- a/doc/src/sgml/ref/prepare_transaction.sgml
+++ b/doc/src/sgml/ref/prepare_transaction.sgml
@@ -99,8 +99,8 @@ PREPARE TRANSACTION <replaceable class="parameter">transaction_id</replaceable>
<para>
It is not currently allowed to <command>PREPARE</command> a transaction that
has executed any operations involving temporary tables or the session's
- temporary namespace, created any cursors <literal>WITH HOLD</literal>, or
- executed <command>LISTEN</command>, <command>UNLISTEN</command>, or
+ temporary namespace or remote tables, created any cursors <literal>WITH HOLD</literal>,
+ or executed <command>LISTEN</command>, <command>UNLISTEN</command>, or
<command>NOTIFY</command>.
Those features are too tightly
tied to the current session to be useful in a transaction to be prepared.
fix_message_pg_fdw_v1.difftext/x-patch; name=fix_message_pg_fdw_v1.diffDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 7cd69cc709..2f5c37b159 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -725,7 +725,7 @@ pgfdw_xact_callback(XactEvent event, void *arg)
*/
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot prepare a transaction that modified remote tables")));
+ errmsg("cannot prepare a transaction that has operated on remote tables")));
break;
case XACT_EVENT_PARALLEL_COMMIT:
case XACT_EVENT_COMMIT:
On Fri, Nov 01, 2019 at 05:29:23PM +0100, Gilles Darold wrote:
I have attached a patch to the documentation that adds remote tables to
the list of objects where any operation prevent using a prepared
transaction, currently it is just notified "operations involving
temporary tables or the session's temporary namespace".
Perhaps we had better use foreign tables for the error message and the
docs?
--
Michael
Le 02/11/2019 � 08:31, Michael Paquier a �crit�:
On Fri, Nov 01, 2019 at 05:29:23PM +0100, Gilles Darold wrote:
I have attached a patch to the documentation that adds remote tables to
the list of objects where any operation prevent using a prepared
transaction, currently it is just notified "operations involving
temporary tables or the session's temporary namespace".Perhaps we had better use foreign tables for the error message and the
docs?
--
Michael
Agree, attached is a new version of the patches that replace word remote
by foreign.
--
Gilles
Attachments:
fix_message_pg_fdw_v2.difftext/x-patch; name=fix_message_pg_fdw_v2.diffDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 7cd69cc709..7d0f9ed72a 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -725,7 +725,7 @@ pgfdw_xact_callback(XactEvent event, void *arg)
*/
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot prepare a transaction that modified remote tables")));
+ errmsg("cannot prepare a transaction that has operated on foreign tables")));
break;
case XACT_EVENT_PARALLEL_COMMIT:
case XACT_EVENT_COMMIT:
fix_prepare_transaction_doc-v2.difftext/x-patch; name=fix_prepare_transaction_doc-v2.diffDownload
diff --git a/doc/src/sgml/ref/prepare_transaction.sgml b/doc/src/sgml/ref/prepare_transaction.sgml
index 5016ca287e..d7bf2026f7 100644
--- a/doc/src/sgml/ref/prepare_transaction.sgml
+++ b/doc/src/sgml/ref/prepare_transaction.sgml
@@ -99,8 +99,8 @@ PREPARE TRANSACTION <replaceable class="parameter">transaction_id</replaceable>
<para>
It is not currently allowed to <command>PREPARE</command> a transaction that
has executed any operations involving temporary tables or the session's
- temporary namespace, created any cursors <literal>WITH HOLD</literal>, or
- executed <command>LISTEN</command>, <command>UNLISTEN</command>, or
+ temporary namespace or foreign tables, created any cursors <literal>WITH HOLD</literal>,
+ or executed <command>LISTEN</command>, <command>UNLISTEN</command>, or
<command>NOTIFY</command>.
Those features are too tightly
tied to the current session to be useful in a transaction to be prepared.
Hi Gilles,
On Sat, Nov 2, 2019 at 1:29 AM Gilles Darold <gilles@darold.net> wrote:
As per the following code, t1 is a remote table through postgres_fdw:
test=# BEGIN;
BEGIN
test=# SELECT * FROM t1;
...test=# PREPARE TRANSACTION 'gxid1';
ERROR: cannot prepare a transaction that modified remote tables
I have attached a patch to the documentation that adds remote tables to the list of objects where any operation prevent using a prepared transaction, currently it is just notified "operations involving temporary tables or the session's temporary namespace".
I'm not sure that's a good idea because file_fdw works well for
PREPARE TRANSACTION! How about adding a note about that to the
section of Transaction Management in the postgres_fdw documentation
like the attached?
The second patch modify the message returned by postgres_fdw as per the SELECT statement above the message should be more comprehensible with:
ERROR: cannot PREPARE a transaction that has operated on remote tables
like for temporary objects:
ERROR: cannot PREPARE a transaction that has operated on temporary objects
+1 (I too think it would be better to use "foreign tables" rather
than "remote tables" as pointed by Michael-san, but I think it might
be much better to use "postgres_fdw foreign tables", not just "foreign
tables".)
Best regards,
Etsuro Fujita
Attachments:
improve-postgres-fdw-doc.patchapplication/octet-stream; name=improve-postgres-fdw-doc.patchDownload
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 634a7141ef..ed369cb54b 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -483,6 +483,12 @@
COMMITTED</literal> local transaction. A future
<productname>PostgreSQL</productname> release might modify these rules.
</para>
+
+ <para>
+ Note that it is currently not supported by
+ <filename>postgres_fdw</filename> to prepare the remote transaction for
+ two-phase commit.
+ </para>
</sect2>
<sect2>
Hi Esturo,
Le 05/11/2019 à 10:35, Etsuro Fujita a écrit :
Hi Gilles,
On Sat, Nov 2, 2019 at 1:29 AM Gilles Darold <gilles@darold.net> wrote:
As per the following code, t1 is a remote table through postgres_fdw:
test=# BEGIN;
BEGIN
test=# SELECT * FROM t1;
...test=# PREPARE TRANSACTION 'gxid1';
ERROR: cannot prepare a transaction that modified remote tables
I have attached a patch to the documentation that adds remote tables to the list of objects where any operation prevent using a prepared transaction, currently it is just notified "operations involving temporary tables or the session's temporary namespace".I'm not sure that's a good idea because file_fdw works well for
PREPARE TRANSACTION! How about adding a note about that to the
section of Transaction Management in the postgres_fdw documentation
like the attached?
You are right, read only FDW can be used. A second point in favor of
your remark is that this is the responsibility of the FDW implementation
to throw an error when used with a prepared transaction and I see that
this is not the case for all FDW.
I have attached a single patch that include Etsuro Fujita's patch on
postgres_fdw documentation and mine on postgres_fdw error message with
the precision that it comes from postgres_fdw. The modification about
prepared transaction in PostgreSQL documentation has been removed.
--
Gilles Darold
Attachments:
fix_postgres_fdw_prepared_transaction-v3.difftext/x-patch; name=fix_postgres_fdw_prepared_transaction-v3.diffDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 7cd69cc709..077eaf46ee 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -725,7 +725,7 @@ pgfdw_xact_callback(XactEvent event, void *arg)
*/
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot prepare a transaction that modified remote tables")));
+ errmsg("cannot prepare a transaction that has operated on postgres_fdw foreign tables")));
break;
case XACT_EVENT_PARALLEL_COMMIT:
case XACT_EVENT_COMMIT:
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 634a7141ef..ed369cb54b 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -483,6 +483,12 @@
COMMITTED</literal> local transaction. A future
<productname>PostgreSQL</productname> release might modify these rules.
</para>
+
+ <para>
+ Note that it is currently not supported by
+ <filename>postgres_fdw</filename> to prepare the remote transaction for
+ two-phase commit.
+ </para>
</sect2>
<sect2>
Hi Gilles,
On Tue, Nov 5, 2019 at 8:41 PM Gilles Darold <gilles@darold.net> wrote:
I have attached a single patch that include Etsuro Fujita's patch on
postgres_fdw documentation and mine on postgres_fdw error message with
the precision that it comes from postgres_fdw. The modification about
prepared transaction in PostgreSQL documentation has been removed.
Thanks for the patch! I added the commit message. Does that make
sense? If there are no objections, I'll apply the patch to all
supported branches.
Best regards,
Etsuro Fujita
Attachments:
fix_postgres_fdw_prepared_transaction-v3-efujita.diffapplication/octet-stream; name=fix_postgres_fdw_prepared_transaction-v3-efujita.diffDownload
From 32405d0398106e57beec2454fba70e8cdd830209 Mon Sep 17 00:00:00 2001
From: Etsuro Fujita <efujita@postgresql.org>
Date: Wed, 6 Nov 2019 12:50:21 +0900
Subject: [PATCH] postgres_fdw: Fix error message for PREPARE TRANSACTION.
Currently, postgres_fdw does not support preparing a remote transaction
for two-phase commit even if it is a read-only transaction, but the old
error message appeared to imply that that was not supported if the
remote transaction modified remote tables. Change the error message so
as to include the case where the remote transaction is read-only.
Also add a note about the lack of supporting PREPARE TRANSACTION to the
postgres_fdw documentation.
Back-patch to all supported branches.
Reported-by: Gilles Darold
Author: Gilles Darold and Etsuro Fujita
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/08600ed3-3084-be70-65ba-279ab19618a5%40darold.net
---
contrib/postgres_fdw/connection.c | 2 +-
doc/src/sgml/postgres-fdw.sgml | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 7cd69cc709..077eaf46ee 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -725,7 +725,7 @@ pgfdw_xact_callback(XactEvent event, void *arg)
*/
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot prepare a transaction that modified remote tables")));
+ errmsg("cannot prepare a transaction that has operated on postgres_fdw foreign tables")));
break;
case XACT_EVENT_PARALLEL_COMMIT:
case XACT_EVENT_COMMIT:
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 634a7141ef..ed369cb54b 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -483,6 +483,12 @@
COMMITTED</literal> local transaction. A future
<productname>PostgreSQL</productname> release might modify these rules.
</para>
+
+ <para>
+ Note that it is currently not supported by
+ <filename>postgres_fdw</filename> to prepare the remote transaction for
+ two-phase commit.
+ </para>
</sect2>
<sect2>
--
2.19.2
On Wed, Nov 06, 2019 at 12:57:10PM +0900, Etsuro Fujita wrote:
Thanks for the patch! I added the commit message. Does that make
sense? If there are no objections, I'll apply the patch to all
supported branches.
"postgres_fdw foreign tables" sounds weird to me. Could "foreign
tables using postgres_fdw" be a better wording? I am wondering as
well if we should not split this information into two parts: one for
the actual error message which only mentions foreign tables, and a
second one with a hint to mention that postgres_fdw has been used.
We could have more test cases with 2PC in this module, not necessarily
the responsibility of this patch, but while we're on it..
--
Michael
Hi Michael-san,
On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier <michael@paquier.xyz> wrote:
"postgres_fdw foreign tables" sounds weird to me. Could "foreign
tables using postgres_fdw" be a better wording? I am wondering as
well if we should not split this information into two parts: one for
the actual error message which only mentions foreign tables, and a
second one with a hint to mention that postgres_fdw has been used.
We use "postgres_fdw foreign tables" or "postgres_fdw tables" in
release notes, so I thought it was OK to use that in error messages as
well. But actually, these wordings are not suitable for error
messages?
We could have more test cases with 2PC in this module, not necessarily
the responsibility of this patch, but while we're on it..
Agreed. Will do.
Best regards,
Etsuro Fujita
On Wed, Nov 06, 2019 at 03:12:04PM +0900, Etsuro Fujita wrote:
On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier <michael@paquier.xyz> wrote:
"postgres_fdw foreign tables" sounds weird to me. Could "foreign
tables using postgres_fdw" be a better wording? I am wondering as
well if we should not split this information into two parts: one for
the actual error message which only mentions foreign tables, and a
second one with a hint to mention that postgres_fdw has been used.We use "postgres_fdw foreign tables" or "postgres_fdw tables" in
release notes, so I thought it was OK to use that in error messages as
well. But actually, these wordings are not suitable for error
messages?
It is true that the docs of postgres_fdw use that and that it is used
in some comments. Still, I found this wording a bit weird.. If you
think that what you have is better, I am also fine to let you have the
final word, so please feel to ignore me :)
--
Michael
Hi Michael-san,
On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Nov 06, 2019 at 03:12:04PM +0900, Etsuro Fujita wrote:
On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier <michael@paquier.xyz> wrote:
"postgres_fdw foreign tables" sounds weird to me. Could "foreign
tables using postgres_fdw" be a better wording? I am wondering as
well if we should not split this information into two parts: one for
the actual error message which only mentions foreign tables, and a
second one with a hint to mention that postgres_fdw has been used.We use "postgres_fdw foreign tables" or "postgres_fdw tables" in
release notes, so I thought it was OK to use that in error messages as
well. But actually, these wordings are not suitable for error
messages?It is true that the docs of postgres_fdw use that and that it is used
in some comments. Still, I found this wording a bit weird.. If you
think that what you have is better, I am also fine to let you have the
final word, so please feel to ignore me :)
I'd like to hear the opinions of others.
Best regards,
Etsuro Fujita
Hello.
At Wed, 6 Nov 2019 20:13:10 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
Hi Michael-san,
On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Nov 06, 2019 at 03:12:04PM +0900, Etsuro Fujita wrote:
On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier <michael@paquier.xyz> wrote:
"postgres_fdw foreign tables" sounds weird to me. Could "foreign
tables using postgres_fdw" be a better wording? I am wondering as
well if we should not split this information into two parts: one for
the actual error message which only mentions foreign tables, and a
second one with a hint to mention that postgres_fdw has been used.We use "postgres_fdw foreign tables" or "postgres_fdw tables" in
release notes, so I thought it was OK to use that in error messages as
well. But actually, these wordings are not suitable for error
messages?It is true that the docs of postgres_fdw use that and that it is used
in some comments. Still, I found this wording a bit weird.. If you
think that what you have is better, I am also fine to let you have the
final word, so please feel to ignore me :)I'd like to hear the opinions of others.
FWIW, I see it a bit weird, too. And perhaps "prepare" should be in
upper case letters. Plus, any operation including a SELECT on a
temporary table inhibits PREAPRE TRANSACTION, but the same on a
postgres_fdw foreign tables is not. So the error message is rather
wrong.
A verbose alternative can be:
"cannot PREPARE a transaction that has modified data on foreign tables using postgres_fdw"
Or I think different style is OK here since the message is not of core
but of an extension.
"postgres_fdw doesn't support PREPARE of a transaction that has modified data on foreign tables"
That could be shorter or simpler, of course.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hi Kyotaro,
Le 07/11/2019 à 08:10, Kyotaro Horiguchi a écrit :
Hello.
At Wed, 6 Nov 2019 20:13:10 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
Hi Michael-san,
On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Nov 06, 2019 at 03:12:04PM +0900, Etsuro Fujita wrote:
On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier <michael@paquier.xyz> wrote:
"postgres_fdw foreign tables" sounds weird to me. Could "foreign
tables using postgres_fdw" be a better wording? I am wondering as
well if we should not split this information into two parts: one for
the actual error message which only mentions foreign tables, and a
second one with a hint to mention that postgres_fdw has been used.We use "postgres_fdw foreign tables" or "postgres_fdw tables" in
release notes, so I thought it was OK to use that in error messages as
well. But actually, these wordings are not suitable for error
messages?It is true that the docs of postgres_fdw use that and that it is used
in some comments. Still, I found this wording a bit weird.. If you
think that what you have is better, I am also fine to let you have the
final word, so please feel to ignore me :)I'd like to hear the opinions of others.
FWIW, I see it a bit weird, too. And perhaps "prepare" should be in
upper case letters. Plus, any operation including a SELECT on a
temporary table inhibits PREAPRE TRANSACTION, but the same on a
postgres_fdw foreign tables is not. So the error message is rather
wrong.
This is not what I've experienced, see the first message of the thread.
A SELECT on foreign table prevent to use PREPARE TRANSACTION like with
temporary table. Perhaps postgres_fdw should not throw an error with
readonly queries on foreign tables but I guess that it is pretty hard to
know especially on a later PREPARE event. But maybe I'm wrong, it is not
easy every day :-) Can you share the SQL code you have executed to allow
PREPARE transaction after a SELECT on a postgres_fdw foreign table?
--
Gilles Darold
Horiguchi-san,
On Thu, Nov 7, 2019 at 4:11 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Wed, 6 Nov 2019 20:13:10 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Nov 06, 2019 at 03:12:04PM +0900, Etsuro Fujita wrote:
On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier <michael@paquier.xyz> wrote:
"postgres_fdw foreign tables" sounds weird to me. Could "foreign
tables using postgres_fdw" be a better wording? I am wondering as
well if we should not split this information into two parts: one for
the actual error message which only mentions foreign tables, and a
second one with a hint to mention that postgres_fdw has been used.We use "postgres_fdw foreign tables" or "postgres_fdw tables" in
release notes, so I thought it was OK to use that in error messages as
well. But actually, these wordings are not suitable for error
messages?It is true that the docs of postgres_fdw use that and that it is used
in some comments. Still, I found this wording a bit weird.. If you
think that what you have is better, I am also fine to let you have the
final word, so please feel to ignore me :)I'd like to hear the opinions of others.
FWIW, I see it a bit weird, too.
Only two people complaining about the wording? Considering as well
that we use that wording in the docs and there were no complains about
that IIRC, I don't feel a need to change that, TBH.
And perhaps "prepare" should be in
upper case letters.
Seems like a good idea.
Plus, any operation including a SELECT on a
temporary table inhibits PREAPRE TRANSACTION, but the same on a
postgres_fdw foreign tables is not. So the error message is rather
wrong.A verbose alternative can be:
"cannot PREPARE a transaction that has modified data on foreign tables using postgres_fdw"
I don't think that's better, because that doesn't address the original
issue reported in this thread, as Gilles pointed out just before in
his email. See the commit message in the patch I posted.
Thanks for the comments!
Best regards,
Etsuro Fujita
Hello Gilles. I made a silly mistake.
At Thu, 7 Nov 2019 09:05:55 +0100, Gilles Darold <gilles@darold.net> wrote in
FWIW, I see it a bit weird, too. And perhaps "prepare" should be in
upper case letters. Plus, any operation including a SELECT on a
temporary table inhibits PREAPRE TRANSACTION, but the same on a
postgres_fdw foreign tables is not. So the error message is rather
wrong.This is not what I've experienced, see the first message of the thread.
A SELECT on foreign table prevent to use PREPARE TRANSACTION like with
temporary table. Perhaps postgres_fdw should not throw an error with
readonly queries on foreign tables but I guess that it is pretty hard to
know especially on a later PREPARE event. But maybe I'm wrong, it is not
easy every day :-) Can you share the SQL code you have executed to allow
PREPARE transaction after a SELECT on a postgres_fdw foreign table?
Oooops!
After reading this, I came to be afraid that I did something wrong,
then I rechecked actual behavior. Finally I found that SELECT * FROM
foregn_tbl prohibits PREPARE TRANSACTION. I might have used a local
table instead of foreign tabel at the previous trial.
Sorry for the mistake and thank you for pointing it.
So my fixed proposals are:
"cannot PREPARE a transaction that has operated on foreign tables using postgres_fdw"
Or
"postgres_fdw doesn't support PREPARE of a transaction that has accessed foreign tables"
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hello, Fujita-san.
At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
Only two people complaining about the wording? Considering as well
that we use that wording in the docs and there were no complains about
that IIRC, I don't feel a need to change that, TBH.And perhaps "prepare" should be in
upper case letters.Seems like a good idea.
Plus, any operation including a SELECT on a
temporary table inhibits PREAPRE TRANSACTION, but the same on a
postgres_fdw foreign tables is not. So the error message is rather
wrong.A verbose alternative can be:
"cannot PREPARE a transaction that has modified data on foreign tables using postgres_fdw"
I don't think that's better, because that doesn't address the original
issue reported in this thread, as Gilles pointed out just before in
his email. See the commit message in the patch I posted.
"modified" is my mistake as in the just posted mail. But the most
significant point in the previous mail is using "foreign tables using
postgres_fdw" instead of "postgres_fdw foreign tables". And the other
point is using different message from temporary tables.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Thu, 07 Nov 2019 17:27:47 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
"modified" is my mistake as in the just posted mail. But the most
significant point in the previous mail is using "foreign tables using
postgres_fdw" instead of "postgres_fdw foreign tables". And the other
point is using different message from temporary tables.
I forgot to mention that the comment in XACT_EVENT_PRE_PREPARE
contains the same mistake and needs more or less the same fix.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Horiguchi-san,
On Thu, Nov 7, 2019 at 5:28 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
Only two people complaining about the wording? Considering as well
that we use that wording in the docs and there were no complains about
that IIRC, I don't feel a need to change that, TBH.
But the most
significant point in the previous mail is using "foreign tables using
postgres_fdw" instead of "postgres_fdw foreign tables".
OK, but as I said above, I don't feel the need to change that. How
about leaving it for another patch to improve the wording in that
message and/or the documentation if we really need to do it.
And the other
point is using different message from temporary tables.
You mean we should do s/prepare/PREPARE/?
Best regards,
Etsuro Fujita
Horiguchi-san,
On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
I forgot to mention that the comment in XACT_EVENT_PRE_PREPARE
contains the same mistake and needs more or less the same fix.
Good catch! How about rewriting "We disallow remote transactions that
modified anything" in the comment simply to "We disallow any remote
transactions" or something like that? Attached is an updated patch.
In the patch, I did s/prepare/PREPARE/ to the error message as well,
as you proposed.
Thanks again for reviewing!
Best regards,
Etsuro Fujita
Attachments:
fix_postgres_fdw_prepared_transaction-v4.diffapplication/octet-stream; name=fix_postgres_fdw_prepared_transaction-v4.diffDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 7cd69cc709..27b86a03f8 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -715,17 +715,17 @@ pgfdw_xact_callback(XactEvent event, void *arg)
case XACT_EVENT_PRE_PREPARE:
/*
- * We disallow remote transactions that modified anything,
- * since it's not very reasonable to hold them open until
- * the prepared transaction is committed. For the moment,
- * throw error unconditionally; later we might allow
- * read-only cases. Note that the error will cause us to
- * come right back here with event == XACT_EVENT_ABORT, so
- * we'll clean up the connection state at that point.
+ * We disallow any remote transactions, since it's not
+ * very reasonable to hold them open until the prepared
+ * transaction is committed. For the moment, throw error
+ * unconditionally; later we might allow read-only cases.
+ * Note that the error will cause us to come right back
+ * here with event == XACT_EVENT_ABORT, so we'll clean up
+ * the connection state at that point.
*/
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot prepare a transaction that modified remote tables")));
+ errmsg("cannot PREPARE a transaction that has operated on postgres_fdw foreign tables")));
break;
case XACT_EVENT_PARALLEL_COMMIT:
case XACT_EVENT_COMMIT:
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 634a7141ef..ed369cb54b 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -483,6 +483,12 @@
COMMITTED</literal> local transaction. A future
<productname>PostgreSQL</productname> release might modify these rules.
</para>
+
+ <para>
+ Note that it is currently not supported by
+ <filename>postgres_fdw</filename> to prepare the remote transaction for
+ two-phase commit.
+ </para>
</sect2>
<sect2>
On Thu, Nov 07, 2019 at 06:40:36PM +0900, Etsuro Fujita wrote:
On Thu, Nov 7, 2019 at 5:28 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
Only two people complaining about the wording? Considering as well
That's like.. Half the folks participating to this thread ;)
that we use that wording in the docs and there were no complains about
that IIRC, I don't feel a need to change that, TBH.But the most
significant point in the previous mail is using "foreign tables using
postgres_fdw" instead of "postgres_fdw foreign tables".OK, but as I said above, I don't feel the need to change that. How
about leaving it for another patch to improve the wording in that
message and/or the documentation if we really need to do it.
Fine by me. If I were to put a number on that, I would be around +-0,
so I don't have an actual objection with your point of view either.
--
Michael
Le 07/11/2019 à 11:52, Etsuro Fujita a écrit :
Horiguchi-san,
On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:I forgot to mention that the comment in XACT_EVENT_PRE_PREPARE
contains the same mistake and needs more or less the same fix.Good catch! How about rewriting "We disallow remote transactions that
modified anything" in the comment simply to "We disallow any remote
transactions" or something like that? Attached is an updated patch.
In the patch, I did s/prepare/PREPARE/ to the error message as well,
as you proposed.Thanks again for reviewing!
Best regards,
Etsuro Fujita
Looks good for me,
--
Gilles Darold
Hi Michael-san,
On Fri, Nov 8, 2019 at 9:10 AM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Nov 07, 2019 at 06:40:36PM +0900, Etsuro Fujita wrote:
On Thu, Nov 7, 2019 at 5:28 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
Only two people complaining about the wording? Considering as well
That's like.. Half the folks participating to this thread ;)
Right...
that we use that wording in the docs and there were no complains about
that IIRC, I don't feel a need to change that, TBH.But the most
significant point in the previous mail is using "foreign tables using
postgres_fdw" instead of "postgres_fdw foreign tables".OK, but as I said above, I don't feel the need to change that. How
about leaving it for another patch to improve the wording in that
message and/or the documentation if we really need to do it.Fine by me. If I were to put a number on that, I would be around +-0,
so I don't have an actual objection with your point of view either.
OK, pushed as-is. Thanks for reviewing!
Best regards,
Etsuro Fujita
Hi Gilles,
Show quoted text
On Fri, Nov 8, 2019 at 4:55 PM Gilles Darold <gilles@darold.net> wrote:
Le 07/11/2019 à 11:52, Etsuro Fujita a écrit :
On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:I forgot to mention that the comment in XACT_EVENT_PRE_PREPARE
contains the same mistake and needs more or less the same fix.Good catch! How about rewriting "We disallow remote transactions that
modified anything" in the comment simply to "We disallow any remote
transactions" or something like that? Attached is an updated patch.
In the patch, I did s/prepare/PREPARE/ to the error message as well,
as you proposed.
Looks good for me,
--
Gilles Darold
Hi Gilles,
Sorry, I have sent an unfinished email.
On Fri, Nov 8, 2019 at 5:20 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
On Fri, Nov 8, 2019 at 4:55 PM Gilles Darold <gilles@darold.net> wrote:
Le 07/11/2019 à 11:52, Etsuro Fujita a écrit :
On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:I forgot to mention that the comment in XACT_EVENT_PRE_PREPARE
contains the same mistake and needs more or less the same fix.Good catch! How about rewriting "We disallow remote transactions that
modified anything" in the comment simply to "We disallow any remote
transactions" or something like that? Attached is an updated patch.
In the patch, I did s/prepare/PREPARE/ to the error message as well,
as you proposed.Looks good for me,
Pushed after modifying the commit message a bit. Thanks!
Best regards,
Etsuro Fujita
On Fri, Nov 08, 2019 at 05:25:52PM +0900, Etsuro Fujita wrote:
Pushed after modifying the commit message a bit. Thanks!
Should we have more tests for 2PC then?
--
Michael
Hi Michael,
Le 08/11/2019 à 10:05, Michael Paquier a écrit :
On Fri, Nov 08, 2019 at 05:25:52PM +0900, Etsuro Fujita wrote:
Pushed after modifying the commit message a bit. Thanks!
Should we have more tests for 2PC then?
--
Michael
I don't think so. The support or not of 2PC is on foreign data wrapper
side. In postgres_fdw contrib the error for use with 2PC is not part of
the test but it will be thrown anyway. I guess that a test will be
valuable only if there is support for readonly query.
--
Gilles Darold
On Fri, Nov 08, 2019 at 10:19:01AM +0100, Gilles Darold wrote:
I don't think so. The support or not of 2PC is on foreign data wrapper
side. In postgres_fdw contrib the error for use with 2PC is not part of
the test but it will be thrown anyway. I guess that a test will be
valuable only if there is support for readonly query.
That's what I call a case for negative testing. We don't allow 2PC to
be used so there is a point in having a test to make sure of that.
This way, if the code in this area is refactored or changed, we still
make sure that 2PC is correctly prevented. My suggestion is to close
this gap. One point here is that this requires an alternate output
file because of max_prepared_transactions and there is no point in
creating one with all the tests of postgres_fdw in a single file as we
have now as it would create 8k lines of expected file bloat, so it
would be better to split the tests first. My 2c.
--
Michael
Le 09/11/2019 à 02:22, Michael Paquier a écrit :
On Fri, Nov 08, 2019 at 10:19:01AM +0100, Gilles Darold wrote:
I don't think so. The support or not of 2PC is on foreign data wrapper
side. In postgres_fdw contrib the error for use with 2PC is not part of
the test but it will be thrown anyway. I guess that a test will be
valuable only if there is support for readonly query.That's what I call a case for negative testing. We don't allow 2PC to
be used so there is a point in having a test to make sure of that.
This way, if the code in this area is refactored or changed, we still
make sure that 2PC is correctly prevented. My suggestion is to close
this gap. One point here is that this requires an alternate output
file because of max_prepared_transactions and there is no point in
creating one with all the tests of postgres_fdw in a single file as we
have now as it would create 8k lines of expected file bloat, so it
would be better to split the tests first. My 2c.
--
Michael
Hi Michael, it looks that a separate test is not required at least for
this test. Here is a patch that enable the test in
contrib/postgres_fdw/, expected output:
-- Make sure that 2PC is correctly prevented
BEGIN;
SELECT count(*) FROM ft1;
count
-------
822
(1 row)
-- Must throw an error
PREPARE TRANSACTION 'fdw_tpc';
ERROR: cannot PREPARE a transaction that has operated on
postgres_fdw foreign tables
ROLLBACK;
WARNING: there is no transaction in progress
--
Gilles Darold
http://www.darold.net/
Attachments:
patch_postgres_fdw_2pc_test-v1.difftext/x-patch; name=patch_postgres_fdw_2pc_test-v1.diffDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index f0c842a607..9004e7e076 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8781,3 +8781,16 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
-- Clean-up
RESET enable_partitionwise_aggregate;
+-- Make sure that 2PC is correctly prevented
+BEGIN;
+SELECT count(*) FROM ft1;
+ count
+-------
+ 822
+(1 row)
+
+-- Must throw an error
+PREPARE TRANSACTION 'fdw_tpc';
+ERROR: cannot PREPARE a transaction that has operated on postgres_fdw foreign tables
+ROLLBACK;
+WARNING: there is no transaction in progress
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 630b803e26..5df6fe3ae0 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2479,3 +2479,10 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
-- Clean-up
RESET enable_partitionwise_aggregate;
+
+-- Make sure that 2PC is correctly prevented
+BEGIN;
+SELECT count(*) FROM ft1;
+-- Must throw an error
+PREPARE TRANSACTION 'fdw_tpc';
+ROLLBACK;
On Mon, Nov 11, 2019 at 04:43:18PM +0100, Gilles Darold wrote:
Hi Michael, it looks that a separate test is not required at least for
this test. Here is a patch that enable the test in
contrib/postgres_fdw/, expected output:
Indeed, thanks for looking. I thought that the callback was called
after checking for max_prepared_transaction, but that's not the case.
So let's add at least a test case. Any objections?
--
Michael
On Tue, Nov 12, 2019 at 09:35:03AM +0900, Michael Paquier wrote:
Indeed, thanks for looking. I thought that the callback was called
after checking for max_prepared_transaction, but that's not the case.
So let's add at least a test case. Any objections?
Okay, done.
--
Michael