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

On Thu, Jan 12, 2017 at 9:14 PM, Gilles Darold <gilles.darold@dalibo.com> wrote:
> Le 11/01/2017 à 08:39, Michael Paquier a écrit :
>> On Wed, Dec 14, 2016 at 12:19 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold <gilles.darold@dalibo.com> 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().

+    <para>
+    When logs are written to the file-system their paths, names, and
+    types are recorded in
+    the <xref linkend="storage-pgdata-current-logfiles"> file.  This provides
+    a convenient way to find and access log content without establishing a
+    database connection.
+    </para>
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 <xref
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."

@@ -15441,6 +15441,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY
AS t(ls,n);      </row>
      <row>
+       <entry><literal><function>pg_current_logfile()</function></literal></entry>
+       <entry><type>text</type></entry>
+       <entry>primary log file name in use by the logging collector</entry>
+      </row>
+
+      <row>
+       <entry><literal><function>pg_current_logfile(<type>text</>)</function></literal></entry>
+       <entry><type>text</type></entry>
+       <entry>log file name, of log in the requested format, in use by the
+       logging collector</entry>
+      </row>
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);externvoid 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 <literal>NULL</literal>.  When multiple logfiles exist, each in a
+    different format, <function>pg_current_logfile</function> 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