Re: proposal - queryid can be used as filter for auto_explain

Hello, I have a few comments on this.

+ /*
+ * Inform the postmaster that we want to enable query_id calculation if
+ * compute_query_id is set to auto.
+ */
+ EnableQueryId();

This comment is not entirely accurate because when the module is
loaded using LOAD, the setting is enabled only for the current
session, not in the postmaster.

Separately from that, I don't think auto_explain should enable query
ID calculation when it is not actually going to use it. Instead, I
wonder whether this should be done in assign_log_queryids(). This
would naturally tie enabling query ID calculation to the state of the
GUC, including configuration reloads, as well as session-local use via
LOAD, while avoiding enabling it in sessions that never use
auto_explain.

+ if (queryId != INT64CONST(0))
+ {
+ int i;
+
...
+ for (i = 0; i < queryId_filter->nqueryids; i++)
+ {

I think it would be better to write this as "for (int i = ...".

- if (msec >= auto_explain_log_min_duration)
+ if (log_filter && msec >= auto_explain_log_min_duration)

I think the overall decision logic for deciding whether to log a plan
could be reorganized to avoid unnecessary query ID lookups.

+ /*
+ * We expect small number of watched queryids, and then
+ * a linear seaching is the fastest. As an alternative
+ * we can sort the array of queryId, and we can search
+ * there by bisection.
+ */

I also think a linear search is fine here. If this ever turns out to
be insufficient, I expect we'd simply switch to a more appropriate
algorithm, regardless of whether the alternatives are mentioned here
or not. Also, "is the fastest" seems stronger than necessary. I think
we usually write comments like:

> We expect only a few watched query IDs, so a linear search is sufficient.

+ if (log_filter && msec >= auto_explain_log_min_duration)

This part made me pause because the relationship between log_filter
and log_min_duration is not immediately obvious.

Initially, I assumed they were independent options, so I expected an
OR relationship. However, the implementation uses AND. If users also
assume these are independent settings, the behavior may be
surprising. Perhaps it would make sense to introduce a separate
log_queryid_min_duration parameter if these are intended to be
independent controls.

+ /*
+ * queryid is 64bit integer. strtol fails on 32bit or on MSWin.
+ * In other places, queryid passed (from, to SQL) as generic int8,
+ * so we can use int8in routine. int8in can raise an exception,
+ * so pg_strtoint64_safe is used instead.
+ */

I don't think this comment is necessary. Using pg_strtoint64_safe()
already makes the intent clear.

I also think it would be good to add a test for the LOAD case.

Regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center