Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 04/13] wal: refactor wal_write_to_disk()
Date: Wed, 16 Jun 2021 08:22:15 +0200
Message-ID: <9940ffc4-9d92-58b7-e95a-1128048e21da@tarantool.org> (raw)
In-Reply-To: <YMkRlFjtKlqN8skS@grain>



On 15.06.2021 22:46, Cyrill Gorcunov wrote:
> On Fri, Jun 11, 2021 at 11:56:12PM +0200, Vladislav Shpilevoy wrote:
>> It didn't have a single fail path. That led to some amount of code
>> duplication, and it complicated future patches where the journal
>> entries are going to get a proper error reason instead of default
>> -1 without any details.
>>
>> The patch is a preparation for #6027 where it is wanted to have
>> more detailed errors on journal entry/transaction fail instead
>> of ER_WAL_IO for everything. Sometimes it can override a real
>> error like a cascade rollback, or a transaction conflict.
>>
>> Part of #6027
>> ---
>> @@ -1038,7 +1036,10 @@ wal_write_to_disk(struct cmsg *msg)
>>  {
>>  	struct wal_writer *writer = &wal_writer_singleton;
>>  	struct wal_msg *wal_msg = (struct wal_msg *) msg;
>>  	struct error *error;
>> +	assert(!stailq_empty(&wal_msg->commit));
> 
> Hi Vlad, you know I don't understand why we need this assert...

Otherwise in case of, for instance, rotate fail, the rollback won't
start.

>>  	/*
>>  	 * Track all vclock changes made by this batch into
>> @@ -1058,23 +1059,17 @@ wal_write_to_disk(struct cmsg *msg)
>>  
>>  	if (writer->is_in_rollback) {
>>  		/* We're rolling back a failed write. */
>> -		stailq_concat(&wal_msg->rollback, &wal_msg->commit);
>> -		vclock_copy(&wal_msg->vclock, &writer->vclock);
>> -		return;
>> +		goto done;
> 
> Jumps to "done" label change the code logic. Before the patch if we
> reached the write and say wal_opt_rotate failed we set up is_in_rollback
> sign and exit early, after the patch we start notifying watchers that
> there "write" happened which means relay code will be woken up while there
> no new data on disk level at all, which means watchers wanna be notified
> for no reason, no? Or I miss something obvious?

You didn't miss anything. But I see no harm in that. WAL write fail is
extremely rare, so a rare spurious wakeup won't do anything bad. I
decided the code reusability and simplicity is more important here.

  reply	other threads:[~2021-06-16  6:22 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-11 21:56 [Tarantool-patches] [PATCH 00/13] Applier rollback reason Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 01/13] error: introduce ER_CASCADE_ROLLBACK Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 10/13] txn: install proper diag errors on txn fail Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 11/13] wal: introduce JOURNAL_ENTRY_ERR_CASCADE Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 12/13] txn: introduce TXN_SIGNATURE_ABORT Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 13/13] txn: stop TXN_SIGNATURE_ABORT override Vladislav Shpilevoy via Tarantool-patches
2021-06-15 13:44   ` Serge Petrenko via Tarantool-patches
2021-06-15 19:34     ` Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 02/13] test: remove replica-applier-rollback.lua Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 03/13] journal: make journal_write() set diag on error Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 04/13] wal: refactor wal_write_to_disk() Vladislav Shpilevoy via Tarantool-patches
2021-06-15 20:46   ` Cyrill Gorcunov via Tarantool-patches
2021-06-16  6:22     ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-06-16  8:02       ` Cyrill Gorcunov via Tarantool-patches
2021-06-16 23:32         ` Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 05/13] diag: introduce diag_set_detailed() Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 06/13] wal: encapsulate ER_WAL_IO Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 07/13] txn: change limbo rollback check in the trigger Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 08/13] journal: introduce proper error codes Vladislav Shpilevoy via Tarantool-patches
2021-06-11 21:56 ` [Tarantool-patches] [PATCH 09/13] txn: assert after WAL write that txn is not done Vladislav Shpilevoy via Tarantool-patches
2021-06-15 13:43 ` [Tarantool-patches] [PATCH 00/13] Applier rollback reason Serge Petrenko via Tarantool-patches
2021-06-16 23:32 ` Vladislav Shpilevoy 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=9940ffc4-9d92-58b7-e95a-1128048e21da@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git