On 07/13/2016 11:14 AM, Fabien COELHO wrote:
>> (The code structure in here seems like a complete mess to me, but probably
>> now is not the time to refactor it.)
>
> I fully agree that the code structure is a total mess:-( Maybe I'll try to
> submit a simpler one some day.
>
> Basically the doCustom function is not resilient, you cannot exit from
> anywhere and hope that re-entring would achieve a consistent behavior.
>
> While reading the code to find a better place for a return, I noted some
> possible inconsistencies in recording stats, which are noted as comments
> in the attached patch.
>
> Calling chooseScript is done both from outside for initialization and from
> inside doCustom, where it could be done once and more clearly in doCustom.
>
> Boolean listen is not reset because the script is expected to execute
> directly the start of the next statement. I succeeded in convincing myself
> that it actually works, but it is unobvious to spot why. I think that a
> simpler pattern would be welcome. Also, some other things (eg prepared)
> are not reset in all cases, not sure why.
>
> The goto should probably be replaced by a while.
>
> ...
Yeah, it really is quite a mess. I tried to review your patch, and I
think it's correct, but I couldn't totally convince myself, because of
the existing messiness of the logic. So I bit the bullet and started
refactoring.
I came up with the attached. It refactors the logic in doCustom() into a
state machine. I think this is much clearer, what do you think?
> @@ -1892,6 +1895,7 @@ top:
> /*
> * Read and discard the query result; note this is not included in
> * the statement latency numbers.
> + * Should this be done before recording the statement stats?
> */
> res = PQgetResult(st->con);
> switch (PQresultStatus(res))
Well, the comment right there says "note this is not included in the
statement latency numbers", so apparently it's intentional. Whether it's
a good idea or not, I don't know :-). It does seem a bit surprising.
But what seems more bogus to me is that we do that after recording the
*transaction* stats, if this was the last command. So the PQgetResult()
of the last command in the transaction is not included in the
transaction stats, even though the PQgetResult() calls for any previous
commands are. (Perhaps that's what you meant too?)
I changed that in my patch, it would've been inconvenient to keep that
old behavior, and it doesn't make any sense to me anyway.
- Heikki