[Tarantool-patches] [PATCH 04/13] wal: refactor wal_write_to_disk()

[Vladislav Shpilevoy]

Hi! Thanks for the review!

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

I added the panic:

====================
diff --git a/src/box/wal.c b/src/box/wal.c
index 40382e791..cb8b2eac1 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -1039,7 +1039,8 @@ wal_write_to_disk(struct cmsg *msg)
 	struct stailq_entry *last_committed = NULL;
 	struct journal_entry *entry;
 	struct error *error;
-	assert(!stailq_empty(&wal_msg->commit));
+	if (stailq_empty(&wal_msg->commit))
+		panic("Attempted to write an empty batch to WAL");
 
 	/*
 	 * Track all vclock changes made by this batch into