Horton

Thread

Commits

  1. Add regression test for two-phase transaction in postgres_fdw

  2. postgres_fdw: Fix error message for PREPARE TRANSACTION.

  1. [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Gilles DAROLD <gilles@darold.net> — 2019-11-01T16:29:23Z

    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/

  2. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Michael Paquier <michael@paquier.xyz> — 2019-11-02T07:31:29Z

    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

  3. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Gilles Darold <gillesdarold@gmail.com> — 2019-11-03T08:12:38Z

    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

  4. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-05T09:35:54Z

    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

  5. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Gilles DAROLD <gilles@darold.net> — 2019-11-05T11:41:41Z

    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

  6. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-06T03:57:10Z

    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

  7. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Michael Paquier <michael@paquier.xyz> — 2019-11-06T04:13:12Z

    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

  8. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-06T06:12:04Z

    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

  9. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Michael Paquier <michael@paquier.xyz> — 2019-11-06T07:35:10Z

    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

  10. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-06T11:13:10Z

    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

  11. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-11-07T07:10:47Z

    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

  12. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Gilles DAROLD <gilles@darold.net> — 2019-11-07T08:05:55Z

    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

  13. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-07T08:20:07Z

    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

  14. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-11-07T08:22:18Z

    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

  15. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-11-07T08:27:47Z

    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

  16. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-11-07T08:31:29Z

    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

  17. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-07T09:40:36Z

    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

  18. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-07T10:52:42Z

    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

  19. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Michael Paquier <michael@paquier.xyz> — 2019-11-08T00:10:04Z

    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

  20. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Gilles DAROLD <gilles@darold.net> — 2019-11-08T07:55:08Z

    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

  21. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-08T08:19:34Z

    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

  22. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-08T08:20:33Z

    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
>

  23. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-11-08T08:25:52Z

    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

  24. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Michael Paquier <michael@paquier.xyz> — 2019-11-08T09:05:07Z

    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

  25. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Gilles DAROLD <gilles@darold.net> — 2019-11-08T09:19:01Z

    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

  26. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Michael Paquier <michael@paquier.xyz> — 2019-11-09T01:22:51Z

    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

  27. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Gilles DAROLD <gilles@darold.net> — 2019-11-11T15:43:18Z

    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/

  28. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Michael Paquier <michael@paquier.xyz> — 2019-11-12T00:35:03Z

    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

  29. Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

    Michael Paquier <michael@paquier.xyz> — 2019-11-13T04:53:24Z

    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