Thread: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdwmessage.

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/

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

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
preventusing a prepared transaction, currently it is just notified "operations involving temporary tables or the
session'stemporary 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
morecomprehensible 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

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
preventusing a prepared transaction, currently it is just notified "operations involving temporary tables or the
session'stemporary 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

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

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

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,

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

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/

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