tdb transaction nesting and ctdb

[ronnie sahlberg]

Hi,

Please see below the alternative approach I have changed CTDB to use.

It adds a new error : TDB_ERR_NESTING which is returned by
tdb_transaction_start() when it fails to start a new transaction since it
would cause nesting.

It adds a new flag : TDB_ALLOW_NESTING.
Unless this flag is set, tdb_transaction_start() will fail as above if this
would cause nesting.


This unfortunately breaks the current semantics since some callers of
tdb_transaction_start() does use, and knows how to handle nesting dafely.
And these callers, who know they can safely use nesting must be changed to
set the TDB_ALLOW_NESTING flag.


I think this, though it breaks current users of transactions, is preferable
since it does default to "safe" and nesting is only available to those apps
that know how to handle error cases with nesting in a safe manner.


regards
ronnie sahlberg



commit 3e49e41c21eb8c53084aa8cc7fd3557bdd8eb7b6
Author: Ronnie Sahlberg <ronniesahlberg at gmail.com>
Date:   Mon May 25 17:04:42 2009 +1000

    New attempt at TDB transaction nesting allow/disallow.

    Make the default be that transaction is not allowed and any attempt to
creat

    If an application can cope with transaction nesting and the implicit
    semantics of tdb_transaction_commit(), it can enable transaction nesting
    by using the TDB_ALLOW_NESTING flag.

diff --git a/lib/tdb/common/transaction.c b/lib/tdb/common/transaction.c
index 4e2127b..98e8eff 100644
--- a/lib/tdb/common/transaction.c
+++ b/lib/tdb/common/transaction.c
@@ -85,6 +85,13 @@
     still available, but no transaction recovery area is used and no
     fsync/msync calls are made.

+  - if TDB_ALLOW_NESTING is passed to flags in tdb open, or added using
+    tdb_add_flags() transaction is enabled.
+    The default is that transaction nesting is not allowed and an attempt
+    to create a nested transaction will fail with TDB_ERR_NESTING.
+
+    Beware. when transactions are nested a transaction successfully
+    completed with tdb_transaction_commit() can be silently unrolled later.
 */


@@ -409,6 +416,10 @@ int tdb_transaction_start(struct tdb_context *tdb)

        /* cope with nested tdb_transaction_start() calls */
        if (tdb->transaction != NULL) {
+               if (!(tdb->flags & TDB_ALLOW_NESTING)) {
+                       tdb->ecode = TDB_ERR_NESTING;
+                       return -1;
+               }
                tdb->transaction->nesting++;
                TDB_LOG((tdb, TDB_DEBUG_TRACE, "tdb_transaction_start:
nesting %
                         tdb->transaction->nesting));
diff --git a/lib/tdb/include/tdb.h b/lib/tdb/include/tdb.h
index 0008085..e678a7f 100644
--- a/lib/tdb/include/tdb.h
+++ b/lib/tdb/include/tdb.h
@@ -47,13 +47,15 @@ extern "C" {
 #define TDB_NOSYNC   64 /* don't use synchronous transactions */
 #define TDB_SEQNUM   128 /* maintain a sequence number */
 #define TDB_VOLATILE   256 /* Activate the per-hashchain freelist, default
5 */
+#define TDB_ALLOW_NESTING 512 /* Allow transactions to nest */

 #define TDB_ERRCODE(code, ret) ((tdb->ecode = (code)), ret)

 /* error codes */
 enum TDB_ERROR {TDB_SUCCESS=0, TDB_ERR_CORRUPT, TDB_ERR_IO, TDB_ERR_LOCK,
                TDB_ERR_OOM, TDB_ERR_EXISTS, TDB_ERR_NOLOCK,
TDB_ERR_LOCK_TIMEOU
-               TDB_ERR_NOEXIST, TDB_ERR_EINVAL, TDB_ERR_RDONLY};
+               TDB_ERR_NOEXIST, TDB_ERR_EINVAL, TDB_ERR_RDONLY,
+               TDB_ERR_NESTING};

 /* debugging uses one of the following levels */
 enum tdb_debug_level {TDB_DEBUG_FATAL = 0, TDB_DEBUG_ERROR,



regards
ronnie sahlberg


On Tue, Apr 28, 2009 at 11:34 AM, <tridge at samba.org> wrote:

> Hi Ronnie,
>
> I'm looking at your commit in the tdb code for ctdb:
>
>
> http://git.samba.org/?p=sahlberg/ctdb.git;a=commitdiff;h=459e4ee135bd1cd24c15e5325906eb4ecfd550ec;hp=70f21428c9eec96bcc787be191e7478ad68956dc
>
> As we discussed last week, I think that adding a flag that disables
> nested tdb transactions is a good idea, but I think your patch goes
> about it the wrong way.
>
> The reason we should have a flag to disable nested tdb transactions is
> that two pieces of code in the same application that both use
> transactions can easily step on each others toes, which is what
> happened with ctdb. When one piece of code is running a transaction,
> and a second piece of code starts a new transaction, then cancels it,
> the first transaction is currently put into an error state, which
> causes operations to be lost. That is not good, but at least the
> application is told that operations have been lost.
>
> With your change the situation is now worse, as operations can be
> silently lost. You've added a TDB_NO_NESTING flag for tdb, which when
> set means that a new transactions auto-cancels any currently
> outstanding transaction. That means that new transactions will undo
> any previous operations, without the caller of the previous
> transaction having any way to know that this has happened. It may work
> for the specific problem you are addressing in ctdb, but I think it is
> a very poor API.
>
> What I'd suggest is that we have a TDB_NO_NESTED_TRANSACTIONS flag,
> which causes any attempt to create a nested transaction to fail, with
> a new TDB_ERR_NESTING tdb error code. This means that existing
> transaction operations are not lost.
>
> The behaviour you want for ctdb can then be achieved like this:
>
>    ret = tdb_transaction_start(tdb);
>    if (ret == -1 && tdb_error(tdb) == TDB_ERR_NESTING) {
>         DEBUG(0,(__location__ " Cancelling old transaction\n"));
>         tdb_transaction_cancel(tdb);
>         ret = tdb_transaction_start(tdb);
>    }
>
> Does that sound ok?
>
> I also wonder why you set/unset TDB_NO_NESTING in the ctdb code? What
> situations are there in ctdb where you think nested transactions are
> desirable?
>
> The tdb code in ctdb is also starting to diverge a bit from the
> mainline tdb code. I think we should try and keep the two copies of
> tdb in sync as far as possible.
>
> Cheers, Tridge
>