|From:||Michael Paquier <michael(dot)paquier(at)gmail(dot)com>|
|To:||Gilles Darold <gilles(dot)darold(at)dalibo(dot)com>|
|Cc:||Robert Haas <robertmhaas(at)gmail(dot)com>, "Karl O(dot) Pinc" <kop(at)meme(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Patch to implement pg_current_logfile() function|
|Views:||Raw Message | Whole Thread | Download mbox|
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...
> 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
+ 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.
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
+ <entry>primary log file name in use by the logging collector</entry>
+ <entry>log file name, of log in the requested format, in use by the
+ logging collector</entry>
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
+/* 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".
@@ -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 */
+ * 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.
I know I am famous for being noisy on such things, but please be
careful with newline use..
+ /* Unknown format, no space. Return NULL to caller. */
+ lbuffer = '\0';
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
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 :(
|Next Message||Mithun Cy||2017-01-13 04:28:16||Re: Cache Hash Index meta page.|
|Previous Message||Michael Paquier||2017-01-13 03:32:12||Re: Patch to implement pg_current_logfile() function|