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

On Sat, Jan 14, 2017 at 10:02 PM, Gilles Darold
<gilles.darold@dalibo.com> wrote:
> Le 13/01/2017 à 14:09, Michael Paquier a écrit :
>> On Fri, Jan 13, 2017 at 5:48 PM, Gilles Darold <gilles.darold@dalibo.com> wrote:
>>> Le 13/01/2017 à 05:26, Michael Paquier a écrit :
>>>> 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?
>> protocol.sgml, in the protocol-replication part. You could search for
>> the paragraph that contains postmaster.opts. There is no real harm in
>> including current_logfiles in base backups, but that's really in the
>> same bag as postmaster.opts or postmaster.pid, particularly if the log
>> file name has a timestamp.
>
> Thanks for the pointer. After reading this part I think it might only
> concern files that are critical at restore time. I still think that the
> file might not be removed automatically from the backup. If it is
> restored it has strictly no impact at all, it will be removed or
> overwritten at startup. We can let the user choose to remove it from the
> backup or not, the file can be an help to find the latest log file
> related to a backup.

OK, no problem for me. I can see that your patch does the right thing
to ignore the current_logfiles.tmp. Could you please update
t/010_pg_basebackup.pl and add this file to the list of files filled
with DONOTCOPY?

>>>> 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.
>> log_directory could be an absolute path, and we surely don't want to
>> let normal users have a look at it. For example, "show log_directory"
>> can only be seen by superusers. As Stephen Frost is for a couple of
>> months (years?) on a holly war path against super-user checks in
>> system functions, I think that revoking the access to this function is
>> the best thing to do. It is as well easier to restrict first and
>> relax, the reverse is harder to justify from a compatibility point of
>> view.
>
> Right, I've append a REVOKE to the function in file
> backend/catalog/system_views.sql, patch v21 reflect this change.

Thanks. That looks good.

+   {
+       /* Logging collector is not enabled. We don't know where messages are
+        * logged.  Remove outdated file holding the current log filenames.
+        */
+       unlink(LOG_METAINFO_DATAFILE);       return 0
This comment format is not postgres-like.

I think that's all I have for this patch.
--
Michael