Replying to [ticket:47814 mreynolds]:

This affect betxn plugins: retrocl, replication, and repl sync

We need to pass in the parent pblock so we use the existing transaction, otherwise we can run into a deadlock.

What do you mean? The code should be able to get the parent transaction from the thread local storage.

ldbm_back_seq() also needs to make sure it aborts the read transaction before the next retry.

Replying to [comment:1 rmeggins]:

Replying to [ticket:47814 mreynolds]:

This affect betxn plugins: retrocl, replication, and repl sync

We need to pass in the parent pblock so we use the existing transaction, otherwise we can run into a deadlock.

What do you mean? The code should be able to get the parent transaction from the thread local storage.

How? At least not in slapi_seq_callback().

In ldbm_back_seq we get the transaction by calling:

slapi_pblock_get( pb, SLAPI_TXN, (void **)&parent_txn );

But the pblock passed in from slapi_seq_callback() to ldbm_back_seq() is just a generic local variable pblock. So I'm working on passing in the real plugin pblock every time we call slapi_seq_callback() - instead of generating an empty pblock.

ldbm_back_seq() also needs to make sure it aborts the read transaction before the next retry.

Replying to [comment:2 mreynolds]:

Replying to [comment:1 rmeggins]:

Replying to [ticket:47814 mreynolds]:

This affect betxn plugins: retrocl, replication, and repl sync

We need to pass in the parent pblock so we use the existing transaction, otherwise we can run into a deadlock.

What do you mean? The code should be able to get the parent transaction from the thread local storage.

How? At least not in slapi_seq_callback().

In ldbm_back_seq we get the transaction by calling:

slapi_pblock_get( pb, SLAPI_TXN, (void **)&parent_txn );

But the pblock passed in from slapi_seq_callback() to ldbm_back_seq() is just a generic local variable pblock. So I'm working on passing in the real plugin pblock every time we call slapi_seq_callback() - instead of generating an empty pblock.

I think the way it is supposed to work is that if the parent_txn is NULL, the transaction begin code will look in thread local storage. The SLAPI_TXN in the pblock is for those cases where you may want to override the default. See dblayer_txn_begin_ext() for more details.
{{{
if (!parent_txn)
{
/ see if we have a stored parent txn /
back_txn *par_txn_txn = dblayer_get_pvt_txn();
if (par_txn_txn) {
parent_txn = par_txn_txn->back_txn_txn;
}
}
}}}

ldbm_back_seq() also needs to make sure it aborts the read transaction before the next retry.

Replying to [comment:3 rmeggins]:

Replying to [comment:2 mreynolds]:

Replying to [comment:1 rmeggins]:

Replying to [ticket:47814 mreynolds]:

This affect betxn plugins: retrocl, replication, and repl sync

We need to pass in the parent pblock so we use the existing transaction, otherwise we can run into a deadlock.

What do you mean? The code should be able to get the parent transaction from the thread local storage.

How? At least not in slapi_seq_callback().

In ldbm_back_seq we get the transaction by calling:

slapi_pblock_get( pb, SLAPI_TXN, (void **)&parent_txn );

But the pblock passed in from slapi_seq_callback() to ldbm_back_seq() is just a generic local variable pblock. So I'm working on passing in the real plugin pblock every time we call slapi_seq_callback() - instead of generating an empty pblock.

I think the way it is supposed to work is that if the parent_txn is NULL, the transaction begin code will look in thread local storage. The SLAPI_TXN in the pblock is for those cases where you may want to override the default. See dblayer_txn_begin_ext() for more details.
{{{
if (!parent_txn)
{
/ see if we have a stored parent txn /
back_txn *par_txn_txn = dblayer_get_pvt_txn();
if (par_txn_txn) {
parent_txn = par_txn_txn->back_txn_txn;
}
}
}}}

Yeah ldbm_back_seq() does not do this, and we might not have seen this issue at all if it did:

seq.c
if (!parent_txn) {
parent_txn = txn.back_txn_txn;
slapi_pblock_set( pb, SLAPI_TXN, parent_txn );
}

I verified adding the correct plugin pblock did fix the issue, but I'll try using dblayer_get_pvt_txn() in ldbm_back_seq() next...

ldbm_back_seq() also needs to make sure it aborts the read transaction before the next retry.

Replying to [comment:4 mreynolds]:

Replying to [comment:3 rmeggins]:

Replying to [comment:2 mreynolds]:

Replying to [comment:1 rmeggins]:

Replying to [ticket:47814 mreynolds]:

This affect betxn plugins: retrocl, replication, and repl sync

We need to pass in the parent pblock so we use the existing transaction, otherwise we can run into a deadlock.

What do you mean? The code should be able to get the parent transaction from the thread local storage.

How? At least not in slapi_seq_callback().

In ldbm_back_seq we get the transaction by calling:

slapi_pblock_get( pb, SLAPI_TXN, (void **)&parent_txn );

But the pblock passed in from slapi_seq_callback() to ldbm_back_seq() is just a generic local variable pblock. So I'm working on passing in the real plugin pblock every time we call slapi_seq_callback() - instead of generating an empty pblock.

I think the way it is supposed to work is that if the parent_txn is NULL, the transaction begin code will look in thread local storage. The SLAPI_TXN in the pblock is for those cases where you may want to override the default. See dblayer_txn_begin_ext() for more details.
{{{
if (!parent_txn)
{
/ see if we have a stored parent txn /
back_txn *par_txn_txn = dblayer_get_pvt_txn();
if (par_txn_txn) {
parent_txn = par_txn_txn->back_txn_txn;
}
}
}}}

Yeah ldbm_back_seq() does not do this,

It must not do this. The dblayer transaction code should do this automatically when the backend code calls dblayer_txn_begin(). See ldbm_back_add() for example.

and we might not have seen this issue at all if it did:

seq.c
if (!parent_txn) {
parent_txn = txn.back_txn_txn;
slapi_pblock_set( pb, SLAPI_TXN, parent_txn );
}

I verified adding the correct plugin pblock did fix the issue,

What exactly did you do?

but I'll try using dblayer_get_pvt_txn() in ldbm_back_seq() next...

See above.

ldbm_back_seq() also needs to make sure it aborts the read transaction before the next retry.

Replying to [comment:5 rmeggins]:

Replying to [comment:4 mreynolds]:

Replying to [comment:3 rmeggins]:

Replying to [comment:2 mreynolds]:

Replying to [comment:1 rmeggins]:

Replying to [ticket:47814 mreynolds]:

This affect betxn plugins: retrocl, replication, and repl sync

We need to pass in the parent pblock so we use the existing transaction, otherwise we can run into a deadlock.

What do you mean? The code should be able to get the parent transaction from the thread local storage.

How? At least not in slapi_seq_callback().

In ldbm_back_seq we get the transaction by calling:

slapi_pblock_get( pb, SLAPI_TXN, (void **)&parent_txn );

But the pblock passed in from slapi_seq_callback() to ldbm_back_seq() is just a generic local variable pblock. So I'm working on passing in the real plugin pblock every time we call slapi_seq_callback() - instead of generating an empty pblock.

I think the way it is supposed to work is that if the parent_txn is NULL, the transaction begin code will look in thread local storage. The SLAPI_TXN in the pblock is for those cases where you may want to override the default. See dblayer_txn_begin_ext() for more details.
{{{
if (!parent_txn)
{
/ see if we have a stored parent txn /
back_txn *par_txn_txn = dblayer_get_pvt_txn();
if (par_txn_txn) {
parent_txn = par_txn_txn->back_txn_txn;
}
}
}}}

Yeah ldbm_back_seq() does not do this,

It must not do this. The dblayer transaction code should do this automatically when the backend code calls dblayer_txn_begin(). See ldbm_back_add() for example.

Actually I was testing removing the code that set parent_txn in ldbm_back_seq() so parent_txn was NULL going into dblayer_txn_begin().

It finds the parent_txn from thread storage, but the caller never has its parent_txn updated and it still hangs. Looks like the local thread storage approach is not working correctly. Still investigating...

and we might not have seen this issue at all if it did:

seq.c
if (!parent_txn) {
parent_txn = txn.back_txn_txn;
slapi_pblock_set( pb, SLAPI_TXN, parent_txn );
}

I verified adding the correct plugin pblock did fix the issue,

What exactly did you do?

I pass the plugin pblock through all the retrocl calls up to when it calls slapi_seq_search().

but I'll try using dblayer_get_pvt_txn() in ldbm_back_seq() next...

See above.

ldbm_back_seq() also needs to make sure it aborts the read transaction before the next retry.

This is actually a problem in ldbm_back_seq() where the transaction is committed too early. Closing this ticket as invalid, and working on the original ticket(https://fedorahosted.org/389/ticket/47602) that introduced the txn commit in the wrong place.

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

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.