{{{
- 4933 int return_value = -1;
+ 4933 int return_value = SLAPI_BACK_TRANSACTION_NOT_SUPPORTED;
}}}
Is this necessary? This means that NULL == sdn returns SLAPI_BACK_TRANSACTION_NOT_SUPPORTED instead of -1

{{{
return_value = dblayer_txn_begin(be, parent, &current);
}}}

If transactions are not supported, this will set return_value to SLAPI_BACK_TRANSACTION_NOT_SUPPORTED

Replying to [comment:5 rmeggins]:

{{{
- 4933 int return_value = -1;
+ 4933 int return_value = SLAPI_BACK_TRANSACTION_NOT_SUPPORTED;
}}}
Is this necessary? This means that NULL == sdn returns SLAPI_BACK_TRANSACTION_NOT_SUPPORTED instead of -1

Previously if transactions are not enabled we returned 0 from dblayer_txn_begin(), and I thought the problem was that we were returning a generic "-1", and only a NULL "sdn" or "be" would trigger that.

Maybe these are two separate issues that I mixed up. I'll remove this part of the fix.

{{{
return_value = dblayer_txn_begin(be, parent, &current);
}}}

If transactions are not supported, this will set return_value to SLAPI_BACK_TRANSACTION_NOT_SUPPORTED

I think your fix can have an unintended side effect. We have in dblayer_txn_begin():
if (SERIALLOCK(li)) {
dblayer_lock_backend(be);
}
rc = dblayer_txn_begin_ext(li,parent_txn,txn,PR_TRUE);
if (rc && SERIALLOCK(li)) {
dblayer_unlock_backend(be);
}

So in case that we don't have txns (SLAPI_BACK_TRANSACTION_NOT_SUPPORTED), we would also not use the backend lock.

But this will now also return from dblayer_txn_begin without taking the backend lock.
I think it should be something like:
if (rc && (rc != SLAPI_BACK_TRANSACTION_NOT_SUPPORTED) && SERIALLOCK(li)) {

dblayer_unlock_backend(be);

}

Replying to [comment:8 lkrispen]:

But this will now also return from dblayer_txn_begin without taking the backend lock.
I think it should be something like:
if (rc && (rc != SLAPI_BACK_TRANSACTION_NOT_SUPPORTED) && SERIALLOCK(li)) {

dblayer_unlock_backend(be);

}

But we are returning an error to the caller before taking any lock:

3620:

if (!priv->dblayer_enable_transactions){
    return SLAPI_BACK_TRANSACTION_NOT_SUPPORTED;
}

So why should we lock/unlock it? I thought your original concern was that we were taking the lock on a possible error when we should not be.

Can you clarify what you are worried about? Are there cases were we want to take the lock, even if we don't support transactions?

Thanks,
Mark

The backend lock is used independently of the transaction. The calls to dblayer_lock_backend dblayer_unlock_backend have only been moved into the dblayer_begin|commit_txn some time ago.

If transactions are not enabled we still need to call dblayer_lock_backend.

Replying to [comment:10 lkrispen]:

The backend lock is used independently of the transaction. The calls to dblayer_lock_backend dblayer_unlock_backend have only been moved into the dblayer_begin|commit_txn some time ago.

If transactions are not enabled we still need to call dblayer_lock_backend.

Thanks for clarification, but I still have a question about your other comment:

You said we should add something like:
if (rc && (rc != SLAPI_BACK_TRANSACTION_NOT_SUPPORTED) && SERIALLOCK(li)) {

Why do we need to add: (rc != SLAPI_BACK_TRANSACTION_NOT_SUPPORTED).

If we took the lock:

    if (SERIALLOCK(li)) {
        dblayer_lock_backend(be);
    }

Why would we "not" want to unlock if it transactions are not enabled(rc != SLAPI_BACK_TRANSACTION_NOT_SUPPORTED)?

Thanks again,
Mark

Well if txn_begin fails, it will be retried and attempt again to get the backend lock, so it should be unlocked. I thought if it fails because txns are not enabled we should keep the lock, but
I think we need to check also how the backend functions eg ldbm_back_modify handle SLAPI_BACK_TRANSACTION_NOT_SUPPORTED, in that case it should not be retried.

Replying to [comment:12 lkrispen]:

Well if txn_begin fails, it will be retried and attempt again to get the backend lock, so it should be unlocked. I thought if it fails because txns are not enabled we should keep the lock, but
I think we need to check also how the backend functions eg ldbm_back_modify handle SLAPI_BACK_TRANSACTION_NOT_SUPPORTED, in that case it should not be retried.

I just created the new return code SLAPI_BACK_TRANSACTION_NOT_SUPPORTED. So this is all new. We only return SLAPI_BACK_TRANSACTION_NOT_SUPPORTED from dblayer_txn_begin_ext() if !priv->dblayer_enable_transactions, but this does impact a lot of other routines.

Perhaps this is much more involved than just returning a useful error code. I will continue to investigate.

What about changing only slapi_back_transaction_begin(), slapi_back_transaction_commit(), and slapi_back_transaction_abort(), like this:

{{{
int
slapi_back_transaction_begin(Slapi_PBlock pb)
{
IFP txn_begin = NULL;
if (slapi_pblock_get(pb, SLAPI_PLUGIN_DB_BEGIN_FN, (void)&txn_begin) ||
!txn_begin) {
return SLAPI_BACK_TRANSACTION_NOT_SUPPORTED;
}
return txn_begin(pb);
}
}}}
would that solve the problem?

yes, that would fix the issue with the plugin return codes and leave the backend functions untouched

Rich's suggestion was already present in the original fix, so I removed the dblayer.c changes. Sending out for review.

Looks good, but we need the same in slapi_back_transaction_commit and _abort as well.

revision 3
0001-Ticket-47329-Improve-slapi_back_transaction_begin-re.patch

Replying to [comment:17 rmeggins]:

Looks good, but we need the same in slapi_back_transaction_commit and _abort as well.

Done, new patch(revision 3) is attached.

git merge ticket47329
Updating 4aed9c6..8879ed2
Fast-forward
ldap/servers/slapd/backend.c | 26 +++++++++++++++++++++-----
ldap/servers/slapd/slapi-plugin.h | 2 ++
2 files changed, 23 insertions(+), 5 deletions(-)

git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 1.15 KiB, done.
Total 7 (delta 5), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
4aed9c6..8879ed2 master -> master

commit 8879ed2

Linked to Bugzilla bug: https://bugzilla.redhat.com/show_bug.cgi?id=1016749 (''Red Hat Enterprise Linux 7'')

c5764fb..038b12a 389-ds-base-1.3.0 -> 389-ds-base-1.3.0
commit ad64367
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Jun 25 10:38:26 2013 -0400
commit 1c59258
Author: Mark Reynolds mreynolds@redhat.com
Date: Fri Jun 21 10:47:09 2013 -0400
5844c56..13dd962 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit 21dccd3
Author: Mark Reynolds mreynolds@redhat.com
Date: Tue Jun 25 10:38:26 2013 -0400
commit badcb1a
Author: Mark Reynolds mreynolds@redhat.com
Date: Fri Jun 21 10:47:09 2013 -0400

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/666

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.