Re: [HACKERS] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Tue, Mar 7, 2017 at 3:21 AM, Robert Haas  wrote:
> On Sat, Mar 4, 2017 at 10:32 AM, Tom Lane  wrote:
>> Without having actually looked at this patch, I would say that if it added
>> a direct call of fopen() to backend-side code, that was already the wrong
>> thing.  Almost always, AllocateFile() would be a better choice, not only
>> because it's tied into transaction abort, but also because it knows how to
>> release virtual FDs in event of ENFILE/EMFILE errors.  If there is some
>> convincing reason why you shouldn't use AllocateFile(), then a safe
>> cleanup pattern would be to have the fclose() in a PG_CATCH stanza.
>
> I think that my previous remarks on this issue were simply muddled
> thinking.  The SQL-callable function pg_current_logfile() does use
> AllocateFile(), so the ERROR which may occur afterward if the file is
> corrupted is no problem.  The syslogger, on the other hand, uses
> logfile_open() to open the file, but it's careful not to throw an
> ERROR while the file is open, just like other code which runs in the
> syslogger.  So now I think there's no bug here.

-   /*
-* No space found, file content is corrupted.  Return NULL to the
-* caller and inform him on the situation.
-*/
+   /* Uh oh.  No newline found, so file content is corrupted. */
This one just made me smile.
-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Sat, Mar 4, 2017 at 10:32 AM, Tom Lane  wrote:
> Without having actually looked at this patch, I would say that if it added
> a direct call of fopen() to backend-side code, that was already the wrong
> thing.  Almost always, AllocateFile() would be a better choice, not only
> because it's tied into transaction abort, but also because it knows how to
> release virtual FDs in event of ENFILE/EMFILE errors.  If there is some
> convincing reason why you shouldn't use AllocateFile(), then a safe
> cleanup pattern would be to have the fclose() in a PG_CATCH stanza.

I think that my previous remarks on this issue were simply muddled
thinking.  The SQL-callable function pg_current_logfile() does use
AllocateFile(), so the ERROR which may occur afterward if the file is
corrupted is no problem.  The syslogger, on the other hand, uses
logfile_open() to open the file, but it's careful not to throw an
ERROR while the file is open, just like other code which runs in the
syslogger.  So now I think there's no bug here.

-- 
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] Patch to implement pg_current_logfile() function

[Tom Lane]

"Karl O. Pinc"  writes:
> On Sat, 4 Mar 2017 14:26:54 +0530
> Robert Haas  wrote:
>> On Fri, Mar 3, 2017 at 7:21 PM, Karl O. Pinc  wrote:
>>> But if the code does not exit the loop then
>>> before calling elog(ERROR), shouldn't it
>>> also call FreeFile(fd)?  

>> Hmm.  Normally error recovery cleans up opened files, but since
>> logfile_open() directly calls fopen(), that's not going to work here.
>> So that does seem to be a problem.

> Should something different be done to open the file or is it
> enough to call FreeFile(fd) to clean up properly?

I would say that in at least 99.44% of cases, if you think you need to
do some cleanup action immediately before an elog(ERROR), that means
You're Doing It Wrong.  That can only be a safe coding pattern in a
segment of code in which no called function does, *or ever will*, throw
elog(ERROR) itself.  Straight-line code with no reason ever to call
anything else might qualify, but otherwise you're taking too much risk
of current or future breakage.  You need some mechanism that will ensure
cleanup after any elog call anywhere, and the backend environment offers
lots of such mechanisms.

Without having actually looked at this patch, I would say that if it added
a direct call of fopen() to backend-side code, that was already the wrong
thing.  Almost always, AllocateFile() would be a better choice, not only
because it's tied into transaction abort, but also because it knows how to
release virtual FDs in event of ENFILE/EMFILE errors.  If there is some
convincing reason why you shouldn't use AllocateFile(), then a safe
cleanup pattern would be to have the fclose() in a PG_CATCH stanza.

(FWIW, I don't particularly agree with Michael's objection to "break"
after elog(ERROR).  Our traditional coding style is to write such things
anyway, so as to avoid drawing complaints from compilers that don't know
that elog(ERROR) doesn't return.  You could argue that that's an obsolete
reason, but I don't think I want to see it done some places and not
others.  Consistency of coding style is a good thing.)

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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 4 Mar 2017 14:26:54 +0530
Robert Haas  wrote:

> On Fri, Mar 3, 2017 at 7:21 PM, Karl O. Pinc  wrote:
> > But if the code does not exit the loop then
> > before calling elog(ERROR), shouldn't it
> > also call FreeFile(fd)?  
> 
> Hmm.  Normally error recovery cleans up opened files, but since
> logfile_open() directly calls fopen(), that's not going to work here.
> So that does seem to be a problem.

Should something different be done to open the file or is it
enough to call FreeFile(fd) to clean up properly?

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Fri, Mar 3, 2017 at 7:21 PM, Karl O. Pinc  wrote:
> But if the code does not exit the loop then
> before calling elog(ERROR), shouldn't it
> also call FreeFile(fd)?

Hmm.  Normally error recovery cleans up opened files, but since
logfile_open() directly calls fopen(), that's not going to work here.
So that does seem to be a problem.

-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Fri, Mar 3, 2017 at 11:54 AM, Michael Paquier
 wrote:
> On Fri, Mar 3, 2017 at 3:18 PM, Robert Haas  wrote:
>> Hopefully I haven't broken anything; please let me know if you
>> encounter any issues.
>
> Reading what has just been committed...
>
> +   /*
> +* No space found, file content is corrupted.  Return NULL to the
> +* caller and inform him on the situation.
> +*/
> +   elog(ERROR,
> +"missing space character in \"%s\"", LOG_METAINFO_DATAFILE);
> +   break;
> There is no need to issue a break after a elog(ERROR).

True, but it's not wrong, either.  We do it all the time.

git grep -A2 elog.*ERROR
/break


The fact that the comment doesn't match the code, though, is wrong.  Oops.

> +* No newlinei found, file content is corrupted.  Return NULL to
> s/newlinei/newline/

That's also a problem, and that comment also refers to returning,
which we don't.

-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Fri, 3 Mar 2017 15:24:53 +0900
Michael Paquier  wrote:
> 
> +   /*
> +* No space found, file content is corrupted.  Return
> NULL to the
> +* caller and inform him on the situation.
> +*/
> +   elog(ERROR,
> +"missing space character in \"%s\"",
> LOG_METAINFO_DATAFILE);
> +   break;
> There is no need to issue a break after a elog(ERROR).

There's 2 cases of issuing a break after a elog(ERROR),
both in the same loop.

And the comment could then be amended as well to just:

  No space found, file content is corrupted

Because (I presume) there's no returning of any value
going on.

But if the code does not exit the loop then
before calling elog(ERROR), shouldn't it
also call FreeFile(fd)?

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Fri, Mar 3, 2017 at 3:18 PM, Robert Haas  wrote:
> Hopefully I haven't broken anything; please let me know if you
> encounter any issues.

Reading what has just been committed...

+   /*
+* No space found, file content is corrupted.  Return NULL to the
+* caller and inform him on the situation.
+*/
+   elog(ERROR,
+"missing space character in \"%s\"", LOG_METAINFO_DATAFILE);
+   break;
There is no need to issue a break after a elog(ERROR).

+* No newlinei found, file content is corrupted.  Return NULL to
s/newlinei/newline/
-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Mon, Feb 20, 2017 at 4:37 AM, Gilles Darold  wrote:
>> I think it's sufficient to just remove the file once on postmaster
>> startup before trying to launch the syslogger for the first time.
>> logging_collector is PGC_POSTMASTER, so if it's not turned on when the
>> cluster first starts, it can't be activated later.  If it dies, that
>> doesn't seem like a reason to remove the file.  We're going to restart
>> the syslogger, and when we do, it can update the file.
>>
>
> I've attached a new full v30 patch that include last patch from Karl.
> Now the current_logfile file is removed only at postmaster startup, just
> before launching the syslogger for the first time.

Committed with some changes.

- Added missing REVOKE for pg_current_logfile(text).
- Added error checks for unlink() and logfile_open().
- Changed error messages to use standard wording (among other reasons,
this helps minimize translation work).
- Removed update_metainfo_datafile() call that ran in the postmaster
and added a similar call that runs in the syslogger.
- doc: Removed an index entry that had no existing parallel.

Hopefully I haven't broken anything; please let me know if you
encounter any issues.

Thanks,

-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 16/02/2017 à 16:13, Robert Haas a écrit :
> On Wed, Feb 15, 2017 at 4:03 PM, Alvaro Herrera
>  wrote:
>> So what is going on here is that SysLogger_Start() wants to unlink the
>> current-logfile file if the collector is not enabled.  This should
>> probably be split out into a separate new function, for two reasons:
>> first, it doesn't seem good idea to have SysLogger_Start do something
>> other than start the logger; and second, so that we don't have a syscall
>> on each ServerLoop iteration.  That new function should be called from
>> some other place -- perhaps reaper() and just before entering
>> ServerLoop, so that the file is deleted if the syslogger goes away or is
>> not started.
> I think it's sufficient to just remove the file once on postmaster
> startup before trying to launch the syslogger for the first time.
> logging_collector is PGC_POSTMASTER, so if it's not turned on when the
> cluster first starts, it can't be activated later.  If it dies, that
> doesn't seem like a reason to remove the file.  We're going to restart
> the syslogger, and when we do, it can update the file.
>

I've attached a new full v30 patch that include last patch from Karl.
Now the current_logfile file is removed only at postmaster startup, just
before launching the syslogger for the first time.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 95afc2c..a668456 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4280,6 +4280,11 @@ SELECT * FROM parent WHERE key = 2400;
   where to log
  
 
+ 
+   current_logfiles
+   and the log_destination configuration parameter
+ 
+
  
 
  
@@ -4310,6 +4315,27 @@ SELECT * FROM parent WHERE key = 2400;
  must be enabled to generate
 CSV-format log output.

+   
+When either stderr or
+csvlog are included, the file
+current_logfiles is created to record the location
+of the log file(s) currently in use by the logging collector and the
+associated logging destination. This provides a convenient way to
+find the logs currently in use by the instance. Here is an example of
+this file's content:
+
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+
+
+current_logfiles is recreated when a new log file
+is created as an effect of rotation, and
+when log_destination is reloaded.  It is removed when
+neither stderr
+nor csvlog are included
+in log_destination, and when the logging collector is
+disabled.
+   
 

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index d7738b1..cfa6349 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15468,6 +15468,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15686,6 +15693,45 @@ SET search_path TO schema , schema, ..

 

+pg_current_logfile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+ current_logfiles
+ and the pg_current_logfile function
+   
+
+   
+Logging
+current_logfiles file and the pg_current_logfile
+function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of the log file(s) currently in use by the logging collector.
+The path includes the  directory
+and the log file name.  Log collection must be enabled or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply, as text,
+either csvlog or stderr as the value of the
+optional parameter. The return value is NULL when the
+log format requested is not a configured
+.  The
+pg_current_logfiles reflects the contents of the
+current_logfiles file.
+   
+
+   
 pg_my_temp_schema

 
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 127b759..262adea 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -36,6 +36,10 @@ these required items, the cluster configuration files
 PGDATA, although it is possible to place them elsewhere.
 
 
+
+  current_logfiles
+
+
 
 Contents of PGDATA
 
@@ -61,6 +65,12 @@ Item
 
 
 
+ current_logfiles
+ File recording the log file(s) currently written to by the logging
+  collector
+
+
+
  global
  Subdirectory containing cluster-wide 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Robert Haas]

On Wed, Feb 15, 2017 at 4:03 PM, Alvaro Herrera
 wrote:
> So what is going on here is that SysLogger_Start() wants to unlink the
> current-logfile file if the collector is not enabled.  This should
> probably be split out into a separate new function, for two reasons:
> first, it doesn't seem good idea to have SysLogger_Start do something
> other than start the logger; and second, so that we don't have a syscall
> on each ServerLoop iteration.  That new function should be called from
> some other place -- perhaps reaper() and just before entering
> ServerLoop, so that the file is deleted if the syslogger goes away or is
> not started.

I think it's sufficient to just remove the file once on postmaster
startup before trying to launch the syslogger for the first time.
logging_collector is PGC_POSTMASTER, so if it's not turned on when the
cluster first starts, it can't be activated later.  If it dies, that
doesn't seem like a reason to remove the file.  We're going to restart
the syslogger, and when we do, it can update the file.

-- 
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] Patch to implement pg_current_logfile() function

[Tom Lane]

"Karl O. Pinc"  writes:
> On a different topic, I pulled from master and now
> I see that git finds the following untracked:
>   src/bin/pg_basebackup/pg_receivexlog
>   src/bin/pg_resetxlog/
>   src/bin/pg_xlogdump/

Those got renamed, cf
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=85c11324cabaddcfaf3347df78555b30d27c5b5a

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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 15 Feb 2017 15:23:00 -0500
Robert Haas  wrote:

> +ereport(WARNING,
> +(errcode(ERRCODE_INTERNAL_ERROR),
> + errmsg("corrupted data found in \"%s\"",
> +LOG_METAINFO_DATAFILE)));
> 
> elog seems fine here.  There's no need for this to be translatable.
> Also, I'd change the level to ERROR.
> 
> + errhint("The supported log formats are
> \"stderr\""
> +" and \"csvlog\".")));
> 
> I think our preferred style is not to break strings across lines like
> this.
> 
> +log_filepath[strcspn(log_filepath, "\n")] = '\0';
> 
> We have no existing dependency on strcspn().  It might be better not
> to add one just for this feature; I suggest using strchr() instead,
> which is widely used in our existing code.


Attached is a v29 patch which fixes the above problems.

The Syslogger hunk remains to be fixed.  I have no plans
to do so at this time.

Note that since I have to write an "if" anyway, I'm going ahead
and raising an error condition when there's no newline in the
current_logfiles file.   The strcspn() ignored the missing newline.
The new code could do so as well by negating the if condition
should that be preferable.


On a different topic, I pulled from master and now
I see that git finds the following untracked:

src/bin/pg_basebackup/pg_receivexlog
src/bin/pg_resetxlog/
src/bin/pg_xlogdump/

I'd appreciate knowing if I'm doing something dumb
on my end to make this happen.  Thanks.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 963e70c..ae1fa0b 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -906,6 +906,7 @@ pg_current_logfile(PG_FUNCTION_ARGS)
 	char*logfmt;
 	char*log_filepath;
 	char*log_format = lbuffer;
+	char*nlpos;
 
 	/* The log format parameter is optional */
 	if (PG_NARGS() == 0 || PG_ARGISNULL(0))
@@ -918,8 +919,7 @@ pg_current_logfile(PG_FUNCTION_ARGS)
 			ereport(ERROR,
 	(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 	 errmsg("log format \"%s\" is not supported", logfmt),
-	 errhint("The supported log formats are \"stderr\""
-	" and \"csvlog\".")));
+	 errhint("The supported log formats are \"stderr\" and \"csvlog\".")));
 	}
 
 	fd = AllocateFile(LOG_METAINFO_DATAFILE, "r");
@@ -947,19 +947,28 @@ pg_current_logfile(PG_FUNCTION_ARGS)
 		if (log_filepath == NULL)
 		{
 			/*
-			 * Corrupted data found, return NULL to the caller and
+			 * Corrupted data found, no space.  Return NULL to the caller and
 			 * inform him on the situation.
 			 */
-			ereport(WARNING,
-	(errcode(ERRCODE_INTERNAL_ERROR),
-	 errmsg("corrupted data found in \"%s\"",
-			LOG_METAINFO_DATAFILE)));
+			elog(ERROR,
+ "missing space character in \"%s\"", LOG_METAINFO_DATAFILE);
 			break;
 		}
 
 		*log_filepath = '\0';
 		log_filepath++;
-		log_filepath[strcspn(log_filepath, "\n")] = '\0';
+		nlpos = strchr(log_filepath, '\n');
+		if (nlpos == NULL)
+		{
+			/*
+			 * Corrupted data found, no newline.  Return NULL to the caller
+			 * and inform him on the situation.
+			 */
+			elog(ERROR,
+ "missing newline character in \"%s\"", LOG_METAINFO_DATAFILE);
+			break;
+		}
+		*nlpos = '\0';
 
 		if (logfmt == NULL || strcmp(logfmt, log_format) == 0)
 		{

-- 
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] Patch to implement pg_current_logfile() function

[Tom Lane]

"Karl O. Pinc"  writes:
> On Wed, 15 Feb 2017 15:23:00 -0500
> Robert Haas  wrote:
>> I think our preferred style is not to break strings across lines like
>> this.

> How do you do that and not exceed the 80 character line limit?
> Just ignore the line length limit?

Right.  It depends on context, but letting isolated lines wrap doesn't
do that much damage to readability, IMO anyway.  (If you've got a bunch
of these adjacent to each other, you might choose to break them to
reduce confusion.)

The advantage of not breaking up error strings is that it makes grepping
for them more reliable, when you're wondering "where in the sources did
this error come from?".  If you get a report about "could not frob a
whatzit" and grep for "frob a whatzit", but somebody decided to break
that string in the middle to avoid a line wrap, you won't find the spot.

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] Patch to implement pg_current_logfile() function

[Alvaro Herrera]

Robert Haas wrote:
> On Fri, Jan 20, 2017 at 2:09 AM, Michael Paquier
>  wrote:
> > Okay I just did it. At the same time the check for ferror is not
> > necessary as fgets() returns NULL on an error as well so that's dead
> > code. I have also removed the useless call to FreeFile().
> 
> diff --git a/src/backend/postmaster/postmaster.c
> b/src/backend/postmaster/postmaster.c
> index 271c492..a7ebb74 100644
> --- a/src/backend/postmaster/postmaster.c
> +++ b/src/backend/postmaster/postmaster.c
> @@ -1733,7 +1733,7 @@ ServerLoop(void)
>  }
> 
>  /* If we have lost the log collector, try to start a new one */
> -if (SysLoggerPID == 0 && Logging_collector)
> +if (SysLoggerPID == 0)
>  SysLoggerPID = SysLogger_Start();
> 
>  /*
> 
> This hunk has zero chance of being acceptable, I think.  We're not
> going to start running the syslogger in even when it's not configured
> to run.

So what is going on here is that SysLogger_Start() wants to unlink the
current-logfile file if the collector is not enabled.  This should
probably be split out into a separate new function, for two reasons:
first, it doesn't seem good idea to have SysLogger_Start do something
other than start the logger; and second, so that we don't have a syscall
on each ServerLoop iteration.  That new function should be called from
some other place -- perhaps reaper() and just before entering
ServerLoop, so that the file is deleted if the syslogger goes away or is
not started.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 15 Feb 2017 15:23:00 -0500
Robert Haas  wrote:

> + errhint("The supported log formats are
> \"stderr\""
> +" and \"csvlog\".")));
> 
> I think our preferred style is not to break strings across lines like
> this.

How do you do that and not exceed the 80 character line limit?
Just ignore the line length limit?


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Fri, Jan 20, 2017 at 2:09 AM, Michael Paquier
 wrote:
> Okay I just did it. At the same time the check for ferror is not
> necessary as fgets() returns NULL on an error as well so that's dead
> code. I have also removed the useless call to FreeFile().

diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 271c492..a7ebb74 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1733,7 +1733,7 @@ ServerLoop(void)
 }

 /* If we have lost the log collector, try to start a new one */
-if (SysLoggerPID == 0 && Logging_collector)
+if (SysLoggerPID == 0)
 SysLoggerPID = SysLogger_Start();

 /*

This hunk has zero chance of being acceptable, I think.  We're not
going to start running the syslogger in even when it's not configured
to run.

I think what should happen is that the postmaster should try to remove
any old metainfo datafile before it reaches this point, and then if
the logging collector is started it will recreate it.

+ereport(WARNING,
+(errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("corrupted data found in \"%s\"",
+LOG_METAINFO_DATAFILE)));

elog seems fine here.  There's no need for this to be translatable.
Also, I'd change the level to ERROR.

+ errhint("The supported log formats are \"stderr\""
+" and \"csvlog\".")));

I think our preferred style is not to break strings across lines like this.

+log_filepath[strcspn(log_filepath, "\n")] = '\0';

We have no existing dependency on strcspn().  It might be better not
to add one just for this feature; I suggest using strchr() instead,
which is widely used in our existing code.

-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Fri, Jan 20, 2017 at 10:14 PM, Michael Paquier
 wrote:
> On Fri, Jan 20, 2017 at 10:11 PM, Alvaro Herrera
>  wrote:
>>> diff --git a/src/backend/replication/basebackup.c 
>>> b/src/backend/replication/basebackup.c
>>
>>> @@ -148,6 +149,9 @@ static const char *excludeFiles[] =
>>>   /* Skip auto conf temporary file. */
>>>   PG_AUTOCONF_FILENAME ".tmp",
>>>
>>> + /* Skip current log file temporary file */
>>> + LOG_METAINFO_DATAFILE_TMP,
>>> +
>>
>> Sorry if this has already been answered, but why are we not also
>> skipping LOG_METAINFO_DATAFILE itself?  I can't see a scenario where
>> it's useful to have that file in a base backup, since it's very likely
>> that the log file used when the backup is restored will be different.
>
> I have done the same remark upthread, and Gilles has pointed out that
> it would be useful to get the last log file that was used by a backup.
> Including this file represents no harm as well.

Moved to CF 2017-03.
-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Fri, Jan 20, 2017 at 10:11 PM, Alvaro Herrera
 wrote:
>> diff --git a/src/backend/replication/basebackup.c 
>> b/src/backend/replication/basebackup.c
>
>> @@ -148,6 +149,9 @@ static const char *excludeFiles[] =
>>   /* Skip auto conf temporary file. */
>>   PG_AUTOCONF_FILENAME ".tmp",
>>
>> + /* Skip current log file temporary file */
>> + LOG_METAINFO_DATAFILE_TMP,
>> +
>
> Sorry if this has already been answered, but why are we not also
> skipping LOG_METAINFO_DATAFILE itself?  I can't see a scenario where
> it's useful to have that file in a base backup, since it's very likely
> that the log file used when the backup is restored will be different.

I have done the same remark upthread, and Gilles has pointed out that
it would be useful to get the last log file that was used by a backup.
Including this file represents no harm as well.
-- 
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] Patch to implement pg_current_logfile() function

[Alvaro Herrera]

> diff --git a/src/backend/replication/basebackup.c 
> b/src/backend/replication/basebackup.c

> @@ -148,6 +149,9 @@ static const char *excludeFiles[] =
>   /* Skip auto conf temporary file. */
>   PG_AUTOCONF_FILENAME ".tmp",
>  
> + /* Skip current log file temporary file */
> + LOG_METAINFO_DATAFILE_TMP,
> +

Sorry if this has already been answered, but why are we not also
skipping LOG_METAINFO_DATAFILE itself?  I can't see a scenario where
it's useful to have that file in a base backup, since it's very likely
that the log file used when the backup is restored will be different.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Fri, Jan 20, 2017 at 3:29 AM, Karl O. Pinc  wrote:
> On Thu, 19 Jan 2017 12:12:18 -0300
> Alvaro Herrera  wrote:
>
>> Karl O. Pinc wrote:
>> > On Wed, 18 Jan 2017 19:27:40 -0300
>> > Alvaro Herrera  wrote:
>>
>> > > I thought this part was odd -- I mean, why is SysLogger_Start()
>> > > being called if the collector is not enabled?  Turns out we do it
>> > > and return early if not enabled.  But not in all cases -- there
>> > > is one callsite in postmaster.c that avoids the call if the
>> > > collector is disabled. That needs to be changed if we want this
>> > > to work reliably.
>> >
>> > Is this an argument for having the current_logfiles always exist
>> > and be empty when there is no in-filesystem logfile?  It always felt
>> > to me that the code would be simpler that way.
>>
>> I don't know.  I am just saying that you need to patch postmaster.c
>> line 1726 to remove the second arm of the &&.
>
> Gilles,
>
> I'm not available just now.  Can you do this or enlist Michael?

Okay I just did it. At the same time the check for ferror is not
necessary as fgets() returns NULL on an error as well so that's dead
code. I have also removed the useless call to FreeFile().
-- 
Michael
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c77a..427980d77c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4218,6 +4218,11 @@ SELECT * FROM parent WHERE key = 2400;
   where to log
  
 
+ 
+   current_logfiles
+   and the log_destination configuration parameter
+ 
+
  
 
  
@@ -4248,6 +4253,27 @@ SELECT * FROM parent WHERE key = 2400;
  must be enabled to generate
 CSV-format log output.

+   
+When either stderr or
+csvlog are included, the file
+current_logfiles is created to record the location
+of the log file(s) currently in use by the logging collector and the
+associated logging destination. This provides a convenient way to
+find the logs currently in use by the instance. Here is an example of
+this file's content:
+
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+
+
+current_logfiles is recreated when a new log file
+is created as an effect of rotation, and
+when log_destination is reloaded.  It is removed when
+neither stderr
+nor csvlog are included
+in log_destination, and when the logging collector is
+disabled.
+   
 

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2504a466e6..5b0be26e0a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15449,6 +15449,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS 
t(ls,n);
   
 
   
+   
pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  

pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15667,6 +15674,45 @@ SET search_path TO schema , 
schema, ..

 

+pg_current_logfile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+ current_logfiles
+ and the pg_current_logfile function
+   
+
+   
+Logging
+current_logfiles file and the pg_current_logfile
+function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of the log file(s) currently in use by the logging collector.
+The path includes the  directory
+and the log file name.  Log collection must be enabled or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply, as text,
+either csvlog or stderr as the value of the
+optional parameter. The return value is NULL when the
+log format requested is not a configured
+.  The
+pg_current_logfiles reflects the contents of the
+current_logfiles file.
+   
+
+   
 pg_my_temp_schema

 
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824dfc..9865751aa5 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -36,6 +36,10 @@ these required items, the cluster configuration files
 PGDATA, although it is possible to place them elsewhere.
 
 
+
+  current_logfiles
+
+
 
 Contents of PGDATA
 
@@ -61,6 +65,12 @@ Item
 
 
 
+ current_logfiles
+ File recording the log file(s) currently written to by the logging
+  collector
+
+
+
  global
  Subdirectory containing cluster-wide tables, such as
  pg_database
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Fri, Jan 20, 2017 at 12:08 AM, Karl O. Pinc  wrote:
> Is this an argument for having the current_logfiles always exist
> and be empty when there is no in-filesystem logfile?  It always felt
> to me that the code would be simpler that way.

Well, you'll need to do something in any case if the logging_collector
is found disabled and the syslogger process is restarted. So just
removing it looked cleaner to me. I am not strongly attached to one
way of doing or the other though.
-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 19 Jan 2017 12:12:18 -0300
Alvaro Herrera  wrote:

> Karl O. Pinc wrote:
> > On Wed, 18 Jan 2017 19:27:40 -0300
> > Alvaro Herrera  wrote:  
> 
> > > I thought this part was odd -- I mean, why is SysLogger_Start()
> > > being called if the collector is not enabled?  Turns out we do it
> > > and return early if not enabled.  But not in all cases -- there
> > > is one callsite in postmaster.c that avoids the call if the
> > > collector is disabled. That needs to be changed if we want this
> > > to work reliably.  
> > 
> > Is this an argument for having the current_logfiles always exist
> > and be empty when there is no in-filesystem logfile?  It always felt
> > to me that the code would be simpler that way.  
> 
> I don't know.  I am just saying that you need to patch postmaster.c
> line 1726 to remove the second arm of the &&.

Gilles,

I'm not available just now.  Can you do this or enlist Michael?

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Alvaro Herrera]

Karl O. Pinc wrote:
> On Wed, 18 Jan 2017 19:27:40 -0300
> Alvaro Herrera  wrote:

> > I thought this part was odd -- I mean, why is SysLogger_Start() being
> > called if the collector is not enabled?  Turns out we do it and return
> > early if not enabled.  But not in all cases -- there is one callsite
> > in postmaster.c that avoids the call if the collector is disabled.
> > That needs to be changed if we want this to work reliably.
> 
> Is this an argument for having the current_logfiles always exist
> and be empty when there is no in-filesystem logfile?  It always felt
> to me that the code would be simpler that way.

I don't know.  I am just saying that you need to patch postmaster.c line
1726 to remove the second arm of the &&.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 19:27:40 -0300
Alvaro Herrera  wrote:

> Karl O. Pinc wrote:
> 
> > @@ -511,10 +519,16 @@ int
> >  SysLogger_Start(void)
> >  {
> > pid_t   sysloggerPid;
> > -   char   *filename;
> >  
> > +   /*
> > +* Logging collector is not enabled. We don't know where
> > messages are
> > +* logged.  Remove outdated file holding the current log
> > filenames.
> > +*/
> > if (!Logging_collector)
> > +   {
> > +   unlink(LOG_METAINFO_DATAFILE);
> > return 0;
> > +   }  
> 
> I thought this part was odd -- I mean, why is SysLogger_Start() being
> called if the collector is not enabled?  Turns out we do it and return
> early if not enabled.  But not in all cases -- there is one callsite
> in postmaster.c that avoids the call if the collector is disabled.
> That needs to be changed if we want this to work reliably.

Is this an argument for having the current_logfiles always exist
and be empty when there is no in-filesystem logfile?  It always felt
to me that the code would be simpler that way.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 19 Jan 2017 16:59:10 +0900
Michael Paquier  wrote:

> On Thu, Jan 19, 2017 at 7:27 AM, Alvaro Herrera
>  wrote:

> > I don't think the "abstract names" stuff is an improvement (just
> > look at the quoting mess in ConfigureNamesString).  I think we
> > should do without that; at least as part of this patch.  If you
> > think there's code that can get better because of the idea, let's
> > see it.  

Fair enough.

FWIW, when I first wrote the "abstract names" stuff there were another
half dozen or so occurrences of the constants.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Thu, Jan 19, 2017 at 7:27 AM, Alvaro Herrera
 wrote:
> I thought this part was odd -- I mean, why is SysLogger_Start() being
> called if the collector is not enabled?  Turns out we do it and return
> early if not enabled.  But not in all cases -- there is one callsite in
> postmaster.c that avoids the call if the collector is disabled.  That
> needs to be changed if we want this to work reliably.

Indeed. And actually it is fine to remove the call to FreeFile() in
the error code path of pg_current_logfile().

> I don't think the "abstract names" stuff is an improvement (just look at
> the quoting mess in ConfigureNamesString).  I think we should do without
> that; at least as part of this patch.  If you think there's code that
> can get better because of the idea, let's see it.

Agreed.
-- 
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] Patch to implement pg_current_logfile() function

[Alvaro Herrera]

Karl O. Pinc wrote:

> @@ -511,10 +519,16 @@ int
>  SysLogger_Start(void)
>  {
>   pid_t   sysloggerPid;
> - char   *filename;
>  
> + /*
> +  * Logging collector is not enabled. We don't know where messages are
> +  * logged.  Remove outdated file holding the current log filenames.
> +  */
>   if (!Logging_collector)
> + {
> + unlink(LOG_METAINFO_DATAFILE);
>   return 0;
> + }

I thought this part was odd -- I mean, why is SysLogger_Start() being
called if the collector is not enabled?  Turns out we do it and return
early if not enabled.  But not in all cases -- there is one callsite in
postmaster.c that avoids the call if the collector is disabled.  That
needs to be changed if we want this to work reliably.

I don't think the "abstract names" stuff is an improvement (just look at
the quoting mess in ConfigureNamesString).  I think we should do without
that; at least as part of this patch.  If you think there's code that
can get better because of the idea, let's see it.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Thu, Jan 19, 2017 at 6:56 AM, Karl O. Pinc  wrote:
> On Wed, 18 Jan 2017 15:08:09 -0600
> "Karl O. Pinc"  wrote:
>
>> I would like to see index entries for "current_logfiles"
>> so this stuff is findable.
>
> Attached is a v27 of the patch.
>
> I polished some of the sentences in the documentation
> and added index entries.
>
> Also attached are a series of 2 patches to apply on top
> of v27 which make symbols out of hardcoded constants.

+ 
+   current_logfiles
+   and the log_destination configuration parameter
+ 
I have been wondering about this portion a bit, and actually that's pretty nice.

So patch is marked as ready for committer, the version proposed here
not using SRFs per Gilles' input on the matter.
-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Thu, Jan 19, 2017 at 6:08 AM, Karl O. Pinc  wrote:
> On Wed, 18 Jan 2017 15:52:36 -0500
> Robert Haas  wrote:
>
>> On Wed, Jan 18, 2017 at 12:08 PM, Karl O. Pinc  wrote:
>> > Seems to me that the file format should
>> > be documented if there's any intention that the end user
>> > look at or otherwise use the file's content.
>> >
>> > It's fine with me if the content of current_logfiles
>> > is supposed to be internal to PG and not exposed
>> > to the end user.  I'm writing to make sure that
>> > this is a considered decision.
>>
>> On the whole, documenting it seems better than documenting it,
>> provided there's a logical place to include it in the existing
>> documentation.
>>
>> But, anyway, Michael shouldn't remove it without some explanation or
>> discussion.
>
> He explained that where it was looked clunky, it being
> inside a table that otherwise has rows that are not tall.
>
> And, it looks like I'm wrong.  The format is documented
> by way of an example in section 19.8.1. Where To Log
> under log_destination (string).
>
> Sorry for the bother.

Er, well. I kept the same detail verbosity in the docs...

> I would like to see index entries for "current_logfiles"
> so this stuff is findable.

Why not.
-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 15:08:09 -0600
"Karl O. Pinc"  wrote:

> I would like to see index entries for "current_logfiles"
> so this stuff is findable.

Attached is a v27 of the patch.

I polished some of the sentences in the documentation
and added index entries.

Also attached are a series of 2 patches to apply on top
of v27 which make symbols out of hardcoded constants.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c..427980d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4218,6 +4218,11 @@ SELECT * FROM parent WHERE key = 2400;
   where to log
  
 
+ 
+   current_logfiles
+   and the log_destination configuration parameter
+ 
+
  
 
  
@@ -4248,6 +4253,27 @@ SELECT * FROM parent WHERE key = 2400;
  must be enabled to generate
 CSV-format log output.

+   
+When either stderr or
+csvlog are included, the file
+current_logfiles is created to record the location
+of the log file(s) currently in use by the logging collector and the
+associated logging destination. This provides a convenient way to
+find the logs currently in use by the instance. Here is an example of
+this file's content:
+
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+
+
+current_logfiles is recreated when a new log file
+is created as an effect of rotation, and
+when log_destination is reloaded.  It is removed when
+neither stderr
+nor csvlog are included
+in log_destination, and when the logging collector is
+disabled.
+   
 

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2504a46..5b0be26 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15449,6 +15449,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15667,6 +15674,45 @@ SET search_path TO schema , schema, ..

 

+pg_current_logfile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+ current_logfiles
+ and the pg_current_logfile function
+   
+
+   
+Logging
+current_logfiles file and the pg_current_logfile
+function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of the log file(s) currently in use by the logging collector.
+The path includes the  directory
+and the log file name.  Log collection must be enabled or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply, as text,
+either csvlog or stderr as the value of the
+optional parameter. The return value is NULL when the
+log format requested is not a configured
+.  The
+pg_current_logfiles reflects the contents of the
+current_logfiles file.
+   
+
+   
 pg_my_temp_schema

 
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..9865751 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -36,6 +36,10 @@ these required items, the cluster configuration files
 PGDATA, although it is possible to place them elsewhere.
 
 
+
+  current_logfiles
+
+
 
 Contents of PGDATA
 
@@ -61,6 +65,12 @@ Item
 
 
 
+ current_logfiles
+ File recording the log file(s) currently written to by the logging
+  collector
+
+
+
  global
  Subdirectory containing cluster-wide tables, such as
  pg_database
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 07f291b..cbeeab8 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1055,6 +1055,7 @@ REVOKE EXECUTE ON FUNCTION pg_xlog_replay_pause() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_xlog_replay_resume() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_current_logfile() FROM public;
 
 REVOKE EXECUTE ON FUNCTION pg_stat_reset() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_shared(text) FROM public;
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 13a0301..e6899c6 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -146,6 +146,7 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 15:52:36 -0500
Robert Haas  wrote:

> On Wed, Jan 18, 2017 at 12:08 PM, Karl O. Pinc  wrote:
> > Seems to me that the file format should
> > be documented if there's any intention that the end user
> > look at or otherwise use the file's content.
> >
> > It's fine with me if the content of current_logfiles
> > is supposed to be internal to PG and not exposed
> > to the end user.  I'm writing to make sure that
> > this is a considered decision.  
> 
> On the whole, documenting it seems better than documenting it,
> provided there's a logical place to include it in the existing
> documentation.
> 
> But, anyway, Michael shouldn't remove it without some explanation or
> discussion.

He explained that where it was looked clunky, it being
inside a table that otherwise has rows that are not tall.

And, it looks like I'm wrong.  The format is documented
by way of an example in section 19.8.1. Where To Log
under log_destination (string).

Sorry for the bother.

I would like to see index entries for "current_logfiles"
so this stuff is findable.

Regards.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Wed, Jan 18, 2017 at 12:08 PM, Karl O. Pinc  wrote:
> Seems to me that the file format should
> be documented if there's any intention that the end user
> look at or otherwise use the file's content.
>
> It's fine with me if the content of current_logfiles
> is supposed to be internal to PG and not exposed
> to the end user.  I'm writing to make sure that
> this is a considered decision.

On the whole, documenting it seems better than documenting it,
provided there's a logical place to include it in the existing
documentation.

But, anyway, Michael shouldn't remove it without some explanation or discussion.

-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Wed, Jan 18, 2017 at 12:56 PM, Karl O. Pinc  wrote:
> If it were me I'd have the documentation mention
> that the pg_current_logfiles() result is
> supplied on a "best effort" basis and under
> rare conditions may be outdated.   The
> sentence in the pg_current_logfles() docs
> which reads "pg_current_logfiles reflects the contents
> of the file current_logfiles." now carries little
> meaning because the current_logfiles docs no
> longer mention that the file content may be outdated.

Generally that's going to be true of any function or SQL statement
that returns data which can change.  The only way it isn't true in
some particular case is if a function has some kind of snapshot
semantics or returns with a lock on the underlying object held.  So it
doesn't seem particularly noteworthy here.

-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 11:08:23 -0600
"Karl O. Pinc"  wrote:

> Hi Micheal,
> 
> On Wed, 18 Jan 2017 10:26:43 -0600
> "Karl O. Pinc"  wrote:
> 
> > > v26 patch attached which fixes this.
> 
> I was glancing over the changes to the documentation
> you made between the v22 and v25

If it were me I'd have the documentation mention
that the pg_current_logfiles() result is
supplied on a "best effort" basis and under
rare conditions may be outdated.   The
sentence in the pg_current_logfles() docs
which reads "pg_current_logfiles reflects the contents 
of the file current_logfiles." now carries little
meaning because the current_logfiles docs no
longer mention that the file content may be outdated.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Micheal,

On Wed, 18 Jan 2017 10:26:43 -0600
"Karl O. Pinc"  wrote:

> > v26 patch attached which fixes this.  

I was glancing over the changes to the documentation
you made between the v22 and v25 and from looking at the diffs 
it seems the format of the current_logfiles file content is no longer
documented.  Seems to me that the file format should
be documented if there's any intention that the end user
look at or otherwise use the file's content.

It's fine with me if the content of current_logfiles
is supposed to be internal to PG and not exposed
to the end user.  I'm writing to make sure that
this is a considered decision.

I also see that all the index entries in the docs
to the current_logfiles file were removed.  (And
I think maybe some index entries to various places
where pg_current_logfiles() was mentioned in the docs.)
I see no reason why we shouldn't have more rather
than fewer index entries in the docs.

I haven't made any sort of though review of your
changes to the docs but this jumped out at me
and I wanted to comment before the patches got
passed on to the committers.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 10:11:20 -0600
"Karl O. Pinc"  wrote:

> You must write
>   errcode(ERRCODE_INTERNAL_ERROR)
> instead of just
>   ERRCODE_INTERNAL_ERROR
> 
> If you don't you get back 01000 for the error code.

> v26 patch attached which fixes this.

Attached are revised versions of the 2 (optional) patches which
make hardcoded constants into symbols to apply on
top of the v26 patch.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v26.diff.abstract_guc_part1
Description: Binary data


patch_pg_current_logfile-v26.diff.abstract_guc_part2
Description: Binary data

-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 13:26:46 +0900
Michael Paquier  wrote:

> On Wed, Jan 18, 2017 at 11:36 AM, Karl O. Pinc  wrote:
> > On Wed, 18 Jan 2017 11:08:25 +0900
> > Michael Paquier  wrote:
> >  
> >> Our ideas rather map here, ERRCODE_INTERNAL_ERROR would be adapted
> >> for this situation. Do any of you want to give it a shot or should
> >> I?  

> What do you think about that?
> +   log_filepath = strchr(lbuffer, ' ');
> +   if (log_filepath == NULL)
> +   {
> +   /*
> +* Corrupted data found, return NULL to the caller and
> still
> +* inform him on the situation.
> +*/
> +   ereport(WARNING,
> +   (ERRCODE_INTERNAL_ERROR,
> +errmsg("corrupted data found in \"%s\"",
> +   LOG_METAINFO_DATAFILE)));
> +   break;
> +   }

You must write

  errcode(ERRCODE_INTERNAL_ERROR)

instead of just
  
  ERRCODE_INTERNAL_ERROR

If you don't you get back 01000 for the error code.
Test by using psql with '\set VERBOSITY verbose'.

v26 patch attached which fixes this.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c..3188496 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4248,6 +4248,25 @@ SELECT * FROM parent WHERE key = 2400;
  must be enabled to generate
 CSV-format log output.

+   
+When either stderr or
+csvlog are included, the file
+current_logfiles gets created and records the location
+of the log file(s) currently in use by the logging collector and the
+associated logging destination. This provides a convenient way to
+find the logs currently in use by the instance. Here is an example of
+contents of this file:
+
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+
+Note that this file gets updated when a new log file is created
+as an effect of rotation or when log_destination is
+reloaded.  current_logfiles is removed when
+stderr and csvlog
+are not included in log_destination or when the
+logging collector is disabled.
+   
 

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index eb1b698..8524dff 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15659,6 +15666,34 @@ SET search_path TO schema , schema, ..

 

+pg_current_logfile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of the log file(s) currently in use by the logging collector.
+The path includes the  directory
+and the log file name.  Log collection must be enabled or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply, as text,
+either csvlog or stderr as the value of the
+optional parameter. The return value is NULL when the
+log format requested is not a configured
+.
+pg_current_logfiles reflects the contents of the
+file current_logfiles.
+   
+
+   
 pg_my_temp_schema

 
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..388ed34 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -61,6 +61,12 @@ Item
 
 
 
+ current_logfiles
+ File recording the log file(s) currently written to by the logging
+  collector
+
+
+
  global
  Subdirectory containing cluster-wide tables, such as
  pg_database
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 31aade1..1d7f68b 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1055,6 +1055,7 @@ REVOKE EXECUTE ON FUNCTION pg_xlog_replay_pause() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_xlog_replay_resume() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_current_logfile() FROM public;
 
 REVOKE EXECUTE ON FUNCTION pg_stat_reset() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_shared(text) FROM 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Wed, Jan 18, 2017 at 11:36 AM, Karl O. Pinc  wrote:
> On Wed, 18 Jan 2017 11:08:25 +0900
> Michael Paquier  wrote:
>
>> Our ideas rather map here, ERRCODE_INTERNAL_ERROR would be adapted for
>> this situation. Do any of you want to give it a shot or should I?
>
> You're welcome to it.

What do you think about that?
+   log_filepath = strchr(lbuffer, ' ');
+   if (log_filepath == NULL)
+   {
+   /*
+* Corrupted data found, return NULL to the caller and still
+* inform him on the situation.
+*/
+   ereport(WARNING,
+   (ERRCODE_INTERNAL_ERROR,
+errmsg("corrupted data found in \"%s\"",
+   LOG_METAINFO_DATAFILE)));
+   break;
+   }

Recent commits 352a24a1 (not my fault) and e7b020f7 (my fault) are
creating conflicts, so a rebase was as well welcome.
-- 
Michael
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c77a..3188496bc2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4248,6 +4248,25 @@ SELECT * FROM parent WHERE key = 2400;
  must be enabled to generate
 CSV-format log output.

+   
+When either stderr or
+csvlog are included, the file
+current_logfiles gets created and records the location
+of the log file(s) currently in use by the logging collector and the
+associated logging destination. This provides a convenient way to
+find the logs currently in use by the instance. Here is an example of
+contents of this file:
+
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+
+Note that this file gets updated when a new log file is created
+as an effect of rotation or when log_destination is
+reloaded.  current_logfiles is removed when
+stderr and csvlog
+are not included in log_destination or when the
+logging collector is disabled.
+   
 

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e31868ba..c756fbe066 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS 
t(ls,n);
   
 
   
+   
pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  

pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15659,6 +15666,34 @@ SET search_path TO schema , 
schema, ..

 

+pg_current_logfile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of the log file(s) currently in use by the logging collector.
+The path includes the  directory
+and the log file name.  Log collection must be enabled or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply, as text,
+either csvlog or stderr as the value of the
+optional parameter. The return value is NULL when the
+log format requested is not a configured
+.
+pg_current_logfiles reflects the contents of the
+file current_logfiles.
+   
+
+   
 pg_my_temp_schema

 
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824dfc..388ed344ee 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -61,6 +61,12 @@ Item
 
 
 
+ current_logfiles
+ File recording the log file(s) currently written to by the logging
+  collector
+
+
+
  global
  Subdirectory containing cluster-wide tables, such as
  pg_database
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 31aade102b..1d7f68b8a4 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1055,6 +1055,7 @@ REVOKE EXECUTE ON FUNCTION pg_xlog_replay_pause() FROM 
public;
 REVOKE EXECUTE ON FUNCTION pg_xlog_replay_resume() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_current_logfile() FROM public;
 
 REVOKE EXECUTE ON FUNCTION pg_stat_reset() FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stat_reset_shared(text) FROM public;
diff --git a/src/backend/postmaster/syslogger.c 
b/src/backend/postmaster/syslogger.c
index 13a03014eb..e6899c6246 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -146,6 +146,7 @@ static char *logfile_getname(pg_time_t timestamp, const 
char *suffix);
 static 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 18 Jan 2017 11:08:25 +0900
Michael Paquier  wrote:

> Our ideas rather map here, ERRCODE_INTERNAL_ERROR would be adapted for
> this situation. Do any of you want to give it a shot or should I?

You're welcome to it.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Wed, Jan 18, 2017 at 7:36 AM, Karl O. Pinc  wrote:
> Maybe.  It's not user-supplied data that's corrupted but it is
> PG generated data which is generated for and supplied to the user.
> I just looked at all uses of XX001 and it is true that they all
> involve corruption of user-supplied data.
>
> If you don't want to use XX001 use XX000.  It does not seem worth
> making a new error code for just this one case.

Our ideas rather map here, ERRCODE_INTERNAL_ERROR would be adapted for
this situation. Do any of you want to give it a shot or should I?
-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Wed, Jan 18, 2017 at 3:06 AM, Gilles Darold  wrote:
> This have already been discuted previously in this thread, one of my
> previous patch version has implemented this behavior but we decide that
> what we really want is to be able to use the function using the
> following simple query:
>
> SELECT pg_read_file(pg_current_logfiles());
>
> and not something like
>
> SELECT pg_read_file(SELECT file FROM pg_current_logfiles() LIMIT 1);
> or
> SELECT pg_read_file(SELECT file FROM pg_current_logfiles() WHERE
> method='stderr');
>
> You can also obtain the "kind" of output from the SRF function using:
>
> SELECT pg_read_file('current_logfiles');

I don't really understand this argument as you can do that as well:
SELECT pg_read_file(file) FROM pg_current_logfiles WHERE method = 'stderr';

> I'm not against adding a warning or error message here even if it may
> never occurs, but we need a new error code as it seems to me that no
> actual error code can be used.

ERRCODE_INTERNAL_ERROR, no?
-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 17 Jan 2017 23:00:43 +0100
Gilles Darold  wrote:

> Le 17/01/2017 à 19:58, Karl O. Pinc a écrit :
> > On Tue, 17 Jan 2017 19:06:22 +0100
> > Gilles Darold  wrote:
> >  
> >> Le 17/01/2017 à 03:22, Michael Paquier a écrit :  
> >>> On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc 
> >>> wrote:
>  On January 15, 2017 11:47:51 PM CST, Michael Paquier
>   wrote:
> >  
> > Also, I would rather see an ERROR returned to the user in case
> > of bad data in current_logfiles, I did not change that either as
> > that's the original intention of Gilles.
> >> I'm not against adding a warning or error message here even if it
> >> may never occurs, but we need a new error code as it seems to me
> >> that no actual error code can be used.
> > Seems to me the XX001 "data_corrupted" sub-category of
> > XX000 "internal_error" is appropriate.  
> 
> I don't think so, this file doesn't contain any data and we must not
> report such error in this case. Somethink like "bad/unknow file
> format" would be better.

Maybe.  It's not user-supplied data that's corrupted but it is
PG generated data which is generated for and supplied to the user.
I just looked at all uses of XX001 and it is true that they all
involve corruption of user-supplied data.

If you don't want to use XX001 use XX000.  It does not seem worth
making a new error code for just this one case.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 17/01/2017 à 19:58, Karl O. Pinc a écrit :
> On Tue, 17 Jan 2017 19:06:22 +0100
> Gilles Darold  wrote:
>
>> Le 17/01/2017 à 03:22, Michael Paquier a écrit :
>>> On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc 
>>> wrote:  
 On January 15, 2017 11:47:51 PM CST, Michael Paquier
  wrote:  
>
> Also, I would rather see an ERROR returned to the user in case of
> bad data in current_logfiles, I did not change that either as
> that's the original intention of Gilles.  
>> I'm not against adding a warning or error message here even if it may
>> never occurs, but we need a new error code as it seems to me that no
>> actual error code can be used.  
> Seems to me the XX001 "data_corrupted" sub-category of
> XX000 "internal_error" is appropriate.

I don't think so, this file doesn't contain any data and we must not
report such error in this case. Somethink like "bad/unknow file format"
would be better.

 

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org



-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 17 Jan 2017 19:06:22 +0100
Gilles Darold  wrote:

> Le 17/01/2017 à 03:22, Michael Paquier a écrit :
> > On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc 
> > wrote:  
> >> On January 15, 2017 11:47:51 PM CST, Michael Paquier
> >>  wrote:  


> >>> Also, I would rather see an ERROR returned to the user in case of
> >>> bad data in current_logfiles, I did not change that either as
> >>> that's the original intention of Gilles.  

> I'm not against adding a warning or error message here even if it may
> never occurs, but we need a new error code as it seems to me that no
> actual error code can be used.  

Seems to me the XX001 "data_corrupted" sub-category of
XX000 "internal_error" is appropriate.

https://www.postgresql.org/docs/devel/static/errcodes-appendix.html

As a dbadmin/sysadm I'd defiantly like to see something in the log if
you're going to raise anything user-side.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 17/01/2017 à 03:22, Michael Paquier a écrit :
> On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc  wrote:
>> On January 15, 2017 11:47:51 PM CST, Michael Paquier 
>>  wrote:
>>> On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc  wrote:
>>> With all those issues fixed, I finish with the attached, that I am
>>> fine to pass down to a committer. I still think that this should be
>>> only one function using a SRF with rows shaped as (type, path) for
>>> simplicity, but as I am visibly outnumbered I won't insist further.
>> That also makes a certain amount of sense to me but I can't say I have 
>> thought deeply about it. Haven't paid any attention to this issue and am 
>> fine letting things move forward as is.
> Gilles, what's your opinion here? As the author that's your call at
> the end. What the point here is would be to change
> pg_current_logfiles() to something like that:
> =# SELECT * FROM pg_current_logfiles();
>  method | file
> |
>  stderr | pg_log/postgresql.log
>  csvlog | pg_log/postgresql.csv
> And using this SRF users can filter the method with a WHERE clause.
> And as a result the 1-arg version is removed. No rows are returned if
> current_logfiles does not exist. I don't think there is much need for
> a system view either.

This have already been discuted previoulsy in this thread, one of my
previous patch version has implemented this behavior but we decide that
what we really want is to be able to use the function using the
following simple query:

SELECT pg_read_file(pg_current_logfiles());

and not something like

SELECT pg_read_file(SELECT file FROM pg_current_logfiles() LIMIT 1);
or
SELECT pg_read_file(SELECT file FROM pg_current_logfiles() WHERE
method='stderr');

You can also obtain the "kind" of output from the SRF function using:

SELECT pg_read_file('current_logfiles');


>>> Also, I would rather see an ERROR returned to the user in case of bad
>>> data in current_logfiles, I did not change that either as that's the
>>> original intention of Gilles.
>> I could be wrong but I seem to recall that Robert recommended against an 
>> error message. If there is bad data then something is really wrong, up to 
>> some sort of an attack on the back end. Because this sort of thing simply 
>> shouldn't happen it's enough in my opinion to guard against buffer overruns 
>> etc and just get on with life. If something goes unexpectedly and badly 
>> wrong with internal data structures in general there would have to be all 
>> kinds of additional code to cover every possible problem and that doesn't 
>> seem to make sense.
> Hm... OK. At the same time not throwing at least a WARNING is
> confusing, because a NULL result is returned as well even if the input
> method is incorrect and even if the method is correct but it is not
> present in current_logfiles. As the user is thought as a trusted user
> as it has access to this function, I would think that being verbose on
> the error handling, or at least warnings, would make things easier to
> analyze.

I'm not against adding a warning or error message here even if it may
never occurs, but we need a new error code as it seems to me that no
actual error code can be used.  


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org



-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 17 Jan 2017 07:58:43 -0600
"Karl O. Pinc"  wrote:

> On Tue, 17 Jan 2017 11:22:45 +0900
> Michael Paquier  wrote:
> 
> > On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc 
> > wrote:  
> 
> > >>Also, I would rather see an ERROR returned to the user in case of
> > >>bad data in current_logfiles,

> > 
> > Hm... OK. At the same time not throwing at least a WARNING is
> > confusing

What I find a little disconcerting is that there'd be nothing in the
logs.



Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Tue, 17 Jan 2017 11:22:45 +0900
Michael Paquier  wrote:

> On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc  wrote:

> >>Also, I would rather see an ERROR returned to the user in case of
> >>bad data in current_logfiles, I did not change that either as
> >>that's the original intention of Gilles.  
> >
> > I could be wrong but I seem to recall that Robert recommended
> > against an error message. If there is bad data then something is
> > really wrong, up to some sort of an attack on the back end. Because
> > this sort of thing simply shouldn't happen it's enough in my
> > opinion to guard against buffer overruns etc and just get on with
> > life. If something goes unexpectedly and badly wrong with internal
> > data structures in general there would have to be all kinds of
> > additional code to cover every possible problem and that doesn't
> > seem to make sense.  
> 
> Hm... OK. At the same time not throwing at least a WARNING is
> confusing, because a NULL result is returned as well even if the input
> method is incorrect and even if the method is correct but it is not
> present in current_logfiles. As the user is thought as a trusted user
> as it has access to this function, I would think that being verbose on
> the error handling, or at least warnings, would make things easier to
> analyze.

These are all valid points.

In the context of reliability it's worth noting that
pg_current_logfile() results are inherently unreliable.
If the OS returns ENFILE or EMFILE when opening the current_logfiles
file (but not previously) the content, and so pg_current_logfile()
results, will be outdated until the next logfile rotation.
You wouldn't expect this to happen, but it might.  Which
is similar to the situation where the content of the current_logfiles
is corrupted; very unexpected but possible.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Tue, Jan 17, 2017 at 2:21 AM, Karl O. Pinc  wrote:
> On January 15, 2017 11:47:51 PM CST, Michael Paquier 
>  wrote:
>>On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc  wrote:
>>With all those issues fixed, I finish with the attached, that I am
>>fine to pass down to a committer. I still think that this should be
>>only one function using a SRF with rows shaped as (type, path) for
>>simplicity, but as I am visibly outnumbered I won't insist further.
>
> That also makes a certain amount of sense to me but I can't say I have 
> thought deeply about it. Haven't paid any attention to this issue and am fine 
> letting things move forward as is.

Gilles, what's your opinion here? As the author that's your call at
the end. What the point here is would be to change
pg_current_logfiles() to something like that:
=# SELECT * FROM pg_current_logfiles();
 method | file
|
 stderr | pg_log/postgresql.log
 csvlog | pg_log/postgresql.csv
And using this SRF users can filter the method with a WHERE clause.
And as a result the 1-arg version is removed. No rows are returned if
current_logfiles does not exist. I don't think there is much need for
a system view either.

>>Also, I would rather see an ERROR returned to the user in case of bad
>>data in current_logfiles, I did not change that either as that's the
>>original intention of Gilles.
>
> I could be wrong but I seem to recall that Robert recommended against an 
> error message. If there is bad data then something is really wrong, up to 
> some sort of an attack on the back end. Because this sort of thing simply 
> shouldn't happen it's enough in my opinion to guard against buffer overruns 
> etc and just get on with life. If something goes unexpectedly and badly wrong 
> with internal data structures in general there would have to be all kinds of 
> additional code to cover every possible problem and that doesn't seem to make 
> sense.

Hm... OK. At the same time not throwing at least a WARNING is
confusing, because a NULL result is returned as well even if the input
method is incorrect and even if the method is correct but it is not
present in current_logfiles. As the user is thought as a trusted user
as it has access to this function, I would think that being verbose on
the error handling, or at least warnings, would make things easier to
analyze.

> Sorry about the previous email with empty content. My email client got away 
> from me.

No problem. That happens.
-- 
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] Patch to implement pg_current_logfile() function

[Alvaro Herrera]

Karl O. Pinc wrote:

> I do have a question here regards code formatting.
> The patch now contains:
> 
> if (log_filepath == NULL)
> {
> /* Bad data. Avoid segfaults etc. and return NULL to caller. */
> break;
> }
> 
> I'm not sure how this fits in with PG coding style,
> whether the {} should be removed or what.  I've looked
> around and can't find an example of an if with a single
> line then block and a comment.  Maybe this means that
> I shouldn't be doing this, but if I put the comment above
> the if then it will "run into" the comment block which
> immediately precedes the above code which describes
> a larger body of code.  So perhaps someone should look
> at this and tell me how to improve it.

I think this style is good.  Following the style guide to the letter
would lead to remove the braces and keep the comment where it is;
pgindent will correctly keep at its current indentation.  We do use that
style in a couple of places.  It looks a bit clunky.  In most places we
do keep those braces, for readability and future-proofing in case
somebody inadvertently introduces another statement to the "block".  We
don't automatically remove braces anyway.  (We used to do that, but
stopped a few years ago shortly after introducing PG_TRY).

Putting the comment outside (above) the "if" would be wrong, too; you'd
have to rephrase the comment in a conditional tense.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On January 15, 2017 11:47:51 PM CST, Michael Paquier 
 wrote:
>On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc  wrote:
>
>
>> Attached also are 2 patches which abstract some hardcoded
>> constants into symbols.  Whether to apply them is a matter
>> of style and left to the maintainers, which is why these
>> are separate patches.
>
>Making the strings csvlog, stderr and eventlog variables? Why not
>because the patch presented here uses them in new places. Now it is
>not like it is a huge maintenance burden to keep them as strings, so I
>would personally not bother much.

To my mind it's a matter readability. It is useful to be able to search for or 
identify quickly when reading, e. g., all the places where the keyword stderr 
relates to output log destination and not some other more common use. The 
downside is it is more code...


>OK. I have done a round of hands-on review on this patch, finishing
>with the attached. In the things I have done:

Thank you.

>With all those issues fixed, I finish with the attached, that I am
>fine to pass down to a committer. I still think that this should be
>only one function using a SRF with rows shaped as (type, path) for
>simplicity, but as I am visibly outnumbered I won't insist further.

That also makes a certain amount of sense to me but I can't say I have thought 
deeply about it. Haven't paid any attention to this issue and am fine letting 
things move forward as is.

>Also, I would rather see an ERROR returned to the user in case of bad
>data in current_logfiles, I did not change that either as that's the
>original intention of Gilles.

I could be wrong but I seem to recall that Robert recommended against an error 
message. If there is bad data then something is really wrong, up to some sort 
of an attack on the back end. Because this sort of thing simply shouldn't 
happen it's enough in my opinion to guard against buffer overruns etc and just 
get on with life. If something goes unexpectedly and badly wrong with internal 
data structures in general there would have to be all kinds of additional code 
to cover every possible problem and that doesn't seem to make sense.

Sorry about the previous email with empty content. My email client got away 
from me.


Karl 
Free  Software:  "You don't pay back you pay forward."
-- Robert A. Heinlein



-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On January 15, 2017 11:47:51 PM CST, Michael Paquier 
 wrote:
>On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc  wrote:
>> I do have a question here regards code formatting.
>> The patch now contains:
>>
>> if (log_filepath == NULL)
>> {
>> /* Bad data. Avoid segfaults etc. and return NULL to caller.
>*/
>> break;
>> }
>>
>> I'm not sure how this fits in with PG coding style,
>> whether the {} should be removed or what.  I've looked
>> around and can't find an example of an if with a single
>> line then block and a comment.  Maybe this means that
>> I shouldn't be doing this, but if I put the comment above
>> the if then it will "run into" the comment block which
>> immediately precedes the above code which describes
>> a larger body of code.  So perhaps someone should look
>> at this and tell me how to improve it.
>
>Including brackets in this case makes a more readable code. That's the
>pattern respected the most in PG code. See for example
>XLogInsertRecord():
>else
>{
>/*
>  * This was an xlog-switch record, but the current insert location was
>  * already exactly at the beginning of a segment, so there was no need
> * to do anything.
> */
>}
>
>> Attached also are 2 patches which abstract some hardcoded
>> constants into symbols.  Whether to apply them is a matter
>> of style and left to the maintainers, which is why these
>> are separate patches.
>
>Making the strings csvlog, stderr and eventlog variables? Why not
>because the patch presented here uses them in new places. Now it is
>not like it is a huge maintenance burden to keep them as strings, so I
>would personally not bother much.
>
>> The first patch changes only code on the master
>> branch, the 2nd patch changes the new code.
>
>Thanks for keeping things separated.
>
>> I have not looked further at the patch or checked
>> to see that all changes previously recommended have been
>> made.  Michael, if you're confident that everything is good
>> go ahead and move the patchs forward to the maintainers.  Otherwise
>> please let me know and I'll see about finding time
>> for further review.
>
>OK. I have done a round of hands-on review on this patch, finishing
>with the attached. In the things I have done:
>- Elimination of useless noise diff
>- Fixes some typos (logile, log file log, etc.)
>- Adjusted docs.
>- Docs were overdoing in storage.sgml. Let's keep the description
>simple.
>- Having a paragraph at the beginning of "Error Reporting and Logging"
>in config.sgml does not look like a good idea to me. As the updates of
>current_logfiles is associated with log_destination I'd rather see
>this part added in the description of the GUC.
>- Missed a (void) in the definition of update_metainfo_datafile().
>- Moved update_metainfo_datafile() before the signal handler routines
>in syslogger.c for clarity.
>- The last "return" is useless in update_metainfo_datafile().
>- In pg_current_logfile(), it is useless to call PG_RETURN_NULL after
>emitting an ERROR message.
>- Two calls to FreeFile(fd) have been forgotten in
>pg_current_logfile(). In one case, errno needs to be saved beforehand
>to be sure that the correct error string is generated for the user.
>- A portion of 010_pg_basebackup.pl was not updated.
>- Incorrect header ordering in basebackup.c.
>- Decoding of CR and LF characters seem to work fine when
>log_file_name include some.
>
>With all those issues fixed, I finish with the attached, that I am
>fine to pass down to a committer. I still think that this should be
>only one function using a SRF with rows shaped as (type, path) for
>simplicity, but as I am visibly outnumbered I won't insist further.
>Also, I would rather see an ERROR returned to the user in case of bad
>data in current_logfiles, I did not change that either as that's the
>original intention of Gilles.
>-- 
>Michael


Karl 
Free  Software:  "You don't pay back you pay forward."
-- Robert A. Heinlein

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Mon, Jan 16, 2017 at 1:14 PM, Karl O. Pinc  wrote:
> I do have a question here regards code formatting.
> The patch now contains:
>
> if (log_filepath == NULL)
> {
> /* Bad data. Avoid segfaults etc. and return NULL to caller. */
> break;
> }
>
> I'm not sure how this fits in with PG coding style,
> whether the {} should be removed or what.  I've looked
> around and can't find an example of an if with a single
> line then block and a comment.  Maybe this means that
> I shouldn't be doing this, but if I put the comment above
> the if then it will "run into" the comment block which
> immediately precedes the above code which describes
> a larger body of code.  So perhaps someone should look
> at this and tell me how to improve it.

Including brackets in this case makes a more readable code. That's the
pattern respected the most in PG code. See for example
XLogInsertRecord():
else
{
/*
 * This was an xlog-switch record, but the current insert location was
 * already exactly at the beginning of a segment, so there was no need
 * to do anything.
 */
}

> Attached also are 2 patches which abstract some hardcoded
> constants into symbols.  Whether to apply them is a matter
> of style and left to the maintainers, which is why these
> are separate patches.

Making the strings csvlog, stderr and eventlog variables? Why not
because the patch presented here uses them in new places. Now it is
not like it is a huge maintenance burden to keep them as strings, so I
would personally not bother much.

> The first patch changes only code on the master
> branch, the 2nd patch changes the new code.

Thanks for keeping things separated.

> I have not looked further at the patch or checked
> to see that all changes previously recommended have been
> made.  Michael, if you're confident that everything is good
> go ahead and move the patchs forward to the maintainers.  Otherwise
> please let me know and I'll see about finding time
> for further review.

OK. I have done a round of hands-on review on this patch, finishing
with the attached. In the things I have done:
- Elimination of useless noise diff
- Fixes some typos (logile, log file log, etc.)
- Adjusted docs.
- Docs were overdoing in storage.sgml. Let's keep the description simple.
- Having a paragraph at the beginning of "Error Reporting and Logging"
in config.sgml does not look like a good idea to me. As the updates of
current_logfiles is associated with log_destination I'd rather see
this part added in the description of the GUC.
- Missed a (void) in the definition of update_metainfo_datafile().
- Moved update_metainfo_datafile() before the signal handler routines
in syslogger.c for clarity.
- The last "return" is useless in update_metainfo_datafile().
- In pg_current_logfile(), it is useless to call PG_RETURN_NULL after
emitting an ERROR message.
- Two calls to FreeFile(fd) have been forgotten in
pg_current_logfile(). In one case, errno needs to be saved beforehand
to be sure that the correct error string is generated for the user.
- A portion of 010_pg_basebackup.pl was not updated.
- Incorrect header ordering in basebackup.c.
- Decoding of CR and LF characters seem to work fine when
log_file_name include some.

With all those issues fixed, I finish with the attached, that I am
fine to pass down to a committer. I still think that this should be
only one function using a SRF with rows shaped as (type, path) for
simplicity, but as I am visibly outnumbered I won't insist further.
Also, I would rather see an ERROR returned to the user in case of bad
data in current_logfiles, I did not change that either as that's the
original intention of Gilles.
-- 
Michael
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c77a..3188496bc2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4248,6 +4248,25 @@ SELECT * FROM parent WHERE key = 2400;
  must be enabled to generate
 CSV-format log output.

+   
+When either stderr or
+csvlog are included, the file
+current_logfiles gets created and records the location
+of the log file(s) currently in use by the logging collector and the
+associated logging destination. This provides a convenient way to
+find the logs currently in use by the instance. Here is an example of
+contents of this file:
+
+stderr pg_log/postgresql.log
+csvlog pg_log/postgresql.csv
+
+Note that this file gets updated when a new log file is created
+as an effect of rotation or when log_destination is
+reloaded.  current_logfiles is removed when
+stderr and csvlog
+are not included in log_destination or when the
+logging collector is disabled.
+   
 

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e31868ba..c756fbe066 100644
--- a/doc/src/sgml/func.sgml

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 15 Jan 2017 07:20:40 -0600
"Karl O. Pinc"  wrote:

> On Sun, 15 Jan 2017 14:54:44 +0900
> Michael Paquier  wrote:
> 
> > I think that's all I have for this patch.  

> I'd like to submit with it an addendum patch that
> makes symbols out of some constants.  I'll see if I can
> get that done soon.

Attached is a version 23 of the patch.  The only change
is follow-through and cleanup of code posted in-line in emails.

  src/backend/utils/adt/misc.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)

This includes making comments into full sentences.


I do have a question here regards code formatting.
The patch now contains:

if (log_filepath == NULL)
{
/* Bad data. Avoid segfaults etc. and return NULL to caller. */
break;
}

I'm not sure how this fits in with PG coding style,
whether the {} should be removed or what.  I've looked
around and can't find an example of an if with a single
line then block and a comment.  Maybe this means that
I shouldn't be doing this, but if I put the comment above
the if then it will "run into" the comment block which
immediately precedes the above code which describes
a larger body of code.  So perhaps someone should look
at this and tell me how to improve it.


Attached also are 2 patches which abstract some hardcoded
constants into symbols.  Whether to apply them is a matter
of style and left to the maintainers, which is why these
are separate patches.  

The first patch changes only code on the master
branch, the 2nd patch changes the new code.


I have not looked further at the patch or checked
to see that all changes previously recommended have been
made.  Michael, if you're confident that everything is good
go ahead and move the patchs forward to the maintainers.  Otherwise
please let me know and I'll see about finding time
for further review.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c..c44984d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4211,6 +4211,17 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+
+ When logs are written to the file system, the  file includes the type,
+ the location and the name of the logs currently in use. This provides a
+ convenient way to find the logs currently in used by the instance.
+
+$ cat $PGDATA/current_logfiles
+stderr pg_log/postgresql-2017-01-13_085812.log
+
+
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e3186..693669b 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15658,6 +15665,39 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as text, either csvlog
+or stderr as the value of the optional parameter. The
+return value is NULL when the log format requested
+is not a configured .
+
+pg_current_logfiles reflects the content of the
+ file.  All caveats
+regards current_logfiles content are applicable
+to pg_current_logfiles' return value.
+   
+

 pg_my_temp_schema

diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..3ce2a7e 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -60,6 +60,38 @@ Item
  Subdirectory containing per-database subdirectories
 
 
+
+ 
+  
+   current_logfiles
+  
+  
+   Logging
+   current_logfiles file
+  
+  current_logfiles
+ 
+ 
+  
+  A file recording the log file(s) currently written to by the syslogger
+  and the file's log formats, stderr
+  or csvlog.  Each line of the file is a space separated list of
+  two elements: the log format and the full path to the log file 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 15 Jan 2017 14:54:44 +0900
Michael Paquier  wrote:

> I think that's all I have for this patch.

I'd like to submit with it an addendum patch that
makes symbols out of some constants.  I'll see if I can
get that done soon.


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 15/01/2017 à 06:54, Michael Paquier a écrit :
> On Sat, Jan 14, 2017 at 10:02 PM, Gilles Darold
>  wrote:
>> Le 13/01/2017 à 14:09, Michael Paquier a écrit :
>>> On Fri, Jan 13, 2017 at 5:48 PM, Gilles Darold  
>>> wrote:
 Le 13/01/2017 à 05:26, Michael Paquier a écrit :
> Surely the temporary file of current_logfiles should not be included
> in base backups (look at basebackup.c). Documentation needs to reflect
> that as well. Also, should we have current_logfiles in a base backup?
> I would think no.
 Done but can't find any documentation about the file exclusion, do you
 have a pointer?
>>> protocol.sgml, in the protocol-replication part. You could search for
>>> the paragraph that contains postmaster.opts. There is no real harm in
>>> including current_logfiles in base backups, but that's really in the
>>> same bag as postmaster.opts or postmaster.pid, particularly if the log
>>> file name has a timestamp.
>> Thanks for the pointer. After reading this part I think it might only
>> concern files that are critical at restore time. I still think that the
>> file might not be removed automatically from the backup. If it is
>> restored it has strictly no impact at all, it will be removed or
>> overwritten at startup. We can let the user choose to remove it from the
>> backup or not, the file can be an help to find the latest log file
>> related to a backup.
> OK, no problem for me. I can see that your patch does the right thing
> to ignore the current_logfiles.tmp. Could you please update
> t/010_pg_basebackup.pl and add this file to the list of files filled
> with DONOTCOPY?
Applied in attached patch v22.

> pg_current_logfile() can be run by *any* user. We had better revoke
> its access in system_views.sql!
 Why? There is no special information returned by this function unless
 the path but it can be retrieve using show log_directory.
>>> log_directory could be an absolute path, and we surely don't want to
>>> let normal users have a look at it. For example, "show log_directory"
>>> can only be seen by superusers. As Stephen Frost is for a couple of
>>> months (years?) on a holly war path against super-user checks in
>>> system functions, I think that revoking the access to this function is
>>> the best thing to do. It is as well easier to restrict first and
>>> relax, the reverse is harder to justify from a compatibility point of
>>> view.
>> Right, I've append a REVOKE to the function in file
>> backend/catalog/system_views.sql, patch v21 reflect this change.
> Thanks. That looks good.
>
> +   {
> +   /* Logging collector is not enabled. We don't know where messages are
> +* logged.  Remove outdated file holding the current log filenames.
> +*/
> +   unlink(LOG_METAINFO_DATAFILE);
> return 0
> This comment format is not postgres-like.

Fixed too.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 07afa3c..c44984d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4211,6 +4211,17 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+
+ When logs are written to the file system, the  file includes the type,
+ the location and the name of the logs currently in use. This provides a
+ convenient way to find the logs currently in used by the instance.
+
+$ cat $PGDATA/current_logfiles
+stderr pg_log/postgresql-2017-01-13_085812.log
+
+
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e3186..693669b 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15658,6 +15665,39 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Sat, Jan 14, 2017 at 10:02 PM, Gilles Darold
 wrote:
> Le 13/01/2017 à 14:09, Michael Paquier a écrit :
>> On Fri, Jan 13, 2017 at 5:48 PM, Gilles Darold  
>> wrote:
>>> Le 13/01/2017 à 05:26, Michael Paquier a écrit :
 Surely the temporary file of current_logfiles should not be included
 in base backups (look at basebackup.c). Documentation needs to reflect
 that as well. Also, should we have current_logfiles in a base backup?
 I would think no.
>>> Done but can't find any documentation about the file exclusion, do you
>>> have a pointer?
>> protocol.sgml, in the protocol-replication part. You could search for
>> the paragraph that contains postmaster.opts. There is no real harm in
>> including current_logfiles in base backups, but that's really in the
>> same bag as postmaster.opts or postmaster.pid, particularly if the log
>> file name has a timestamp.
>
> Thanks for the pointer. After reading this part I think it might only
> concern files that are critical at restore time. I still think that the
> file might not be removed automatically from the backup. If it is
> restored it has strictly no impact at all, it will be removed or
> overwritten at startup. We can let the user choose to remove it from the
> backup or not, the file can be an help to find the latest log file
> related to a backup.

OK, no problem for me. I can see that your patch does the right thing
to ignore the current_logfiles.tmp. Could you please update
t/010_pg_basebackup.pl and add this file to the list of files filled
with DONOTCOPY?

 pg_current_logfile() can be run by *any* user. We had better revoke
 its access in system_views.sql!
>>> Why? There is no special information returned by this function unless
>>> the path but it can be retrieve using show log_directory.
>> log_directory could be an absolute path, and we surely don't want to
>> let normal users have a look at it. For example, "show log_directory"
>> can only be seen by superusers. As Stephen Frost is for a couple of
>> months (years?) on a holly war path against super-user checks in
>> system functions, I think that revoking the access to this function is
>> the best thing to do. It is as well easier to restrict first and
>> relax, the reverse is harder to justify from a compatibility point of
>> view.
>
> Right, I've append a REVOKE to the function in file
> backend/catalog/system_views.sql, patch v21 reflect this change.

Thanks. That looks good.

+   {
+   /* Logging collector is not enabled. We don't know where messages are
+* logged.  Remove outdated file holding the current log filenames.
+*/
+   unlink(LOG_METAINFO_DATAFILE);
return 0
This comment format is not postgres-like.

I think that's all I have for this patch.
-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 13/01/2017 à 14:09, Michael Paquier a écrit :
> On Fri, Jan 13, 2017 at 5:48 PM, Gilles Darold  
> wrote:
>> Le 13/01/2017 à 05:26, Michael Paquier a écrit :
>>> Surely the temporary file of current_logfiles should not be included
>>> in base backups (look at basebackup.c). Documentation needs to reflect
>>> that as well. Also, should we have current_logfiles in a base backup?
>>> I would think no.
>> Done but can't find any documentation about the file exclusion, do you
>> have a pointer?
> protocol.sgml, in the protocol-replication part. You could search for
> the paragraph that contains postmaster.opts. There is no real harm in
> including current_logfiles in base backups, but that's really in the
> same bag as postmaster.opts or postmaster.pid, particularly if the log
> file name has a timestamp.
Thanks for the pointer. After reading this part I think it might only
concern files that are critical at restore time. I still think that the
file might not be removed automatically from the backup. If it is
restored it has strictly no impact at all, it will be removed or
overwritten at startup. We can let the user choose to remove it from the
backup or not, the file can be an help to find the latest log file
related to a backup.

>
>>> pg_current_logfile() can be run by *any* user. We had better revoke
>>> its access in system_views.sql!
>> Why? There is no special information returned by this function unless
>> the path but it can be retrieve using show log_directory.
> log_directory could be an absolute path, and we surely don't want to
> let normal users have a look at it. For example, "show log_directory"
> can only be seen by superusers. As Stephen Frost is for a couple of
> months (years?) on a holly war path against super-user checks in
> system functions, I think that revoking the access to this function is
> the best thing to do. It is as well easier to restrict first and
> relax, the reverse is harder to justify from a compatibility point of
> view.

Right, I've append a REVOKE to the function in file
backend/catalog/system_views.sql, patch v21 reflect this change.

Thanks.

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 30dd54c..393195f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4211,6 +4211,17 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+
+ When logs are written to the file system, the  file includes the type,
+ the location and the name of the logs currently in use. This provides a
+ convenient way to find the logs currently in used by the instance.
+
+$ cat $PGDATA/current_logfiles
+stderr pg_log/postgresql-2017-01-13_085812.log
+
+
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e3186..693669b 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,13 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile(text)
+   text
+   Primary log file name, or log in the requested format,
+   currently in use by the logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15658,6 +15665,39 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple log files exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as text, either csvlog
+or stderr as the value of the optional parameter. The
+return value is NULL when the log format requested
+is not a configured .
+
+pg_current_logfiles reflects the content of the
+ file.  All caveats
+regards current_logfiles content are applicable
+to pg_current_logfiles' return value.
+   
+

 pg_my_temp_schema

diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..3ce2a7e 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -60,6 +60,38 @@ Item
  Subdirectory containing per-database subdirectories
 
 
+
+ 
+  
+   current_logfiles
+  
+  
+   Logging
+   current_logfiles file
+  
+  current_logfiles
+ 
+ 
+  
+  A file recording the log file(s) currently written 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Sat, Jan 14, 2017 at 1:43 AM, Kevin Grittner  wrote:
> On Fri, Jan 13, 2017 at 7:09 AM, Michael Paquier
>  wrote:
>> There is no real harm in including current_logfiles in base
>> backups, but that's really in the same bag as postmaster.opts or
>> postmaster.pid, particularly if the log file name has a
>> timestamp.
>
> I'm going to dispute that -- if postmaster.opts and postmaster.pid
> are present when you restore, it takes away a level of insurance
> against restoring a corrupted image of the database without knowing
> it.  In particular, if the backup_label file is deleted (which
> happens with alarmingly frequency), the startup code may think it
> is dealing with a cluster that crashed rather than with a restore
> of a backup.  This often leads to corruption (anything from
> "database can't start" to subtle index corruption that isn't
> noticed for months).  The presence of log files from the time of
> the backup do not present a similar hazard.
>
> So while I agree that there is no harm in including
> current_logfiles in base backups, I object to the comparisons to
> the more dangerous files.

Good point.
-- 
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] Patch to implement pg_current_logfile() function

[Kevin Grittner]

On Fri, Jan 13, 2017 at 7:09 AM, Michael Paquier
 wrote:

> There is no real harm in including current_logfiles in base
> backups, but that's really in the same bag as postmaster.opts or
> postmaster.pid, particularly if the log file name has a
> timestamp.

I'm going to dispute that -- if postmaster.opts and postmaster.pid
are present when you restore, it takes away a level of insurance
against restoring a corrupted image of the database without knowing
it.  In particular, if the backup_label file is deleted (which
happens with alarmingly frequency), the startup code may think it
is dealing with a cluster that crashed rather than with a restore
of a backup.  This often leads to corruption (anything from
"database can't start" to subtle index corruption that isn't
noticed for months).  The presence of log files from the time of
the backup do not present a similar hazard.

So while I agree that there is no harm in including
current_logfiles in base backups, I object to the comparisons to
the more dangerous files.

--
Kevin Grittner
EDB: 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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Fri, Jan 13, 2017 at 5:48 PM, Gilles Darold  wrote:
> Le 13/01/2017 à 05:26, Michael Paquier a écrit :
>> Surely the temporary file of current_logfiles should not be included
>> in base backups (look at basebackup.c). Documentation needs to reflect
>> that as well. Also, should we have current_logfiles in a base backup?
>> I would think no.
> Done but can't find any documentation about the file exclusion, do you
> have a pointer?

protocol.sgml, in the protocol-replication part. You could search for
the paragraph that contains postmaster.opts. There is no real harm in
including current_logfiles in base backups, but that's really in the
same bag as postmaster.opts or postmaster.pid, particularly if the log
file name has a timestamp.

>> pg_current_logfile() can be run by *any* user. We had better revoke
>> its access in system_views.sql!
> Why? There is no special information returned by this function unless
> the path but it can be retrieve using show log_directory.

log_directory could be an absolute path, and we surely don't want to
let normal users have a look at it. For example, "show log_directory"
can only be seen by superusers. As Stephen Frost is for a couple of
months (years?) on a holly war path against super-user checks in
system functions, I think that revoking the access to this function is
the best thing to do. It is as well easier to restrict first and
relax, the reverse is harder to justify from a compatibility point of
view.
-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 13/01/2017 à 05:26, Michael Paquier a écrit :
> OK. I have been looking at this patch.
>
> git diff --check is very unhappy, particularly because of
> update_metainfo_datafile().
Sorry, fixed.

> +
> +When logs are written to the file-system their paths, names, and
> +types are recorded in
> +the  file.  This provides
> +a convenient way to find and access log content without establishing a
> +database connection.
> +
> File system is used as a name here. In short, "file-system" -> "file
> system". I think that it would be a good idea to show up the output of
> this file. This could be reworded a bit:
> "When logs are written to the file system, the  linkend="storage-pgdata-current-logfiles"> file includes the location,
> the name and the type of the logs currently in use. This provides a
> convenient way to find the logs currently in used by the instance."
Changes applied. Output example added:

 
$ cat $PGDATA/current_logfiles
stderr pg_log/postgresql-2017-01-13_085812.log



> @@ -15441,6 +15441,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY
> AS t(ls,n);
>
>
>
> +   
> pg_current_logfile()
> +   text
> +   primary log file name in use by the logging collector
> +  
> +
> +  
> +   
> pg_current_logfile(text)
> +   text
> +   log file name, of log in the requested format, in use by the
> +   logging collector
> +  
> You could just use one line, using squared brackets for the additional
> argument. The description looks imprecise, perhaps something like that
> would be better: "Log file name currently in use by the logging
> collector".
OK, back to a single row entry with optional parameter.

> +/* in backend/utils/adt/misc.c and backend/postmaster/syslogger.c */
> +/*
> + * Name of file holding the paths, names, and types of the files where 
> current
> + * log messages are written, when the log collector is enabled.  Useful
> + * outside of Postgres when finding the name of the current log file in the
> + * case of time-based log rotation.
> + */
> Not mentioning the file names here would be better. If this spreads in
> the future updates would likely be forgotten. This paragraph could be
> reworked: "configuration file saving meta-data information about the
> log files currently in use by the system logging process".
Removed.

> --- a/src/include/miscadmin.h
> +++ b/src/include/miscadmin.h
> @@ -466,5 +466,4 @@ extern bool has_rolreplication(Oid roleid);
>  /* in access/transam/xlog.c */
>  extern bool BackupInProgress(void);
>  extern void CancelBackup(void);
> -
>  #endif   /* MISCADMIN_H */
> Unrelated diff.
Removed

> +   /*
> +* Force rewriting last log filename when reloading configuration,
> +* even if rotation_requested is false, log_destination may have
> +* been changed and we don't want to wait the next file rotation.
> +*/
> +   update_metainfo_datafile();
> +
> }
> I know I am famous for being noisy on such things, but please be
> careful with newline use..
That's ok for me, unnecessary newline removed.

> +   else
> +   {
> +   /* Unknown format, no space. Return NULL to caller. */
> +   lbuffer[0] = '\0';
> +   break;
> +   }
> Hm. I would think that an ERROR is more useful to the user here.
The problem addressed here might never happen unless file corruption but
if you know an error code that can be use in this case I will add the
error message. I can not find any error code usable in this case.

> Perhaps pg_current_logfile() should be marked as STRICT?
I don't think so, the log format parameter is optional, and passing NULL
might be like passing no parameter.

> Would it make sense to have the 0-argument version be a SRF returning
> rows of '(log_destination,location)'? We could just live with this
> function I think, and let the caller filter by logging method.
No, this case have already been eliminated during the previous review.

> +is NULL.  When multiple logfiles exist, each in a
> +different format, pg_current_logfile called
> s/logfiles/log files/.
Applied.

> Surely the temporary file of current_logfiles should not be included
> in base backups (look at basebackup.c). Documentation needs to reflect
> that as well. Also, should we have current_logfiles in a base backup?
> I would think no.
Done but can't find any documentation about the file exclusion, do you
have a pointer?

> pg_current_logfile() can be run by *any* user. We had better revoke
> its access in system_views.sql!
Why? There is no special information returned by this function unless
the path but it can be retrieve using show log_directory.

Attached is patch v20.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 30dd54c..393195f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4211,6 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Thu, Jan 12, 2017 at 9:14 PM, Gilles Darold  wrote:
> Le 11/01/2017 à 08:39, Michael Paquier a écrit :
>> On Wed, Dec 14, 2016 at 12:19 AM, Robert Haas  wrote:
>>> On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold  
>>> wrote:
 Applied in last version of the patch v18 as well as removing of an
 unused variable.
>>> OK, this looks a lot closer to being committable.  But:
>>>
>>> [long review]
>>
>> Gilles, could you update the patch based on this review from Robert? I
>> am marking the patch as "waiting on author" for the time being. I
>> looked a bit at the patch but did not notice anything wrong with it,
>> but let's see with a fresh version...
> Hi,
>
> My bad, I was thinking that Karl has planned an update of the patch in
> his last post, sorry for my misunderstanding.
>
> Here is attached v19 of the patch that might fix all comment from
> Robert's last review.

OK. I have been looking at this patch.

git diff --check is very unhappy, particularly because of
update_metainfo_datafile().

+
+When logs are written to the file-system their paths, names, and
+types are recorded in
+the  file.  This provides
+a convenient way to find and access log content without establishing a
+database connection.
+
File system is used as a name here. In short, "file-system" -> "file
system". I think that it would be a good idea to show up the output of
this file. This could be reworded a bit:
"When logs are written to the file system, the  file includes the location,
the name and the type of the logs currently in use. This provides a
convenient way to find the logs currently in used by the instance."

@@ -15441,6 +15441,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY
AS t(ls,n);
   

   
+   
pg_current_logfile()
+   text
+   primary log file name in use by the logging collector
+  
+
+  
+   
pg_current_logfile(text)
+   text
+   log file name, of log in the requested format, in use by the
+   logging collector
+  
You could just use one line, using squared brackets for the additional
argument. The description looks imprecise, perhaps something like that
would be better: "Log file name currently in use by the logging
collector".

+/* in backend/utils/adt/misc.c and backend/postmaster/syslogger.c */
+/*
+ * Name of file holding the paths, names, and types of the files where current
+ * log messages are written, when the log collector is enabled.  Useful
+ * outside of Postgres when finding the name of the current log file in the
+ * case of time-based log rotation.
+ */
Not mentioning the file names here would be better. If this spreads in
the future updates would likely be forgotten. This paragraph could be
reworked: "configuration file saving meta-data information about the
log files currently in use by the system logging process".

--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -466,5 +466,4 @@ extern bool has_rolreplication(Oid roleid);
 /* in access/transam/xlog.c */
 extern bool BackupInProgress(void);
 extern void CancelBackup(void);
-
 #endif   /* MISCADMIN_H */
Unrelated diff.

+   /*
+* Force rewriting last log filename when reloading configuration,
+* even if rotation_requested is false, log_destination may have
+* been changed and we don't want to wait the next file rotation.
+*/
+   update_metainfo_datafile();
+
}
I know I am famous for being noisy on such things, but please be
careful with newline use..

+   else
+   {
+   /* Unknown format, no space. Return NULL to caller. */
+   lbuffer[0] = '\0';
+   break;
+   }
Hm. I would think that an ERROR is more useful to the user here.

Perhaps pg_current_logfile() should be marked as STRICT?

Would it make sense to have the 0-argument version be a SRF returning
rows of '(log_destination,location)'? We could just live with this
function I think, and let the caller filter by logging method.

+is NULL.  When multiple logfiles exist, each in a
+different format, pg_current_logfile called
s/logfiles/log files/

Surely the temporary file of current_logfiles should not be included
in base backups (look at basebackup.c). Documentation needs to reflect
that as well. Also, should we have current_logfiles in a base backup?
I would think no.

pg_current_logfile() can be run by *any* user. We had better revoke
its access in system_views.sql!

I have been playing with this patch a bit, and could not make the
system crash :(
-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Thu, Jan 12, 2017 at 9:55 PM, Karl O. Pinc  wrote:
> On Thu, 12 Jan 2017 13:14:28 +0100
> Gilles Darold  wrote:
>
>> My bad, I was thinking that Karl has planned an update of the patch in
>> his last post, sorry for my misunderstanding.
>
> I was, but have been swept along by events and not gotten to it.

No problem. Thanks for the update.
-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 12 Jan 2017 13:14:28 +0100
Gilles Darold  wrote:

> My bad, I was thinking that Karl has planned an update of the patch in
> his last post, sorry for my misunderstanding.

I was, but have been swept along by events and not gotten to it.



Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 11/01/2017 à 08:39, Michael Paquier a écrit :
> On Wed, Dec 14, 2016 at 12:19 AM, Robert Haas  wrote:
>> On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold  
>> wrote:
>>> Applied in last version of the patch v18 as well as removing of an
>>> unused variable.
>> OK, this looks a lot closer to being committable.  But:
>>
>> [long review]
>  
> Gilles, could you update the patch based on this review from Robert? I
> am marking the patch as "waiting on author" for the time being. I
> looked a bit at the patch but did not notice anything wrong with it,
> but let's see with a fresh version...
Hi,

My bad, I was thinking that Karl has planned an update of the patch in
his last post, sorry for my misunderstanding.

Here is attached v19 of the patch that might fix all comment from
Robert's last review.

Best regards,

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 30dd54c..dd23932 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4211,6 +4211,14 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+
+When logs are written to the file-system their paths, names, and
+types are recorded in
+the  file.  This provides
+a convenient way to find and access log content without establishing a
+database connection.
+
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e3186..e4723f4 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile()
+   text
+   primary log file name in use by the logging collector
+  
+
+  
+   pg_current_logfile(text)
+   text
+   log file name, of log in the requested format, in use by the
+   logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15658,6 +15671,39 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple logfiles exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as text, either csvlog
+or stderr as the value of the optional parameter. The
+return value is NULL when the log format requested
+is not a configured .
+
+pg_current_logfiles reflects the content of the
+ file.  All caveats
+regards current_logfiles content are applicable
+to pg_current_logfiles' return value.
+   
+

 pg_my_temp_schema

diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..3ce2a7e 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -60,6 +60,38 @@ Item
  Subdirectory containing per-database subdirectories
 
 
+
+ 
+  
+   current_logfiles
+  
+  
+   Logging
+   current_logfiles file
+  
+  current_logfiles
+ 
+ 
+  
+  A file recording the log file(s) currently written to by the syslogger
+  and the file's log formats, stderr
+  or csvlog.  Each line of the file is a space separated list of
+  two elements: the log format and the full path to the log file including the
+  value of .  The log format must be present
+  in  to be listed in
+  current_logfiles.
+  
+
+  
+  The current_logfiles file
+  is present only when  is
+  activated and when at least one of stderr or
+  csvlog value is present in
+  .
+  
+ 
+
+
 
  global
  Subdirectory containing cluster-wide tables, such as
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 13a0301..ecaeeef 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -146,7 +146,7 @@ static char *logfile_getname(pg_time_t timestamp, const char *suffix);
 static void set_next_rotation_time(void);
 static void sigHupHandler(SIGNAL_ARGS);
 static void sigUsr1Handler(SIGNAL_ARGS);
-
+static void update_metainfo_datafile(void);
 
 /*
  * Main entry point for syslogger process
@@ -348,6 +348,13 @@ SysLoggerMain(int argc, char *argv[])
 rotation_disabled = false;
 rotation_requested = true;
 			}
+			/*
+			 * Force rewriting last log filename when reloading configuration,
+			 * even if 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Wed, Dec 14, 2016 at 12:19 AM, Robert Haas  wrote:
> On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold  
> wrote:
>> Applied in last version of the patch v18 as well as removing of an
>> unused variable.
>
> OK, this looks a lot closer to being committable.  But:
>
> [long review]

Gilles, could you update the patch based on this review from Robert? I
am marking the patch as "waiting on author" for the time being. I
looked a bit at the patch but did not notice anything wrong with it,
but let's see with a fresh version...
-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

I don't plan to be able to get back to this patch until late
this week or early next week.  My plan is to then go though
the whole thing and fix everything I can find.  If we're both
working at the same time this could lead to wasted effort
so I will write as soon as I start work and if you are working
at the same time I'll send smaller hunks.

By the by, my last email contained some stupidity in it's
code suggestion because it is structured like:

while
  if (...)
do something;
  else
break;
  do more...;

Stupid to have an if/else construct.

while
  if (!...)
break;
  do something;
  do more...;

Is cleaner.  Apologies if my coding out loud is confusing things.
I'll fix this in the next go-round if you don't get to it first.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Thu, Dec 8, 2016 at 4:34 PM, Karl O. Pinc  wrote:
>>  I do think that the blizzard of small patches that you've
>> submitted in some of your emails may possibly be a bit overwhelming.
>> We shouldn't really need a stack of a dozen or more patches to
>> implement one small feature.  Declarative partitioning only had 7.
>> Why does this need more than twice that number?
>
> I'm trying to break up changes into patches which are as small
> as possible to make them more easily understood by providing
> a guide for the reader/reviewer.  So rather than
> a single patch I'd make, say, 3.  One for documentation to describe
> the change.  Another which does whatever refactoring is necessary
> to the existing code, but which otherwise does not introduce any
> functional changes.  And another which adds the new code which makes
> use of the refactoring.  At each stage the code should continue
> to work without bugs.  The other party can then decide to incorporate
> the patchs into the main patch, which is itself another attached
> patch.

Sure, I don't think the intention is bad.  It didn't really work out
here, perhaps because the individual changes were too small.  I think
for small changes it's often more helpful to say "change X to be like
Y" than to provide a patch, or else it's worth combining several small
patches just to keep the number from getting too big.  I don't know; I
can't point to anything you really did wrong here; the process just
didn't seem to be converging.

> It is worth noting that the PG pg_current_logfile() function
> is dependent upon the content of the current_logfiles file.  So
> if the file is wrong the function will also give wrong answers.
> But I don't care if you don't.

Well, I want the current_logfiles file to be correct, but that doesn't
mean that it's reasonable to expect people to enumerate all the
logfiles that have ever existed by running it.  That would be fragile
for tons of reasons, like whether or not the server hits the
connection limit at the wrong time, whether network connectivity is
unimpaired, whether the cron job that's supposed to run it
periodically hiccups for some stupid reason, and many others.

> A related thought is this.  There are 3 abstraction levels of
> concern.  The level beneath PG, the PG level, and the level
> of the user's application.  When PG detects an error from either
> "above" or "below" its job is to describe the problem from
> its own perspective.  HINTs are attempts to cross the boundary
> into the application's perspective.

Yes, I agree.  EnterpriseDB occasionally get complaints from our
customers saying that PG is buggy because it reported an operating
system error.  No, really, if the operating system can't read the
file, that's not our fault!  Call your sysadmin!  I also agree with
your perspective on HINTs.

-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold  wrote:
> Applied in last version of the patch v18 as well as removing of an
> unused variable.

OK, this looks a lot closer to being committable.  But:

@@ -570,11 +583,13 @@ SysLogger_Start(void)
  * a time-based rotation.
  */
 first_syslogger_file_time = time(NULL);
-filename = logfile_getname(first_syslogger_file_time, NULL);
+last_file_name = logfile_getname(first_syslogger_file_time, NULL);
+
+syslogFile = logfile_open(last_file_name, "a", false);

-syslogFile = logfile_open(filename, "a", false);
+update_metainfo_datafile();

-pfree(filename);
+pfree(last_file_name);

 #ifdef EXEC_BACKEND
 switch ((sysloggerPid = syslogger_forkexec()))

This leaves last_file_name pointing to free'd storage, which seems
like a recipe for bugs, because the next call to
update_metainfo_datafile() might try to follow that pointer.  I think
you need to make a clear decision about what the contract is for
last_file_name and last_csv_file_name.  Perhaps the idea is to always
keep them valid, but then you need to free the old value when
reassigning them and not free the current value.

The changes to logfile_rotate() appear to be mostly unrelated
refactoring which Karl was proposing for separate commit, not to be
incorporated into this patch.  Please remove whatever deltas are not
essential to the new feature being implemented.

misc.c no longer needs to add an include of .  I hope, anyway.

+ errmsg("log format not supported, possible
values are stderr or csvlog")));

This doesn't follow our message style guidelines.
https://www.postgresql.org/docs/devel/static/error-style-guide.html

Basically, a comma splice is not an acceptable way of joining together
two independent thoughts.  Obviously, people speak and write that way
informally all the time, but we try to be a bit rigorous about grammar
in our documentation and error messages.  I think the way to do this
would be something like errmsg("log format \"%s\" is not supported"),
errhint("The supported log formats are \"stderr\" and \"csvlog\".").

+FreeFile(fd);
+ereport(ERROR,
+(errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m",
+LOG_METAINFO_DATAFILE)));

You don't need to FreeFile(fd) before ereport(ERROR).  See header
comments for AllocateFile().

+/* report the entry corresponding to the requested format */
+if (strcmp(logfmt, log_format) == 0)
+break;
+}
+/* Close the current log filename file. */
+if (FreeFile(fd))
+ereport(ERROR,
+(errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m",
+LOG_METAINFO_DATAFILE)));
+
+if (lbuffer[0] == '\0')
+PG_RETURN_NULL();
+
+/* Recheck requested log format against the one extracted from the file */
+if (logfmt != NULL && (log_format == NULL ||
+strcmp(logfmt, log_format) != 0))
+PG_RETURN_NULL();

Your email upthread claims that you fixed this per my suggestions, but
it doesn't look fixed to me.  That check to see whether lbuffer[0] ==
'\0' is either protecting against nothing at all (in which case it
could be removed) or it's protecting against some weirdness that can
only happen here because of the strange way this logic is written.
It's hard to understand all the cases here because we can exit the
loop either having found the entry we want or not, and there's no
clear indication of which one it is.  Why not change the if-statement
at the end of the loop like this:

if (logfmt == NULL || strcmp(logfmt, log_format) == 0)
{
FreeFile(fd);
PG_RETURN_TEXT_P(cstring_to_text(log_filepath));
}

Then after the loop, just:

FreeFile(fd);
PG_RETURN_NULL();

+  
+  A file recording the log file(s) currently written to by the syslogger
+  and the file's log formats, stderr
+  or csvlog.  Each line of the file is a space separated list of
+  two elements: the log format and the full path to the log file including the
+  value of .  The log format must be present
+  in  to be listed in
+  current_logfiles.
+  
+
+  
+
+  pg_ctl
+  and current_logfiles
+
+
+  stderr
+  and current_logfiles
+
+
+  log_destination configuration parameter
+  and current_logfiles
+
+
+
+Although logs directed to stderr may be written
+to the filesystem, when the writing of stderr is
+managed outside of the PostgreSQL database
+server the location of such files in the filesystem is not reflected in
+the content of current_logfiles.  One such case is
+when the pg_ctl command is used to start
+the postgres database server, capture
+the stderr output of the server, and direct it to a
+file.
+
+
+
+There are other notable situations related
+to 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 11/12/2016 à 04:38, Karl O. Pinc a écrit :
> On Sat, 10 Dec 2016 19:41:21 -0600
> "Karl O. Pinc"  wrote:
>
>> On Fri, 9 Dec 2016 23:36:12 -0600
>> "Karl O. Pinc"  wrote:
>>
>>> Instead I propose (code I have not actually executed):
>>> ...
>>> charlbuffer[MAXPGPATH];
>>> char*log_format = lbuffer;
>>> ...
>>>
>>> /* extract log format and log file path from the line */
>>> log_filepath = strchr(lbuffer, ' ');  /* lbuffer == log_format
>>> */ *log_filepath = '\0'; /* terminate log_format */
>>> log_filepath++;   /* start of file path */
>>> log_filepath[strcspn(log_filepath, "\n")] = '\0';  
>> Er, I guess I prefer the more paranoid, just because who knows
>> what might have manged to somehow write the file that's read
>> into lbuffer:
>>
>> ...
>> charlbuffer[MAXPGPATH];
>> char*log_format = lbuffer;
>> ...
>>
>> /* extract log format and log file path from the line */
>> if (log_filepath = strchr(lbuffer, ' ')) /* lbuffer == log_format
>> */ *log_filepath = '\0';/* terminate log_format */
>> log_filepath++;  /* start of file path */
>> log_filepath[strcspn(log_filepath, "\n")] = '\0';
> *sigh*
>
>
> ...
> charlbuffer[MAXPGPATH];
> char*log_format = lbuffer;
> ...
>
> /* extract log format and log file path from the line */
> /* lbuffer == log_format, they share storage */
> if (log_filepath = strchr(lbuffer, ' '))
> *log_filepath = '\0';   /* terminate log_format */
> else
> {
> /* Unknown format, no space. Return NULL to caller. */
> lbuffer[0] = '\0';
> break;
> }
> log_filepath++;  /* start of file path   */
> log_filepath[strcspn(log_filepath, "\n")] = '\0';
>

Applied in last version of the patch v18 as well as removing of an
unused variable.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0fc4e57..871efec 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4169,6 +4169,14 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+
+When logs are written to the file-system their paths, names, and
+types are recorded in
+the  file.  This provides
+a convenient way to find and access log content without establishing a
+database connection.
+
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index eca98df..20de903 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15444,6 +15444,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile()
+   text
+   primary log file name in use by the logging collector
+  
+
+  
+   pg_current_logfile(text)
+   text
+   log file name, of log in the requested format, in use by the
+   logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15661,6 +15674,39 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple logfiles exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as text, either csvlog
+or stderr as the value of the optional parameter. The
+return value is NULL when the log format requested
+is not a configured .
+
+pg_current_logfiles reflects the content of the
+ file.  All caveats
+regards current_logfiles content are applicable
+to pg_current_logfiles' return value.
+   
+

 pg_my_temp_schema

diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..3b003f5 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -60,6 +60,81 @@ Item
  Subdirectory containing per-database subdirectories
 
 
+
+ 
+  
+   current_logfiles
+  
+  
+   Logging
+   current_logfiles file
+  
+  current_logfiles
+ 
+ 
+  
+  A file recording the log file(s) currently written to by the syslogger
+  and the file's log formats, stderr
+  or csvlog.  Each line of the file is a space separated list of
+  two elements: the log format and the full path to the log file including the
+  

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 10 Dec 2016 19:41:21 -0600
"Karl O. Pinc"  wrote:

> On Fri, 9 Dec 2016 23:36:12 -0600
> "Karl O. Pinc"  wrote:
> 
> > Instead I propose (code I have not actually executed):
> > ...
> > charlbuffer[MAXPGPATH];
> > char*log_format = lbuffer;
> > ...
> > 
> > /* extract log format and log file path from the line */
> > log_filepath = strchr(lbuffer, ' ');  /* lbuffer == log_format
> > */ *log_filepath = '\0'; /* terminate log_format */
> > log_filepath++;   /* start of file path */
> > log_filepath[strcspn(log_filepath, "\n")] = '\0';  
> 
> Er, I guess I prefer the more paranoid, just because who knows
> what might have manged to somehow write the file that's read
> into lbuffer:
> 
> ...
> charlbuffer[MAXPGPATH];
> char*log_format = lbuffer;
> ...
> 
> /* extract log format and log file path from the line */
> if (log_filepath = strchr(lbuffer, ' ')) /* lbuffer == log_format
> */ *log_filepath = '\0';/* terminate log_format */
> log_filepath++;  /* start of file path */
> log_filepath[strcspn(log_filepath, "\n")] = '\0';

*sigh*


...
charlbuffer[MAXPGPATH];
char*log_format = lbuffer;
...

/* extract log format and log file path from the line */
/* lbuffer == log_format, they share storage */
if (log_filepath = strchr(lbuffer, ' '))
*log_filepath = '\0';   /* terminate log_format */
else
{
/* Unknown format, no space. Return NULL to caller. */
lbuffer[0] = '\0';
break;
}
log_filepath++;  /* start of file path   */
log_filepath[strcspn(log_filepath, "\n")] = '\0';


> The file read is, of course, normally written by postgres.  But
> possibly writing to unintended memory locations, even virtual address
> NULL, does not seem good.
> 
> Any feedback from more experienced PG developers as how to best handle
> this case would be welcome.
> 
> Regards,
> 
> Karl 
> Free Software:  "You don't pay back, you pay forward."
>  -- Robert A. Heinlein
> 




Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Fri, 9 Dec 2016 23:36:12 -0600
"Karl O. Pinc"  wrote:

> Instead I propose (code I have not actually executed):
> ...
> charlbuffer[MAXPGPATH];
> char*log_format = lbuffer;
> ...
> 
> /* extract log format and log file path from the line */
> log_filepath = strchr(lbuffer, ' ');  /* lbuffer == log_format */
> *log_filepath = '\0'; /* terminate log_format */
> log_filepath++;   /* start of file path */
> log_filepath[strcspn(log_filepath, "\n")] = '\0';

Er, I guess I prefer the more paranoid, just because who knows
what might have manged to somehow write the file that's read
into lbuffer:

...
charlbuffer[MAXPGPATH];
char*log_format = lbuffer;
...

/* extract log format and log file path from the line */
if (log_filepath = strchr(lbuffer, ' ')) /* lbuffer == log_format */
*log_filepath = '\0';/* terminate log_format */
log_filepath++;  /* start of file path */
log_filepath[strcspn(log_filepath, "\n")] = '\0';

The file read is, of course, normally written by postgres.  But possibly
writing to unintended memory locations, even virtual address NULL, does
not seem good.

Any feedback from more experienced PG developers as how to best handle
this case would be welcome.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Fri, 9 Dec 2016 23:41:25 +0100
Gilles Darold  wrote:

>   /* extract log format and log file path from the line */
>   log_filepath = strchr(lbuffer, ' ');
>   log_filepath++;
>   lbuffer[log_filepath-lbuffer-1] = '\0';
>   log_format = lbuffer;
>   *strchr(log_filepath, '\n') = '\0';

Instead I propose (code I have not actually executed):
...
charlbuffer[MAXPGPATH];
char*log_format = lbuffer;
...

/* extract log format and log file path from the line */
log_filepath = strchr(lbuffer, ' ');  /* lbuffer == log_format */
*log_filepath = '\0'; /* terminate log_format */
log_filepath++;   /* start of file path */
log_filepath[strcspn(log_filepath, "\n")] = '\0';


My first thought was to follow your example and begin with
log_format = lbuffer;
But upon reflection I think changing the declaration of log_format
to use an initializer better expresses how storage is always shared.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 08/12/2016 à 00:52, Robert Haas a écrit :
> On Tue, Dec 6, 2016 at 6:11 PM, Gilles Darold  
> wrote:
>> It seems that all fixes have been included in this patch.
> Yikes.  This patch has certainly had a lot of review, but it has lots
> of basic inconsistencies with PostgreSQL coding style, which one would
> think somebody would have noticed by now, and there are multiple
> places where the logic seems to do in a complicated way something that
> could be done significantly more simply.  I don't have any objection
> to the basic idea of this patch, but it's got to look and feel like
> the surrounding PostgreSQL code.  And not be unnecessarily
> complicated.
>
> Detailed comments below:
>
> The SGML doesn't match the surrounding style - for example, 
> typically appears on a line by itself.
Fixed.

> +if (!Logging_collector) {
>
> Formatting.
Fixed.

> rm_log_metainfo() could be merged into logfile_writename(), since
> they're always called in conjunction and in the same pattern.  The
> function is poorly named; it should be something like
> update_metainfo_datafile().
Merge and logfile_writename() function renamed as suggested.

> +if (errno == ENFILE || errno == EMFILE)
> +ereport(LOG,
> +(errmsg("system is too busy to write logfile meta
> info, %s will be updated on next rotation (or use SIGHUP to retry)",
> LOG_METAINFO_DATAFILE)));
>
> This seems like a totally unprincipled reaction to a purely arbitrary
> subset of errors.  EMFILE or ENFILE doesn't represent a general "too
> busy" condition, and logfile_open() has already logged the real error.
Removed.

> +snprintf(tempfn, sizeof(tempfn), "%s.tmp", LOG_METAINFO_DATAFILE);
>
> You don't really need to use snprintf() here.  You could #define
> LOG_METAINFO_DATAFILE_TMP LOG_METAINFO_DATAFILE ".tmp" and compute
> this at compile time instead of runtime.
Done and added to syslogger.h

> +if (PG_NARGS() == 1) {
> +fmt = PG_ARGISNULL(0) ? NULL : PG_GETARG_TEXT_PP(0);
> +if (fmt != NULL) {
> +logfmt = text_to_cstring(fmt);
> +if ( (strcmp(logfmt, "stderr") != 0) &&
> +(strcmp(logfmt, "csvlog") != 0) ) {
> +ereport(ERROR,
> +(errmsg("log format %s not supported, possible values are
> stderr or csvlog", logfmt)));
> +PG_RETURN_NULL();
> +}
> +}
> +} else {
> +logfmt = NULL;
> +}
>
> Formatting.  This is unlike PostgreSQL style in multiple ways,
> including cuddled braces, extra parentheses and spaces, and use of
> braces around a single-line block.  Also, this could be written in a
> much less contorted way, like:
>
> if (PG_NARGS() == 0 || PG_ARGISNULL(0))
> logfmt = NULL;
> else
> {
> logfmt = text_to_cstring(PG_GETARG_TEXT_PP(0));
> if (strcmp(logfmt, "stderr") != 0 && strcmp(logfmt, "csvlog") != 0)
>  ereport(...);
> }
Applied.

> Also, the ereport() needs an errcode(), not just an errmsg().
> Otherwise it defaults to ERRCODE_INTERNAL_ERROR, which is not correct.
Add errcode(ERRCODE_INVALID_PARAMETER_VALUE) to the ereport call.

> +/* Check if current log file is present */
> +if (stat(LOG_METAINFO_DATAFILE, _buf) != 0)
> +PG_RETURN_NULL();
>
> Useless test.  The code that follows catches this case anyway and
> handles it the same way.  Which is good, because this has an inherent
> race condition. The previous if (!Logging_collector) test sems fairly
> useless too; unless there's a bug in your syslogger.c code, the file
> won't exist anyway, so we'll return NULL for that reason with no extra
> code needed here.
That's right, both test have been removed.

> +/*
> +* Read the file to gather current log filename(s) registered
> +* by the syslogger.
> +*/
>
> Whitespace isn't right.
>
> +while (fgets(lbuffer, sizeof(lbuffer), fd) != NULL) {
> +charlog_format[10];
> +int i = 0, space_pos = 0;
> +
> +/* Check for a read error. */
> +if (ferror(fd)) {
>
> More coding style issues.
>
> +ereport(ERROR,
> +(errcode_for_file_access(),
> +errmsg("could not read file \"%s\": %m",
> LOG_METAINFO_DATAFILE)));
> +FreeFile(fd);
> +break;
>
> ereport(ERROR) doesn't return, so the code that follows is pointless.
All issues above are fixed.

> +if (strchr(lbuffer, '\n') != NULL)
> +*strchr(lbuffer, '\n') = '\0';
>
> Probably worth adding a local variable instead of doing this twice.
> Local variables are cheap, and the code would be more readable.
Removed

> +if ((space_pos == 0) && (isspace(lbuffer[i]) != 0))
>
> Too many parentheses.  Also, I think the whole loop in which this is
> contained could be eliminated entirely.  Just search for the first ' '
> character with strchr(); you don't need to are about isspace() because
> you know the code that writes this file uses ' ' 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 8 Dec 2016 11:27:34 -0500
Robert Haas  wrote:

> On Wed, Dec 7, 2016 at 11:02 PM, Karl O. Pinc  wrote:
> > I read this and knew that I hadn't finished review, but didn't
> > immediately respond because I thought the patch had to be
> > marked "ready for committer" on commitfest.postgresql.org
> > before the committers would spend a lot of time on it.  
> 
> Generally that's true, but I was trying to be helpful and trying also
> to move this toward some kind of conclusion.

It has been very helpful, particularly your last reply.  And I think
there's now a clear path forward.

(I'm not looking for any replies to the below, although of course would
welcome whatever you've got to say.)

> It is, of course, not my job to decide who is at fault in whatever may
> or may not have gone wrong during the reviewing process.

Understood.  And I'm not looking for any sort of judgment or
attempting to pass judgment.  It has been frustrating though
and your email provided an opportunity to vent, and to
provide some feedback, of whatever quality, to the review
process.

It's been that much more frustrating because I don't really
care one way or another about the feature.  I was just trying
to build up credit reviewing somebody else's patch, and instead
probably did the opposite with all the thrashing.  :)

>  I do think that the blizzard of small patches that you've
> submitted in some of your emails may possibly be a bit overwhelming.
> We shouldn't really need a stack of a dozen or more patches to
> implement one small feature.  Declarative partitioning only had 7.
> Why does this need more than twice that number?

I'm trying to break up changes into patches which are as small
as possible to make them more easily understood by providing
a guide for the reader/reviewer.  So rather than
a single patch I'd make, say, 3.  One for documentation to describe
the change.  Another which does whatever refactoring is necessary
to the existing code, but which otherwise does not introduce any
functional changes.  And another which adds the new code which makes
use of the refactoring.  At each stage the code should continue
to work without bugs.  The other party can then decide to incorporate
the patchs into the main patch, which is itself another attached
patch.

Do this for several unrelated changes to the main patch and the patches
add up.

Mostly I wouldn't expect various patches to be reviewed by the
committers, they'd get incorporated into the main patch.

This is how I break down the work when I look at code changes.
What's it do?  What does it change/break in the existing code?
How does the new stuff work?  But this process has not worked
here so I guess I'll stop.

But I expect you will see at least 3 patches submitted for
committer review.  I see a number of hardcoded constants,
now that the main patch adds additional code, that can
be made into symbols to, IMO, improve code clarity.
Guiles points out that this is an issue of coding style
and might be considered unnecessary complication.
So they are not in the main patch.  They are attached
(applied to v14 of the main patch; really, the first applies
to the master PG branch and the 2nd to v14 of the
pg_current_logfiles patch) if you want to look
at them now and provide some guidance as to whether they
should be dropped or included in the patch.

> > The extreme case is the attached "cleanup_rotate" patch.
> > (Which applies to v14 of this patch.)  It's nothing but
> > a little bit of tiding up of the master branch,

> I took a look at that patch just now and I guess I don't really see
> the point.

The point would be to make the code easier to read.  Saving cycles
is hardly ever worthwhile in my opinion.  The whole point
of code is to be read by people and be understandable.
If it's not understood it's useless going forward.

Once I've gone to the effort to understand something
written, that I'm going to be responsible for maintaining, I like to
see it written as clearly as possible.  In my experience
if you don't do this then little confusions multiply and
eventually the whole thing turns to mud.

The trade-off, as you say, is the overhead involved
in running minor changes through the production
process.  I figure this can be minimized by bundling
such changes with related substantial changes.

Again, it's not working so I'll drop it.

> > FYI.  The point of the "retry_current_logfiles"
> > patch series is to handle the case
> > where logfile_writename gets a ENFILE or EMFILE.
> > When this happens the current_logfiles file can
> > "skip" and not contain the name(s) of the current
> > log file for an entire log rotation.  To miss, say,
> > a month of logs because the logs only rotate monthly
> > and your log processing is based on reading the
> > current_logfiles file sounds like a problem.  
> 
> I think if you are trying to enumerate the names of your logfiles by
> any sort of polling mechanism, rather than by seeing what 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Robert Haas]

On Wed, Dec 7, 2016 at 11:02 PM, Karl O. Pinc  wrote:
> I read this and knew that I hadn't finished review, but didn't
> immediately respond because I thought the patch had to be
> marked "ready for committer" on commitfest.postgresql.org
> before the committers would spend a lot of time on it.

Generally that's true, but I was trying to be helpful and trying also
to move this toward some kind of conclusion.

> I've been introducing some complication, but I hope it's necessary.
> To keep the review process simple my plan has been to submit
> multiple patches.  One with the basics and more on top of that
> that introduce complication that can be considered and accepted or
> rejected.  So I send emails with multiple patches, some that I think
> need to go into the core patch and others to be kept separate.
> But, although I try, this does not seem to have been communicated
> because I keep getting emails back that contain only a single patch.
>
> Maybe something's wrong with my review process but I don't know
> how to fix it.

It is, of course, not my job to decide who is at fault in whatever may
or may not have gone wrong during the reviewing process.  It is not
only not my job, but would be unproductive, since the goal here is for
people to contribute more, not less.  I have done enough review in
this community to have experienced the problem of people who say that
they took your suggestions when in fact they didn't, or who randomly
de-improve things in future patch versions that were more correct in
earlier versions.  I agree that on occasions when that happens, it is
indeed very frustrating.  Whether that's happened in this case, I
don't know.  I do think that the blizzard of small patches that you've
submitted in some of your emails may possibly be a bit overwhelming.
We shouldn't really need a stack of a dozen or more patches to
implement one small feature.  Declarative partitioning only had 7.
Why does this need more than twice that number?

> The extreme case is the attached "cleanup_rotate" patch.
> (Which applies to v14 of this patch.)  It's nothing but
> a little bit of tiding up of the master branch, but does
> touch code that's already being modified so it seems
> like the committers would want to look at it at the same
> time as when reviewing the pg_current_logfile patch.
> Once you've looked at the pg_current_logfile patch
> you've already looked at and modified code in the function
> the "cleanup_rotate" patch touches.

I took a look at that patch just now and I guess I don't really see
the point.  I mean, it will save a few cycles, and that's not nothing,
but it's not much, either.  I don't see a reason to complicate the
basic feature patch by entangling it with such a change - it just
makes it harder to get the actual feature committed.  And I kind of
wonder if the time it will take to review and commit that patch is
even worth it.  There must be hundreds of places in our code base
where you could do stuff like that, but only a few of those save
enough cycles to be worth bothering with.  To be fair, if that patch
were submitted independently of the rest of this and nobody objected,
I'd probably commit it; I mean, why not?  But this thread is now over
100 emails long without reaching a happy conclusion, and one good way
to expedite the path to a happy conclusion is to drop all of the
nonessential bits.  And that change certainly qualifies as
nonessential.

> FYI.  The point of the "retry_current_logfiles"
> patch series is to handle the case
> where logfile_writename gets a ENFILE or EMFILE.
> When this happens the current_logfiles file can
> "skip" and not contain the name(s) of the current
> log file for an entire log rotation.  To miss, say,
> a month of logs because the logs only rotate monthly
> and your log processing is based on reading the
> current_logfiles file sounds like a problem.

I think if you are trying to enumerate the names of your logfiles by
any sort of polling mechanism, rather than by seeing what files show
up in your logging directory, you are doing it wrong.  So, I don't see
that this matters at all.

> The point of the code is to report not just the real error, but what
> effect the real error has -- that the current_logfiles file did not
> get updated in a timely fashion.  Maybe this isn't necessary, especially
> if the write of current_logfiles gets retried on failure.  Or maybe
> the right thing to do is to give logfile_open() an argument that
> let's it elaborate it's error message.  Any guidance here would
> be appreciated.

Generally speaking, I would say that it's the user's job to decide
what the impact of an error is; our job is only to tell them what
happened.  There are occasional exceptions; for example:

rhaas=# select ablance from pgbench_accounts;
ERROR:  column "ablance" does not exist
LINE 1: select ablance from pgbench_accounts;
   ^
HINT:  Perhaps you meant to reference the column "pgbench_accounts.abalance".

The HINT 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hello Robert,

On Wed, 7 Dec 2016 18:52:24 -0500
Robert Haas  wrote:

> On Tue, Dec 6, 2016 at 6:11 PM, Gilles Darold
>  wrote:
> > It seems that all fixes have been included in this patch.  

I read this and knew that I hadn't finished review, but didn't
immediately respond because I thought the patch had to be
marked "ready for committer" on commitfest.postgresql.org
before the committers would spend a lot of time on it.

I don't have the time to fully respond, or I'd be able to
attach new code, but want to comment before anybody else
spends a lot of time on this patch.

> Yikes.  This patch has certainly had a lot of review, but it has lots
> of basic inconsistencies with PostgreSQL coding style, which one would
> think somebody would have noticed by now,

Yes and no.  I don't really know what the coding style is and
rather than have to make multiple corrections to code that might
get weeded out and discarded I've been focusing on getting the logic
right.  Really, I've not put a thought to it except for trying to write
what I write like what's there, and sticking to < 80 chars per line.

There has been lots of review.
The only truly effective way I've found to communicate regards
the pg_current_logfiles patch has been to write code and provide
detailed test cases.  I could be wrong, but unless I submit
code I don't feel like I've been understood.
I just haven't been interested in re-formatting
somebody else's code before I think the code really works.

>  I don't have any objection
> to the basic idea of this patch, but it's got to look and feel like
> the surrounding PostgreSQL code.  And not be unnecessarily
> complicated.

I've been introducing some complication, but I hope it's necessary.
To keep the review process simple my plan has been to submit
multiple patches.  One with the basics and more on top of that
that introduce complication that can be considered and accepted or
rejected.  So I send emails with multiple patches, some that I think
need to go into the core patch and others to be kept separate.
But, although I try, this does not seem to have been communicated
because I keep getting emails back that contain only a single patch.

Maybe something's wrong with my review process but I don't know
how to fix it.

If you think a single patch is the way to go I can open up
separate tickets at commitfest.postgresql.org.  But this seems
like overkill because all the patches touch the same code.

The extreme case is the attached "cleanup_rotate" patch.
(Which applies to v14 of this patch.)  It's nothing but
a little bit of tiding up of the master branch, but does
touch code that's already being modified so it seems
like the committers would want to look at it at the same
time as when reviewing the pg_current_logfile patch.  
Once you've looked at the pg_current_logfile patch
you've already looked at and modified code in the function
the "cleanup_rotate" patch touches.

But the "cleanup_rotate" patch got included
in the v15 version of the patch and is also in v16.
I'm expecting to submit it as a separate patch
along with the pg_current_logfile patch and tried
to be very very clear about this.  It
really has nothing to do with the pg_current_logfile
functionality.  (And is the only "separate" patch
which really has nothing to do with the pg_current_logfile
functionality.)

More examples of separate patches are below.  Any guidance
would be appreciated.

> 
> Detailed comments below:

> rm_log_metainfo() could be merged into logfile_writename(), since
> they're always called in conjunction and in the same pattern. 

This would be true, if it weren't for the attached
"retry_current_logfiles" patches that were applied in
v15 of the patch and removed from v16 due to some
mis-communication I don't understand.

(But the attached patches apply on top of the the v14 patch.
I don't have time to refactor them now.)

FYI.  The point of the "retry_current_logfiles"
patch series is to handle the case
where logfile_writename gets a ENFILE or EMFILE.
When this happens the current_logfiles file can
"skip" and not contain the name(s) of the current
log file for an entire log rotation.  To miss, say,
a month of logs because the logs only rotate monthly
and your log processing is based on reading the
current_logfiles file sounds like a problem.

What I was attempting to communicate in my email response
to the v15 patch is that the (attached, but applies to the
v14 patch again) "backoff" patch should be submitted
as a separate patch.  It seems over-complicated, but
exists in case a committer is worried about re-trying
writes on a system that's busy enough to generate ENFILE
or EMFILE errors.


> +if (errno == ENFILE || errno == EMFILE)
> +ereport(LOG,
> +(errmsg("system is too busy to write logfile meta
> info, %s will be updated on next rotation (or use SIGHUP to retry)",
> LOG_METAINFO_DATAFILE)));
> 
> This seems like a totally 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Tom Lane]

Robert Haas  writes:
> Yikes.  This patch has certainly had a lot of review, but it has lots
> of basic inconsistencies with PostgreSQL coding style, which one would
> think somebody would have noticed by now, and there are multiple
> places where the logic seems to do in a complicated way something that
> could be done significantly more simply.  I don't have any objection
> to the basic idea of this patch, but it's got to look and feel like
> the surrounding PostgreSQL code.  And not be unnecessarily
> complicated.

A lot of the code-formatting issues could probably be fixed by running it
through pgindent.  Also, when you are starting from code that was written
with little regard for Postgres layout conventions, it's a really good
idea to see what pgindent will do to it, because it might not look very
nice afterwards.

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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Tue, Dec 6, 2016 at 6:11 PM, Gilles Darold  wrote:
> It seems that all fixes have been included in this patch.

Yikes.  This patch has certainly had a lot of review, but it has lots
of basic inconsistencies with PostgreSQL coding style, which one would
think somebody would have noticed by now, and there are multiple
places where the logic seems to do in a complicated way something that
could be done significantly more simply.  I don't have any objection
to the basic idea of this patch, but it's got to look and feel like
the surrounding PostgreSQL code.  And not be unnecessarily
complicated.

Detailed comments below:

The SGML doesn't match the surrounding style - for example, 
typically appears on a line by itself.

+if (!Logging_collector) {

Formatting.

rm_log_metainfo() could be merged into logfile_writename(), since
they're always called in conjunction and in the same pattern.  The
function is poorly named; it should be something like
update_metainfo_datafile().

+if (errno == ENFILE || errno == EMFILE)
+ereport(LOG,
+(errmsg("system is too busy to write logfile meta
info, %s will be updated on next rotation (or use SIGHUP to retry)",
LOG_METAINFO_DATAFILE)));

This seems like a totally unprincipled reaction to a purely arbitrary
subset of errors.  EMFILE or ENFILE doesn't represent a general "too
busy" condition, and logfile_open() has already logged the real error.

+snprintf(tempfn, sizeof(tempfn), "%s.tmp", LOG_METAINFO_DATAFILE);

You don't really need to use snprintf() here.  You could #define
LOG_METAINFO_DATAFILE_TMP LOG_METAINFO_DATAFILE ".tmp" and compute
this at compile time instead of runtime.

+if (PG_NARGS() == 1) {
+fmt = PG_ARGISNULL(0) ? NULL : PG_GETARG_TEXT_PP(0);
+if (fmt != NULL) {
+logfmt = text_to_cstring(fmt);
+if ( (strcmp(logfmt, "stderr") != 0) &&
+(strcmp(logfmt, "csvlog") != 0) ) {
+ereport(ERROR,
+(errmsg("log format %s not supported, possible values are
stderr or csvlog", logfmt)));
+PG_RETURN_NULL();
+}
+}
+} else {
+logfmt = NULL;
+}

Formatting.  This is unlike PostgreSQL style in multiple ways,
including cuddled braces, extra parentheses and spaces, and use of
braces around a single-line block.  Also, this could be written in a
much less contorted way, like:

if (PG_NARGS() == 0 || PG_ARGISNULL(0))
logfmt = NULL;
else
{
logfmt = text_to_cstring(PG_GETARG_TEXT_PP(0));
if (strcmp(logfmt, "stderr") != 0 && strcmp(logfmt, "csvlog") != 0)
 ereport(...);
}

Also, the ereport() needs an errcode(), not just an errmsg().
Otherwise it defaults to ERRCODE_INTERNAL_ERROR, which is not correct.

+/* Check if current log file is present */
+if (stat(LOG_METAINFO_DATAFILE, _buf) != 0)
+PG_RETURN_NULL();

Useless test.  The code that follows catches this case anyway and
handles it the same way.  Which is good, because this has an inherent
race condition. The previous if (!Logging_collector) test sems fairly
useless too; unless there's a bug in your syslogger.c code, the file
won't exist anyway, so we'll return NULL for that reason with no extra
code needed here.

+/*
+* Read the file to gather current log filename(s) registered
+* by the syslogger.
+*/

Whitespace isn't right.

+while (fgets(lbuffer, sizeof(lbuffer), fd) != NULL) {
+charlog_format[10];
+int i = 0, space_pos = 0;
+
+/* Check for a read error. */
+if (ferror(fd)) {

More coding style issues.

+ereport(ERROR,
+(errcode_for_file_access(),
+errmsg("could not read file \"%s\": %m",
LOG_METAINFO_DATAFILE)));
+FreeFile(fd);
+break;

ereport(ERROR) doesn't return, so the code that follows is pointless.

+if (strchr(lbuffer, '\n') != NULL)
+*strchr(lbuffer, '\n') = '\0';

Probably worth adding a local variable instead of doing this twice.
Local variables are cheap, and the code would be more readable.

+if ((space_pos == 0) && (isspace(lbuffer[i]) != 0))

Too many parentheses.  Also, I think the whole loop in which this is
contained could be eliminated entirely.  Just search for the first ' '
character with strchr(); you don't need to are about isspace() because
you know the code that writes this file uses ' ' specifically.
Overwrite it with '\0'.  And then use a pointer to lbuffer for the log
format and a pointer to lbuffer + strchr_result + 1 for the pathname.

+if ((space_pos != (int)strlen("stderr")) &&
+(space_pos != (int)strlen("csvlog")))
+{
+ereport(ERROR,
+(errmsg("unexpected line format in file %s",
LOG_METAINFO_DATAFILE)));
+break;
+}

I think this is pointless.  Validating the length of the log format
but not the content seems kind of silly, 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 02/12/2016 à 02:08, Karl O. Pinc a écrit :
> On Sun, 27 Nov 2016 21:54:46 +0100
> Gilles Darold  wrote:
>
>> I've attached the v15 of the patch 
>> I've not applied patch patch_pg_current_logfile-v14.diff.backoff to
>> prevent constant call of logfile_writename() on a busy system (errno =
>> ENFILE | EMFILE).
> I don't think it should be applied and included in the basic
> functionality patch in any case. I think it needs to be submitted as a
> separate patch along with the basic functionality patch.  Backing off
> the retry of the current_logfiles write could be overly fancy and
> simply not needed.
>
>> I think this can be done quite simply by testing if
>> log rotate is still enabled. This is possible because function
>> logfile_rotate() is already testing if  errno = ENFILE | EMFILE and in
>> this case rotation_disabled is set to true. So the following test
>> should do the work:
>>
>>if (log_metainfo_stale && !rotation_disabled)
>>logfile_writename();
>>
>> This is included in v15 patch.
> I don't see this helping much, if at all.
>
> First, it's not clear just when rotation_disabled can be false
> when log_metainfo_stale is true.  The typical execution
> path is for logfile_writename() to be called after rotate_logfile()
> has already set rotataion_disabled to true.   logfile_writename()
> is the only place setting log_metainfo_stale to true and
> rotate_logfile() the only place settig rotation_disabled to false.
>
> While it's possible
> that log_metainfo_stale might be set to true when logfile_writename()
> is called from within open_csvlogfile(), this does not help with the
> stderr case.  IMO better to just test for log_metaifo_stale at the
> code snippet above.
>
> Second, the whole point of retrying the logfile_writename() call is
> to be sure that the current_logfiles file is updated before the logs
> rotate.  Waiting until logfile rotation is enabled defeats the purpose.

Ok, sorry I've misunderstood your previous post. Current v16
attached patch removed your change about log_meta_info
stale and fix the use of sscanf to read the file.

It seems that all fixes have been included in this patch.

Regards

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0fc4e57..41144cb 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4169,6 +4169,12 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+When logs are written to the file-system their paths, names, and
+types are recorded in
+the  file.  This provides
+a convenient way to find and access log content without establishing a
+database connection.
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index eca98df..47ca846 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15444,6 +15444,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile()
+   text
+   primary log file name in use by the logging collector
+  
+
+  
+   pg_current_logfile(text)
+   text
+   log file name, of log in the requested format, in use by the
+   logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15661,6 +15674,39 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple logfiles exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as text, either csvlog
+or stderr as the value of the optional parameter. The
+return value is NULL when the log format requested
+is not a configured .
+
+   pg_current_logfiles reflects the content of the
+ file.  All caveats
+regards current_logfiles content are applicable
+to pg_current_logfiles' return value.
+   
+

 pg_my_temp_schema

diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..928133c 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -60,6 +60,79 @@ Item
  Subdirectory containing per-database subdirectories
 
 
+
+ 
+  
+   current_logfiles
+  
+  
+   Logging
+   current_logfiles file
+  
+  

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 12:08 PM, Karl O. Pinc  wrote:

> On Sun, 27 Nov 2016 21:54:46 +0100
> Gilles Darold  wrote:
>
> > I've attached the v15 of the patch
>
> > I've not applied patch patch_pg_current_logfile-v14.diff.backoff to
> > prevent constant call of logfile_writename() on a busy system (errno =
> > ENFILE | EMFILE).
>
> I don't think it should be applied and included in the basic
> functionality patch in any case. I think it needs to be submitted as a
> separate patch along with the basic functionality patch.  Backing off
> the retry of the current_logfiles write could be overly fancy and
> simply not needed.
>
> > I think this can be done quite simply by testing if
> > log rotate is still enabled. This is possible because function
> > logfile_rotate() is already testing if  errno = ENFILE | EMFILE and in
> > this case rotation_disabled is set to true. So the following test
> > should do the work:
> >
> >if (log_metainfo_stale && !rotation_disabled)
> >logfile_writename();
> >
> > This is included in v15 patch.
>
> I don't see this helping much, if at all.
>
> First, it's not clear just when rotation_disabled can be false
> when log_metainfo_stale is true.  The typical execution
> path is for logfile_writename() to be called after rotate_logfile()
> has already set rotataion_disabled to true.   logfile_writename()
> is the only place setting log_metainfo_stale to true and
> rotate_logfile() the only place settig rotation_disabled to false.
>
> While it's possible
> that log_metainfo_stale might be set to true when logfile_writename()
> is called from within open_csvlogfile(), this does not help with the
> stderr case.  IMO better to just test for log_metaifo_stale at the
> code snippet above.
>
> Second, the whole point of retrying the logfile_writename() call is
> to be sure that the current_logfiles file is updated before the logs
> rotate.  Waiting until logfile rotation is enabled defeats the purpose.


Moved to next CF with "needs review" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> I've attached the v15 of the patch 

> I've not applied patch patch_pg_current_logfile-v14.diff.backoff to
> prevent constant call of logfile_writename() on a busy system (errno =
> ENFILE | EMFILE).

I don't think it should be applied and included in the basic
functionality patch in any case. I think it needs to be submitted as a
separate patch along with the basic functionality patch.  Backing off
the retry of the current_logfiles write could be overly fancy and
simply not needed.

> I think this can be done quite simply by testing if
> log rotate is still enabled. This is possible because function
> logfile_rotate() is already testing if  errno = ENFILE | EMFILE and in
> this case rotation_disabled is set to true. So the following test
> should do the work:
> 
>if (log_metainfo_stale && !rotation_disabled)
>logfile_writename();
> 
> This is included in v15 patch.

I don't see this helping much, if at all.

First, it's not clear just when rotation_disabled can be false
when log_metainfo_stale is true.  The typical execution
path is for logfile_writename() to be called after rotate_logfile()
has already set rotataion_disabled to true.   logfile_writename()
is the only place setting log_metainfo_stale to true and
rotate_logfile() the only place settig rotation_disabled to false.

While it's possible
that log_metainfo_stale might be set to true when logfile_writename()
is called from within open_csvlogfile(), this does not help with the
stderr case.  IMO better to just test for log_metaifo_stale at the
code snippet above.

Second, the whole point of retrying the logfile_writename() call is
to be sure that the current_logfiles file is updated before the logs
rotate.  Waiting until logfile rotation is enabled defeats the purpose.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> I've attached the v15 of the patch that applies your changes/fixes ...

Per the following:

On Mon, 21 Nov 2016 13:17:17 -0500
Robert Haas  wrote:

> It would really be much better to submit anything that's not
> absolutely necessary for the new feature as a separate patch, rather
> than bundling things together.

The following patch should really be submitted (along with
your patch) as a separate and independent patch:

patch_pg_current_logfile-v14.diff.cleanup_rotate

It introduces no new functionality and only cleans up existing code.
The idea is to give the maintainers only one thing to consider
at a time.  It can be looked at along with your patch since it
touches the same code but, like the GUC symbol patch, it's
not a necessary part of your patch's functionality.

At present patch_pg_current_logfile-v14.diff.cleanup_rotate is bundled
in with the v15 patch.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 28/11/2016 à 18:54, Karl O. Pinc a écrit :
> Hi Gilles,
>
> On Sun, 27 Nov 2016 21:54:46 +0100
> Gilles Darold  wrote:
>
>> I've attached the v15 of the patch that applies your changes/fixes and
>> remove the call to strtok().
> I like the idea of replacing the strtok() call with sscanf()
> but I see problems.
>
> It won't work if log_filename begins with a space because
> (the docs say) that a space in the sscanf() format represents
> any amount of whitespace.
Right

> As written, there's a potential buffer overrun in log_format.
> You could fix this by making log_format as large as lbuffer,
> but this seems clunky.

Sorry, Isee that I forgot to apply the control in number of character
read by sscanf.

The problem about white space make me though that the use of sscanf is
not the right solution, I will rewrite this part but I can not do that
before the end of the week.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org



-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> I've attached the v15 of the patch that applies your changes/fixes and
> remove the call to strtok().

I like the idea of replacing the strtok() call with sscanf()
but I see problems.

It won't work if log_filename begins with a space because
(the docs say) that a space in the sscanf() format represents
any amount of whitespace.

As written, there's a potential buffer overrun in log_format.
You could fix this by making log_format as large as lbuffer,
but this seems clunky.

Regards,


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sun, 27 Nov 2016 21:54:46 +0100
Gilles Darold  wrote:

> Again, patches patch_pg_current_logfile-v14.diff.doc_linux_default-v2
> have not been included because I don't see any reason to talk
> especially about systemd. If you talk about systemd you must talk
> about other stderr handler by all systems. IMO saying that
> current_logfile is present only if logging_collector is enabled and
> log_destination include stderr or/and csvlog is enough, no need to
> talk about systemd and behavior of Linux distributions.

Fair enough.  And I'd sooner not talk about systemd or other such
specifics too.

The concern I'm attempting to address is that the patch touts
the current_logfiles file in the section on logging without
reservation.  But anyone compiling from source or using most
pre-built Linux binaries won't have the file. (And the
pg_current_logfiles() function won't work either.)

Maybe the answer is to  change

When logs are written to the file-system ...

to

When the PostgreSQL backend
database server write logs to the file-system (which is often
not the default configuration) ...

?

Or something.  This also seems verbose, yet incomplete because
although it mentions that the default is to not have the backend
write logs it does not say anything about where to look to change
the default.

The thing is, on most default setups you do get logs written to the
filesystem, but they are not written by the PG backend.

I'll let you (or anyone else who might be concerned) move this forward
because I don't seem to have a good answer.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Hi Karl,

I've attached the v15 of the patch that applies your changes/fixes and
remove the call to strtok().

I've not applied patch patch_pg_current_logfile-v14.diff.backoff to
prevent constant call of logfile_writename() on a busy system (errno =
ENFILE | EMFILE). I think this can be done quite simply by testing if
log rotate is still enabled. This is possible because function
logfile_rotate() is already testing if  errno = ENFILE | EMFILE and in
this case rotation_disabled is set to true. So the following test should
do the work:

   if (log_metainfo_stale && !rotation_disabled)
   logfile_writename();

This is included in v15 patch.

I've also not added patches
patch_pg_current_logfile-v14.diff.conditional,
patch_pg_current_logfile-v14.diff.logdest_change and
patch_pg_current_logfile-v14.diff.logdest_change-part2 because the case
your are trying to fix with lot of code can not appears. You said that
when csv logging is turned back on, the stale csv path is written to
current_logfiles. This is not the case, last_csv_file_name is
immediately changed when the log file is open, see open_csvlogfile().
After running your use case I was not able to reproduce the bug you are
describing.

Again, patches patch_pg_current_logfile-v14.diff.doc_linux_default-v2
have not been included because I don't see any reason to talk especially
about systemd. If you talk about systemd you must talk about other
stderr handler by all systems. IMO saying that current_logfile is
present only if logging_collector is enabled and log_destination include
stderr or/and csvlog is enough, no need to talk about systemd and
behavior of Linux distributions.

Regards

Le 23/11/2016 à 10:21, Karl O. Pinc a écrit :
> Hi Gilles,
>
> On Sat, 19 Nov 2016 12:58:47 +0100
> Gilles Darold  wrote:
>
>> ... attached v14 of the patch. 
> Attached are patches for your consideration and review.
> (Including your latest v14 patch for completeness.)
>
> Some of the attached patches (like the GUC symbol
> patch you've seen before) are marked to be submitted
> as separate patches to the maintainers when we send
> them code for review.  These need looking over by
> somebody, I hope you, before they get sent on so
> please comment on these if you're not going to look
> at them or if you see a problem with them.   (Or if
> you like them.  :)  Thanks.
>
> I also have comments at the bottom regards problems
> I see but haven't patched.
>
> ---
>
> patch_pg_current_logfile-v14.diff
>
> Applies on top of master.
>
> The current patch.
>
> ---
>
> patch_pg_current_logfile-v14.diff.startup_docs
>
> For consideration of inclusion in "main" patch.
>
> Applies on top of patch_pg_current_logfile-v14.diff
>
> A documentation fix.
>
> (Part of) my previous doc patch was wrong; current_logfiles is not
> unconditionally written on startup.
>
> ---
>
> patch_pg_current_logfile-v14.diff.bool_to_int
>
> Do not include in "main" patch, submit to maintainers separately.
>
> Applies on top of patch_pg_current_logfile-v14.diff
>
> The bool types on the stack in logfile_rotate() are
> my work.  Bools on the stack don't make sense as far
> as hardware goes, so the compiler's optimizer should change
> them to int.  This patch changes the bools to ints
> should that be to someone's taste.
>
> ---
>
> patch_pg_current_logfile-v14.diff.logdest_change
>
> For consideration of inclusion in "main" patch.
>
> Applies on top of patch_pg_current_logfile-v14.diff.
>
> Fixes a bug where, when log_destination changes and the config
> file is reloaded, a no-longer-used logfile path may be written
> to the current_logfiles file.  The chain of events would be
> as follows:
>
> 1) PG configured with csvlog in log_destination. Logs are written.
>
> This makes last_csv_file_name non-NULL.
>
>
> 2) PG config changed to remove csvlog from log_destination
> and SIGHUP sent.
>
> This removes the csvlog path from current_logfiles but does not
> make last_csv_file_name NULL.  last_csv_file_name has the old
> path in it.
>
>
> 3) PG configured to add csvlog back to log_destination and
> SIGHUP sent.
>
> When csvlogging is turned back on, the stale csv path is written
> to current_logfiles.  This is overwritten as soon as some csv logs
> are written because the new csv logfile must be opened, but this is
> still a problem.  Even if it happens to be impossible at the moment
> to get past step 2 to step 3 without having some logs written it seems
> a lot more robust to manually "expire" the last*_file_name variable
> content when log_destination changes.
>
>
> So what the patch does is "scrub" the "last_*file_name" variables
> when the config file changes.
>
> FWIW, I moved the logfile_writename() call toward the top of the
> if block just to keep all the code which sorta-kinda involves
> the old_log_destination variable together.
>
> ---
>
> patch_pg_current_logfile-v14.diff.logdest_change-part2
>
> Do not include in "main" patch, submit to maintainers 

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 23 Nov 2016 23:08:18 -0600
"Karl O. Pinc"  wrote:

> On Wed, 23 Nov 2016 03:21:05 -0600
> "Karl O. Pinc"  wrote:
> 
> > On Sat, 19 Nov 2016 12:58:47 +0100
> > Gilles Darold  wrote:
> >   
> > > ... attached v14 of the patch. 
> 
> > patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3  
> 
> > Re-try the write of current_logfiles should it fail because the
> > system is too busy.  

> ---
> 
> patch_pg_current_logfile-v14.diff.doc_linux_default
> 
> Applies on top of
> patch_pg_current_logfile-v14.diff.pg_ctl_and_more_docs
> 
> Mentions, in the body of the docs, defaults and their impact on
> current_logfiles and pg_current_logfiles.  It seems appropriate
> to mention this in the main documentation and in the overall context
> of logging.

Attached is version 2 of this patch.  Adds an index entry.

patch_pg_current_logfile-v14.diff.doc_linux_default-v2

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
 top of
> p

patch_pg_current_logfile-v14.diff.doc_linux_default-v2
Description: Binary data

-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 23 Nov 2016 03:21:05 -0600
"Karl O. Pinc"  wrote:

> On Sat, 19 Nov 2016 12:58:47 +0100
> Gilles Darold  wrote:
> 
> > ... attached v14 of the patch.   

> patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3

> Re-try the write of current_logfiles should it fail because the
> system is too busy.

Attached are 2 more doc patchs.

---

patch_pg_current_logfile-v14.diff.pg_ctl_and_more_docs

Applies on top of
patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3

Mentions pg_ctl and how using it might affect whether logfile
paths are captured in the current_logfiles file.  I think
the default RedHat packaging captures logs this way.  The
Debian default is to ship with logging_collector=off
and to rely on pg_ctrl via pg_ctlcluster to manage
log writing.  Both RH and Debian then pass stderr to systemd
and that does log management.  I don't
recall other distros' practice.  If the default distro
packaging means that current_logfiles/pg_current_logfile()
"don't work" this could be an issue to address in the
documentation.  Certainly the systemd reliance on stderr
capture and it's subsuming of logging functionality could
also be an issue.

Cross reference the current_logfiles file in the pg_current_logfile()
docs.

Marks up stderr appropriately.

Adds index entries.

---

patch_pg_current_logfile-v14.diff.doc_linux_default

Applies on top of
patch_pg_current_logfile-v14.diff.pg_ctl_and_more_docs

Mentions, in the body of the docs, defaults and their impact on
current_logfiles and pg_current_logfiles.  It seems appropriate
to mention this in the main documentation and in the overall context
of logging.

Adds index entries.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v14.diff.pg_ctl_and_more_docs
Description: Binary data


patch_pg_current_logfile-v14.diff.doc_linux_default
Description: Binary data

-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 23 Nov 2016 10:39:28 -0500
Tom Lane  wrote:

> Robert Haas  writes:
> > On Wed, Nov 23, 2016 at 4:21 AM, Karl O. Pinc 
> > wrote:  
> >> The bool types on the stack in logfile_rotate() are
> >> my work.  Bools on the stack don't make sense as far
> >> as hardware goes, so the compiler's optimizer should change
> >> them to int.  This patch changes the bools to ints
> >> should that be to someone's taste.  
> 
> > That does not seem like a good idea from here.  Even if it buys some
> > microscopic speedup, in a place like this it won't matter.  It's
> > more important that the code be clear, and using an int where you
> > really intend a bool isn't an improvement as far as clarity goes.  
> 
> Not to mention that the "bools on the stack don't make sense" premise
> is bogus anyway.

Thanks.  I don't think I've paid attention since 8088 days, just always
used bools.  We'll drop that patch then.

But this reminds me.  I should have used a bool return value.

Attached is a version 2 of
patch_pg_current_logfile-v14.diff.deletion-v2


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


patch_pg_current_logfile-v14.diff.deletion-v2
Description: Binary data

-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 23/11/2016 à 16:26, Tom Lane a écrit :
> "Karl O. Pinc"  writes:
>> Maybe on the 2nd call to strtok() you could pass ""
>> as the 2nd argument?  That'd be a little wonky but
>> the man page does not say you can't have an empty
>> set of delimiters.  On the other hand strtok() is
>> not a perfect choice, you don't want to "collapse"
>> adjacent delimiters in the parsed string or ignore
>> leading spaces.  I'd prefer a strstr() solution.
> I'd stay away from strtok() no matter what.  The process-wide static
> state it implies is dangerous: if you use it, you're betting that
> you aren't interrupting some caller's use, nor will any callee decide
> to use it.  In a system as large as Postgres, that's a bad bet, or
> would be if we didn't discourage use of strtok() pretty hard.
>
> As far as I can find, there are exactly two users of strtok() in
> the backend, and they're already playing with fire because one
> calls the other (look in utils/misc/tzparser.c).  I don't want the
> hazard to get any larger.
>
>   regards, tom lane

Understood, I will remove and replace this call from the patch and more
generally from my programming.

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org



-- 
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] Patch to implement pg_current_logfile() function

[Tom Lane]

Robert Haas  writes:
> On Wed, Nov 23, 2016 at 4:21 AM, Karl O. Pinc  wrote:
>> The bool types on the stack in logfile_rotate() are
>> my work.  Bools on the stack don't make sense as far
>> as hardware goes, so the compiler's optimizer should change
>> them to int.  This patch changes the bools to ints
>> should that be to someone's taste.

> That does not seem like a good idea from here.  Even if it buys some
> microscopic speedup, in a place like this it won't matter.  It's more
> important that the code be clear, and using an int where you really
> intend a bool isn't an improvement as far as clarity goes.

Not to mention that the "bools on the stack don't make sense" premise
is bogus anyway.

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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Wed, Nov 23, 2016 at 4:21 AM, Karl O. Pinc  wrote:
> patch_pg_current_logfile-v14.diff.bool_to_int
>
> Do not include in "main" patch, submit to maintainers separately.
>
> Applies on top of patch_pg_current_logfile-v14.diff
>
> The bool types on the stack in logfile_rotate() are
> my work.  Bools on the stack don't make sense as far
> as hardware goes, so the compiler's optimizer should change
> them to int.  This patch changes the bools to ints
> should that be to someone's taste.

That does not seem like a good idea from here.  Even if it buys some
microscopic speedup, in a place like this it won't matter.  It's more
important that the code be clear, and using an int where you really
intend a bool isn't an improvement as far as clarity goes.

-- 
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] Patch to implement pg_current_logfile() function

[Tom Lane]

"Karl O. Pinc"  writes:
> Maybe on the 2nd call to strtok() you could pass ""
> as the 2nd argument?  That'd be a little wonky but
> the man page does not say you can't have an empty
> set of delimiters.  On the other hand strtok() is
> not a perfect choice, you don't want to "collapse"
> adjacent delimiters in the parsed string or ignore
> leading spaces.  I'd prefer a strstr() solution.

I'd stay away from strtok() no matter what.  The process-wide static
state it implies is dangerous: if you use it, you're betting that
you aren't interrupting some caller's use, nor will any callee decide
to use it.  In a system as large as Postgres, that's a bad bet, or
would be if we didn't discourage use of strtok() pretty hard.

As far as I can find, there are exactly two users of strtok() in
the backend, and they're already playing with fire because one
calls the other (look in utils/misc/tzparser.c).  I don't want the
hazard to get any larger.

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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Wed, 23 Nov 2016 03:21:05 -0600
"Karl O. Pinc"  wrote:

> But also, you can't use strtok() to parse lbuffer because
> the path you're returning can contain a space. 

Maybe on the 2nd call to strtok() you could pass ""
as the 2nd argument?  That'd be a little wonky but
the man page does not say you can't have an empty
set of delimiters.  On the other hand strtok() is
not a perfect choice, you don't want to "collapse"
adjacent delimiters in the parsed string or ignore
leading spaces.  I'd prefer a strstr() solution.

Or strchr() even better.

Regards.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

Hi Gilles,

On Sat, 19 Nov 2016 12:58:47 +0100
Gilles Darold  wrote:

> ... attached v14 of the patch. 

Attached are patches for your consideration and review.
(Including your latest v14 patch for completeness.)

Some of the attached patches (like the GUC symbol
patch you've seen before) are marked to be submitted
as separate patches to the maintainers when we send
them code for review.  These need looking over by
somebody, I hope you, before they get sent on so
please comment on these if you're not going to look
at them or if you see a problem with them.   (Or if
you like them.  :)  Thanks.

I also have comments at the bottom regards problems
I see but haven't patched.

---

patch_pg_current_logfile-v14.diff

Applies on top of master.

The current patch.

---

patch_pg_current_logfile-v14.diff.startup_docs

For consideration of inclusion in "main" patch.

Applies on top of patch_pg_current_logfile-v14.diff

A documentation fix.

(Part of) my previous doc patch was wrong; current_logfiles is not
unconditionally written on startup.

---

patch_pg_current_logfile-v14.diff.bool_to_int

Do not include in "main" patch, submit to maintainers separately.

Applies on top of patch_pg_current_logfile-v14.diff

The bool types on the stack in logfile_rotate() are
my work.  Bools on the stack don't make sense as far
as hardware goes, so the compiler's optimizer should change
them to int.  This patch changes the bools to ints
should that be to someone's taste.

---

patch_pg_current_logfile-v14.diff.logdest_change

For consideration of inclusion in "main" patch.

Applies on top of patch_pg_current_logfile-v14.diff.

Fixes a bug where, when log_destination changes and the config
file is reloaded, a no-longer-used logfile path may be written
to the current_logfiles file.  The chain of events would be
as follows:

1) PG configured with csvlog in log_destination. Logs are written.

This makes last_csv_file_name non-NULL.


2) PG config changed to remove csvlog from log_destination
and SIGHUP sent.

This removes the csvlog path from current_logfiles but does not
make last_csv_file_name NULL.  last_csv_file_name has the old
path in it.


3) PG configured to add csvlog back to log_destination and
SIGHUP sent.

When csvlogging is turned back on, the stale csv path is written
to current_logfiles.  This is overwritten as soon as some csv logs
are written because the new csv logfile must be opened, but this is
still a problem.  Even if it happens to be impossible at the moment
to get past step 2 to step 3 without having some logs written it seems
a lot more robust to manually "expire" the last*_file_name variable
content when log_destination changes.


So what the patch does is "scrub" the "last_*file_name" variables
when the config file changes.

FWIW, I moved the logfile_writename() call toward the top of the
if block just to keep all the code which sorta-kinda involves
the old_log_destination variable together.

---

patch_pg_current_logfile-v14.diff.logdest_change-part2

Do not include in "main" patch, submit to maintainers separately.

Applies on top of patch_pg_current_logfile-v14.diff.logdest_change

Adds a PGLogDestination typedef.  A separate patch since this may be
overkill and more about coding style than anything else.

---

And now, a series of patches to fix the problem where, at least
potentially, logfile_writename() gets a ENFILE or EMFILE and
therefore a log file path does not ever get written to the
current_logfiles file.  The point of this patch series is to retry until
the content of current_logfiles is correct.

All of these are for consideration of inclusion in "main" patch.


patch_pg_current_logfile-v14.diff.retry_current_logfiles-part1

Applies on top of patch_pg_current_logfile-v14.diff.logdest_change

A documentation patch.  Notes that (even with retrying) the
current_logfiles content might not be right.


patch_pg_current_logfile-v14.diff.retry_current_logfiles-part2

Applies on top of
patch_pg_current_logfile-v14.diff.retry_current_logfiles-part1

Remove arguments from logfile_writename().  Use static storage
instead.  We always update last_file_name and last_csv_file_name
whenever logfile_writename() is called so there's not much of
a change here.



patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3

Applies on top of
patch_pg_current_logfile-v14.diff.retry_current_logfiles-part2

Re-try the write of current_logfiles should it fail because the
system is too busy.

---

patch_pg_current_logfile-v14.diff.backoff

Do not include in "main" patch, submit to maintainers separately.

Applies on top of
patch_pg_current_logfile-v14.diff.retry_current_logfiles-part3

Introduces a backoff when retrying to write the current_logfiles file,
because retrying when failure is due to a busy system only makes the
system more busy.  This may well be over-engineering but I thought
I'd present the code and let more experienced people decide.

I have yet to really test this patch.

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Mon, 21 Nov 2016 13:17:17 -0500
Robert Haas  wrote:

> > I've a couple of other patches that do
> > a little cleanup on master that I'd also like to submit
> > along with your patch. 

> It would really be much better to submit anything that's not
> absolutely necessary for the new feature as a separate patch, rather
> than bundling things together.

My plan is separate patches, one email.

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Patch to implement pg_current_logfile() function

[Robert Haas]

On Sat, Nov 19, 2016 at 8:51 AM, Karl O. Pinc  wrote:
> On Sat, 19 Nov 2016 12:58:47 +0100
> Gilles Darold  wrote:
>
>> All patches you've submitted on tha v13 patch have been applied and
>> are present in attached v14 of the patch. I have not included the
>> patches about GUC changes because I'm not sure that adding a new file
>> (include/utils/guc_values.h) just for that will be accepted or that it
>> will not require a more global work to add other GUC values. However
>> perhaps this patch can be submitted separately if the decision is not
>> taken here.
>
> Understood.  I've a couple of other patches that do
> a little cleanup on master that I'd also like to submit
> along with your patch.  This on the theory that
> the maintainers will be looking at this code anyway
> because your patch touches it.  All this can be submitted
> for their review at once.  My approach is to be minimally invasive on
> a per-patch basis (i.e. your patch) but add small patches
> that make existing code "better" without touching
> functionality.  (Deleting unnecessary statements, etc.)
> The overall goal being a better code base.

It would really be much better to submit anything that's not
absolutely necessary for the new feature as a separate patch, rather
than bundling things together.

-- 
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Sat, 19 Nov 2016 18:59:49 +0100
Gilles Darold  wrote:

> Le 19/11/2016 à 16:22, Karl O. Pinc a écrit :
> > Hi Gilles,
> >
> > On Tue, 15 Nov 2016 15:15:52 -0600
> > "Karl O. Pinc"  wrote:
> >  
> >>> On Mon, 7 Nov 2016 23:29:28 +0100
> >>> Gilles Darold  wrote:
>    - Do not write current_logfiles when log_collector is activated
>  but log_destination doesn't contained stderr or csvlog. This was
>  creating an empty file that can confuse the user.   
> >> Whether to write current_logfiles only when there's a stderr or
> >> csvlog seems dependent up on whether the current_logfiles file is
> >> for human or program use.  If for humans, don't write it unless it
> >> has content. If for programs, why make the program always have to
> >> check to see if the file exists before reading it?  Failing to
> >> check is just one more cause of bugs.  
> > What are your thoughts on this?  I'm leaning toward current_logfiles
> > being for programs, not people.  So it should be present whenever
> > logging_collector is on.  
> 
> My though is that it is better to not have an empty file even if
> log_collector is on.

Thanks.   I wrote to be sure that you'd considered my thoughts.

I don't see a point in further discussion.  I may submit a separate
patch to the maintainers when we're ready to send them code so they can
see how the code looks with current_logfiles always in existence.

Further thoughts below.  No need to read them or respond.

> Programs can not be confused but human yes, if
> the file is present but empty, someone may want to check why this
> file is empty.

IMO if they want to know why it's empty they could read the
docs.

> Also having a file containing two lines with just the
> log format without path is worst for confusion than having an empty
> file.

I agree that it makes no sense to list log formats without paths.
 
> In other words, from a program point of view, to gather last log
> filename, existence of the file or not doesn't make any difference.
> This is the responsibility of the programer to cover all cases. In a
> non expert user point of view, there will always the question: why
> this file is empty? If the file is not present, the question will
> stay: how I to get current log filename?

As a non-expert looking at the default data_directory (etc.) almost
nothing makes sense.  I see the non-expert using the
pg_current_logfile() function from within Postgres.

I also see a lot of non-experts writing program to get log data and
then getting errors when the config changes and current_logfile goes
away.  Not having data in a opened file generally means a program
does nothing. an appropriate action when there are no log files
in the filesystem.  But not accounting for a non-existent file leads
to crashes.

Regards,


Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


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