Tarantool development patches archive
 help / color / mirror / Atom feed
From: Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 04/13] wal: refactor wal_write_to_disk()
Date: Wed, 16 Jun 2021 11:02:11 +0300
Message-ID: <YMmwA//5TKFulb2Z@grain> (raw)
In-Reply-To: <9940ffc4-9d92-58b7-e95a-1128048e21da@tarantool.org>

On Wed, Jun 16, 2021 at 08:22:15AM +0200, Vladislav Shpilevoy wrote:
> 
> 
> 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.

But the current whole code logic is assuming that if commit chain
is empty then there was no data to write or rollback, iow codeflow
is using list emptiness as a sign of data to procceed. If we really
want to prohibit calling wal_write_to_disk with no entries then
maybe better put panic here?

The wal_msg->commit entry is accessed later in code anyway via bpu,
which means that if we add

	if (stailq_empty(&wal_msg->commit))
		panic("wal: attempt to update vclock without data");

this won't cause perf degradation since after a few lines you gonna be
testing @commit again where bpu entry is already filled and this will
allow us to catch problems on release builds as well.

Don't get me wrong please I simply don't like assert() with a passion
because it hides problems which might happen on release builds.

Anyway, just a proposal, if you still prefer calling assert here, ok
let it be.

> > 
> > 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.

OK, thanks for explanation!

  reply	other threads:[~2021-06-16  8:02 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
2021-06-16  8:02       ` Cyrill Gorcunov via Tarantool-patches [this message]
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=YMmwA//5TKFulb2Z@grain \
    --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