On Fri, Mar 15, 2024 at 12:22 PM Kristian Nielsen <knielsen@knielsen-hq.org> wrote:
Brandon Nesterenko <brandon.nesterenko@mariadb.com> writes:

>> But once we fix the await_all_slave_replies(), do we even need the
>> is_awaiting_semi_sync_ack flag any more? If so, why? If not, then this
>> point
>> becomes moot anyway.
>>
>
> Ah, right, this is actually a bug as is, as when we are in
> kill_threads_phase_1 we
> test for is_awaiting_semi_sync_ack, which would only be set on the leader
> binlog thread.. So then in kill_threads_phase_1, we'd just check if the THD
> is in
> Active_tranx to see if we should skip the kill.

I still don't understand why we want to skip the kill?

I think the point of SHUTDOWN WAIT FOR ALL SLAVES is to ensure that slaves
have time to get all transactions that were binlogged on the master, before
it shuts down. This will be ensured by await_all_slave_replies(), which I
think can be done without relying on any other session/THD waiting for
specific ACKs.

In fact, any session could in principle be killed anyway (by the user), and
we would still need await_all_slave_replies() to work, right? Or a session
could be just before or after waiting for its ACK, and then it will be
killed anyway.

So I don't understand why the shutdown should skip killing these specific
sessions in this specific case?

On the other hand, I'm not sure this is that important, can't really think
of any harm in skipping the kill either. I'm mostly trying to understand the
reason for doing this.

I see your point. Though likely not too important, it adds complexity to the
code, so I've reverted that part (originally added by MDEV-11853).

The PR (PR 3089 / branch 10.6-MDEV-33551) has been updated per your
latest review.
 
Brandon