[Tarantool-patches] [PATCH v3 04/10] box: make clear_synchro_queue() write a PROMOTE entry instead of CONFIRM + ROLLBACK

[Serge Petrenko]

16.04.2021 02:20, Vladislav Shpilevoy пишет:
> Thanks for working on this!
>
> See 2 comments below.
>
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 70b325180..9adb6ba46 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -1556,7 +1556,19 @@ box_clear_synchro_queue(bool try_wait)
>>   				 "new synchronous transactions appeared");
>>   			rc = -1;
>>   		} else {
>> -			txn_limbo_force_empty(&txn_limbo, wait_lsn);
>> +			/*
>> +			 * Term parameter is unused now, We'll pass
>> +			 * box_raft()->term there later.
>> +			 */
>> +			txn_limbo_write_promote(&txn_limbo, wait_lsn, 0);
>> +			struct synchro_request req = {
>> +				.type = 0, /* unused */
>> +				.replica_id = 0, /* unused */
>> +				.origin_id = instance_id,
>> +				.lsn = wait_lsn,
>> +				.term = 0, /* unused */
> 1. Aren't the unused fields nullified anyway according to
> the standard?

We had this conversation with Cyrill recently. I don't have a good 
explanation
for this anyway, but here's the one I have:

>> Is there some particular meaning of zeroifying designated assignments?
>> I mean why not simply
>>
>> 			struct synchro_request req = {
>> 				.origin_id	= instance_id,
>> 				.lsn		= wait_lsn,
>> 			};
>>
>> or you wanted to pay attention that the left of the fields are
>> unused? Just curious, I'm fine with current code.
> I went for your option at first, and it's the one I'd prefer.
> But with it I got failed builds in some CI jobs.
>
> It said something like "sorry, not yet implemented: struct partial
> initialization"
>


>
>> +			};
>> +			txn_limbo_read_promote(&txn_limbo, &req);
>>   			assert(txn_limbo_is_empty(&txn_limbo));
>>   		}
>>   	}
>> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
>> index d29722ef7..bfe0ad302 100644
>> --- a/src/box/txn_limbo.c
>> +++ b/src/box/txn_limbo.c
>> @@ -464,6 +470,32 @@ txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
>>   		box_update_ro_summary();
>>   }
>>   
>> +void
>> +txn_limbo_write_promote(struct txn_limbo *limbo, int64_t lsn, uint64_t term)
>> +{
>> +	limbo->confirmed_lsn = lsn;
>> +	/*
>> +	 * We make sure that promote is only written once everything this
>> +	 * instance has may be confirmed.
>> +	 */
>> +	struct txn_limbo_entry *e = txn_limbo_last_synchro_entry(limbo);
>> +	assert(e == NULL || e->lsn <= lsn);
>> +	(void) e;
>> +	txn_limbo_write_synchro(limbo, IPROTO_PROMOTE, lsn, term);
>> +	limbo->is_in_rollback = false;
> 2. How is it possible that there was a rollback in progress at
> the same time?

Sorry, I was trying to replicate txn_limbo_write_confirm/rollback behaviour,
but forgot to set limbo->is_in_rollback = true before the journal write:

=======================================

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index e6f644bc0..93c8994b7 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -474,6 +474,7 @@ void
  txn_limbo_write_promote(struct txn_limbo *limbo, int64_t lsn, uint64_t 
term)
  {
         limbo->confirmed_lsn = lsn;
+       limbo->is_in_rollback = true;
         /*
          * We make sure that promote is only written once everything this
          * instance has may be confirmed.



-- 
Serge Petrenko