[prev in list] [next in list] [prev in thread] [next in thread]
List: pgsql-hackers
Subject: Re: [HACKERS] ProcessUtility_hook
From: Itagaki Takahiro <itagaki.takahiro () oss ! ntt ! co ! jp>
Date: 2009-11-30 7:26:01
Message-ID: 20091130162601.64D2.52131E4D () oss ! ntt ! co ! jp
[Download RAW message or body]
Euler Taveira de Oliveira <euler@timbira.com> wrote:
> The functionality is divided in two parts. The first part is a hook in the
> utility module. The idea is capture the commands that doesn't pass through
> executor. I'm afraid that that hook will be used only for capturing non-DML
> queries. If so, why don't we hack the tcop/postgres.c and grab those queries
> from the same point we log statements?
DDLs can be used from user defined functions. It has the same reason
why we have executor hooks instead of tcop hooks.
> The second part is to use that hook to capture non-DML commands for
> pg_stat_statements module.
- I fixed a bug that it should handle only isTopLevel command.
- A new parameter pg_stat_statements.track_ddl (boolean) is
added to enable or disable the feature.
> Do we need to have rows = 0 for non-DML commands?
> Maybe NULL could be an appropriate value.
Yes, but it requires additional management to handle 0 and NULL
separately. I don't think it is worth doing.
> The PREPARE command stopped to count
> the number of rows. Should we count the rows in EXECUTE command or in the
> PREPARE command?
It requires major rewrites of EXECUTE command to pass the number of
affected rows to caller. I doubt it is worth fixing because almost all
drivers use protocol-level prepared statements instead of PREPARE+EXECUTE.
> The other command that doesn't count properly is COPY. Could
> you fix that?
I added codes for it.
> I'm concerned about storing some commands that expose passwords
> like CREATE ROLE foo PASSWORD 'secret'; I know that the queries are only
> showed to superusers but maybe we should add this information to documentation
> or provide a mechanism to exclude those commands.
I think there is no additonal problem there because we can see the 'secret'
command using pg_stat_activity even now.
> I don't know if it is worth the trouble adding some code to capture VACUUM and
> ANALYZE commands called inside autovacuum.
I'd like to exclude VACUUM and ANALYZE by autovacuum because
pg_stat_statements should not filled with those commands.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
["ProcessUtility_hook_20091130.patch" (application/octet-stream)]
diff -cprN head/contrib/pg_stat_statements/pg_stat_statements.c \
work/contrib/pg_stat_statements/pg_stat_statements.c
*** head/contrib/pg_stat_statements/pg_stat_statements.c 2009-07-27 \
13:09:55.000000000 +0900
--- work/contrib/pg_stat_statements/pg_stat_statements.c 2009-11-30 \
16:18:22.577527226 +0900
***************
*** 24,29 ****
--- 24,30 ----
#include "access/hash.h"
#include "catalog/pg_type.h"
+ #include "commands/copy.h"
#include "executor/executor.h"
#include "executor/instrument.h"
#include "mb/pg_wchar.h"
***************
*** 32,37 ****
--- 33,39 ----
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/spin.h"
+ #include "tcop/utility.h"
#include "utils/builtins.h"
#include "utils/hsearch.h"
#include "utils/guc.h"
*************** static shmem_startup_hook_type prev_shme
*** 113,118 ****
--- 115,121 ----
static ExecutorStart_hook_type prev_ExecutorStart = NULL;
static ExecutorRun_hook_type prev_ExecutorRun = NULL;
static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
+ static ProcessUtility_hook_type prev_ProcessUtility = NULL;
/* Links to shared memory state */
static pgssSharedState *pgss = NULL;
*************** typedef enum
*** 124,133 ****
{
PGSS_TRACK_NONE, /* track no statements */
PGSS_TRACK_TOP, /* only top level statements */
! PGSS_TRACK_ALL, /* all statements, including nested ones */
} PGSSTrackLevel;
! static const struct config_enum_entry track_options[] = {
{"none", PGSS_TRACK_NONE, false},
{"top", PGSS_TRACK_TOP, false},
{"all", PGSS_TRACK_ALL, false},
--- 127,137 ----
{
PGSS_TRACK_NONE, /* track no statements */
PGSS_TRACK_TOP, /* only top level statements */
! PGSS_TRACK_ALL /* all statements, including nested ones */
} PGSSTrackLevel;
! static const struct config_enum_entry track_options[] =
! {
{"none", PGSS_TRACK_NONE, false},
{"top", PGSS_TRACK_TOP, false},
{"all", PGSS_TRACK_ALL, false},
*************** static const struct config_enum_entry tr
*** 136,141 ****
--- 140,146 ----
static int pgss_max; /* max # statements to track */
static int pgss_track; /* tracking level */
+ static bool pgss_track_ddl; /* whether to track ddl commands */
static bool pgss_save; /* whether to save stats across shutdown */
*************** static bool pgss_save; /* whether to s
*** 146,152 ****
--- 151,159 ----
/*---- Function declarations ----*/
void _PG_init(void);
+ #ifdef NOT_USED
void _PG_fini(void);
+ #endif
Datum pg_stat_statements_reset(PG_FUNCTION_ARGS);
Datum pg_stat_statements(PG_FUNCTION_ARGS);
*************** static void pgss_ExecutorRun(QueryDesc *
*** 161,170 ****
ScanDirection direction,
long count);
static void pgss_ExecutorEnd(QueryDesc *queryDesc);
static uint32 pgss_hash_fn(const void *key, Size keysize);
static int pgss_match_fn(const void *key1, const void *key2, Size keysize);
! static void pgss_store(const char *query,
! const Instrumentation *instr, uint32 rows);
static Size pgss_memsize(void);
static pgssEntry *entry_alloc(pgssHashKey *key);
static void entry_dealloc(void);
--- 168,179 ----
ScanDirection direction,
long count);
static void pgss_ExecutorEnd(QueryDesc *queryDesc);
+ static void pgss_ProcessUtility(Node *parsetree,
+ const char *queryString, ParamListInfo params, bool isTopLevel,
+ DestReceiver *dest, char *completionTag);
static uint32 pgss_hash_fn(const void *key, Size keysize);
static int pgss_match_fn(const void *key1, const void *key2, Size keysize);
! static void pgss_store(const char *query, double total_time, uint64 rows);
static Size pgss_memsize(void);
static pgssEntry *entry_alloc(pgssHashKey *key);
static void entry_dealloc(void);
*************** _PG_init(void)
*** 214,219 ****
--- 223,238 ----
NULL,
NULL);
+ DefineCustomBoolVariable("pg_stat_statements.track_ddl",
+ "Selects whether DDL commands are tracked by pg_stat_statements.",
+ NULL,
+ &pgss_track_ddl,
+ true,
+ PGC_SUSET,
+ 0,
+ NULL,
+ NULL);
+
DefineCustomBoolVariable("pg_stat_statements.save",
"Save pg_stat_statements statistics across server shutdowns.",
NULL,
*************** _PG_init(void)
*** 245,252 ****
--- 264,274 ----
ExecutorRun_hook = pgss_ExecutorRun;
prev_ExecutorEnd = ExecutorEnd_hook;
ExecutorEnd_hook = pgss_ExecutorEnd;
+ prev_ProcessUtility = ProcessUtility_hook;
+ ProcessUtility_hook = pgss_ProcessUtility;
}
+ #ifdef NOT_USED
/*
* Module unload callback
*/
*************** _PG_fini(void)
*** 257,264 ****
--- 279,288 ----
ExecutorStart_hook = prev_ExecutorStart;
ExecutorRun_hook = prev_ExecutorRun;
ExecutorEnd_hook = prev_ExecutorEnd;
+ ProcessUtility_hook = prev_ProcessUtility;
shmem_startup_hook = prev_shmem_startup_hook;
}
+ #endif
/*
* shmem_startup hook: allocate or attach to shared memory,
*************** pgss_ExecutorEnd(QueryDesc *queryDesc)
*** 539,545 ****
InstrEndLoop(queryDesc->totaltime);
pgss_store(queryDesc->sourceText,
! queryDesc->totaltime,
queryDesc->estate->es_processed);
}
--- 563,569 ----
InstrEndLoop(queryDesc->totaltime);
pgss_store(queryDesc->sourceText,
! queryDesc->totaltime->total,
queryDesc->estate->es_processed);
}
*************** pgss_ExecutorEnd(QueryDesc *queryDesc)
*** 550,555 ****
--- 574,632 ----
}
/*
+ * ProcessUtility hook
+ */
+ static void
+ pgss_ProcessUtility(Node *parsetree, const char *queryString,
+ ParamListInfo params, bool isTopLevel,
+ DestReceiver *dest, char *completionTag)
+ {
+ if (pgss_track_ddl && isTopLevel && pgss_enabled())
+ {
+ instr_time start;
+ instr_time duration;
+ uint64 rows = 0;
+
+ INSTR_TIME_SET_CURRENT(start);
+
+ nested_level++;
+ PG_TRY();
+ {
+ if (prev_ProcessUtility)
+ prev_ProcessUtility(parsetree, queryString, params, isTopLevel, dest, \
completionTag); + else if ((nodeTag(parsetree)) == T_CopyStmt)
+ {
+ rows = DoCopy((CopyStmt *) parsetree, queryString);
+ if (completionTag)
+ snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
+ "COPY " UINT64_FORMAT, rows);
+ }
+ else
+ standard_ProcessUtility(parsetree, queryString, params, isTopLevel, dest, \
completionTag); + nested_level--;
+ }
+ PG_CATCH();
+ {
+ nested_level--;
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ INSTR_TIME_SET_CURRENT(duration);
+ INSTR_TIME_SUBTRACT(duration, start);
+
+ pgss_store(queryString, INSTR_TIME_GET_DOUBLE(duration), rows);
+ }
+ else
+ {
+ if (prev_ProcessUtility)
+ prev_ProcessUtility(parsetree, queryString, params, isTopLevel, dest, \
completionTag); + else
+ standard_ProcessUtility(parsetree, queryString, params, isTopLevel, dest, \
completionTag); + }
+ }
+
+ /*
* Calculate hash value for a key
*/
static uint32
*************** pgss_match_fn(const void *key1, const vo
*** 587,593 ****
* Store some statistics for a statement.
*/
static void
! pgss_store(const char *query, const Instrumentation *instr, uint32 rows)
{
pgssHashKey key;
double usage;
--- 664,670 ----
* Store some statistics for a statement.
*/
static void
! pgss_store(const char *query, double total_time, uint64 rows)
{
pgssHashKey key;
double usage;
*************** pgss_store(const char *query, const Inst
*** 631,637 ****
SpinLockAcquire(&e->mutex);
e->counters.calls += 1;
! e->counters.total_time += instr->total;
e->counters.rows += rows;
e->counters.usage += usage;
SpinLockRelease(&e->mutex);
--- 708,714 ----
SpinLockAcquire(&e->mutex);
e->counters.calls += 1;
! e->counters.total_time += total_time;
e->counters.rows += rows;
e->counters.usage += usage;
SpinLockRelease(&e->mutex);
diff -cprN head/doc/src/sgml/pgstatstatements.sgml \
work/doc/src/sgml/pgstatstatements.sgml
*** head/doc/src/sgml/pgstatstatements.sgml 2009-05-18 20:08:24.000000000 +0900
--- work/doc/src/sgml/pgstatstatements.sgml 2009-11-30 16:18:00.392275000 +0900
***************
*** 176,181 ****
--- 176,198 ----
<varlistentry>
<term>
+ <varname>pg_stat_statements.track_ddl</varname> (<type>boolean</type>)
+ </term>
+
+ <listitem>
+ <para>
+ <varname>pg_stat_statements.track_ddl</varname> controls whether DDL
+ commands are counted by the module.
+ Specify <literal>on</> to track DDL commands except <command>SELECT</>,
+ <command>INSERT</>, <command>UPDATE</> and <command>DELETE</> statements.
+ The default value is <literal>on</>.
+ Only superusers can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>
<varname>pg_stat_statements.save</varname> (<type>boolean</type>)
</term>
diff -cprN head/src/backend/tcop/utility.c work/src/backend/tcop/utility.c
*** head/src/backend/tcop/utility.c 2009-11-21 05:38:11.000000000 +0900
--- work/src/backend/tcop/utility.c 2009-11-30 15:16:17.189475832 +0900
***************
*** 58,63 ****
--- 58,66 ----
#include "utils/syscache.h"
+ /* Hooks for plugins to get control in ProcessUtility() */
+ ProcessUtility_hook_type ProcessUtility_hook = NULL;
+
/*
* Verify user has ownership of specified relation, else ereport.
*
*************** check_xact_readonly(Node *parsetree)
*** 244,249 ****
--- 247,256 ----
* completionTag is only set nonempty if we want to return a nondefault status.
*
* completionTag may be NULL if caller doesn't want a status string.
+ *
+ * We provide a function hook variable that lets loadable plugins
+ * get control when ProcessUtility is called. Such a plugin would
+ * normally call standard_ProcessUtility().
*/
void
ProcessUtility(Node *parsetree,
*************** ProcessUtility(Node *parsetree,
*** 260,265 ****
--- 267,286 ----
if (completionTag)
completionTag[0] = '\0';
+ if (ProcessUtility_hook)
+ (*ProcessUtility_hook) (parsetree, queryString, params, isTopLevel, dest, \
completionTag); + else
+ standard_ProcessUtility(parsetree, queryString, params, isTopLevel, dest, \
completionTag); + }
+
+ void
+ standard_ProcessUtility(Node *parsetree,
+ const char *queryString,
+ ParamListInfo params,
+ bool isTopLevel,
+ DestReceiver *dest,
+ char *completionTag)
+ {
switch (nodeTag(parsetree))
{
/*
diff -cprN head/src/include/tcop/utility.h work/src/include/tcop/utility.h
*** head/src/include/tcop/utility.h 2009-01-02 02:24:01.000000000 +0900
--- work/src/include/tcop/utility.h 2009-11-30 15:16:17.189475832 +0900
***************
*** 17,25 ****
--- 17,34 ----
#include "tcop/tcopprot.h"
+ /* Hook for plugins to get control in ProcessUtility() */
+ typedef void (*ProcessUtility_hook_type) (Node *parsetree,
+ const char *queryString, ParamListInfo params, bool isTopLevel,
+ DestReceiver *dest, char *completionTag);
+ extern PGDLLIMPORT ProcessUtility_hook_type ProcessUtility_hook;
+
extern void ProcessUtility(Node *parsetree, const char *queryString,
ParamListInfo params, bool isTopLevel,
DestReceiver *dest, char *completionTag);
+ extern void standard_ProcessUtility(Node *parsetree, const char *queryString,
+ ParamListInfo params, bool isTopLevel,
+ DestReceiver *dest, char *completionTag);
extern bool UtilityReturnsTuples(Node *parsetree);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic