Code review comment for lp://staging/~free.ekanayaka/storm/tpc-support

  >>>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.

I see, but if your backend does not support TPC, then
transaction.commit() will fail, because if you enable TPC on the store
with zstorm.set_default_tpc('my-store', False), then the data manager
will try to call begin()/prepare(), which would fail if the backend does
not support that.

  > 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

Agreed (see also the following comment).

  >>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.

Okay, as it seems more correct to move commit() to tpc_finish() also for
the non-TPC case, and it simplifies code too, I went for it. I also
modified sortKey() to make non-TPC stores commit first and added a test
for this. So the use case is supported now.

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

Why? ZStorm is already supporting mixing TPC and non-TPC stores via the
set_default_tpc() method? And that's from the very first version of the
branch in this MP.

  >>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

Yeah, sorry, I actually meant that postgresql seems happy to re-use a
transaction ID once a prepared transaction with that ID has been
committed or rolled back. You can't do that concurrently, as you show.

  > 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?

Sure, I did both.

  >>=== 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.

I still I don't understand why you are saying this, as mentioned above
this possible with the set_default_tpc() method, which works per-store,
unless I'm missing something

  >>>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 :-)

:)