From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Magnus Hagander <magnus@hagander.net>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Bruce Momjian <bruce@momjian.us>
Cc: Christoph Berg <myon@debian.org>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: compute_query_id and pg_stat_statements
Date: Thu, 13 May 2021 20:04:37 -0400
Message-ID: <20210514000437.GA14223@alvherre.pgsql> (raw)
In-Reply-To: <20210513002623.eugftm4nk2lvvks3@nol>
Here's a first attempt at what was suggested. If you say "auto" it
remains auto in SHOW, but it gets enabled if a module asks for it.
Not final yet, but I thought I'd throw it out for early commentary ...
--
Álvaro Herrera Valdivia, Chile
Attachments:Change-compute_query_id-to-an-enum-GUC-v3.patch (text/x-diff, 13.7K) downloaddiff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.outindex 40b5109b55..f7ce01539f 100644--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out@@ -1067,4 +1067,20 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%';
2
(1 row)
+-- Check that pg_stat_statements() will complain if the configuration appears+-- to be broken.+SET compute_query_id = off;+SELECT pg_stat_statements_reset();+ pg_stat_statements_reset +--------------------------+ +(1 row)++SELECT count(*) FROM pg_stat_statements;+WARNING: query identifier calculation is disabled+ count +-------+ 0+(1 row)+
DROP EXTENSION pg_stat_statements;
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.cindex a85f962801..fab684a649 100644--- a/contrib/pg_stat_statements/pg_stat_statements.c+++ b/contrib/pg_stat_statements/pg_stat_statements.c@@ -369,6 +369,12 @@ _PG_init(void)
if (!process_shared_preload_libraries_in_progress)
return;
+ /*+ * Inform the postmaster that we want to enable query_id calculation if+ * compute_query_id is set to auto.+ */+ EnableQueryId();+
/*
* Define (or redefine) custom GUC variables.
*/
@@ -1617,6 +1623,18 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
*/
LWLockAcquire(pgss->lock, LW_SHARED);
+ /*+ * If no query_id has been computed for the calling query and there is+ * no entries stored, then there's likely a configuration error that caller+ * may not be aware of so point it out.+ */+ if (pgstat_get_my_query_id() == UINT64CONST(0) &&+ compute_query_id == COMPUTE_QUERY_ID_OFF &&+ hash_get_num_entries(pgss_hash) == 0)+ ereport(WARNING,+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),+ errmsg("query identifier calculation is disabled"));+
if (showtext)
{
/*
diff --git a/contrib/pg_stat_statements/pg_stat_statements.conf b/contrib/pg_stat_statements/pg_stat_statements.confindex e47b26040f..13346e2807 100644--- a/contrib/pg_stat_statements/pg_stat_statements.conf+++ b/contrib/pg_stat_statements/pg_stat_statements.conf@@ -1,2 +1 @@
shared_preload_libraries = 'pg_stat_statements'
-compute_query_id = ondiff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sqlindex bc3b6493e6..827a8e3d18 100644--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql@@ -437,4 +437,10 @@ SELECT (
SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%';
+-- Check that pg_stat_statements() will complain if the configuration appears+-- to be broken.+SET compute_query_id = off;+SELECT pg_stat_statements_reset();+SELECT count(*) FROM pg_stat_statements;+
DROP EXTENSION pg_stat_statements;
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgmlindex 45bd1f1b7e..60d8b24f5e 100644--- a/doc/src/sgml/config.sgml+++ b/doc/src/sgml/config.sgml@@ -7627,7 +7627,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
<variablelist>
<varlistentry id="guc-compute-query-id" xreflabel="compute_query_id">
- <term><varname>compute_query_id</varname> (<type>boolean</type>)+ <term><varname>compute_query_id</varname> (<type>enum</type>)
<indexterm>
<primary><varname>compute_query_id</varname> configuration parameter</primary>
</indexterm>
@@ -7643,7 +7643,12 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
identifier to be computed. Note that an external module can
alternatively be used if the in-core query identifier computation
method is not acceptable. In this case, in-core computation
- must be disabled. The default is <literal>off</literal>.+ must be always disabled.+ Valid values are <literal>off</literal> (always disabled),+ <literal>on</literal> (always enabled) and <literal>auto</literal>,+ which let modules such as <xref linkend="pgstatstatements"/>+ automatically enable it.+ The default is <literal>auto</literal>.
</para>
<note>
<para>
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgmlindex bc2b6038ee..acfb134797 100644--- a/doc/src/sgml/pgstatstatements.sgml+++ b/doc/src/sgml/pgstatstatements.sgml@@ -22,10 +22,15 @@
<para>
The module will not track statistics unless query
- identifiers are calculated. This can be done by enabling <xref- linkend="guc-compute-query-id"/> or using a third-party module that- computes its own query identifiers. Note that all statistics tracked- by this module must be reset if the query identifier method is changed.+ identifiers are calculated. This is done by automatically when this+ extension is configured in <literal>shared_preload_libraries</literal> and+ <xref linkend="guc-compute-query-id"/> is set to <literal>auto</literal>+ (which is the default value), or always if <xref+ linkend="guc-compute-query-id"/> is set to <literal>on</literal>.+ You must set <xref linkend="guc-compute-query-id"/> to <literal>off</literal>+ if you want to use a third-party module that computes its own query+ identifiers. Note that all statistics tracked by this module must be+ reset if the query identifier method is changed.
</para>
<para>
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.cindex 1202bf85a3..9a60865d19 100644--- a/src/backend/commands/explain.c+++ b/src/backend/commands/explain.c@@ -245,7 +245,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->summary = (summary_set) ? es->summary : es->analyze;
query = castNode(Query, stmt->query);
- if (compute_query_id)+ if (IsQueryIdEnabled())
jstate = JumbleQuery(query, pstate->p_sourcetext);
if (post_parse_analyze_hook)
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.cindex 168198acd1..201b88d1ad 100644--- a/src/backend/parser/analyze.c+++ b/src/backend/parser/analyze.c@@ -124,7 +124,7 @@ parse_analyze(RawStmt *parseTree, const char *sourceText,
query = transformTopLevelStmt(pstate, parseTree);
- if (compute_query_id)+ if (IsQueryIdEnabled())
jstate = JumbleQuery(query, sourceText);
if (post_parse_analyze_hook)
@@ -163,7 +163,7 @@ parse_analyze_varparams(RawStmt *parseTree, const char *sourceText,
/* make sure all is well with parameter types */
check_variable_parameters(pstate, query);
- if (compute_query_id)+ if (IsQueryIdEnabled())
jstate = JumbleQuery(query, sourceText);
if (post_parse_analyze_hook)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.cindex 6833f0f7f2..9ca1095f47 100644--- a/src/backend/postmaster/postmaster.c+++ b/src/backend/postmaster/postmaster.c@@ -521,6 +521,7 @@ typedef struct
pg_time_t first_syslogger_file_time;
bool redirection_done;
bool IsBinaryUpgrade;
+ bool auto_query_id_enabled;
int max_safe_fds;
int MaxBackends;
#ifdef WIN32
@@ -6168,6 +6169,7 @@ save_backend_variables(BackendParameters *param, Port *port,
param->redirection_done = redirection_done;
param->IsBinaryUpgrade = IsBinaryUpgrade;
+ param->auto_query_id_enabled = auto_query_id_enabled;
param->max_safe_fds = max_safe_fds;
param->MaxBackends = MaxBackends;
@@ -6401,6 +6403,7 @@ restore_backend_variables(BackendParameters *param, Port *port)
redirection_done = param->redirection_done;
IsBinaryUpgrade = param->IsBinaryUpgrade;
+ auto_query_id_enabled = param->auto_query_id_enabled;
max_safe_fds = param->max_safe_fds;
MaxBackends = param->MaxBackends;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.cindex 6200699ddd..f9c6ca80f1 100644--- a/src/backend/tcop/postgres.c+++ b/src/backend/tcop/postgres.c@@ -704,7 +704,7 @@ pg_analyze_and_rewrite_params(RawStmt *parsetree,
query = transformTopLevelStmt(pstate, parsetree);
- if (compute_query_id)+ if (IsQueryIdEnabled())
jstate = JumbleQuery(query, query_string);
if (post_parse_analyze_hook)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex eb7f7181e4..4d88982a75 100644--- a/src/backend/utils/misc/guc.c+++ b/src/backend/utils/misc/guc.c@@ -101,6 +101,7 @@
#include "utils/plancache.h"
#include "utils/portal.h"
#include "utils/ps_status.h"
+#include "utils/queryjumble.h"
#include "utils/rls.h"
#include "utils/snapmgr.h"
#include "utils/tzparser.h"
@@ -402,6 +403,23 @@ static const struct config_enum_entry backslash_quote_options[] = {
{NULL, 0, false}
};
+/*+ * Although only "on", "off", and "auto" are documented, we accept all the+ * likely variants of "on" and "off".+ */+static const struct config_enum_entry compute_query_id_options[] = {+ {"auto", COMPUTE_QUERY_ID_AUTO, false},+ {"on", COMPUTE_QUERY_ID_ON, false},+ {"off", COMPUTE_QUERY_ID_OFF, false},+ {"true", COMPUTE_QUERY_ID_ON, true},+ {"false", COMPUTE_QUERY_ID_OFF, true},+ {"yes", COMPUTE_QUERY_ID_ON, true},+ {"no", COMPUTE_QUERY_ID_OFF, true},+ {"1", COMPUTE_QUERY_ID_ON, true},+ {"0", COMPUTE_QUERY_ID_OFF, true},+ {NULL, 0, false}+};+
/*
* Although only "on", "off", and "partition" are documented, we
* accept all the likely variants of "on" and "off".
@@ -534,7 +552,6 @@ extern const struct config_enum_entry dynamic_shared_memory_options[];
/*
* GUC option variables that are exported from this module
*/
-bool compute_query_id = false;
bool log_duration = false;
bool Debug_print_plan = false;
bool Debug_print_parse = false;
@@ -1441,15 +1458,6 @@ static struct config_bool ConfigureNamesBool[] =
true,
NULL, NULL, NULL
},
- {- {"compute_query_id", PGC_SUSET, STATS_MONITORING,- gettext_noop("Compute query identifiers."),- NULL- },- &compute_query_id,- false,- NULL, NULL, NULL- },
{
{"log_parser_stats", PGC_SUSET, STATS_MONITORING,
gettext_noop("Writes parser performance statistics to the server log."),
@@ -4619,6 +4627,16 @@ static struct config_enum ConfigureNamesEnum[] =
NULL, NULL, NULL
},
+ {+ {"compute_query_id", PGC_SUSET, STATS_MONITORING,+ gettext_noop("Compute query identifiers."),+ NULL+ },+ &compute_query_id,+ COMPUTE_QUERY_ID_AUTO, compute_query_id_options,+ NULL, NULL, NULL+ },+
{
{"constraint_exclusion", PGC_USERSET, QUERY_TUNING_OTHER,
gettext_noop("Enables the planner to use constraints to optimize queries."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sampleindex efde01ee56..6e36e4c2ef 100644--- a/src/backend/utils/misc/postgresql.conf.sample+++ b/src/backend/utils/misc/postgresql.conf.sample@@ -604,7 +604,7 @@
# - Monitoring -
-#compute_query_id = off+#compute_query_id = auto
#log_statement_stats = off
#log_parser_stats = off
#log_planner_stats = off
diff --git a/src/backend/utils/misc/queryjumble.c b/src/backend/utils/misc/queryjumble.cindex f004a9ce8c..c4683d9ed2 100644--- a/src/backend/utils/misc/queryjumble.c+++ b/src/backend/utils/misc/queryjumble.c@@ -39,6 +39,12 @@
#define JUMBLE_SIZE 1024 /* query serialization buffer size */
+/* GUC parameters */+int compute_query_id = COMPUTE_QUERY_ID_AUTO;++/* True when a module requests query IDs and they're set auto */+bool auto_query_id_enabled = false;+
static uint64 compute_utility_query_id(const char *str, int query_location, int query_len);
static void AppendJumble(JumbleState *jstate,
const unsigned char *item, Size size);
@@ -132,6 +138,33 @@ JumbleQuery(Query *query, const char *querytext)
return jstate;
}
+/*+ * Enables query identifier computation if the GUC is set to auto.+ *+ * Third-party plugins can use this function to inform core that they require+ * a query identifier to be computed.+ */+void+EnableQueryId(void)+{+ if (compute_query_id == COMPUTE_QUERY_ID_AUTO)+ auto_query_id_enabled = true;+}++/*+ * Returns whether query identifier computation has been enabled, either+ * directly in the GUC or by a module when the setting is 'auto'.+ */+bool+IsQueryIdEnabled(void)+{+ if (compute_query_id == COMPUTE_QUERY_ID_OFF)+ return false;+ if (compute_query_id == COMPUTE_QUERY_ID_ON)+ return true;+ return auto_query_id_enabled;+}+
/*
* Compute a query identifier for the given utility query string.
*/
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.hindex 24a5d9d3fb..a7c3a4958e 100644--- a/src/include/utils/guc.h+++ b/src/include/utils/guc.h@@ -247,7 +247,6 @@ extern bool log_btree_build_stats;
extern PGDLLIMPORT bool check_function_bodies;
extern bool session_auth_is_superuser;
-extern bool compute_query_id;
extern bool log_duration;
extern int log_parameter_max_length;
extern int log_parameter_max_length_on_error;
diff --git a/src/include/utils/queryjumble.h b/src/include/utils/queryjumble.hindex 83ba7339fa..60389f15b4 100644--- a/src/include/utils/queryjumble.h+++ b/src/include/utils/queryjumble.h@@ -52,7 +52,23 @@ typedef struct JumbleState
int highest_extern_param_id;
} JumbleState;
-const char *CleanQuerytext(const char *query, int *location, int *len);-JumbleState *JumbleQuery(Query *query, const char *querytext);+/* Values for the compute_query_id GUC */+typedef enum+{+ COMPUTE_QUERY_ID_OFF,+ COMPUTE_QUERY_ID_ON,+ COMPUTE_QUERY_ID_AUTO+} ComputeQueryIdType;++/* GUC parameters */+extern int compute_query_id;++/* Not a GUC: set true if any modules turn "auto" into "on" */+extern bool auto_query_id_enabled;++extern const char *CleanQuerytext(const char *query, int *location, int *len);+extern JumbleState *JumbleQuery(Query *query, const char *querytext);+extern void EnableQueryId(void);+extern bool IsQueryIdEnabled(void);
#endif /* QUERYJUMBLE_H */
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, rjuju123@gmail.com, pavel.stehule@gmail.com, horikyota.ntt@gmail.com, magnus@hagander.net, masao.fujii@oss.nttdata.com, michael@paquier.xyz, andres@anarazel.de, tgl@sss.pgh.pa.us, sfrost@snowman.net, bruce@momjian.us, myon@debian.org, peter.eisentraut@enterprisedb.com, pgsql-hackers@lists.postgresql.org
Subject: Re: compute_query_id and pg_stat_statements
In-Reply-To: <20210514000437.GA14223@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox