Re: Logging query parmeters in auto_explain

Michael Paquier <michael@paquier.xyz> writes:

> On Thu, Jun 09, 2022 at 11:55:11PM +0100, Dagfinn Ilmari Mannsåker wrote:
>> Done (and more tests added), v3 attached.
>
> One thing I am wondering is if we'd better mention errdetail_params()
> at the top of the initial check done in ExplainQueryParameters().
> That's a minor issue, though.
>
> +sub query_log
> +{
> Perhaps a short description at the top of this routine to explain it
> is worth it?

Done. I also moved the function to the bottom of the file, to avoid
distracting from the actual test queries.

> The test still does a set of like() and unlike() after running each
> query when the parameter updates are done.  One thing I would have
> played with is to group the set of logs expected or not expected as
> parameters of the centralized routine, but that would reduce the
> customization of the test names, so at the end the approach you have
> taken for query_log() looks good to me.

I did consider passing the tests as a data structure to the function,
but that would amount to specifying exactly the same things but as a
data structure, and then calling the appropriate function by reference,
which just makes things more cluttered.

If we were using TAP subtests, it might make a sense to have the
function run each set of related tests in a subtest, rather than having
multiple subtest calls at the top level, but we don't, so it doesn't.

> +$node->stop('fast');
> There is no need for that.  The END block of Cluster.pm does that
> already.

Ah, I was not aware of that. The call was there in the original version,
so I had just left it in. Removed.

v4 patch attached.

- ilmari