Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
"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
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
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
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
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
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
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
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 tables, such as pg_database
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
"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
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
"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
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
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
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
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
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
> 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
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 07f291b7cd..cbeeab80b5 100644 --- a/src/backend/catalog/system_views.sql
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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
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
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
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
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
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
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
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 @@ static char *logfile_getnam
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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
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
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
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
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 public; diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/sysl
Re: [HACKERS] Patch to implement pg_current_logfile() function
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 void set_next_rotation_time(void); static
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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
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
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
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
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
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
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
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
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
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
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
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 +++ b/doc/src/sgm
Re: [HACKERS] Patch to implement pg_current_logfile() function
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 including the + value of . The log format must be present +
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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 text, either csvlog +or stderr as the value of the
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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 to by the syslogger + and t
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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
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
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
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
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
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 rotation_requested is false, log_destination may have +
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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
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 stderr logging. +Non-Po
Re: [HACKERS] Patch to implement pg_current_logfile() function
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 + value of . The log format must
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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
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, &stat_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 ' ' specifically. > Overwrite
Re: [HACKERS] Patch to implement pg_current_logfile() function
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 files show > up in your logging direct
Re: [HACKERS] Patch to implement pg_current_logfile() function
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 -- which you'll
Re: [HACKERS] Patch to implement pg_current_logfile() function
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 unprincipled reaction to a purely arbitrary > subset of
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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, &stat_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, and why do either? T
Re: [HACKERS] Patch to implement pg_current_logfile() function
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 + + current_logfiles + + +
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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
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
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
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
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 separately. > > Applies on
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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
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
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
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
"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
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
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. --- The remaining patches h
Re: [HACKERS] Patch to implement pg_current_logfile() function
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
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
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