Tarantool development patches archive
 help / color / mirror / Atom feed
From: Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: v.shpilevoy@tarantool.org, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 2/2] box: fix an assertion failure in box.ctl.promote()
Date: Wed, 26 May 2021 11:25:29 +0300	[thread overview]
Message-ID: <aad0a4b2-e68b-e39b-8b30-709940ebfd1d@tarantool.org> (raw)
In-Reply-To: <YK3nQA1hQLoU0UFE@grain>



26.05.2021 09:14, Cyrill Gorcunov пишет:
> On Tue, May 25, 2021 at 01:39:29PM +0300, Serge Petrenko wrote:

Hi! Thanks for the review!

>> box.ctl.promote() used to assume that the last synchronous entry is
>> already written to WAL by the time it's called. This is not the case
>> when promote is executed on the limbo owner. The last synchronous entry
>> might still be en route to WAL.
> Typo "en" -> "in".

No, "en route" means в пути/по пути.

>
>> @@ -1618,14 +1618,29 @@ box_promote(void)
>>   				 txn_limbo.owner_id);
>>   			return -1;
>>   		}
>> +		if (txn_limbo_is_empty(&txn_limbo)) {
>> +			wait_lsn = txn_limbo.confirmed_lsn;
>> +			goto promote;
>> +		}
>>   	}
>>   
>> -	/*
>> -	 * promote() is a no-op on the limbo owner, so all the rows
>> -	 * in the limbo must've come through the applier meaning they already
>> -	 * have an lsn assigned, even if their WAL write hasn't finished yet.
>> -	 */
>> -	wait_lsn = txn_limbo_last_synchro_entry(&txn_limbo)->lsn;
>> +	struct txn_limbo_entry *last_entry;
>> +	last_entry = txn_limbo_last_synchro_entry(&txn_limbo);
>> +	/* Wait for the last entries WAL write. */
>> +	if (last_entry->lsn < 0) {
>> +		if (wal_sync(NULL) < 0)
>> +			return -1;
>> +		if (txn_limbo_is_empty(&txn_limbo)) {
>> +			wait_lsn = txn_limbo.confirmed_lsn;
>> +			goto promote;
>> +		}
>> +		if (last_entry != txn_limbo_last_synchro_entry(&txn_limbo)) {
> This is a bit dangerous. We cache a pointer and then go to fiber_yield,
> which switches context, at this moment the pointer become dangling one
> and we simply can't be sure if it _were_ reused. IOW, Serge are we
> 100% sure that the same pointer with same address but with new data
> won't appear here as last entry in limbo?

I agree this solution is not perfect.

An alternative would be to do the following:
1) Check that the limbo owner hasn't changed
2) Check that the last entry has positive lsn (e.g. it's not a new entry 
which
     wasn't yet written to WAL). And that this lsn is equal to the lsn 
of our entry.

But what if our entry was confirmed and destroyed during wal_sync()? We 
can't compare
other entries lsn with this ones.
>
>> +			diag_set(ClientError, ER_QUORUM_WAIT, quorum,
>> +				 "new synchronous transactions appeared");
>> +			return -1;
>> +		}
>> +	}
>> +	wait_lsn = last_entry->lsn;
>>   	assert(wait_lsn > 0);

-- 
Serge Petrenko


  reply	other threads:[~2021-05-26  8:25 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-25 10:39 [Tarantool-patches] [PATCH v2 0/2] " Serge Petrenko via Tarantool-patches
2021-05-25 10:39 ` [Tarantool-patches] [PATCH v2 1/2] box: refactor in_promote using a guard Serge Petrenko via Tarantool-patches
2021-05-26  7:25   ` Cyrill Gorcunov via Tarantool-patches
2021-05-27 10:57     ` Serge Petrenko via Tarantool-patches
2021-05-27 11:02       ` Cyrill Gorcunov via Tarantool-patches
2021-05-25 10:39 ` [Tarantool-patches] [PATCH v2 2/2] box: fix an assertion failure in box.ctl.promote() Serge Petrenko via Tarantool-patches
2021-05-26  6:14   ` Cyrill Gorcunov via Tarantool-patches
2021-05-26  8:25     ` Serge Petrenko via Tarantool-patches [this message]
2021-05-26 18:46       ` Vladislav Shpilevoy via Tarantool-patches
2021-05-27 10:53         ` Serge Petrenko via Tarantool-patches
2021-05-27 11:03           ` Cyrill Gorcunov via Tarantool-patches
2021-05-27 19:30           ` Vladislav Shpilevoy via Tarantool-patches
2021-06-01 12:20 ` [Tarantool-patches] [PATCH v2 0/2] " Kirill Yukhin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aad0a4b2-e68b-e39b-8b30-709940ebfd1d@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=sergepetrenko@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 2/2] box: fix an assertion failure in box.ctl.promote()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox