On 2021-May-14, Julien Rouhaud wrote:
> On Fri, May 14, 2021 at 12:20:00PM +0900, Fujii Masao wrote:
> > +void
> > +EnableQueryId(void)
> > +{
> > + if (compute_query_id == COMPUTE_QUERY_ID_AUTO)
> > + auto_query_id_enabled = true;
> >
> > Shouldn't EnableQueryId() enable auto_query_id_enabled whatever
> > compute_query_id is?
> > Otherwise, for example, the following scenario can happen and it's a bit
> > strange.
> >
> > 1. The server starts up with shared_preload_libraries=pg_stat_statements
> > and compute_query_id=on
> > 2. compute_query_id is set to auto and the configuration file is reloaded
> > Then, even though compute_query_id is auto and pg_stat_statements is loaded,
> > query ids are not computed and no queries are tracked by pg_stat_statements.
>
> +1.
That makes sense. Done in this version.
I took out the new WARNING in pg_stat_statements. It's not clear to me
that that's terribly useful (it stops working as soon as you have one
query in the pg_stat_statements stash and later disable everything).
Maybe there is an useful warning to add, but I think it's independent of
changing the GUC behabior.
I also made IsQueryIdEnabled() a static inline in queryjumble.h, to
avoid a function call at every site where we need that. Also did some
light doc editing.
I think I should aim at pushing this tomorrow morning.
> Note that if you switch from compute_query_id = off + custom
> query_id + pg_stat_statements to compute_query_id = auto then thing will
> immediately break (as we instruct third-party plugins authors to error out in
> that case), which is way better than breaking at the next random service
> restart.
Hmm, ok. I tested pg_queryid and that behavior of throwing an error
seems quite unhelpful -- it basically makes every single query fail if
you set things wrong. I think that instruction is bogus and should be
reconsidered. Maybe pg_queryid could use a custom Boolean GUC that
tells it to overwrite the core query_id if that is enabled, or to just
silently do nothing in that case.
While thinking about this patch it occurred to that an useful gadget
might be to let pg_stat_statements be loaded always, but with
compute_query_id=false; so it's never active; except if you
ALTER {DATABASE, USER} foo SET (compute_query_id=on);
so that it's possible to enable it selectively. I think this doesn't
currently achieve anything because pgss_store is always called
regardless of query ID being active (so you'd always have at least one
function call as performance penalty, only that the work would be for
naught), but that seems a simple change to make. I didn't look closely
to see what other things would need patched.
--
Álvaro Herrera 39°49'30"S 73°17'W
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)
>From acd29794dbba5e9828d78fd348621374a73c6d7a Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Fri, 14 May 2021 16:55:18 -0400
Subject: [PATCH v4] Allow compute_query_id set to 'auto'
---
.../pg_stat_statements/pg_stat_statements.c | 6 +++
.../pg_stat_statements.conf | 1 -
doc/src/sgml/config.sgml | 9 ++++-
doc/src/sgml/pgstatstatements.sgml | 14 +++----
src/backend/commands/explain.c | 2 +-
src/backend/parser/analyze.c | 4 +-
src/backend/postmaster/postmaster.c | 3 ++
src/backend/tcop/postgres.c | 2 +-
src/backend/utils/misc/guc.c | 38 ++++++++++++++-----
src/backend/utils/misc/postgresql.conf.sample | 2 +-
src/backend/utils/misc/queryjumble.c | 23 +++++++++++
src/include/utils/guc.h | 1 -
src/include/utils/queryjumble.h | 33 +++++++++++++++-
13 files changed, 108 insertions(+), 30 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index a85f962801..09433c8c96 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.
*/
diff --git a/contrib/pg_stat_statements/pg_stat_statements.conf b/contrib/pg_stat_statements/pg_stat_statements.conf
index 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 = on
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 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.sgml
index bc2b6038ee..aa332d8cc2 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -18,18 +18,14 @@
<xref linkend="guc-shared-preload-libraries"/> in
<filename>postgresql.conf</filename>, because it requires additional shared memory.
This means that a server restart is needed to add or remove the module.
+ In addition, query identifier calculation must be enabled in order for the
+ module to be active, which is done automatically if <xref linkend="guc-compute-query-id"/>
+ is set to <literal>auto</literal> or <literal>on</literal>, or any third-party
+ module that calculates query identifiers is loaded.
</para>
<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.
- </para>
-
- <para>
- When <filename>pg_stat_statements</filename> is loaded, it tracks
+ When <filename>pg_stat_statements</filename> is active, it tracks
statistics across all databases of the server. To access and manipulate
these statistics, the module provides views
<structname>pg_stat_statements</structname> and
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 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.c
index 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.c
index 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.c
index dd2ade7bb6..8cea10c901 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.c
index eb7f7181e4..ee731044b6 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.sample
index 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.c
index f004a9ce8c..5b442debf0 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 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);
@@ -91,6 +97,10 @@ CleanQuerytext(const char *query, int *location, int *len)
return query;
}
+/*
+ * This should only be called if IsQueryIdEnabled()
+ * return true.
+ */
JumbleState *
JumbleQuery(Query *query, const char *querytext)
{
@@ -132,6 +142,19 @@ JumbleQuery(Query *query, const char *querytext)
return jstate;
}
+/*
+ * Enables query identifier computation.
+ *
+ * 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_OFF)
+ query_id_enabled = true;
+}
+
/*
* Compute a query identifier for the given utility query string.
*/
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 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.h
index 83ba7339fa..15f9e71f93 100644
--- a/src/include/utils/queryjumble.h
+++ b/src/include/utils/queryjumble.h
@@ -52,7 +52,36 @@ 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;
+
+
+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 query_id_enabled;
+
+/*
+ * Returns whether query identifier computation has been enabled, either
+ * directly in the GUC or by a module when the setting is 'auto'.
+ */
+static inline bool
+IsQueryIdEnabled(void)
+{
+ if (compute_query_id == COMPUTE_QUERY_ID_OFF)
+ return false;
+ if (compute_query_id == COMPUTE_QUERY_ID_ON)
+ return true;
+ return query_id_enabled;
+}
#endif /* QUERYJUMBLE_H */
--
2.20.1
