Hello
+ PGC_SUSET | GUC_LIST_INPUT,
+ 0,
Shouldn't that be PGC_SUSET, GUC_LIST_INPUT?
yes, fixed
+ /*
+ * In almost all cases, the queryid is computed due pg_stat_statements.
+ * Without log_queryids computing queryid is not necessary, but it can
+ * be hard to enable or disable queryid in dependecy of log_queryids.
+ * There are two possibilities - force queryid computing, or ignore
+ * queries without computed queryid (computing should be forced by setting
+ * compute_query_id). Boths probably can work, first looks more clean
+ * at this moment.
+ */
+ EnableQueryId();
I was not sure if we need to "force" queryid commuting, because usually it will be computed due pg_stat_statements.
computing queryid is critical for pg_stat_statements, auto_explain can work without it - only log_queryids will be ineffective.
But now I am sure, so EnableQueryId should be used in auto_explain too. For consistency, for testing independence.
At the end - users can set compute_query_id to off.
I changed this comment to the same form as other usage of EnableQueryId.
This comment seems misleading to me. Based on it, I would think that
we always force queryid computing, but EnableQueryId doesn't do
anything with compute_query_id = off, so it seems like the second
choice instead? (also typo: Boths)
+ result = (auto_explain_queryids *) guc_malloc(LOG, allocsize);
+ if (result == NULL)
+ return false;
This leaks rawstring/elemlist, is that intentional?
Boths are allocated from shortlife memory context, and will be released early. So there is no real risk of memory leaks.
+ if (*newval == NULL || *newval[0] == '\0')
That should be probably (*newval)[0].
I removed this - SplitGUCList can process empty strings. I leave this routine, when I detect a zero element list.
Thank you for check and comments
Regards
Pavel
