Re: 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.

> + <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."
Changes applied. Output example added:

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

> @@ -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".
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 <literal>NULL</literal>. When multiple logfiles exist, each in a
> + different format, <function>pg_current_logfile</function> 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