On Sat, Mar 3, 2012 at 2:56 AM, Free Ekanayaka
<email address hidden> wrote:

>> However, for backends that don't support TPC we are
>> moving the actual commit from happening in the commit() method
>> to the tpc_finish() method.
>
> Uhm, the branch is *not* moving that. The StoreDataManager.commit
> method reads:
>
>    def commit(self, txn):
>        if self._store._tpc:
>            self._store.prepare()
>        else:
>            try:
>                self._store.commit()
>            finally:
>                self._hook_register_transaction_event()
>
> so it explicitly keeps the old behavior and we *do* commit there
> stores which are not set for TPC.
>
> Perhaps you mean that the branch *should* change this? That would
> indeed support the use case that you point about a single store
> not capable of TPC and all others set for TPC.

What I meant was that if you enable TPC on the store, the commit
happens at a different stage even if your backend does not support
TPC.

If you are mixing TPC capable and non-TPC capable backends, the 'best'
ordering seems to be:

transaction.commit
  1) prepare TPC capable backends for commit
transaction.tpc_finish
  1) commit non-TPC capable backends
  2) commit TPC capable backends

> By the way, note that according to the documention of
> IDataManager commiting the changes in tpc_finish() should be
> actually the correct behavior, whereas the one we have
> now for stores not set for TPC (running Store.commit in
> StoreDataManager.commit) is not:

Agreed.

> So if we want to support the use case you mention, I can change
> the current behavior and run store.commit() in tpc_finish() for
> stores not set for TPC, and change sortKey() to have non-TPC
> stores commit first.
>
> This would change the current behavior (though in a way more
> compliant with the IDataManager contract), but I can't think of
> bad consequences.

Right. It is an edge case that may never actually apply to anyone, so
I'm not going to mandate it for landing. It does seem correct to me.
We do need to ensure that the flush happens earlier, but you are
already doing that.

<update>See below, changing this seems pointless as ZStorm will not
support mixing TPC and non-TPC stores without further
extension</update>

> Good point, fixed.
>
> I just didn't add a test for re-using the same xid as PostgreSQL
> seems just happy with passing the same transaction_id string to
> two different PREPARE TRANSACTION commands in two different
> transactions. It feels like a questionable behavior, but
> apparently that's the way it is. It's surely not something we
> want to rely on or suggest users to rely on.

So PostgreSQL does raise an exception if you attempt to reuse an xid:

postgres=# begin;
BEGIN
postgres=# prepare transaction 'tid';
PREPARE TRANSACTION
postgres=#
postgres=# begin;
BEGIN
postgres=# prepare transaction 'tid';
ERROR:  transaction identifier "tid" is already in use
postgres=#

What is happening is apparent from Python - the exception does not get
raised about a duplicate xid until the tpc_prepare().

Python 2.7.2+ (default, Oct  4 2011, 20:06:09)
[GCC 4.6.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import psycopg2
>>> con = psycopg2.connect('')
>>> xid = con.xid(0,'foo','bar')
>>> con.tpc_begin(xid)
>>> con.tpc_prepare()
>>> con.tpc_begin(xid)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
psycopg2.ProgrammingError: tpc_begin cannot be used inside a transaction
>>> con.rollback()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
psycopg2.ProgrammingError: rollback cannot be used during a two-phase
transaction
>>> con = psycopg2.connect('')
>>> con.tpc_begin(xid)
>>> con.tpc_prepare()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
psycopg2.ProgrammingError: transaction identifier "0_Zm9v_YmFy" is
already in use


One other issue that I just noticed - PostgreSQL defaults to having
two phase commit transactions disabled (max_prepared_transactions ==
0). store.execute('show max_prepared_transactions') will return the
current setting - is it worth skipping tests if
max_prepared_transactions is 0? Or just adding this setting to the
docs on running the test suite?

> +class TwoPhaseCommitDisconnectionTest(object):
> +
> + def test_begin_after_rollback_with_disconnection_error(self):
> + """
> + If a rollback fails because of a disconnection error, the two-phase
> + transaction should be properly reset.
> + """
> + xid = Xid(0, "foo", "bar")
> + self.connection.begin(xid)
> + self.connection.execute("SELECT 1")
> + self.proxy.stop()
> + self.connection.rollback()
> + self.proxy.start()
> + self.connection.begin(xid)
> + result = self.connection.execute("SELECT 1")
> + self.assertTrue(result.get_one())
>
>> You seem to be relying on the fact that self.connection.begin(xid)
>> will fail if the previous transaction using that xid was not
>> rolled back. This is fragile, because that transaction may not
>> have been rolled back yet;
>
> Yes, I'm relying on it, but I think it's safe because here we are
> not testing backend-related behavior, but rather Storm behavior.

So my mistake was thinking that a TPC transaction had been started,
but it hasn't as no PREPARE has been issued. In fact, this test
doesn't create any TPC transactions at all at the PostgreSQL level -
it is just exercising the Python level which all seems fine.

> === modified file 'tests/databases/postgres.py'
>
> +class PostgresTest(DatabaseTest, TwoPhaseCommitTest, TestHelper):
>
>> We are running the TwoPhaseCommitTests with PostgreSQL. Shouldn't we
>> also be running them under the other backends? I think all the previous
>> tests should pass with the other backends despite the fact they don't
>> actually do TPC.
>
> So the reason why I didn't add "dummy" implementations of begin/prepare
> for the backend that don't support TPC, is basically this one (from
> the second paragraph on):
>
> https://bugs.launchpad.net/storm/+bug/132485/comments/4

Ok. I understand this reasoning. It does have the side effect though
of not allowing you to manage both TPC capable and TPC incapable
backends with a single ZStorm instance. It also makes some of my
earlier comments invalid - no need to ensure order of TPC and non-TPC
stores as we can't mix them.

So for now I'd suggest that TPC support in ZStorm requires all stores
to support TPC, which gets this landed sooner. Mix & match can be
added later if anyone actually wants to do that.

>> We should have a test that TPC is actually happening with PostgreSQL.
>> I think this is important as I don't think we have anything else
>> ensuring we actually get around to invoking the real connection.tpc_*
>> methods. To do this we need to 'begin, update, prepare'. Then open
>> a new psycopg2 connection and make the connection.tpc_commit(xid)
>> call. That should succeed, and if you force the original connection
>> to reconnect the changes should be visible.
>
> Sounds good. I ended up adding a Connection.recover() method, and optional
> xid parameters to Connection.commit() and Connection.rollback() in order
> to support the full story at the Connection level, and test it in unit-tests.
>
> We might possibly want to expose this at the Store level too.

Meh. Adding them to the Connection class is already overkill IMO :-)


-- 
Stuart Bishop <email address hidden>