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

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

16.04.2021 12:28, Serge Petrenko via Tarantool-patches пишет:
>
>
> 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.
>
>
>
One more diff on top:

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

diff --git a/src/box/box.cc b/src/box/box.cc
index 73a9da56a..e0bed74f1 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1562,13 +1562,13 @@ box_clear_synchro_queue(bool try_wait)
                          */
                         txn_limbo_write_promote(&txn_limbo, wait_lsn, 0);
                         struct synchro_request req = {
-                               .type = 0, /* unused */
-                               .replica_id = 0, /* unused */
+                               .type = IPROTO_PROMOTE,
+                               .replica_id = former_leader_id,
                                 .origin_id = instance_id,
                                 .lsn = wait_lsn,
                                 .term = 0, /* unused */
                         };
-                       txn_limbo_read_promote(&txn_limbo, &req);
+                       txn_limbo_process(&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 93c8994b7..6657990e8 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -486,7 +486,11 @@ txn_limbo_write_promote(struct txn_limbo *limbo, 
int64_t lsn, uint64_t term)
         limbo->is_in_rollback = false;
  }

-void
+/**
+ * Process a PROMOTE request, i.e. confirm all entries <= @req.lsn and 
rollback all
+ * entries > @req.lsn.
+ */
+static void
  txn_limbo_read_promote(struct txn_limbo *limbo,
                        const struct synchro_request *req)
  {
diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
index 4a1c43856..f35771dc9 100644
--- a/src/box/txn_limbo.h
+++ b/src/box/txn_limbo.h
@@ -278,14 +278,6 @@ txn_limbo_wait_confirm(struct txn_limbo *limbo);
  void
  txn_limbo_write_promote(struct txn_limbo *limbo, int64_t lsn, uint64_t 
term);

-/**
- * Process a PROMOTE request, i.e. confirm all entries <= @req.lsn and 
rollback all
- * entries > @req.lsn.
- */
-void
-txn_limbo_read_promote(struct txn_limbo *limbo,
-                      const struct synchro_request *req);
-
  /**
   * Update qsync parameters dynamically.
   */


-- 
Serge Petrenko