replication commands and log_statements
Hi,
Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2014-06-11 19:34:25 +0900, Fujii Masao wrote:
Hi,
Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?
Yes, I think we should do that. I can't really see it causing too much
volume...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Hi,
Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?
+1. I think adding a separate parameter is the way to go.
The other option would be to turn log_statements into a parameter that you
specify multiple ones - so instead of "all" today it would be "ddl,dml,all"
or something like that, and then you'd also add "replication" as an option.
But that would cause all sorts of backwards compatibility annoyances.. And
do you really want to be able to say things like "ddl,all" meanin you'd get
ddl and select but not dml?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 2014-06-11 13:42:58 +0200, Magnus Hagander wrote:
On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Hi,
Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?+1. I think adding a separate parameter is the way to go.
Why? I can't really see a case where the log volume by replication
connections is going to be significant in comparison to 'all'?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 11, 2014 at 2:17 PM, Andres Freund <andres@2ndquadrant.com>
wrote:
On 2014-06-11 13:42:58 +0200, Magnus Hagander wrote:
On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:
Hi,
Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?+1. I think adding a separate parameter is the way to go.
Why? I can't really see a case where the log volume by replication
connections is going to be significant in comparison to 'all'?
Yes, but how would you specify for example "i want DDL and all replication
commands" (which is quite a reasonable thing to log, I believe). If you
actually require it to be set to "all", most people won't have any use at
all for it...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote:
Yes, but how would you specify for example "i want DDL and all replication
commands" (which is quite a reasonable thing to log, I believe). If you
actually require it to be set to "all", most people won't have any use at
all for it...
That's a reasonable feature request - but imo it's a separate
one. It seems pretty noncontroversial and simple to make
log_statement=all log replication commands. What you want - and you're
sure not alone with that - is something considerably more complex...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund <andres@2ndquadrant.com>
wrote:
On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote:
Yes, but how would you specify for example "i want DDL and all
replication
commands" (which is quite a reasonable thing to log, I believe). If you
actually require it to be set to "all", most people won't have any use at
all for it...That's a reasonable feature request - but imo it's a separate
one. It seems pretty noncontroversial and simple to make
log_statement=all log replication commands. What you want - and you're
sure not alone with that - is something considerably more complex...
I'm just saying if we're going to do it, let's do it right from the
beginning.
Or are you suggesting this would be something we'd backpatch? Given the
lack of complaints about it, I'd rather suggest we don't backpatch
anything, and instead make the correct feature in the first pace for the
next release.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 2014-06-11 14:50:34 +0200, Magnus Hagander wrote:
On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund <andres@2ndquadrant.com>
wrote:On 2014-06-11 14:22:43 +0200, Magnus Hagander wrote:
Yes, but how would you specify for example "i want DDL and all
replication
commands" (which is quite a reasonable thing to log, I believe). If you
actually require it to be set to "all", most people won't have any use at
all for it...That's a reasonable feature request - but imo it's a separate
one. It seems pretty noncontroversial and simple to make
log_statement=all log replication commands. What you want - and you're
sure not alone with that - is something considerably more complex...
I'm just saying if we're going to do it, let's do it right from the
beginning.
Your wish just seems like a separate feature to me. Including
replication commands in 'all' seems correct independent of the desire
for a more granular control.
Or are you suggesting this would be something we'd backpatch?
Thought about it for a sec, and then decided it doesn't seem warranted.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
Your wish just seems like a separate feature to me. Including
replication commands in 'all' seems correct independent of the desire
for a more granular control.
No, I think I've got to vote with the other side on that.
The reason we can have log_statement as a scalar progression
"none < ddl < mod < all" is that there's little visible use-case
for logging DML but not DDL, nor for logging SELECTS but not
INSERT/UPDATE/DELETE. However, logging replication commands seems
like something people would reasonably want an orthogonal control for.
There's no nice way to squeeze such a behavior into log_statement.
I guess you could say that log_statement treats replication commands
as if they were DDL, but is that really going to satisfy users?
I think we should consider log_statement to control logging of
SQL only, and invent a separate GUC (or, in the future, likely
more than one GUC?) for logging of replication activity.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 11, 2014 at 11:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@2ndquadrant.com> writes:
Your wish just seems like a separate feature to me. Including
replication commands in 'all' seems correct independent of the desire
for a more granular control.No, I think I've got to vote with the other side on that.
The reason we can have log_statement as a scalar progression
"none < ddl < mod < all" is that there's little visible use-case
for logging DML but not DDL, nor for logging SELECTS but not
INSERT/UPDATE/DELETE. However, logging replication commands seems
like something people would reasonably want an orthogonal control for.
There's no nice way to squeeze such a behavior into log_statement.I guess you could say that log_statement treats replication commands
as if they were DDL, but is that really going to satisfy users?I think we should consider log_statement to control logging of
SQL only, and invent a separate GUC (or, in the future, likely
more than one GUC?) for logging of replication activity.
Seems reasonable. OK. The attached patch adds log_replication_command
parameter which causes replication commands to be logged. I added this to
next CF.
Regards,
--
Fujii Masao
Attachments:
0001-Add-log_replication_command-configuration-parameter.patchtext/x-patch; charset=US-ASCII; name=0001-Add-log_replication_command-configuration-parameter.patchDownload
From 9874e36a8f65667d1f4d3a9a8d1d87d2d20f5188 Mon Sep 17 00:00:00 2001
From: MasaoFujii <masao.fujii@gmail.com>
Date: Thu, 12 Jun 2014 19:32:00 +0900
Subject: [PATCH] Add log_replication_command configuration parameter.
This GUC causes replication commands like IDENTIFY_SYSTEM
to be logged in the server log.
---
doc/src/sgml/config.sgml | 16 ++++++++++++++++
src/backend/replication/walsender.c | 11 +++++++++--
src/backend/utils/misc/guc.c | 9 +++++++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/replication/walsender.h | 1 +
5 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 697cf99..8532f08 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4534,6 +4534,22 @@ FROM pg_stat_activity;
</listitem>
</varlistentry>
+ <varlistentry id="guc-log-replication-command" xreflabel="log_replication_command">
+ <term><varname>log_replication_command</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>log_replication_command</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Causes each replication command to be logged in the server log.
+ See <xref linkend="protocol-replication"> for more information about
+ replication command. The default value is <literal>off</>.
+ Only superusers can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-log-temp-files" xreflabel="log_temp_files">
<term><varname>log_temp_files</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 088ee2c..009ad92 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -108,6 +108,7 @@ bool am_db_walsender = false; /* Connected to a database? */
int max_wal_senders = 0; /* the maximum number of concurrent walsenders */
int wal_sender_timeout = 60 * 1000; /* maximum time to send one
* WAL data message */
+bool log_replication_command = false;
/*
* State for WalSndWakeupRequest
@@ -1261,13 +1262,19 @@ exec_replication_command(const char *cmd_string)
MemoryContext old_context;
/*
+ * Log replication command if log_replication_command is enabled.
+ * Even when it's disabled, log the command with DEBUG1 level for
+ * backward compatibility.
+ */
+ ereport(log_replication_command ? LOG : DEBUG1,
+ (errmsg("received replication command: %s", cmd_string)));
+
+ /*
* CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
* command arrives. Clean up the old stuff if there's anything.
*/
SnapBuildClearExportedSnapshot();
- elog(DEBUG1, "received replication command: %s", cmd_string);
-
CHECK_FOR_INTERRUPTS();
cmd_context = AllocSetContextCreate(CurrentMemoryContext,
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1d094f0..427af79 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -931,6 +931,15 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
+ {"log_replication_command", PGC_SUSET, LOGGING_WHAT,
+ gettext_noop("Logs each replication command."),
+ NULL
+ },
+ &log_replication_command,
+ false,
+ NULL, NULL, NULL
+ },
+ {
{"debug_assertions", PGC_USERSET, DEVELOPER_OPTIONS,
gettext_noop("Turns on various assertion checks."),
gettext_noop("This is a debugging aid."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index d109394..70d86cf 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -431,6 +431,7 @@
# e.g. '<%u%%%d> '
#log_lock_waits = off # log lock waits >= deadlock_timeout
#log_statement = 'none' # none, ddl, mod, all
+#log_replication_command = off
#log_temp_files = -1 # log temporary files equal or larger
# than the specified size in kilobytes;
# -1 disables, 0 logs all temp files
diff --git a/src/include/replication/walsender.h b/src/include/replication/walsender.h
index cff2be6..0095bde 100644
--- a/src/include/replication/walsender.h
+++ b/src/include/replication/walsender.h
@@ -25,6 +25,7 @@ extern bool wake_wal_senders;
/* user-settable parameters */
extern int max_wal_senders;
extern int wal_sender_timeout;
+extern bool log_replication_command;
extern void InitWalSender(void);
extern void exec_replication_command(const char *query_string);
--
1.7.1
On Wed, Jun 11, 2014 at 7:42 AM, Magnus Hagander <magnus@hagander.net> wrote:
Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?+1. I think adding a separate parameter is the way to go.
The other option would be to turn log_statements into a parameter that you
specify multiple ones
I kind of like this idea, but...
- so instead of "all" today it would be "ddl,dml,all"
or something like that, and then you'd also add "replication" as an option.
But that would cause all sorts of backwards compatibility annoyances.. And
do you really want to be able to say things like "ddl,all" meanin you'd get
ddl and select but not dml?
...you lost me here. I mean, I think it could be quite useful to
redefine the existing GUC as a list. We could continue to have ddl,
dml, and all as tokens that would be in the list, but you wouldn't
write "ddl,dml,all" because "all" would include everything that those
other ones would log. But then you could have combinations like
"dml,replication" and so on. And you could do much more fine-grained
things, like allow log_statement=create,alter,drop to log all such
statements but not, for example, cluster.
I think if we go the route of adding a separate GUC for this, we're
going to get tired of adding GUCs way before we've come close to
meeting the actual requirements in this area. A comma-separated list
of tokens seems to offer a lot more flexibility.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/06/14 20:37, Fujii Masao wrote:
On Wed, Jun 11, 2014 at 11:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@2ndquadrant.com> writes:
Your wish just seems like a separate feature to me. Including
replication commands in 'all' seems correct independent of the desire
for a more granular control.No, I think I've got to vote with the other side on that.
The reason we can have log_statement as a scalar progression
"none < ddl < mod < all" is that there's little visible use-case
for logging DML but not DDL, nor for logging SELECTS but not
INSERT/UPDATE/DELETE. However, logging replication commands seems
like something people would reasonably want an orthogonal control for.
There's no nice way to squeeze such a behavior into log_statement.I guess you could say that log_statement treats replication commands
as if they were DDL, but is that really going to satisfy users?I think we should consider log_statement to control logging of
SQL only, and invent a separate GUC (or, in the future, likely
more than one GUC?) for logging of replication activity.Seems reasonable. OK. The attached patch adds log_replication_command
parameter which causes replication commands to be logged. I added this to
next CF.
A quick review:
- Compiles against HEAD
- Works as advertised
- Code style looks fine
A couple of suggestions:
- minor rewording for the description, mentioning that statements will
still be logged as DEBUG1 even if parameter set to 'off' (might
prevent reports of the kind "I set it to 'off', why am I still seeing
log entries?").
<para>
Causes each replication command to be logged in the server log.
See <xref linkend="protocol-replication"> for more information about
replication commands. The default value is <literal>off</>. When set to
<literal>off</>, commands will be logged at log level <literal>DEBUG1</literal>.
Only superusers can change this setting.
</para>
- I feel it would be more consistent to use the plural form
for this parameter, i.e. "log_replication_commands", in line with
"log_lock_waits", "log_temp_files", "log_disconnections" etc.
- It might be an idea to add a cross-reference to this parameter from
the "Streaming Replication Protocol" page:
http://www.postgresql.org/docs/devel/static/protocol-replication.html
Regards
Ian Barwick
--
Ian Barwick http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 12, 2014 at 10:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jun 11, 2014 at 7:42 AM, Magnus Hagander <magnus@hagander.net> wrote:
Replication commands like IDENTIFY_COMMAND are not logged even when
log_statements is set to all. Some users who use log_statements to
audit *all* statements might dislike this current situation. So I'm
thinking to change log_statements or add something like log_replication
so that we can log replication commands. Thought?+1. I think adding a separate parameter is the way to go.
The other option would be to turn log_statements into a parameter that you
specify multiple onesI kind of like this idea, but...
- so instead of "all" today it would be "ddl,dml,all"
or something like that, and then you'd also add "replication" as an option.
But that would cause all sorts of backwards compatibility annoyances.. And
do you really want to be able to say things like "ddl,all" meanin you'd get
ddl and select but not dml?...you lost me here. I mean, I think it could be quite useful to
redefine the existing GUC as a list. We could continue to have ddl,
dml, and all as tokens that would be in the list, but you wouldn't
write "ddl,dml,all" because "all" would include everything that those
other ones would log. But then you could have combinations like
"dml,replication" and so on.
Yep, that seems useful, even aside from logging of replication command topic.
OK, I've just implemented the patch (attached) which does this, i.e., redefines
log_statement as a list. Thanks to the patch, log_statement can be set
to "none",
"ddl", "mod", "dml", "all", and any combinations of them. The meanings of
"none", "ddl", "mod" and "all" are the same as they are now. New setting value
"dml" loggs both data modification statements like INSERT and query access
statements like SELECT and COPY TO.
What about applying this patch first?
Regards,
--
Fujii Masao
Attachments:
0001-Redefine-log_statement-as-a-list.patchtext/x-patch; charset=US-ASCII; name=0001-Redefine-log_statement-as-a-list.patchDownload
From e6a67acd0866e2b14fc716ef8a17cc7ac870e50f Mon Sep 17 00:00:00 2001
From: MasaoFujii <masao.fujii@gmail.com>
Date: Fri, 20 Jun 2014 20:39:08 +0900
Subject: [PATCH] Redefine log_statement as a list.
---
doc/src/sgml/config.sgml | 14 +++++-
src/backend/tcop/fastpath.c | 2 +-
src/backend/tcop/postgres.c | 3 +-
src/backend/tcop/utility.c | 60 +++++++++++++-------------
src/backend/utils/misc/guc.c | 97 ++++++++++++++++++++++++++++++++++--------
src/include/tcop/tcopprot.h | 15 +++---
src/include/tcop/utility.h | 2 +-
7 files changed, 132 insertions(+), 61 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8f0ca4c..e93dd37 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4490,7 +4490,7 @@ FROM pg_stat_activity;
</varlistentry>
<varlistentry id="guc-log-statement" xreflabel="log_statement">
- <term><varname>log_statement</varname> (<type>enum</type>)
+ <term><varname>log_statement</varname> (<type>string</type>)
<indexterm>
<primary><varname>log_statement</> configuration parameter</primary>
</indexterm>
@@ -4498,14 +4498,16 @@ FROM pg_stat_activity;
<listitem>
<para>
Controls which SQL statements are logged. Valid values are
- <literal>none</> (off), <literal>ddl</>, <literal>mod</>, and
- <literal>all</> (all statements). <literal>ddl</> logs all data definition
+ <literal>none</> (off), <literal>ddl</>, <literal>mod</>, <literal>dml</>,
+ and <literal>all</> (all statements). <literal>ddl</> logs all data definition
statements, such as <command>CREATE</>, <command>ALTER</>, and
<command>DROP</> statements. <literal>mod</> logs all
<literal>ddl</> statements, plus data-modifying statements
such as <command>INSERT</>,
<command>UPDATE</>, <command>DELETE</>, <command>TRUNCATE</>,
and <command>COPY FROM</>.
+ <literal>dml</> logs all data-modifying statements, plus query access
+ statements such as <command>SELECT</> and <command>COPY TO</>.
<command>PREPARE</>, <command>EXECUTE</>, and
<command>EXPLAIN ANALYZE</> statements are also logged if their
contained command is of an appropriate type. For clients using
@@ -4515,6 +4517,12 @@ FROM pg_stat_activity;
</para>
<para>
+ This parameter can be set to a list of desired SQL statements separated by
+ commas. Note that if <literal>none</> is specified in the list, other settings
+ are ignored and then no SQL statements are logged.
+ </para>
+
+ <para>
The default is <literal>none</>. Only superusers can change this
setting.
</para>
diff --git a/src/backend/tcop/fastpath.c b/src/backend/tcop/fastpath.c
index 9f50c5a..c170e54 100644
--- a/src/backend/tcop/fastpath.c
+++ b/src/backend/tcop/fastpath.c
@@ -340,7 +340,7 @@ HandleFunctionRequest(StringInfo msgBuf)
fetch_fp_info(fid, fip);
/* Log as soon as we have the function OID and name */
- if (log_statement == LOGSTMT_ALL)
+ if (log_statement & LOGSTMT_OTHER)
{
ereport(LOG,
(errmsg("fastpath function call: \"%s\" (OID %u)",
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 6b4221a..8a78989 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -89,6 +89,7 @@ CommandDest whereToSendOutput = DestDebug;
bool Log_disconnections = false;
int log_statement = LOGSTMT_NONE;
+char *log_statement_string = NULL;
/* GUC variable for maximum stack depth (measured in kilobytes) */
int max_stack_depth = 100;
@@ -2017,7 +2018,7 @@ check_log_statement(List *stmt_list)
{
Node *stmt = (Node *) lfirst(stmt_item);
- if (GetCommandLogLevel(stmt) <= log_statement)
+ if (GetCommandLogLevel(stmt) & log_statement)
return true;
}
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 3423898..8498e30 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -2448,10 +2448,10 @@ CreateCommandTag(Node *parsetree)
* This must handle all command types, but since the vast majority
* of 'em are utility commands, it seems sensible to keep it here.
*/
-LogStmtLevel
+int
GetCommandLogLevel(Node *parsetree)
{
- LogStmtLevel lev;
+ int lev;
switch (nodeTag(parsetree))
{
@@ -2466,24 +2466,24 @@ GetCommandLogLevel(Node *parsetree)
if (((SelectStmt *) parsetree)->intoClause)
lev = LOGSTMT_DDL; /* SELECT INTO */
else
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_QUERY;
break;
/* utility statements --- same whether raw or cooked */
case T_TransactionStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_DeclareCursorStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_ClosePortalStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_FetchStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER; /* should this be QUERY? */
break;
case T_CreateSchemaStmt:
@@ -2541,7 +2541,7 @@ GetCommandLogLevel(Node *parsetree)
if (((CopyStmt *) parsetree)->is_from)
lev = LOGSTMT_MOD;
else
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_QUERY;
break;
case T_PrepareStmt:
@@ -2563,12 +2563,12 @@ GetCommandLogLevel(Node *parsetree)
if (ps)
lev = GetCommandLogLevel(ps->plansource->raw_parse_tree);
else
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
}
break;
case T_DeallocateStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_RenameStmt:
@@ -2652,7 +2652,7 @@ GetCommandLogLevel(Node *parsetree)
break;
case T_DoStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_CreatedbStmt:
@@ -2672,19 +2672,19 @@ GetCommandLogLevel(Node *parsetree)
break;
case T_NotifyStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_ListenStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_UnlistenStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_LoadStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_ClusterStmt:
@@ -2692,7 +2692,7 @@ GetCommandLogLevel(Node *parsetree)
break;
case T_VacuumStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_ExplainStmt:
@@ -2714,7 +2714,7 @@ GetCommandLogLevel(Node *parsetree)
return GetCommandLogLevel(stmt->query);
/* Plain EXPLAIN isn't so interesting */
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
}
break;
@@ -2727,19 +2727,19 @@ GetCommandLogLevel(Node *parsetree)
break;
case T_AlterSystemStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_VariableSetStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_VariableShowStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_DiscardStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_CreateTrigStmt:
@@ -2787,19 +2787,19 @@ GetCommandLogLevel(Node *parsetree)
break;
case T_LockStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_ConstraintsSetStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_CheckPointStmt:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
case T_ReindexStmt:
- lev = LOGSTMT_ALL; /* should this be DDL? */
+ lev = LOGSTMT_OTHER; /* should this be DDL? */
break;
case T_CreateConversionStmt:
@@ -2838,7 +2838,7 @@ GetCommandLogLevel(Node *parsetree)
switch (stmt->commandType)
{
case CMD_SELECT:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_QUERY;
break;
case CMD_UPDATE:
@@ -2850,7 +2850,7 @@ GetCommandLogLevel(Node *parsetree)
default:
elog(WARNING, "unrecognized commandType: %d",
(int) stmt->commandType);
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
}
}
@@ -2864,7 +2864,7 @@ GetCommandLogLevel(Node *parsetree)
switch (stmt->commandType)
{
case CMD_SELECT:
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_QUERY;
break;
case CMD_UPDATE:
@@ -2880,7 +2880,7 @@ GetCommandLogLevel(Node *parsetree)
default:
elog(WARNING, "unrecognized commandType: %d",
(int) stmt->commandType);
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
}
@@ -2890,7 +2890,7 @@ GetCommandLogLevel(Node *parsetree)
default:
elog(WARNING, "unrecognized node type: %d",
(int) nodeTag(parsetree));
- lev = LOGSTMT_ALL;
+ lev = LOGSTMT_OTHER;
break;
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6902c23..67c3552 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -160,6 +160,8 @@ static bool call_string_check_hook(struct config_string * conf, char **newval,
static bool call_enum_check_hook(struct config_enum * conf, int *newval,
void **extra, GucSource source, int elevel);
+static bool check_logstmt(char **newval, void **extra, GucSource source);
+static void assign_logstmt(const char *newval, void *extra);
static bool check_log_destination(char **newval, void **extra, GucSource source);
static void assign_log_destination(const char *newval, void *extra);
@@ -275,14 +277,6 @@ static const struct config_enum_entry log_error_verbosity_options[] = {
{NULL, 0, false}
};
-static const struct config_enum_entry log_statement_options[] = {
- {"none", LOGSTMT_NONE, false},
- {"ddl", LOGSTMT_DDL, false},
- {"mod", LOGSTMT_MOD, false},
- {"all", LOGSTMT_ALL, false},
- {NULL, 0, false}
-};
-
static const struct config_enum_entry isolation_level_options[] = {
{"serializable", XACT_SERIALIZABLE, false},
{"repeatable read", XACT_REPEATABLE_READ, false},
@@ -2966,6 +2960,17 @@ static struct config_string ConfigureNamesString[] =
},
{
+ {"log_statement", PGC_SUSET, LOGGING_WHAT,
+ gettext_noop("Sets the type of statements logged."),
+ NULL,
+ GUC_LIST_INPUT
+ },
+ &log_statement_string,
+ "none",
+ check_logstmt, assign_logstmt, NULL
+ },
+
+ {
{"log_destination", PGC_SIGHUP, LOGGING_WHERE,
gettext_noop("Sets the destination for server log output."),
gettext_noop("Valid values are combinations of \"stderr\", "
@@ -3366,16 +3371,6 @@ static struct config_enum ConfigureNamesEnum[] =
},
{
- {"log_statement", PGC_SUSET, LOGGING_WHAT,
- gettext_noop("Sets the type of statements logged."),
- NULL
- },
- &log_statement,
- LOGSTMT_NONE, log_statement_options,
- NULL, NULL, NULL
- },
-
- {
{"syslog_facility", PGC_SIGHUP, LOGGING_WHERE,
gettext_noop("Sets the syslog \"facility\" to be used when syslog enabled."),
NULL
@@ -8993,6 +8988,72 @@ call_enum_check_hook(struct config_enum * conf, int *newval, void **extra,
*/
static bool
+check_logstmt(char **newval, void **extra, GucSource source)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+ int newlogstmt = 0;
+ int *myextra;
+ bool nolog = false;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of identifiers */
+ if (!SplitIdentifierString(rawstring, ',', &elemlist))
+ {
+ /* syntax error in list */
+ GUC_check_errdetail("List syntax is invalid.");
+ pfree(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ char *tok = (char *) lfirst(l);
+
+ if (pg_strcasecmp(tok, "none") == 0)
+ nolog = true;
+ else if (pg_strcasecmp(tok, "ddl") == 0)
+ newlogstmt |= LOGSTMT_DDL;
+ else if (pg_strcasecmp(tok, "mod") == 0)
+ newlogstmt |= (LOGSTMT_DDL | LOGSTMT_MOD);
+ else if (pg_strcasecmp(tok, "dml") == 0)
+ newlogstmt |= (LOGSTMT_MOD | LOGSTMT_QUERY);
+ else if (pg_strcasecmp(tok, "all") == 0)
+ newlogstmt = LOGSTMT_ALL;
+ else
+ {
+ GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+ pfree(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+ }
+
+ /* If "none" is set, ignore other settings and don't log any statements */
+ if (nolog)
+ newlogstmt = LOGSTMT_NONE;
+
+ pfree(rawstring);
+ list_free(elemlist);
+
+ myextra = (int *) guc_malloc(ERROR, sizeof(int));
+ *myextra = newlogstmt;
+ *extra = (void *) myextra;
+
+ return true;
+}
+
+static void
+assign_logstmt(const char *newval, void *extra)
+{
+ log_statement = *((int *) extra);
+}
+
+static bool
check_log_destination(char **newval, void **extra, GucSource source)
{
char *rawstring;
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index 60f7532..837111c 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -36,15 +36,16 @@ extern int PostAuthDelay;
/* GUC-configurable parameters */
-typedef enum
-{
- LOGSTMT_NONE, /* log no statements */
- LOGSTMT_DDL, /* log data definition statements */
- LOGSTMT_MOD, /* log modification statements, plus DDL */
- LOGSTMT_ALL /* log all statements */
-} LogStmtLevel;
+/* Log statement bigmap */
+#define LOGSTMT_NONE 0 /* log no statements */
+#define LOGSTMT_DDL 1 /* log data definition statements */
+#define LOGSTMT_MOD 2 /* log modification statements */
+#define LOGSTMT_QUERY 4 /* log query access statements */
+#define LOGSTMT_OTHER 8 /* log other statements */
+#define LOGSTMT_ALL 15 /* log all statements */
extern int log_statement;
+extern char *log_statement_string;
extern List *pg_parse_query(const char *query_string);
extern List *pg_analyze_and_rewrite(Node *parsetree, const char *query_string,
diff --git a/src/include/tcop/utility.h b/src/include/tcop/utility.h
index 3ecf931..a3852da 100644
--- a/src/include/tcop/utility.h
+++ b/src/include/tcop/utility.h
@@ -45,7 +45,7 @@ extern Query *UtilityContainsQuery(Node *parsetree);
extern const char *CreateCommandTag(Node *parsetree);
-extern LogStmtLevel GetCommandLogLevel(Node *parsetree);
+extern int GetCommandLogLevel(Node *parsetree);
extern bool CommandIsReadOnly(Node *parsetree);
--
1.7.1
Fujii Masao <masao.fujii@gmail.com> writes:
OK, I've just implemented the patch (attached) which does this, i.e., redefines
log_statement as a list. Thanks to the patch, log_statement can be set
to "none",
"ddl", "mod", "dml", "all", and any combinations of them. The meanings of
"none", "ddl", "mod" and "all" are the same as they are now. New setting value
"dml" loggs both data modification statements like INSERT and query access
statements like SELECT and COPY TO.
I still don't like this one bit. It's turning log_statement from a
reasonably clean design into a complete mess, which will be made even
worse after you add replication control to it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 20, 2014 at 10:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
OK, I've just implemented the patch (attached) which does this, i.e., redefines
log_statement as a list. Thanks to the patch, log_statement can be set
to "none",
"ddl", "mod", "dml", "all", and any combinations of them. The meanings of
"none", "ddl", "mod" and "all" are the same as they are now. New setting value
"dml" loggs both data modification statements like INSERT and query access
statements like SELECT and COPY TO.I still don't like this one bit. It's turning log_statement from a
reasonably clean design into a complete mess, which will be made even
worse after you add replication control to it.
Yeah, I'd vote as well to let log_statement as it is (without
mentioning the annoyance it would cause to users), and to have
replication statement logging managed with a separate GUC for clarity.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 20, 2014 at 9:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
OK, I've just implemented the patch (attached) which does this, i.e., redefines
log_statement as a list. Thanks to the patch, log_statement can be set
to "none",
"ddl", "mod", "dml", "all", and any combinations of them. The meanings of
"none", "ddl", "mod" and "all" are the same as they are now. New setting value
"dml" loggs both data modification statements like INSERT and query access
statements like SELECT and COPY TO.I still don't like this one bit. It's turning log_statement from a
reasonably clean design into a complete mess, which will be made even
worse after you add replication control to it.
Well, I don't object to having a separate GUC for replication command
logging if people prefer that design. But let's not have any
delusions of adequacy about log_statement. I've had more than one
conversation with customers about that particular parameter, all of
which involved the customer being unhappy that there were only four
choices and they couldn't log the stuff that they cared about without
logging a lot of other stuff that they didn't care about. Now,
providing more choices there will, indisputably, add complexity, but
it will also provide utility.
What we're talking about here is not unlike what we went through with
EXPLAIN syntax. We repeatedly rejected patches that might have added
useful functionality to EXPLAIN on the grounds that (1) we didn't want
to make new keywords and (2) even if we did add new keywords,
extending the EXPLAIN productions would produce grammar conflicts.
Then, we implemented the extensible-options stuff, and suddenly it
became possible for people to write patches adding useful
functionality to EXPLAIN that didn't get sunk before it got out of the
gate, and since then we've gotten a new EXPLAIN option every release
or two, and IMHO all of those options are pretty useful. Similarly,
building a logging facility that meets the needs of real users is
going to require a configuration method more flexible than a total
order with four choices. I happen to think a list of comma-separated
tokens is a pretty good choice, but something else could be OK, too.
We need something better than what we've got now, though.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Robert Haas (robertmhaas@gmail.com) wrote:
Similarly,
building a logging facility that meets the needs of real users is
going to require a configuration method more flexible than a total
order with four choices. I happen to think a list of comma-separated
tokens is a pretty good choice, but something else could be OK, too.
We need something better than what we've got now, though.
Absolutely, and not all of that should be done through postgresql.conf,
imv. The level of flexibility that is being asked for, from what I've
seen and heard, really needs to be met with new catalog tables or
extending existing ones. The nearby thread on pg_audit would be a good
example.
I certainly don't want to be specifying specific table names or
providing a list of roles through any GUC (or configuration file of any
kind). I'm not adverse to improving log_statement to a list with tokens
that indicate various meaning levels but anything done through that
mechanism will be very coarse.
Thanks,
Stephen
On Mon, Jun 23, 2014 at 11:15 AM, Stephen Frost <sfrost@snowman.net> wrote:
* Robert Haas (robertmhaas@gmail.com) wrote:
Similarly,
building a logging facility that meets the needs of real users is
going to require a configuration method more flexible than a total
order with four choices. I happen to think a list of comma-separated
tokens is a pretty good choice, but something else could be OK, too.
We need something better than what we've got now, though.Absolutely, and not all of that should be done through postgresql.conf,
imv. The level of flexibility that is being asked for, from what I've
seen and heard, really needs to be met with new catalog tables or
extending existing ones. The nearby thread on pg_audit would be a good
example.I certainly don't want to be specifying specific table names or
providing a list of roles through any GUC (or configuration file of any
kind). I'm not adverse to improving log_statement to a list with tokens
that indicate various meaning levels but anything done through that
mechanism will be very coarse.
True, but it would be enough to let you make a list of commands you'd
like to audit, and I think that might be valuable enough to justify
the price of admission. I certainly agree that logging based on which
object is being accessed needs a different design, tied into the
catalogs.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi.
Do we have any consensus about what to do with these two patches?
1. Introduce a "log_replication_command" setting.
2. Change log_statement to be a list of tokens.
If I understand correctly, there weren't any strong objections to the
former, but the situation is less clear when it comes to the second.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 2, 2014 at 4:26 AM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
Hi.
Do we have any consensus about what to do with these two patches?
1. Introduce a "log_replication_command" setting.
2. Change log_statement to be a list of tokens.If I understand correctly, there weren't any strong objections to the
former, but the situation is less clear when it comes to the second.
On second thought, I'd like to propose the third option. This is the same idea
as Andres suggested upthread. That is, we log replication commands only
when log_statement is set to all. Neither new parameter is introduced nor
log_statement is redefined as a list.
Firstly, since log_statement=all means that all statements are logged, it's
basically natural and intuitive to log even replication commands in that
setting. Secondly, any statements except DDL and data-modifying statements,
e.g., VACUUM, CHECKPOINT, BEGIN, ..etc, are logged only when log_statement=all.
So even if some users want to control the logging of DDL and maintenance
commands orthogonally, which is impossible for now. Therefore, even if the
logging of replication commands which are neither DDL nor data-modifying
statements also cannot be controlled orthogonally, I don't think that users
get so surprised. Of course I have no objection to address the issue by, e.g.,
enabling such orthogonal logging control, in the future, though.
Thought? What about introducing that simple but not so confusing change first?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
At 2014-08-07 23:22:43 +0900, masao.fujii@gmail.com wrote:
That is, we log replication commands only when log_statement is set to
all. Neither new parameter is introduced nor log_statement is
redefined as a list.
That sounds good to me.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 7, 2014 at 12:07 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
At 2014-08-07 23:22:43 +0900, masao.fujii@gmail.com wrote:
That is, we log replication commands only when log_statement is set to
all. Neither new parameter is introduced nor log_statement is
redefined as a list.That sounds good to me.
It sounds fairly unprincipled to me. I liked the idea of making
log_statement a list, but if we aren't gonna do that, I think this
should be a separate parameter.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 8, 2014 at 08:51:13AM -0400, Robert Haas wrote:
On Thu, Aug 7, 2014 at 12:07 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
At 2014-08-07 23:22:43 +0900, masao.fujii@gmail.com wrote:
That is, we log replication commands only when log_statement is set to
all. Neither new parameter is introduced nor log_statement is
redefined as a list.That sounds good to me.
It sounds fairly unprincipled to me. I liked the idea of making
log_statement a list, but if we aren't gonna do that, I think this
should be a separate parameter.
I am unclear there is enough demand for a separate replication logging
parameter --- using log_statement=all made sense to me.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 8, 2014 at 11:00 AM, Bruce Momjian <bruce@momjian.us> wrote:
On Fri, Aug 8, 2014 at 08:51:13AM -0400, Robert Haas wrote:
On Thu, Aug 7, 2014 at 12:07 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
At 2014-08-07 23:22:43 +0900, masao.fujii@gmail.com wrote:
That is, we log replication commands only when log_statement is set to
all. Neither new parameter is introduced nor log_statement is
redefined as a list.That sounds good to me.
It sounds fairly unprincipled to me. I liked the idea of making
log_statement a list, but if we aren't gonna do that, I think this
should be a separate parameter.I am unclear there is enough demand for a separate replication logging
parameter --- using log_statement=all made sense to me.
Most people don't want to turn on log_statement=all because it
produces too much log volume.
See, for example:
http://bonesmoses.org/2014/08/05/on-postgresql-logging-verbosity/
But logging replication commands is quite low-volume, so it is not
hard to imagine someone wanting to log all replication commands but
not all SQL statements.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Aug 9, 2014 at 12:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Aug 8, 2014 at 11:00 AM, Bruce Momjian <bruce@momjian.us> wrote:
On Fri, Aug 8, 2014 at 08:51:13AM -0400, Robert Haas wrote:
On Thu, Aug 7, 2014 at 12:07 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
At 2014-08-07 23:22:43 +0900, masao.fujii@gmail.com wrote:
That is, we log replication commands only when log_statement is set to
all. Neither new parameter is introduced nor log_statement is
redefined as a list.That sounds good to me.
It sounds fairly unprincipled to me. I liked the idea of making
log_statement a list, but if we aren't gonna do that, I think this
should be a separate parameter.I am unclear there is enough demand for a separate replication logging
parameter --- using log_statement=all made sense to me.Most people don't want to turn on log_statement=all because it
produces too much log volume.See, for example:
http://bonesmoses.org/2014/08/05/on-postgresql-logging-verbosity/But logging replication commands is quite low-volume, so it is not
hard to imagine someone wanting to log all replication commands but
not all SQL statements.
You can do that by executing
"ALTER ROLE <replication user> SET log_statement TO 'all'".
If you don't use the replication user to execute SQL statements,
no SQL statements are logged in that setting.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 8, 2014 at 1:04 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Aug 9, 2014 at 12:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Aug 8, 2014 at 11:00 AM, Bruce Momjian <bruce@momjian.us> wrote:
On Fri, Aug 8, 2014 at 08:51:13AM -0400, Robert Haas wrote:
On Thu, Aug 7, 2014 at 12:07 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
At 2014-08-07 23:22:43 +0900, masao.fujii@gmail.com wrote:
That is, we log replication commands only when log_statement is set to
all. Neither new parameter is introduced nor log_statement is
redefined as a list.That sounds good to me.
It sounds fairly unprincipled to me. I liked the idea of making
log_statement a list, but if we aren't gonna do that, I think this
should be a separate parameter.I am unclear there is enough demand for a separate replication logging
parameter --- using log_statement=all made sense to me.Most people don't want to turn on log_statement=all because it
produces too much log volume.See, for example:
http://bonesmoses.org/2014/08/05/on-postgresql-logging-verbosity/But logging replication commands is quite low-volume, so it is not
hard to imagine someone wanting to log all replication commands but
not all SQL statements.You can do that by executing
"ALTER ROLE <replication user> SET log_statement TO 'all'".
If you don't use the replication user to execute SQL statements,
no SQL statements are logged in that setting.
If you have a user devoted to it, I suppose that's true. I still
think it shouldn't get munged together like that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Robert Haas (robertmhaas@gmail.com) wrote:
On Fri, Aug 8, 2014 at 1:04 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
You can do that by executing
"ALTER ROLE <replication user> SET log_statement TO 'all'".
If you don't use the replication user to execute SQL statements,
no SQL statements are logged in that setting.If you have a user devoted to it, I suppose that's true. I still
think it shouldn't get munged together like that.
Folks *should* have a dedicated replication user, imv. That said, I
agree with Robert in that I don't particularly like this recommendation
for how to enable logging of replication commands. For one thing, it
means having to remember to set the per-role GUC for every replication
user which is created and that's the kind of trivially-missed step that
can get people into trouble.
Thanks,
Stephen
On Tue, Aug 12, 2014 at 2:34 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Aug 8, 2014 at 1:04 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Aug 9, 2014 at 12:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Aug 8, 2014 at 11:00 AM, Bruce Momjian <bruce@momjian.us> wrote:
On Fri, Aug 8, 2014 at 08:51:13AM -0400, Robert Haas wrote:
On Thu, Aug 7, 2014 at 12:07 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
At 2014-08-07 23:22:43 +0900, masao.fujii@gmail.com wrote:
That is, we log replication commands only when log_statement is set to
all. Neither new parameter is introduced nor log_statement is
redefined as a list.That sounds good to me.
It sounds fairly unprincipled to me. I liked the idea of making
log_statement a list, but if we aren't gonna do that, I think this
should be a separate parameter.I am unclear there is enough demand for a separate replication logging
parameter --- using log_statement=all made sense to me.Most people don't want to turn on log_statement=all because it
produces too much log volume.See, for example:
http://bonesmoses.org/2014/08/05/on-postgresql-logging-verbosity/But logging replication commands is quite low-volume, so it is not
hard to imagine someone wanting to log all replication commands but
not all SQL statements.You can do that by executing
"ALTER ROLE <replication user> SET log_statement TO 'all'".
If you don't use the replication user to execute SQL statements,
no SQL statements are logged in that setting.If you have a user devoted to it, I suppose that's true. I still
think it shouldn't get munged together like that.
Why do we need to treat only replication commands as special ones and
add new parameter to display them? I agree to add new parameter like
log_replication to log the replication activity instead of the command
itself, like checkpoint activity which can be enabled by log_checkpoints.
But I'm not sure why logging of replication commands also needs to be
controlled by separate parameter.
And, I still think that those who set log_statement to all expect that all
the commands including replication commands are logged. I'm afraid
that separating only parameter for replication commands might confuse
the users.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 12, 2014 at 9:29 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Aug 12, 2014 at 2:34 AM, Robert Haas <robertmhaas@gmail.com>
wrote:
If you have a user devoted to it, I suppose that's true. I still
think it shouldn't get munged together like that.Why do we need to treat only replication commands as special ones and
add new parameter to display them?
One difference is that replication commands are internally generated
commands. Do we anywhere else log such internally generated
commands with log_statement = all?
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Tue, Aug 12, 2014 at 10:07:34AM +0530, Amit Kapila wrote:
On Tue, Aug 12, 2014 at 9:29 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Aug 12, 2014 at 2:34 AM, Robert Haas <robertmhaas@gmail.com> wrote:
If you have a user devoted to it, I suppose that's true. �I still
think it shouldn't get munged together like that.Why do we need to treat only replication commands as special ones and
add new parameter to display them?One difference is that replication commands are internally generated
commands. Do we anywhere else log such internally generated
commands with log_statement = all?
Good point --- we do not. In fact, this is similar to how we don't log
SPI queries, e.g. SQL queries inside functions. We might want to enable
that someday too. Could we enable logging of both with a single GUC?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Bruce Momjian (bruce@momjian.us) wrote:
On Tue, Aug 12, 2014 at 10:07:34AM +0530, Amit Kapila wrote:
On Tue, Aug 12, 2014 at 9:29 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Aug 12, 2014 at 2:34 AM, Robert Haas <robertmhaas@gmail.com> wrote:
If you have a user devoted to it, I suppose that's true. I still
think it shouldn't get munged together like that.Why do we need to treat only replication commands as special ones and
add new parameter to display them?One difference is that replication commands are internally generated
commands. Do we anywhere else log such internally generated
commands with log_statement = all?
Not entirely sure what you're referring to as 'internally generated'
here.. While they can come from another backend, they don't have to.
Good point --- we do not. In fact, this is similar to how we don't log
SPI queries, e.g. SQL queries inside functions. We might want to enable
that someday too. Could we enable logging of both with a single GUC?
I don't see those as the same at all. I'd like to see improved
flexibility in this area, certainly, but don't want two independent
considerations like these tied to one GUC..
Thanks,
Stephen
On Wed, Aug 13, 2014 at 4:24 AM, Stephen Frost <sfrost@snowman.net> wrote:
* Bruce Momjian (bruce@momjian.us) wrote:
On Tue, Aug 12, 2014 at 10:07:34AM +0530, Amit Kapila wrote:
One difference is that replication commands are internally generated
commands. Do we anywhere else log such internally generated
commands with log_statement = all?Not entirely sure what you're referring to as 'internally generated'
here..
Here 'internally generated' means that user doesn't execute those
statements, rather the replication/backup code form these statements
(IDENTIFY_SYSTEM, TIMELINE_HISTORY, BASE_BACKUP, ...)
and send to server to get the appropriate results.
While they can come from another backend, they don't have to.
Good point --- we do not. In fact, this is similar to how we don't log
SPI queries, e.g. SQL queries inside functions. We might want to enable
that someday too.
Yes, few days back I have seen one of the user expects
such functionality. Refer below mail:
/messages/by-id/CAA4eK1LVuirQnMjV1vTMrG_F+1F9e9-8RDGnwiDsCqVTps1ptQ@mail.gmail.com
Could we enable logging of both with a single GUC?
I don't see those as the same at all. I'd like to see improved
flexibility in this area, certainly, but don't want two independent
considerations like these tied to one GUC..
Agreed, I also think both are different and shouldn't be logged
with one GUC. Here the important thing to decide is which is
the better way to proceed for allowing logging of replication
commands such that it can allow us to extend it for more
things. We have discussed below options:
a. Make log_statement a list of comma separated options
b. Have a separate parameter something like log_replication*
c. Log it when user has mentioned option 'all' in log_statement.
From future extensibility pov, I think option (a) is a good
choice or we could even invent a new scheme for logging
commands which would be something similar to
log_min_messages where we can define levels
level - 0 (none)
level - 1 (dml)
level - 2 (ddl)
level - 4 (replication)
level - 8 (all)
Here if user specifies level = 2, then DDL commands will
get logged and level = 3 would mean log dml and ddl commands.
From backward compatibility, we can keep current parameter
as it is and introduce this a new parameter.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
* Amit Kapila (amit.kapila16@gmail.com) wrote:
On Wed, Aug 13, 2014 at 4:24 AM, Stephen Frost <sfrost@snowman.net> wrote:
Not entirely sure what you're referring to as 'internally generated'
here..Here 'internally generated' means that user doesn't execute those
statements, rather the replication/backup code form these statements
(IDENTIFY_SYSTEM, TIMELINE_HISTORY, BASE_BACKUP, ...)
and send to server to get the appropriate results.
You could argue the same about pg_dump.. I'd not thought of it before,
but it might be kind of neat to have psql support "connect in
replication mode" and then allow the user to run replication commands.
Still, this isn't the same.
Yes, few days back I have seen one of the user expects
such functionality. Refer below mail:
/messages/by-id/CAA4eK1LVuirQnMjV1vTMrG_F+1F9e9-8RDGnwiDsCqVTps1ptQ@mail.gmail.com
Oh, to be clear- I agree that logging of queries executed through SPI is
desirable; I'd certainly like to have that capability without having to
go through the auto-explain module or write my own module. That doesn't
mean we should consider them either "internal" (which I don't really
agree with- consider anonymous DO blocks...) or somehow related to
replication logging.
Could we enable logging of both with a single GUC?
I don't see those as the same at all. I'd like to see improved
flexibility in this area, certainly, but don't want two independent
considerations like these tied to one GUC..Agreed, I also think both are different and shouldn't be logged
with one GUC. Here the important thing to decide is which is
the better way to proceed for allowing logging of replication
commands such that it can allow us to extend it for more
things. We have discussed below options:a. Make log_statement a list of comma separated options
b. Have a separate parameter something like log_replication*
c. Log it when user has mentioned option 'all' in log_statement.
Regarding this, I'm generally in the camp that says to just include it
in 'all' and be done with it- for now. This is just another example
of where we really need a much more granular and configurable framework
for auditing and we're not going to get through GUCs, so let's keep the
GUC based approach simple and familiar to our users and build a proper
auditing system.
Thanks,
Stephen
On Thu, Aug 14, 2014 at 9:26 AM, Stephen Frost <sfrost@snowman.net> wrote:
* Amit Kapila (amit.kapila16@gmail.com) wrote:
On Wed, Aug 13, 2014 at 4:24 AM, Stephen Frost <sfrost@snowman.net> wrote:
Not entirely sure what you're referring to as 'internally generated'
here..Here 'internally generated' means that user doesn't execute those
statements, rather the replication/backup code form these statements
(IDENTIFY_SYSTEM, TIMELINE_HISTORY, BASE_BACKUP, ...)
and send to server to get the appropriate results.You could argue the same about pg_dump.. I'd not thought of it before,
but it might be kind of neat to have psql support "connect in
replication mode" and then allow the user to run replication commands.
You can do that by specifying "replication=1" as the conninfo when
exexcuting psql. For example,
$ psql -d "replication=1"
psql (9.5devel)
Type "help" for help.
postgres=# IDENTIFY_SYSTEM;
systemid | timeline | xlogpos | dbname
---------------------+----------+-----------+--------
6047222920639525794 | 1 | 0/1711678 |
(1 row)
Agreed, I also think both are different and shouldn't be logged
with one GUC. Here the important thing to decide is which is
the better way to proceed for allowing logging of replication
commands such that it can allow us to extend it for more
things. We have discussed below options:a. Make log_statement a list of comma separated options
b. Have a separate parameter something like log_replication*
c. Log it when user has mentioned option 'all' in log_statement.Regarding this, I'm generally in the camp that says to just include it
in 'all' and be done with it- for now. This is just another example
of where we really need a much more granular and configurable framework
for auditing and we're not going to get through GUCs, so let's keep the
GUC based approach simple and familiar to our users and build a proper
auditing system.
+1
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 14, 2014 at 10:40 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Aug 14, 2014 at 9:26 AM, Stephen Frost <sfrost@snowman.net> wrote:
* Amit Kapila (amit.kapila16@gmail.com) wrote:
On Wed, Aug 13, 2014 at 4:24 AM, Stephen Frost <sfrost@snowman.net> wrote:
Not entirely sure what you're referring to as 'internally generated'
here..Here 'internally generated' means that user doesn't execute those
statements, rather the replication/backup code form these statements
(IDENTIFY_SYSTEM, TIMELINE_HISTORY, BASE_BACKUP, ...)
and send to server to get the appropriate results.You could argue the same about pg_dump.. I'd not thought of it before,
but it might be kind of neat to have psql support "connect in
replication mode" and then allow the user to run replication commands.You can do that by specifying "replication=1" as the conninfo when
exexcuting psql. For example,$ psql -d "replication=1"
psql (9.5devel)
Type "help" for help.postgres=# IDENTIFY_SYSTEM;
systemid | timeline | xlogpos | dbname
---------------------+----------+-----------+--------
6047222920639525794 | 1 | 0/1711678 |
(1 row)
More details here:
http://www.postgresql.org/docs/9.4/static/protocol-replication.html
Note as well that not all the commands work though:
=# START_REPLICATION PHYSICAL 0/0;
unexpected PQresultStatus: 8
=# START_REPLICATION PHYSICAL 0/0;
PQexec not allowed during COPY BOTH
We may do something to improve about that but I am not sure it is worth it.
Regards,
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 14, 2014 at 5:56 AM, Stephen Frost <sfrost@snowman.net> wrote:
* Amit Kapila (amit.kapila16@gmail.com) wrote:
Oh, to be clear- I agree that logging of queries executed through SPI is
desirable; I'd certainly like to have that capability without having to
go through the auto-explain module or write my own module. That doesn't
mean we should consider them either "internal" (which I don't really
agree with- consider anonymous DO blocks...) or somehow related to
replication logging.Could we enable logging of both with a single GUC?
I don't see those as the same at all. I'd like to see improved
flexibility in this area, certainly, but don't want two independent
considerations like these tied to one GUC..Agreed, I also think both are different and shouldn't be logged
with one GUC. Here the important thing to decide is which is
the better way to proceed for allowing logging of replication
commands such that it can allow us to extend it for more
things. We have discussed below options:a. Make log_statement a list of comma separated options
b. Have a separate parameter something like log_replication*
c. Log it when user has mentioned option 'all' in log_statement.Regarding this, I'm generally in the camp that says to just include it
in 'all' and be done with it- for now.
Okay, but tomorrow if someone wants to implement a patch to log
statements executed through SPI (statements inside functions), then
what will be your suggestion, does those also can be allowed to log
with 'all' option or you would like to suggest him to wait for a proper
auditing system?
Wouldn't allowing to log everything under 'all' option can start
confusing some users without having individual
(ddl, mod, replication, ...) options for different kind of statements.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Amit,
* Amit Kapila (amit.kapila16@gmail.com) wrote:
On Thu, Aug 14, 2014 at 5:56 AM, Stephen Frost <sfrost@snowman.net> wrote:
Regarding this, I'm generally in the camp that says to just include it
in 'all' and be done with it- for now.Okay, but tomorrow if someone wants to implement a patch to log
statements executed through SPI (statements inside functions), then
what will be your suggestion, does those also can be allowed to log
with 'all' option or you would like to suggest him to wait for a proper
auditing system?
No, I'd suggest having a different option for it as that would be a huge
change for people who are already doing 'all', potentially. Adding the
replication commands is extremely unlikely to cause individuals who are
already logging 'all' any problems, as far as I can tell.
Wouldn't allowing to log everything under 'all' option can start
confusing some users without having individual
(ddl, mod, replication, ...) options for different kind of statements.
I don't see logging replication commands under 'all' as confusing, no.
Thanks,
Stephen
On Thu, Aug 14, 2014 at 7:19 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Amit Kapila (amit.kapila16@gmail.com) wrote:
On Thu, Aug 14, 2014 at 5:56 AM, Stephen Frost <sfrost@snowman.net>
wrote:
Regarding this, I'm generally in the camp that says to just include it
in 'all' and be done with it- for now.Okay, but tomorrow if someone wants to implement a patch to log
statements executed through SPI (statements inside functions), then
what will be your suggestion, does those also can be allowed to log
with 'all' option or you would like to suggest him to wait for a proper
auditing system?No, I'd suggest having a different option for it as that would be a huge
change for people who are already doing 'all', potentially.
Not only that, I think another important reason is that those represent
separate set of functionality and some users could be interested to
log those statements alone or along with other set of commands.
Adding the
replication commands is extremely unlikely to cause individuals who are
already logging 'all' any problems, as far as I can tell.
As 'all' is superset for all commands, so anyway it would have been
logged under 'all', the point is that whether replication commands
can be considered to have a separate log level parameter. To me, it
appears that it deserves a separate log level parameter even though
today number of commands which fall under this category are less,
however it can increase tomorrow. Also if tomorrow there is a consensus
on how to extend log_statement parameter, it is quite likely that
someone will come up with a patch to consider replication commands
as separate log level.
I think ideally it would have been better if we could have logged
replication commands under separate log_level, but as still there
is no consensus on extending log_statement and nobody is even
willing to pursue, it seems okay to go ahead and log these under
'all' level.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sat, Aug 16, 2014 at 10:27 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
I think ideally it would have been better if we could have logged
replication commands under separate log_level, but as still there
is no consensus on extending log_statement and nobody is even
willing to pursue, it seems okay to go ahead and log these under
'all' level.
I think the consensus is clearly for a separate GUC.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 20, 2014 at 2:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Aug 16, 2014 at 10:27 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:I think ideally it would have been better if we could have logged
replication commands under separate log_level, but as still there
is no consensus on extending log_statement and nobody is even
willing to pursue, it seems okay to go ahead and log these under
'all' level.I think the consensus is clearly for a separate GUC.
+1.
--
MichaelOn Wed, Aug 20, 2014 at 1:14 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Aug 20, 2014 at 2:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Aug 16, 2014 at 10:27 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:I think ideally it would have been better if we could have logged
replication commands under separate log_level, but as still there
is no consensus on extending log_statement and nobody is even
willing to pursue, it seems okay to go ahead and log these under
'all' level.I think the consensus is clearly for a separate GUC.
+1.
Okay. Attached is the updated version of the patch which I posted before.
This patch follows the consensus and adds separate parameter
"log_replication_command".
Regards,
--
Fujii Masao
Attachments:
log_replication_command_v2.patchtext/x-patch; charset=US-ASCII; name=log_replication_command_v2.patchDownload
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 4558,4563 **** FROM pg_stat_activity;
--- 4558,4579 ----
</listitem>
</varlistentry>
+ <varlistentry id="guc-log-replication-command" xreflabel="log_replication_command">
+ <term><varname>log_replication_command</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>log_replication_command</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Causes each replication command to be logged in the server log.
+ See <xref linkend="protocol-replication"> for more information about
+ replication command. The default value is <literal>off</>.
+ Only superusers can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-log-temp-files" xreflabel="log_temp_files">
<term><varname>log_temp_files</varname> (<type>integer</type>)
<indexterm>
*** a/src/backend/replication/walsender.c
--- b/src/backend/replication/walsender.c
***************
*** 108,113 **** bool am_db_walsender = false; /* Connected to a database? */
--- 108,114 ----
int max_wal_senders = 0; /* the maximum number of concurrent walsenders */
int wal_sender_timeout = 60 * 1000; /* maximum time to send one
* WAL data message */
+ bool log_replication_command = false;
/*
* State for WalSndWakeupRequest
***************
*** 1268,1280 **** exec_replication_command(const char *cmd_string)
MemoryContext old_context;
/*
* CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
* command arrives. Clean up the old stuff if there's anything.
*/
SnapBuildClearExportedSnapshot();
- elog(DEBUG1, "received replication command: %s", cmd_string);
-
CHECK_FOR_INTERRUPTS();
cmd_context = AllocSetContextCreate(CurrentMemoryContext,
--- 1269,1287 ----
MemoryContext old_context;
/*
+ * Log replication command if log_replication_command is enabled.
+ * Even when it's disabled, log the command with DEBUG1 level for
+ * backward compatibility.
+ */
+ ereport(log_replication_command ? LOG : DEBUG1,
+ (errmsg("received replication command: %s", cmd_string)));
+
+ /*
* CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
* command arrives. Clean up the old stuff if there's anything.
*/
SnapBuildClearExportedSnapshot();
CHECK_FOR_INTERRUPTS();
cmd_context = AllocSetContextCreate(CurrentMemoryContext,
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 925,930 **** static struct config_bool ConfigureNamesBool[] =
--- 925,939 ----
NULL, NULL, NULL
},
{
+ {"log_replication_command", PGC_SUSET, LOGGING_WHAT,
+ gettext_noop("Logs each replication command."),
+ NULL
+ },
+ &log_replication_command,
+ false,
+ NULL, NULL, NULL
+ },
+ {
{"debug_assertions", PGC_INTERNAL, PRESET_OPTIONS,
gettext_noop("Shows whether the running server has assertion checks enabled."),
NULL,
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 431,436 ****
--- 431,437 ----
# e.g. '<%u%%%d> '
#log_lock_waits = off # log lock waits >= deadlock_timeout
#log_statement = 'none' # none, ddl, mod, all
+ #log_replication_command = off
#log_temp_files = -1 # log temporary files equal or larger
# than the specified size in kilobytes;
# -1 disables, 0 logs all temp files
*** a/src/include/replication/walsender.h
--- b/src/include/replication/walsender.h
***************
*** 25,30 **** extern bool wake_wal_senders;
--- 25,31 ----
/* user-settable parameters */
extern int max_wal_senders;
extern int wal_sender_timeout;
+ extern bool log_replication_command;
extern void InitWalSender(void);
extern void exec_replication_command(const char *query_string);
On Tue, Aug 26, 2014 at 7:17 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Aug 20, 2014 at 1:14 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Aug 20, 2014 at 2:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Aug 16, 2014 at 10:27 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:I think ideally it would have been better if we could have logged
replication commands under separate log_level, but as still there
is no consensus on extending log_statement and nobody is even
willing to pursue, it seems okay to go ahead and log these under
'all' level.I think the consensus is clearly for a separate GUC.
+1.
Okay. Attached is the updated version of the patch which I posted before.
This patch follows the consensus and adds separate parameter
"log_replication_command".
Looks fine to me.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 19, 2014 at 5:29 PM, Ian Barwick <ian@2ndquadrant.com> wrote:
On 12/06/14 20:37, Fujii Masao wrote:
On Wed, Jun 11, 2014 at 11:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@2ndquadrant.com> writes:
Your wish just seems like a separate feature to me. Including
replication commands in 'all' seems correct independent of the desire
for a more granular control.No, I think I've got to vote with the other side on that.
The reason we can have log_statement as a scalar progression
"none < ddl < mod < all" is that there's little visible use-case
for logging DML but not DDL, nor for logging SELECTS but not
INSERT/UPDATE/DELETE. However, logging replication commands seems
like something people would reasonably want an orthogonal control for.
There's no nice way to squeeze such a behavior into log_statement.I guess you could say that log_statement treats replication commands
as if they were DDL, but is that really going to satisfy users?I think we should consider log_statement to control logging of
SQL only, and invent a separate GUC (or, in the future, likely
more than one GUC?) for logging of replication activity.Seems reasonable. OK. The attached patch adds log_replication_command
parameter which causes replication commands to be logged. I added this to
next CF.A quick review:
Sorry, I've noticed this email right now. Thanks for reviewing the patch!
- minor rewording for the description, mentioning that statements will
still be logged as DEBUG1 even if parameter set to 'off' (might
prevent reports of the kind "I set it to 'off', why am I still seeing
log entries?").<para>
Causes each replication command to be logged in the server log.
See <xref linkend="protocol-replication"> for more information about
replication commands. The default value is <literal>off</>. When set
to
<literal>off</>, commands will be logged at log level
<literal>DEBUG1</literal>.
Only superusers can change this setting.
</para>
Yep, fixed. Attached is the updated version of the patch.
- I feel it would be more consistent to use the plural form
for this parameter, i.e. "log_replication_commands", in line with
"log_lock_waits", "log_temp_files", "log_disconnections" etc.
But log_statement is in the singular form. So I just used
log_replication_command. For the consistency, maybe we need to
change both parameters in the plural form? I don't have strong
opinion about this.
- It might be an idea to add a cross-reference to this parameter from
the "Streaming Replication Protocol" page:
http://www.postgresql.org/docs/devel/static/protocol-replication.html
Yes. I added that.
Regards,
--
Fujii Masao
Attachments:
log_replication_command_v3.patchtext/x-patch; charset=US-ASCII; name=log_replication_command_v3.patchDownload
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 4558,4563 **** FROM pg_stat_activity;
--- 4558,4581 ----
</listitem>
</varlistentry>
+ <varlistentry id="guc-log-replication-command" xreflabel="log_replication_command">
+ <term><varname>log_replication_command</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>log_replication_command</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Causes each replication command to be logged in the server log.
+ See <xref linkend="protocol-replication"> for more information about
+ replication command. The default value is <literal>off</>. When
+ set to <literal>off</>, commands will be logged at log level
+ <literal>DEBUG1</> (or lower).
+ Only superusers can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-log-temp-files" xreflabel="log_temp_files">
<term><varname>log_temp_files</varname> (<type>integer</type>)
<indexterm>
*** a/doc/src/sgml/protocol.sgml
--- b/doc/src/sgml/protocol.sgml
***************
*** 1306,1311 **** To initiate streaming replication, the frontend sends the
--- 1306,1314 ----
of <literal>true</> tells the backend to go into walsender mode, wherein a
small set of replication commands can be issued instead of SQL statements. Only
the simple query protocol can be used in walsender mode.
+ Replication commands are logged in the server log at log level
+ <literal>DEBUG1</> (or lower) or when
+ <xref linkend="guc-log-replication-command"> is enabled.
Passing <literal>database</> as the value instructs walsender to connect to
the database specified in the <literal>dbname</> parameter, which will allow
the connection to be used for logical replication from that database.
*** a/src/backend/replication/walsender.c
--- b/src/backend/replication/walsender.c
***************
*** 108,113 **** bool am_db_walsender = false; /* Connected to a database? */
--- 108,114 ----
int max_wal_senders = 0; /* the maximum number of concurrent walsenders */
int wal_sender_timeout = 60 * 1000; /* maximum time to send one
* WAL data message */
+ bool log_replication_command = false;
/*
* State for WalSndWakeupRequest
***************
*** 1268,1280 **** exec_replication_command(const char *cmd_string)
MemoryContext old_context;
/*
* CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
* command arrives. Clean up the old stuff if there's anything.
*/
SnapBuildClearExportedSnapshot();
- elog(DEBUG1, "received replication command: %s", cmd_string);
-
CHECK_FOR_INTERRUPTS();
cmd_context = AllocSetContextCreate(CurrentMemoryContext,
--- 1269,1287 ----
MemoryContext old_context;
/*
+ * Log replication command if log_replication_command is enabled.
+ * Even when it's disabled, log the command with DEBUG1 level for
+ * backward compatibility.
+ */
+ ereport(log_replication_command ? LOG : DEBUG1,
+ (errmsg("received replication command: %s", cmd_string)));
+
+ /*
* CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
* command arrives. Clean up the old stuff if there's anything.
*/
SnapBuildClearExportedSnapshot();
CHECK_FOR_INTERRUPTS();
cmd_context = AllocSetContextCreate(CurrentMemoryContext,
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 925,930 **** static struct config_bool ConfigureNamesBool[] =
--- 925,939 ----
NULL, NULL, NULL
},
{
+ {"log_replication_command", PGC_SUSET, LOGGING_WHAT,
+ gettext_noop("Logs each replication command."),
+ NULL
+ },
+ &log_replication_command,
+ false,
+ NULL, NULL, NULL
+ },
+ {
{"debug_assertions", PGC_INTERNAL, PRESET_OPTIONS,
gettext_noop("Shows whether the running server has assertion checks enabled."),
NULL,
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 431,436 ****
--- 431,437 ----
# e.g. '<%u%%%d> '
#log_lock_waits = off # log lock waits >= deadlock_timeout
#log_statement = 'none' # none, ddl, mod, all
+ #log_replication_command = off
#log_temp_files = -1 # log temporary files equal or larger
# than the specified size in kilobytes;
# -1 disables, 0 logs all temp files
*** a/src/include/replication/walsender.h
--- b/src/include/replication/walsender.h
***************
*** 25,30 **** extern bool wake_wal_senders;
--- 25,31 ----
/* user-settable parameters */
extern int max_wal_senders;
extern int wal_sender_timeout;
+ extern bool log_replication_command;
extern void InitWalSender(void);
extern void exec_replication_command(const char *query_string);
On 08/28/2014 11:38 AM, Fujii Masao wrote:
On Thu, Jun 19, 2014 at 5:29 PM, Ian Barwick <ian@2ndquadrant.com> wrote:
- minor rewording for the description, mentioning that statements will
still be logged as DEBUG1 even if parameter set to 'off' (might
prevent reports of the kind "I set it to 'off', why am I still seeing
log entries?").<para>
Causes each replication command to be logged in the server log.
See <xref linkend="protocol-replication"> for more information about
replication commands. The default value is <literal>off</>. When set
to
<literal>off</>, commands will be logged at log level
<literal>DEBUG1</literal>.
Only superusers can change this setting.
</para>Yep, fixed. Attached is the updated version of the patch.
I don't think it's necessary to mention that the commands will still be
logged at DEBUG1 level. We log all kinds of crap at the various DEBUG
levels, and they're not supposed to be used in normal operation.
- I feel it would be more consistent to use the plural form
for this parameter, i.e. "log_replication_commands", in line with
"log_lock_waits", "log_temp_files", "log_disconnections" etc.But log_statement is in the singular form. So I just used
log_replication_command. For the consistency, maybe we need to
change both parameters in the plural form? I don't have strong
opinion about this.
Yeah, we seem to be inconsistent. log_replication_commands would sound
better to me in isolation, but then there is log_statement..
I'll mark this as Ready for Committer in the commitfest app (I assume
you'll take care of committing this yourself when ready).
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thanks for reviewing the patch!
On Wed, Sep 10, 2014 at 4:57 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
On 08/28/2014 11:38 AM, Fujii Masao wrote:
On Thu, Jun 19, 2014 at 5:29 PM, Ian Barwick <ian@2ndquadrant.com> wrote:
- minor rewording for the description, mentioning that statements will
still be logged as DEBUG1 even if parameter set to 'off' (might
prevent reports of the kind "I set it to 'off', why am I still seeing
log entries?").<para>
Causes each replication command to be logged in the server log.
See <xref linkend="protocol-replication"> for more information
about
replication commands. The default value is <literal>off</>. When
set
to
<literal>off</>, commands will be logged at log level
<literal>DEBUG1</literal>.
Only superusers can change this setting.
</para>Yep, fixed. Attached is the updated version of the patch.
I don't think it's necessary to mention that the commands will still be
logged at DEBUG1 level. We log all kinds of crap at the various DEBUG
levels, and they're not supposed to be used in normal operation.
Agreed. I removed that mention from the document.
- I feel it would be more consistent to use the plural form
for this parameter, i.e. "log_replication_commands", in line with
"log_lock_waits", "log_temp_files", "log_disconnections" etc.But log_statement is in the singular form. So I just used
log_replication_command. For the consistency, maybe we need to
change both parameters in the plural form? I don't have strong
opinion about this.Yeah, we seem to be inconsistent. log_replication_commands would sound
better to me in isolation, but then there is log_statement..
Agreed. I changed the GUC name to log_replication_commands.
I'll mark this as Ready for Committer in the commitfest app (I assume you'll
take care of committing this yourself when ready).
Attached is the updated version of the patch. After at least one day
I will commit the patch.
Regards,
--
Fujii Masao
Attachments:
log_replication_command_v4.patchtext/x-patch; charset=US-ASCII; name=log_replication_command_v4.patchDownload
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 4558,4563 **** FROM pg_stat_activity;
--- 4558,4579 ----
</listitem>
</varlistentry>
+ <varlistentry id="guc-log-replication-commands" xreflabel="log_replication_commands">
+ <term><varname>log_replication_commands</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>log_replication_commands</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Causes each replication command to be logged in the server log.
+ See <xref linkend="protocol-replication"> for more information about
+ replication command. The default value is <literal>off</>.
+ Only superusers can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-log-temp-files" xreflabel="log_temp_files">
<term><varname>log_temp_files</varname> (<type>integer</type>)
<indexterm>
*** a/doc/src/sgml/protocol.sgml
--- b/doc/src/sgml/protocol.sgml
***************
*** 1306,1311 **** To initiate streaming replication, the frontend sends the
--- 1306,1313 ----
of <literal>true</> tells the backend to go into walsender mode, wherein a
small set of replication commands can be issued instead of SQL statements. Only
the simple query protocol can be used in walsender mode.
+ Replication commands are logged in the server log when
+ <xref linkend="guc-log-replication-commands"> is enabled.
Passing <literal>database</> as the value instructs walsender to connect to
the database specified in the <literal>dbname</> parameter, which will allow
the connection to be used for logical replication from that database.
*** a/src/backend/replication/walsender.c
--- b/src/backend/replication/walsender.c
***************
*** 108,113 **** bool am_db_walsender = false; /* Connected to a database? */
--- 108,114 ----
int max_wal_senders = 0; /* the maximum number of concurrent walsenders */
int wal_sender_timeout = 60 * 1000; /* maximum time to send one
* WAL data message */
+ bool log_replication_commands = false;
/*
* State for WalSndWakeupRequest
***************
*** 1268,1280 **** exec_replication_command(const char *cmd_string)
MemoryContext old_context;
/*
* CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
* command arrives. Clean up the old stuff if there's anything.
*/
SnapBuildClearExportedSnapshot();
- elog(DEBUG1, "received replication command: %s", cmd_string);
-
CHECK_FOR_INTERRUPTS();
cmd_context = AllocSetContextCreate(CurrentMemoryContext,
--- 1269,1287 ----
MemoryContext old_context;
/*
+ * Log replication command if log_replication_commands is enabled.
+ * Even when it's disabled, log the command with DEBUG1 level for
+ * backward compatibility.
+ */
+ ereport(log_replication_commands ? LOG : DEBUG1,
+ (errmsg("received replication command: %s", cmd_string)));
+
+ /*
* CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
* command arrives. Clean up the old stuff if there's anything.
*/
SnapBuildClearExportedSnapshot();
CHECK_FOR_INTERRUPTS();
cmd_context = AllocSetContextCreate(CurrentMemoryContext,
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 925,930 **** static struct config_bool ConfigureNamesBool[] =
--- 925,939 ----
NULL, NULL, NULL
},
{
+ {"log_replication_commands", PGC_SUSET, LOGGING_WHAT,
+ gettext_noop("Logs each replication command."),
+ NULL
+ },
+ &log_replication_commands,
+ false,
+ NULL, NULL, NULL
+ },
+ {
{"debug_assertions", PGC_INTERNAL, PRESET_OPTIONS,
gettext_noop("Shows whether the running server has assertion checks enabled."),
NULL,
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 431,436 ****
--- 431,437 ----
# e.g. '<%u%%%d> '
#log_lock_waits = off # log lock waits >= deadlock_timeout
#log_statement = 'none' # none, ddl, mod, all
+ #log_replication_commands = off
#log_temp_files = -1 # log temporary files equal or larger
# than the specified size in kilobytes;
# -1 disables, 0 logs all temp files
*** a/src/include/replication/walsender.h
--- b/src/include/replication/walsender.h
***************
*** 25,30 **** extern bool wake_wal_senders;
--- 25,31 ----
/* user-settable parameters */
extern int max_wal_senders;
extern int wal_sender_timeout;
+ extern bool log_replication_commands;
extern void InitWalSender(void);
extern void exec_replication_command(const char *query_string);
On Wed, Sep 10, 2014 at 7:39 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Thanks for reviewing the patch!
On Wed, Sep 10, 2014 at 4:57 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:On 08/28/2014 11:38 AM, Fujii Masao wrote:
On Thu, Jun 19, 2014 at 5:29 PM, Ian Barwick <ian@2ndquadrant.com> wrote:
- minor rewording for the description, mentioning that statements will
still be logged as DEBUG1 even if parameter set to 'off' (might
prevent reports of the kind "I set it to 'off', why am I still seeing
log entries?").<para>
Causes each replication command to be logged in the server log.
See <xref linkend="protocol-replication"> for more information
about
replication commands. The default value is <literal>off</>. When
set
to
<literal>off</>, commands will be logged at log level
<literal>DEBUG1</literal>.
Only superusers can change this setting.
</para>Yep, fixed. Attached is the updated version of the patch.
I don't think it's necessary to mention that the commands will still be
logged at DEBUG1 level. We log all kinds of crap at the various DEBUG
levels, and they're not supposed to be used in normal operation.Agreed. I removed that mention from the document.
- I feel it would be more consistent to use the plural form
for this parameter, i.e. "log_replication_commands", in line with
"log_lock_waits", "log_temp_files", "log_disconnections" etc.But log_statement is in the singular form. So I just used
log_replication_command. For the consistency, maybe we need to
change both parameters in the plural form? I don't have strong
opinion about this.Yeah, we seem to be inconsistent. log_replication_commands would sound
better to me in isolation, but then there is log_statement..Agreed. I changed the GUC name to log_replication_commands.
I'll mark this as Ready for Committer in the commitfest app (I assume you'll
take care of committing this yourself when ready).Attached is the updated version of the patch. After at least one day
I will commit the patch.
Applied. Thanks all!
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers