Re: [HACKERS] Two phase commit in ECPG

Hi all,

I found that ecpg has not been supporting COMMIT PREPARED and ROLLBACK
PREPARED since version 9.0.2.
For example, if the test code does,
EXEC SQL BEGIN;
EXEC SQL INSERT INTO t1 VALUES(1);
EXEC SQL PREPARE TRANSACTION 'gxid';
EXEC SQL COMMIT PREPARED 'gxid';
I got error "COMMIT PREPARED cannot run inside a transaction block".

This cause is that the "begin transaction" is issued automatically
before executing COMMIT PREPARED if we're not in auto commit. The
Commit 816b008eaf1a1ff1069f3bafff363a9a8bf04a21 fixed bug of incorrect
status calculation but at the same time it became the cause of this
behavior.
Is this a bug?

Attached 001 patch fixes this issue and add regression test for two
phase commit in ecpg.

Also, in spite of ecpg identifies these two commands as transaction
command similar to other transaction commands like commit and rollback
the documentation doesn't mentioned about these at all. Attached 002
patch add description about these to documentation.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Dear Sawada-san,

> This cause is that the "begin transaction" is issued automatically
> before executing COMMIT PREPARED if we're not in auto commit. The
> Commit 816b008eaf1a1ff1069f3bafff363a9a8bf04a21 fixed bug of
> incorrect
> status calculation but at the same time it became the cause of this
> behavior.
> Is this a bug?

I'd say so, yes.

As soon as I find time I'll get to it including back porting your
patch.

Thank you very much for spotting and fixing.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL

On Sat, Mar 4, 2017 at 4:11 AM, Michael Meskes <meskes(at)postgresql(dot)org> wrote:
> Dear Sawada-san,
>
>> This cause is that the "begin transaction" is issued automatically
>> before executing COMMIT PREPARED if we're not in auto commit. The
>> Commit 816b008eaf1a1ff1069f3bafff363a9a8bf04a21 fixed bug of
>> incorrect
>> status calculation but at the same time it became the cause of this
>> behavior.
>> Is this a bug?
>
> I'd say so, yes.
>
> As soon as I find time I'll get to it including back porting your
> patch.

Thanks

Previous 002 patch lacked to add describing PREPARE TRANSACTION.
Attached updated 002 patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

> Previous 002 patch lacked to add describing PREPARE TRANSACTION.
> Attached updated 002 patch.

I just committed both patches and a backport of the bug fix itself.
Thanks again for finding and fixing.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL

On Tue, Mar 14, 2017 at 1:35 AM, Michael Meskes <meskes(at)postgresql(dot)org> wrote:
>> Previous 002 patch lacked to add describing PREPARE TRANSACTION.
>> Attached updated 002 patch.
>
> I just committed both patches and a backport of the bug fix itself.
> Thanks again for finding and fixing.
Regression tests for sql/twophase is failing while performing the test
with make installcheck.
+ [NO_PID]: ecpg_check_PQresult on line 32: bad response - ERROR:
prepared transactions are disabled
+ HINT: Set max_prepared_transactions to a nonzero value.

Setting max_prepared_transactions accordingly fixes the issue. But,
I'm not sure whether this test should be performed by installcheck by
default or should only be performed by make
installcheck-prepared-txns.

--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

On Fri, Mar 17, 2017 at 12:17 PM, Kuntal Ghosh
<kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> On Tue, Mar 14, 2017 at 1:35 AM, Michael Meskes <meskes(at)postgresql(dot)org> wrote:
>>> Previous 002 patch lacked to add describing PREPARE TRANSACTION.
>>> Attached updated 002 patch.
>>
>> I just committed both patches and a backport of the bug fix itself.
>> Thanks again for finding and fixing.
> Regression tests for sql/twophase is failing while performing the test
> with make installcheck.
> + [NO_PID]: ecpg_check_PQresult on line 32: bad response - ERROR:
> prepared transactions are disabled
> + HINT: Set max_prepared_transactions to a nonzero value.
>
> Setting max_prepared_transactions accordingly fixes the issue. But,
> I'm not sure whether this test should be performed by installcheck by
> default or should only be performed by make
> installcheck-prepared-txns.
>

Thank you for pointing out.
Yeah, I agree that the twophase regression test should not be
performed by default as long as the default value of
max_prepared_transactions is 0. Similar to this, the isolation check
regression test does same thing. Attached patch removes sql/twophase
from schedule file and add new type of regression test.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Mar 17, 2017 at 4:34 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, Mar 17, 2017 at 12:17 PM, Kuntal Ghosh
> <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
>> On Tue, Mar 14, 2017 at 1:35 AM, Michael Meskes <meskes(at)postgresql(dot)org> wrote:
>>>> Previous 002 patch lacked to add describing PREPARE TRANSACTION.
>>>> Attached updated 002 patch.
>>>
>>> I just committed both patches and a backport of the bug fix itself.
>>> Thanks again for finding and fixing.
>> Regression tests for sql/twophase is failing while performing the test
>> with make installcheck.
>> + [NO_PID]: ecpg_check_PQresult on line 32: bad response - ERROR:
>> prepared transactions are disabled
>> + HINT: Set max_prepared_transactions to a nonzero value.
>>
>> Setting max_prepared_transactions accordingly fixes the issue. But,
>> I'm not sure whether this test should be performed by installcheck by
>> default or should only be performed by make
>> installcheck-prepared-txns.
>>
>
> Thank you for pointing out.
> Yeah, I agree that the twophase regression test should not be
> performed by default as long as the default value of
> max_prepared_transactions is 0. Similar to this, the isolation check
> regression test does same thing. Attached patch removes sql/twophase
> from schedule file and add new type of regression test.
>
The patch looks good. I've performed installcheck and
installcheck-prepared-txns. It's working as it should be.

--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

> Thank you for pointing out.
> Yeah, I agree that the twophase regression test should not be
> performed by default as long as the default value of
> max_prepared_transactions is 0. Similar to this, the isolation check
> regression test does same thing. Attached patch removes sql/twophase
> from schedule file and add new type of regression test.

Would it be possible to include it in "make check"? Any check that
needs manual interaction will not be executed nearly is often as the
others and become much less useful imo.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL

On Fri, Mar 17, 2017 at 5:50 PM, Michael Meskes <meskes(at)postgresql(dot)org> wrote:
>> Thank you for pointing out.
>> Yeah, I agree that the twophase regression test should not be
>> performed by default as long as the default value of
>> max_prepared_transactions is 0. Similar to this, the isolation check
>> regression test does same thing. Attached patch removes sql/twophase
>> from schedule file and add new type of regression test.
>
> Would it be possible to include it in "make check"? Any check that
> needs manual interaction will not be executed nearly is often as the
> others and become much less useful imo.
>

Yes. I added two-phase commit test to "make check" test schedule while
adding new two type of test.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

> Yes. I added two-phase commit test to "make check" test schedule
> while
> adding new two type of test.

Thank you, committed.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL