Re: Patch to implement pg_current_logfile() function

Le 15/01/2017 à 06:54, Michael Paquier a écrit :
> On Sat, Jan 14, 2017 at 10:02 PM, Gilles Darold
> <gilles(dot)darold(at)dalibo(dot)com> wrote:
>> Le 13/01/2017 à 14:09, Michael Paquier a écrit :
>>> On Fri, Jan 13, 2017 at 5:48 PM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)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?
Applied in attached patch v22.

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

Fixed too.

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org