On 2018-Nov-20, Fabien COELHO wrote:
> Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state
> transitions to happen in doCustom's switch (st->state) and nowhere else,
> which is defeated by creating the separate function.
>
> Although it improves readability at one level, it does not help figuring out
> what happens to states, which is my primary concern: The idea is that
> reading doCustom is enough to build and check the automaton, which I had to
> do repeatedly while reviewing Marina's patches.
I adopted your patch, then while going over it I decided to go with my
separation proposal anyway, and re-did it.
Looking at the state of the code before this patch, I totally understand
that you want to get away from the current state of affairs. However, I
don't think my proposal makes matters worse; actually it makes them
better. Please give the code a honest look and think whether the
separation of machine state advances is really worse with my proposal.
I also renamed some things. doCustom() was a pretty bad name, as was
CSTATE_START_THROTTLE.
> Also the added doCustom comment, which announces this property becomes false
> under the refactoring function:
>
> All state changes are performed within this function called by threadRun.
>
> So I would suggest not to create this function.
I agree the state advances in threadRun were real messy.
Thanks for getting rid of the goto.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services