Re: [HACKERS] replication commands and log_statements

2014-09-12 Thread Fujii Masao
On Wed, Sep 10, 2014 at 7:39 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Thanks for reviewing the patch!

 On Wed, Sep 10, 2014 at 4:57 PM, Heikki Linnakangas
 hlinnakan...@vmware.com wrote:
 On 08/28/2014 11:38 AM, Fujii Masao wrote:

 On Thu, Jun 19, 2014 at 5:29 PM, Ian Barwick i...@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 literaloff/. When
 set
 to
  literaloff/, commands will be logged at log level
 literalDEBUG1/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


Re: [HACKERS] replication commands and log_statements

2014-09-10 Thread Heikki Linnakangas

On 08/28/2014 11:38 AM, Fujii Masao wrote:

On Thu, Jun 19, 2014 at 5:29 PM, Ian Barwick i...@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 literaloff/. When set
to
 literaloff/, commands will be logged at log level
literalDEBUG1/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


Re: [HACKERS] replication commands and log_statements

2014-09-10 Thread Fujii Masao
Thanks for reviewing the patch!

On Wed, Sep 10, 2014 at 4:57 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
 On 08/28/2014 11:38 AM, Fujii Masao wrote:

 On Thu, Jun 19, 2014 at 5:29 PM, Ian Barwick i...@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 literaloff/. When
 set
 to
  literaloff/, commands will be logged at log level
 literalDEBUG1/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
*** 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
+   termvarnamelog_replication_commands/varname (typeboolean/type)
+   indexterm
+primaryvarnamelog_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 literaloff/.
+ Only superusers can change this setting.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-log-temp-files xreflabel=log_temp_files
termvarnamelog_temp_files/varname (typeinteger/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 literaltrue/ 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 literaldatabase/ as the value instructs walsender to connect to
  the database specified in the literaldbname/ 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 

Re: [HACKERS] replication commands and log_statements

2014-08-28 Thread Fujii Masao
On Thu, Jun 19, 2014 at 5:29 PM, Ian Barwick i...@2ndquadrant.com wrote:
 On 12/06/14 20:37, Fujii Masao wrote:

 On Wed, Jun 11, 2014 at 11:55 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 Andres Freund and...@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 literaloff/. When set
 to
 literaloff/, commands will be logged at log level
 literalDEBUG1/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
*** 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
+   termvarnamelog_replication_command/varname (typeboolean/type)
+   indexterm
+primaryvarnamelog_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 literaloff/. When
+ set to literaloff/, commands will be logged at log level
+ literalDEBUG1/ (or lower).
+ Only superusers can change this setting.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-log-temp-files xreflabel=log_temp_files
termvarnamelog_temp_files/varname (typeinteger/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 literaltrue/ 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
+ literalDEBUG1/ (or lower) or when
+ xref linkend=guc-log-replication-command is enabled.
  Passing literaldatabase/ as the value instructs walsender to connect to
  the database specified in the literaldbname/ 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;
  

Re: [HACKERS] replication commands and log_statements

2014-08-27 Thread Robert Haas
On Tue, Aug 26, 2014 at 7:17 AM, Fujii Masao masao.fu...@gmail.com wrote:
 On Wed, Aug 20, 2014 at 1:14 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 On Wed, Aug 20, 2014 at 2:06 AM, Robert Haas robertmh...@gmail.com wrote:

 On Sat, Aug 16, 2014 at 10:27 AM, Amit Kapila amit.kapil...@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


Re: [HACKERS] replication commands and log_statements

2014-08-26 Thread Fujii Masao
On Wed, Aug 20, 2014 at 1:14 PM, Michael Paquier
michael.paqu...@gmail.com wrote:



 On Wed, Aug 20, 2014 at 2:06 AM, Robert Haas robertmh...@gmail.com wrote:

 On Sat, Aug 16, 2014 at 10:27 AM, Amit Kapila amit.kapil...@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
*** 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
+   termvarnamelog_replication_command/varname (typeboolean/type)
+   indexterm
+primaryvarnamelog_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 literaloff/.
+ Only superusers can change this setting.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-log-temp-files xreflabel=log_temp_files
termvarnamelog_temp_files/varname (typeinteger/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);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] replication commands and log_statements

2014-08-19 Thread Robert Haas
On Sat, Aug 16, 2014 at 10:27 AM, Amit Kapila amit.kapil...@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


Re: [HACKERS] replication commands and log_statements

2014-08-19 Thread Michael Paquier
On Wed, Aug 20, 2014 at 2:06 AM, Robert Haas robertmh...@gmail.com wrote:

 On Sat, Aug 16, 2014 at 10:27 AM, Amit Kapila amit.kapil...@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.
-- 
Michael


Re: [HACKERS] replication commands and log_statements

2014-08-16 Thread Amit Kapila
On Thu, Aug 14, 2014 at 7:19 PM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
  On Thu, Aug 14, 2014 at 5:56 AM, Stephen Frost sfr...@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


Re: [HACKERS] replication commands and log_statements

2014-08-14 Thread Stephen Frost
Amit,

* Amit Kapila (amit.kapil...@gmail.com) wrote:
 On Thu, Aug 14, 2014 at 5:56 AM, Stephen Frost sfr...@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


signature.asc
Description: Digital signature


Re: [HACKERS] replication commands and log_statements

2014-08-13 Thread Stephen Frost
* Amit Kapila (amit.kapil...@gmail.com) wrote:
 On Wed, Aug 13, 2014 at 4:24 AM, Stephen Frost sfr...@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:
 http://www.postgresql.org/message-id/caa4ek1lvuirqnmjv1vtmrg_f+1f9e9-8rdgnwidscqvtps1...@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


signature.asc
Description: Digital signature


Re: [HACKERS] replication commands and log_statements

2014-08-13 Thread Fujii Masao
On Thu, Aug 14, 2014 at 9:26 AM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
 On Wed, Aug 13, 2014 at 4:24 AM, Stephen Frost sfr...@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


Re: [HACKERS] replication commands and log_statements

2014-08-13 Thread Michael Paquier
On Thu, Aug 14, 2014 at 10:40 AM, Fujii Masao masao.fu...@gmail.com wrote:
 On Thu, Aug 14, 2014 at 9:26 AM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
 On Wed, Aug 13, 2014 at 4:24 AM, Stephen Frost sfr...@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


Re: [HACKERS] replication commands and log_statements

2014-08-13 Thread Amit Kapila
On Thu, Aug 14, 2014 at 5:56 AM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@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


Re: [HACKERS] replication commands and log_statements

2014-08-12 Thread Bruce Momjian
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.fu...@gmail.com wrote:
  On Tue, Aug 12, 2014 at 2:34 AM, Robert Haas robertmh...@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  br...@momjian.ushttp://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


Re: [HACKERS] replication commands and log_statements

2014-08-12 Thread Stephen Frost
* Bruce Momjian (br...@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.fu...@gmail.com wrote:
   On Tue, Aug 12, 2014 at 2:34 AM, Robert Haas robertmh...@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


signature.asc
Description: Digital signature


Re: [HACKERS] replication commands and log_statements

2014-08-12 Thread Amit Kapila
On Wed, Aug 13, 2014 at 4:24 AM, Stephen Frost sfr...@snowman.net wrote:
 * Bruce Momjian (br...@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:
http://www.postgresql.org/message-id/caa4ek1lvuirqnmjv1vtmrg_f+1f9e9-8rdgnwidscqvtps1...@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


Re: [HACKERS] replication commands and log_statements

2014-08-11 Thread Robert Haas
On Fri, Aug 8, 2014 at 1:04 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Sat, Aug 9, 2014 at 12:29 AM, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Aug 8, 2014 at 11:00 AM, Bruce Momjian br...@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 a...@2ndquadrant.com 
 wrote:
  At 2014-08-07 23:22:43 +0900, masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-08-11 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Fri, Aug 8, 2014 at 1:04 PM, Fujii Masao masao.fu...@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


signature.asc
Description: Digital signature


Re: [HACKERS] replication commands and log_statements

2014-08-11 Thread Fujii Masao
On Tue, Aug 12, 2014 at 2:34 AM, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Aug 8, 2014 at 1:04 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Sat, Aug 9, 2014 at 12:29 AM, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Aug 8, 2014 at 11:00 AM, Bruce Momjian br...@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 a...@2ndquadrant.com 
 wrote:
  At 2014-08-07 23:22:43 +0900, masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-08-11 Thread Amit Kapila
On Tue, Aug 12, 2014 at 9:29 AM, Fujii Masao masao.fu...@gmail.com wrote:
 On Tue, Aug 12, 2014 at 2:34 AM, Robert Haas robertmh...@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


Re: [HACKERS] replication commands and log_statements

2014-08-08 Thread Robert Haas
On Thu, Aug 7, 2014 at 12:07 PM, Abhijit Menon-Sen a...@2ndquadrant.com wrote:
 At 2014-08-07 23:22:43 +0900, masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-08-08 Thread Bruce Momjian
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 a...@2ndquadrant.com 
 wrote:
  At 2014-08-07 23:22:43 +0900, masao.fu...@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  br...@momjian.ushttp://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


Re: [HACKERS] replication commands and log_statements

2014-08-08 Thread Robert Haas
On Fri, Aug 8, 2014 at 11:00 AM, Bruce Momjian br...@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 a...@2ndquadrant.com 
 wrote:
  At 2014-08-07 23:22:43 +0900, masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-08-08 Thread Fujii Masao
On Sat, Aug 9, 2014 at 12:29 AM, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Aug 8, 2014 at 11:00 AM, Bruce Momjian br...@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 a...@2ndquadrant.com 
 wrote:
  At 2014-08-07 23:22:43 +0900, masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-08-07 Thread Fujii Masao
On Wed, Jul 2, 2014 at 4:26 AM, Abhijit Menon-Sen a...@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


Re: [HACKERS] replication commands and log_statements

2014-08-07 Thread Abhijit Menon-Sen
At 2014-08-07 23:22:43 +0900, masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-07-01 Thread Abhijit Menon-Sen
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


Re: [HACKERS] replication commands and log_statements

2014-06-23 Thread Robert Haas
On Fri, Jun 20, 2014 at 9:48 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Fujii Masao masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-06-23 Thread Stephen Frost
* Robert Haas (robertmh...@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


signature.asc
Description: Digital signature


Re: [HACKERS] replication commands and log_statements

2014-06-23 Thread Robert Haas
On Mon, Jun 23, 2014 at 11:15 AM, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@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


Re: [HACKERS] replication commands and log_statements

2014-06-20 Thread Fujii Masao
On Thu, Jun 12, 2014 at 10:55 PM, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Jun 11, 2014 at 7:42 AM, Magnus Hagander mag...@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.

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
From e6a67acd0866e2b14fc716ef8a17cc7ac870e50f Mon Sep 17 00:00:00 2001
From: MasaoFujii masao.fu...@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
-  termvarnamelog_statement/varname (typeenum/type)
+  termvarnamelog_statement/varname (typestring/type)
   indexterm
primaryvarnamelog_statement/ configuration parameter/primary
   /indexterm
@@ -4498,14 +4498,16 @@ FROM pg_stat_activity;
   listitem
para
 Controls which SQL statements are logged. Valid values are
-literalnone/ (off), literalddl/, literalmod/, and
-literalall/ (all statements). literalddl/ logs all data definition
+literalnone/ (off), literalddl/, literalmod/, literaldml/,
+and literalall/ (all statements). literalddl/ logs all data definition
 statements, such as commandCREATE/, commandALTER/, and
 commandDROP/ statements. literalmod/ logs all
 literalddl/ statements, plus data-modifying statements
 such as commandINSERT/,
 commandUPDATE/, commandDELETE/, commandTRUNCATE/,
 and commandCOPY FROM/.
+literaldml/ logs all data-modifying statements, plus query access
+statements such as commandSELECT/ and commandCOPY TO/.
 commandPREPARE/, commandEXECUTE/, and
 commandEXPLAIN 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 literalnone/ is specified in the list, other settings
+are ignored and then no SQL statements are logged.
+   /para
+
+   para
 The default is literalnone/. 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 

Re: [HACKERS] replication commands and log_statements

2014-06-20 Thread Tom Lane
Fujii Masao masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-06-20 Thread Michael Paquier
On Fri, Jun 20, 2014 at 10:48 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Fujii Masao masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-06-19 Thread Ian Barwick

On 12/06/14 20:37, Fujii Masao wrote:

On Wed, Jun 11, 2014 at 11:55 PM, Tom Lane t...@sss.pgh.pa.us wrote:

Andres Freund and...@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 literaloff/. When set to
literaloff/, commands will be logged at log level 
literalDEBUG1/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


Re: [HACKERS] replication commands and log_statements

2014-06-12 Thread Fujii Masao
On Wed, Jun 11, 2014 at 11:55 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Andres Freund and...@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
From 9874e36a8f65667d1f4d3a9a8d1d87d2d20f5188 Mon Sep 17 00:00:00 2001
From: MasaoFujii masao.fu...@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
+  termvarnamelog_replication_command/varname (typeboolean/type)
+  indexterm
+   primaryvarnamelog_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 literaloff/.
+Only superusers can change this setting.
+   /para
+  /listitem
+ /varlistentry
+
  varlistentry id=guc-log-temp-files xreflabel=log_temp_files
   termvarnamelog_temp_files/varname (typeinteger/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
--- 

Re: [HACKERS] replication commands and log_statements

2014-06-12 Thread Robert Haas
On Wed, Jun 11, 2014 at 7:42 AM, Magnus Hagander mag...@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


[HACKERS] replication commands and log_statements

2014-06-11 Thread Fujii Masao
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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Andres Freund
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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Magnus Hagander
On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao masao.fu...@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/


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Andres Freund
On 2014-06-11 13:42:58 +0200, Magnus Hagander wrote:
 On Wed, Jun 11, 2014 at 12:34 PM, Fujii Masao masao.fu...@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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Magnus Hagander
On Wed, Jun 11, 2014 at 2:17 PM, Andres Freund and...@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.fu...@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/


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Andres Freund
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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Magnus Hagander
On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund and...@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/


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Andres Freund
On 2014-06-11 14:50:34 +0200, Magnus Hagander wrote:
 On Wed, Jun 11, 2014 at 2:35 PM, Andres Freund and...@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


Re: [HACKERS] replication commands and log_statements

2014-06-11 Thread Tom Lane
Andres Freund and...@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