Re: [HACKERS] Patch to implement pg_current_logfile() function - Mailing list pgsql-hackers

On Fri, Jan 20, 2017 at 2:09 AM, Michael Paquier
<michael.paquier@gmail.com> 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