Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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 signature.asc Description: PGP signature
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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 signature.asc Description: PGP signature
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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/ 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;
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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 signature.asc Description: PGP signature
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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 signature.asc Description: PGP signature
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi Gilles, Sorry, I have sent an unfinished email. On Fri, Nov 8, 2019 at 5:20 PM Etsuro Fujita wrote: > On Fri, Nov 8, 2019 at 4:55 PM Gilles Darold wrote: > > Le 07/11/2019 à 11:52, Etsuro Fujita a écrit : > > > On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi > > > 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi Gilles, On Fri, Nov 8, 2019 at 4:55 PM Gilles Darold wrote: > Le 07/11/2019 à 11:52, Etsuro Fujita a écrit : > > On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi > > 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 >
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi Michael-san, On Fri, Nov 8, 2019 at 9:10 AM Michael Paquier 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 > > wrote: > >> At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita > >> 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Le 07/11/2019 à 11:52, Etsuro Fujita a écrit : > Horiguchi-san, > > On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi > 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
On Thu, Nov 07, 2019 at 06:40:36PM +0900, Etsuro Fujita wrote: > On Thu, Nov 7, 2019 at 5:28 PM Kyotaro Horiguchi > wrote: >> At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita >> 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 signature.asc Description: PGP signature
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Horiguchi-san, On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi 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 fix_postgres_fdw_prepared_transaction-v4.diff Description: Binary data
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Horiguchi-san, On Thu, Nov 7, 2019 at 5:28 PM Kyotaro Horiguchi wrote: > At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita > 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
At Thu, 07 Nov 2019 17:27:47 +0900 (JST), Kyotaro Horiguchi 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hello, Fujita-san. At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hello Gilles. I made a silly mistake. At Thu, 7 Nov 2019 09:05:55 +0100, Gilles Darold 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Horiguchi-san, On Thu, Nov 7, 2019 at 4:11 PM Kyotaro Horiguchi wrote: > At Wed, 6 Nov 2019 20:13:10 +0900, Etsuro Fujita > wrote in > > On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier 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 > > > > 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi Kyotaro, Le 07/11/2019 à 08:10, Kyotaro Horiguchi a écrit : > Hello. > > At Wed, 6 Nov 2019 20:13:10 +0900, Etsuro Fujita > wrote in >> Hi Michael-san, >> >> On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier 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 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hello. At Wed, 6 Nov 2019 20:13:10 +0900, Etsuro Fujita wrote in > Hi Michael-san, > > On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier 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 > > > 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi Michael-san, On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier 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 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
On Wed, Nov 06, 2019 at 03:12:04PM +0900, Etsuro Fujita wrote: > On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier 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 signature.asc Description: PGP signature
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi Michael-san, On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier 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
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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 signature.asc Description: PGP signature
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi Gilles, On Tue, Nov 5, 2019 at 8:41 PM Gilles Darold 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 fix_postgres_fdw_prepared_transaction-v3-efujita.diff Description: Binary data
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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 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 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 local transaction. A future PostgreSQL release might modify these rules. + + + Note that it is currently not supported by + postgres_fdw to prepare the remote transaction for + two-phase commit. +
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
Hi Gilles, On Sat, Nov 2, 2019 at 1:29 AM Gilles Darold 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 improve-postgres-fdw-doc.patch Description: Binary data
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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 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: 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 transaction_id It is not currently allowed to PREPARE a transaction that has executed any operations involving temporary tables or the session's - temporary namespace, created any cursors WITH HOLD, or - executed LISTEN, UNLISTEN, or + temporary namespace or foreign tables, created any cursors WITH HOLD, + or executed LISTEN, UNLISTEN, or NOTIFY. Those features are too tightly tied to the current session to be useful in a transaction to be prepared.
Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.
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 signature.asc Description: PGP signature