Re: Patch to implement pg_current_logfile() function

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