current_logfiles not following group access and instead follows log_file_mode permissions

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

current_logfiles not following group access and instead follows log_file_mode permissions

Haribabu Kommi-2

current_logfiles is a meta data file, that stores the current log writing file, and this file
presents in the data directory. This file doesn't follow the group access mode set at
the initdb time, but it follows the log_file_mode permissions.

without group access permissions, backup with group access can lead to failure.
Attached patch fix the problem.

comments?

Regards,
Haribabu Kommi
Fujitsu Australia

0001-current_logfiles-file-following-group-access-mode.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Michael Paquier-2
On Tue, Jan 15, 2019 at 03:08:41PM +1100, Haribabu Kommi wrote:
> current_logfiles is a meta data file, that stores the current log writing
> file, and this file presents in the data directory. This file
> doesn't follow the group access mode set at the initdb time, but it
> follows the log_file_mode permissions.
>
> Without group access permissions, backup with group access can lead to
> failure.  Attached patch fix the problem.

initdb enforces log_file_mode to 0640 when using the group mode, still
if one enforces the parameter value then current_logfiles would just
stick with it.  This is not really user-friendly.  This impacts also
normal log files as these get included in base backups if the log path
is within the data folder (not everybody uses an absolute path out of
the data folder for the logs).

One way to think about this is that we may want to worry also about
normal log files and document that one had better be careful with the
setting of log_file_mode?  Still, as we are talking about a file
aiming at storing meta-data for log files, something like what you
suggest can make sense.

When discussing about pg_current_logfile(), I raised the point about
not including as well in base backups which would also address the
problem reported here.  However we decided to keep it because it can
be helpful to know what's the last log file associated to a base
backup for debugging purposes:
https://www.postgresql.org/message-id/50b58f25-ab07-f6bd-7a68-68f29f214ce9@...

Instead of what you are proposing, why not revisiting that and just
exclude the file from base backups.  I would be in favor of just doing
that instead of switching the file's permission from log_file_mode to
pg_file_create_mode.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Haribabu Kommi-2

On Tue, Jan 15, 2019 at 4:15 PM Michael Paquier <[hidden email]> wrote:
On Tue, Jan 15, 2019 at 03:08:41PM +1100, Haribabu Kommi wrote:
> current_logfiles is a meta data file, that stores the current log writing
> file, and this file presents in the data directory. This file
> doesn't follow the group access mode set at the initdb time, but it
> follows the log_file_mode permissions.
>
> Without group access permissions, backup with group access can lead to
> failure.  Attached patch fix the problem.

initdb enforces log_file_mode to 0640 when using the group mode, still
if one enforces the parameter value then current_logfiles would just
stick with it.  This is not really user-friendly.  This impacts also
normal log files as these get included in base backups if the log path
is within the data folder (not everybody uses an absolute path out of
the data folder for the logs).

we got this problem when the log_file_mode is set 0600 but the database
file are with group access permissions. In our scenario, the log files are
outside the data folder, so we faced the problem with current_logfiles
file.
 
One way to think about this is that we may want to worry also about
normal log files and document that one had better be careful with the
setting of log_file_mode?  Still, as we are talking about a file
aiming at storing meta-data for log files, something like what you
suggest can make sense.

Yes, with log_file_mode less than 0640 containing the log files inside
the data directory can leads to backup failure. Yes, providing extra
information about group access when log_file_mode is getting chosen.

Another option is how about not letting user to choose less than 0640
when the group access mode is enabled?

 
When discussing about pg_current_logfile(), I raised the point about
not including as well in base backups which would also address the
problem reported here.  However we decided to keep it because it can
be helpful to know what's the last log file associated to a base
backup for debugging purposes:
https://www.postgresql.org/message-id/50b58f25-ab07-f6bd-7a68-68f29f214ce9@...

Instead of what you are proposing, why not revisiting that and just
exclude the file from base backups.  I would be in favor of just doing
that instead of switching the file's permission from log_file_mode to
pg_file_create_mode.

I am not sure how much useful having the details of the log file in the backup.
It may be useful when there is any problem with backup.

Excluding the file in the backup can solve the problem of backup by an
unprivileged user. Is there any scenarios it can cause problems if it
doesn't follow the group access mode?

Regards,
Haribabu Kommi
Fujitsu Australia
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Tom Lane-2
Haribabu Kommi <[hidden email]> writes:
> Excluding the file in the backup can solve the problem of backup by an
> unprivileged user. Is there any scenarios it can cause problems if it
> doesn't follow the group access mode?

The point of this file, as I understood it, was to allow someone who's
allowed to read the log files to find out which one is the latest.  It
makes zero sense for it to have different permissions from the log files,
because doing that would break its only use-case.

I am wondering what is the use-case for a backup arrangement that's so
fragile it can't cope with varying permissions in the data directory.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Tom Lane-2
I wrote:
> Haribabu Kommi <[hidden email]> writes:
>> Excluding the file in the backup can solve the problem of backup by an
>> unprivileged user. Is there any scenarios it can cause problems if it
>> doesn't follow the group access mode?

> The point of this file, as I understood it, was to allow someone who's
> allowed to read the log files to find out which one is the latest.  It
> makes zero sense for it to have different permissions from the log files,
> because doing that would break its only use-case.

On reflection, maybe the problem is not that we're giving the file
the wrong permissions, but that we're putting it in the wrong place?
That is, seems like it should be in the logfile directory not the
data directory.  That would certainly simplify the intended use-case,
and it would fix this complaint too.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Michael Paquier-2
On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
> On reflection, maybe the problem is not that we're giving the file
> the wrong permissions, but that we're putting it in the wrong place?
> That is, seems like it should be in the logfile directory not the
> data directory.  That would certainly simplify the intended use-case,
> and it would fix this complaint too.

Yeah, thinking more on this one using for this file different
permissions than the log files makes little sense, so what you propose
here seems like a sensible thing to do things.  Even if we exclude the
file from native BASE_BACKUP this would not solve the case of custom
backup solutions doing their own copy of things, when they rely on
group-read permissions.  This would not solve completely the problem
anyway if log files are in the data folder, but it would address the
case where the log files are in an absolute path out of the data
folder.

I am adding in CC Gilles who implemented current_logfiles for his
input.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Stephen Frost
Greetings,

* Michael Paquier ([hidden email]) wrote:

> On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
> > On reflection, maybe the problem is not that we're giving the file
> > the wrong permissions, but that we're putting it in the wrong place?
> > That is, seems like it should be in the logfile directory not the
> > data directory.  That would certainly simplify the intended use-case,
> > and it would fix this complaint too.
>
> Yeah, thinking more on this one using for this file different
> permissions than the log files makes little sense, so what you propose
> here seems like a sensible thing to do things.  Even if we exclude the
> file from native BASE_BACKUP this would not solve the case of custom
> backup solutions doing their own copy of things, when they rely on
> group-read permissions.  This would not solve completely the problem
> anyway if log files are in the data folder, but it would address the
> case where the log files are in an absolute path out of the data
> folder.
Actually, I agree with the initial patch on the basis that this file
that's being created (which I'm honestly a bit amazed that we're doing
this way; certainly seems rather grotty to me) is surely not an actual
*log* file and therefore using logfile_open() to open it doesn't seem
quite right.  I would have hoped for a way to pass this information that
didn't involve a file at all, but I'll assume that was discussed already
and good reasons put forth as to why we can't avoid it.

I'm not really sure putting it into the logfile directory is such a hot
idea as users might have set up external log file rotation of files in
that directory.  Of course, in that case they'd probably signal PG right
afterwards and PG would go write out a new file, but it still seems
pretty awkward.  I'm not terribly against solving this issue that way
either though, but I tend to think the originally proposed patch is more
sensible.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Tom Lane-2
Stephen Frost <[hidden email]> writes:
> * Michael Paquier ([hidden email]) wrote:
>> On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
>>> On reflection, maybe the problem is not that we're giving the file
>>> the wrong permissions, but that we're putting it in the wrong place?

> I'm not really sure putting it into the logfile directory is such a hot
> idea as users might have set up external log file rotation of files in
> that directory.  Of course, in that case they'd probably signal PG right
> afterwards and PG would go write out a new file, but it still seems
> pretty awkward.  I'm not terribly against solving this issue that way
> either though, but I tend to think the originally proposed patch is more
> sensible.

I dunno, I think that the current design was made without any thought
whatsoever about the log-files-outside-the-data-directory case.  If
you're trying to set things up that way, it's because you want to give
logfile read access to people who shouldn't be able to look into the
data directory proper.  That makes current_logfiles pretty useless
to such people, as it's now designed.

Now, if the expectation is that current_logfiles is just an internal
working file that users shouldn't access directly, then this argument
is wrong --- but then why is it documented in user-facing docs?

If we're going to accept the patch as-is, then it logically follows
that we should de-document current_logfiles, because we're taking the
position that it's an internal temporary file not meant for user access.

I don't really believe your argument about log rotation: a rotator
would presumably be configured either to pay attention to file name
patterns (which current_logfiles wouldn't match) or to file age
(which current_logfiles shouldn't satisfy either, since it's always
rewritten when we switch logfiles).

If we wanted to worry about that case, a possible solution is to make the
current_logfiles pathname user-configurable so it could be put in some
third directory.  But I think that adds more complexity than is justified
--- and not just for us, but for programs trying to find and use
current_logfiles.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Stephen Frost
Greetings,

* Tom Lane ([hidden email]) wrote:

> Stephen Frost <[hidden email]> writes:
> > * Michael Paquier ([hidden email]) wrote:
> >> On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
> >>> On reflection, maybe the problem is not that we're giving the file
> >>> the wrong permissions, but that we're putting it in the wrong place?
>
> > I'm not really sure putting it into the logfile directory is such a hot
> > idea as users might have set up external log file rotation of files in
> > that directory.  Of course, in that case they'd probably signal PG right
> > afterwards and PG would go write out a new file, but it still seems
> > pretty awkward.  I'm not terribly against solving this issue that way
> > either though, but I tend to think the originally proposed patch is more
> > sensible.
>
> I dunno, I think that the current design was made without any thought
> whatsoever about the log-files-outside-the-data-directory case.  If
> you're trying to set things up that way, it's because you want to give
> logfile read access to people who shouldn't be able to look into the
> data directory proper.  That makes current_logfiles pretty useless
> to such people, as it's now designed.
... or you just want to move the log files to a more sensible location
than the data directory.  The justification for log_file_mode existing
is because you might want to have log files with different privileges,
but that's quite a different thing.

> Now, if the expectation is that current_logfiles is just an internal
> working file that users shouldn't access directly, then this argument
> is wrong --- but then why is it documented in user-facing docs?

I really couldn't say why it's documented in the user-facing docs, and
for my 2c I don't really think it should be- there's a function to get
that information.  Sprinkling the data directory with files for users to
access directly doesn't exactly fit my view of what a good API looks
like.

The fact that there isn't any discussion about where that file actually
lives does make me suspect you're right that log files outside the data
directory wasn't really contemplated.

> If we're going to accept the patch as-is, then it logically follows
> that we should de-document current_logfiles, because we're taking the
> position that it's an internal temporary file not meant for user access.

... and hopefully we'd get rid of it one day entirely.

> I don't really believe your argument about log rotation: a rotator
> would presumably be configured either to pay attention to file name
> patterns (which current_logfiles wouldn't match) or to file age
> (which current_logfiles shouldn't satisfy either, since it's always
> rewritten when we switch logfiles).

Yes, a good pattern would avoid picking up on this file and most are
configured that way (though they are maybe not as specific as you might
think- the default here is just /var/log/postgresql/*.log).

> If we wanted to worry about that case, a possible solution is to make the
> current_logfiles pathname user-configurable so it could be put in some
> third directory.  But I think that adds more complexity than is justified
> --- and not just for us, but for programs trying to find and use
> current_logfiles.

I'd much rather move to get rid of that file rather than increase its
visability- programs should be using the provided function.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Haribabu Kommi-2

On Thu, Jan 17, 2019 at 5:49 AM Stephen Frost <[hidden email]> wrote:
Greetings,

* Tom Lane ([hidden email]) wrote:
> Stephen Frost <[hidden email]> writes:
> > * Michael Paquier ([hidden email]) wrote:
> >> On Tue, Jan 15, 2019 at 10:53:30AM -0500, Tom Lane wrote:
> >>> On reflection, maybe the problem is not that we're giving the file
> >>> the wrong permissions, but that we're putting it in the wrong place?
>
> > I'm not really sure putting it into the logfile directory is such a hot
> > idea as users might have set up external log file rotation of files in
> > that directory.  Of course, in that case they'd probably signal PG right
> > afterwards and PG would go write out a new file, but it still seems
> > pretty awkward.  I'm not terribly against solving this issue that way
> > either though, but I tend to think the originally proposed patch is more
> > sensible.
>
> I dunno, I think that the current design was made without any thought
> whatsoever about the log-files-outside-the-data-directory case.  If
> you're trying to set things up that way, it's because you want to give
> logfile read access to people who shouldn't be able to look into the
> data directory proper.  That makes current_logfiles pretty useless
> to such people, as it's now designed.

... or you just want to move the log files to a more sensible location
than the data directory.  The justification for log_file_mode existing
is because you might want to have log files with different privileges,
but that's quite a different thing.

Thanks for sharing your opinions.

The current_logfiles is used to store the meta data information of current 
writing log files, that is different to log files, so giving permissions of the
log file may not be correct,

> Now, if the expectation is that current_logfiles is just an internal
> working file that users shouldn't access directly, then this argument
> is wrong --- but then why is it documented in user-facing docs?

I really couldn't say why it's documented in the user-facing docs, and
for my 2c I don't really think it should be- there's a function to get
that information.  Sprinkling the data directory with files for users to
access directly doesn't exactly fit my view of what a good API looks
like.

The fact that there isn't any discussion about where that file actually
lives does make me suspect you're right that log files outside the data
directory wasn't really contemplated.

I can only think of reading this file by the user directly when the server
is not available, but I don't find any scenario where that is required?

 
> If we're going to accept the patch as-is, then it logically follows
> that we should de-document current_logfiles, because we're taking the
> position that it's an internal temporary file not meant for user access.

... and hopefully we'd get rid of it one day entirely.

If there is no use of it when server is offline, it will be better to remove that
file with an alternative to provide the current log file name.

With group access mode, the default value of log_file_mode is changed,
Attached patch reflects the same in docs.

Regards,
Haribabu Kommi
Fujitsu Australia

0001-log_file_mode-default-value-update.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Stephen Frost
Greetings,

* Haribabu Kommi ([hidden email]) wrote:

> On Thu, Jan 17, 2019 at 5:49 AM Stephen Frost <[hidden email]> wrote:
> > * Tom Lane ([hidden email]) wrote:
> > > Now, if the expectation is that current_logfiles is just an internal
> > > working file that users shouldn't access directly, then this argument
> > > is wrong --- but then why is it documented in user-facing docs?
> >
> > I really couldn't say why it's documented in the user-facing docs, and
> > for my 2c I don't really think it should be- there's a function to get
> > that information.  Sprinkling the data directory with files for users to
> > access directly doesn't exactly fit my view of what a good API looks
> > like.
> >
> > The fact that there isn't any discussion about where that file actually
> > lives does make me suspect you're right that log files outside the data
> > directory wasn't really contemplated.
>
> I can only think of reading this file by the user directly when the server
> is not available, but I don't find any scenario where that is required?
Yeah, I agree, and if the server isn't running then there really isn't
a "current" logfile, as defined, since the server isn't writing to any
particular log file.

> > > If we're going to accept the patch as-is, then it logically follows
> > > that we should de-document current_logfiles, because we're taking the
> > > position that it's an internal temporary file not meant for user access.
> >
> > ... and hopefully we'd get rid of it one day entirely.
>
> If there is no use of it when server is offline, it will be better to
> remove that
> file with an alternative to provide the current log file name.

It'd probably be good to give folks an opportunity to voice their
opinion regarding their use-case for the file existing as it does and
being documented as it is.  At first blush, to me anyway, it seems like
maybe this was a case of "over-documenting" of the feature by including
in user-facing documentation something that was really there for
internal reasons, but I could certainly be wrong and maybe there's a
reason why it's really necessary to have the file around for users.

> With group access mode, the default value of log_file_mode is changed,
> Attached patch reflects the same in docs.

Yes, we should update the documentation in this regard, though it's
really an independent thing as that documentation should have been
updated in the original group-access patch, so I'll see about fixing
it and back-patching it.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Michael Paquier-2
On Fri, Jan 18, 2019 at 09:50:40AM -0500, Stephen Frost wrote:
> It'd probably be good to give folks an opportunity to voice their
> opinion regarding their use-case for the file existing as it does and
> being documented as it is.  At first blush, to me anyway, it seems like
> maybe this was a case of "over-documenting" of the feature by including
> in user-facing documentation something that was really there for
> internal reasons, but I could certainly be wrong and maybe there's a
> reason why it's really necessary to have the file around for users.

It's not only that.  By keeping the file in its current location, we
can prevent base backups to work even if logs files are out of the
data folder, which is rather user-friendly, and I think that advanced
users of Postgres are careful enough to split log files and main data
folders into different partitions, without symlinks from the data
folder to the log location and with log_directory set to an absolute
path, independent of the rest.  So moving current_logfiles out of the
data folder to the base location of the log paths makes quite some
sense in my opinion for consistency.

Using a new GUC to specify where current_logfiles should be located
does not really justify the code complications in my opinion, and I'd
think that we should allow users with log file access to still look at
it, even manually and connected from the host as this can be useful
for debugging purposes (sometimes clocks of systems get changed as
they are not all the time going throuhg ntpd).
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Stephen Frost
Greetings,

* Michael Paquier ([hidden email]) wrote:

> On Fri, Jan 18, 2019 at 09:50:40AM -0500, Stephen Frost wrote:
> > It'd probably be good to give folks an opportunity to voice their
> > opinion regarding their use-case for the file existing as it does and
> > being documented as it is.  At first blush, to me anyway, it seems like
> > maybe this was a case of "over-documenting" of the feature by including
> > in user-facing documentation something that was really there for
> > internal reasons, but I could certainly be wrong and maybe there's a
> > reason why it's really necessary to have the file around for users.
>
> It's not only that.  By keeping the file in its current location, we
> can prevent base backups to work even if logs files are out of the
> data folder, which is rather user-friendly, and I think that advanced
> users of Postgres are careful enough to split log files and main data
> folders into different partitions, without symlinks from the data
> folder to the log location and with log_directory set to an absolute
> path, independent of the rest.  So moving current_logfiles out of the
> data folder to the base location of the log paths makes quite some
> sense in my opinion for consistency.
As discussed up-thread, if we change current_logfiles to work the way
the rest of our data files do, then base backups would work fine with
the file in its current location.  I don't buy how having that file in
the logfiles directory is more "consistent" with anything either- it's
certainly not a log file itself.

> Using a new GUC to specify where current_logfiles should be located
> does not really justify the code complications in my opinion, and I'd
> think that we should allow users with log file access to still look at
> it, even manually and connected from the host as this can be useful
> for debugging purposes (sometimes clocks of systems get changed as
> they are not all the time going throuhg ntpd).

I agree that we don't need a new GUC for this.  I also don't really see
the use-case for this file being directly exposed to users- we have a
function specifically for this information and that's generally how
users should expect to get information like this- or like what the log
directory *is* to begin with, or where other files reside... I sure hope
that we aren't suggesting that asking users to write a parser for
postgresql.conf, with include directories and files, able to also handle
postgresql.auto.conf, is somehow user-friendly.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Michael Paquier-2
In reply to this post by Stephen Frost
On Fri, Jan 18, 2019 at 09:50:40AM -0500, Stephen Frost wrote:
> Yes, we should update the documentation in this regard, though it's
> really an independent thing as that documentation should have been
> updated in the original group-access patch, so I'll see about fixing
> it and back-patching it.

Stephen, could you apply Hari's patch then?  I am not sure what the
consensus is, but documenting the restriction is the minimum we can
do.

-    The default permissions are <literal>0600</literal>, meaning only the
-    server owner can read or write the log files.  The other commonly
-    useful setting is <literal>0640</literal>, allowing members of the owner's
-    group to read the files.  Note however that to make use of such a
-    setting, you'll need to alter <xref linkend="guc-log-directory"/> to
-    store the files somewhere outside the cluster data directory.  In
-    any case, it's unwise to make the log files world-readable, since
-    they might contain sensitive data.
+    The default permissions are either <literal>0600</literal>, meaning only the
+    server owner can read or write the log files or <literal>0640</literal>, that
+    allows any user in the same group can read the log files, based on the new
+    cluster created with <option>--allow-group-access</option> option of <command>initdb</command>
+    command. Note however that to make use of any setting other than default,
+    you'll need to alter <xref linkend="guc-log-directory"/> to store the files
+    somewhere outside the cluster data directory.

I would formulate that differently, by just adding an extra paragraph
to mention that using <literal>0640</literal> is recommended to be
compatible with initdb's --allow-group-access instead of sticking it
on the middle of the existing paragraph.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Haribabu Kommi-2

On Fri, Feb 1, 2019 at 7:22 PM Michael Paquier <[hidden email]> wrote:
On Fri, Jan 18, 2019 at 09:50:40AM -0500, Stephen Frost wrote:
> Yes, we should update the documentation in this regard, though it's
> really an independent thing as that documentation should have been
> updated in the original group-access patch, so I'll see about fixing
> it and back-patching it.

Stephen, could you apply Hari's patch then?  I am not sure what the
consensus is, but documenting the restriction is the minimum we can
do.

-    The default permissions are <literal>0600</literal>, meaning only the
-    server owner can read or write the log files.  The other commonly
-    useful setting is <literal>0640</literal>, allowing members of the owner's
-    group to read the files.  Note however that to make use of such a
-    setting, you'll need to alter <xref linkend="guc-log-directory"/> to
-    store the files somewhere outside the cluster data directory.  In
-    any case, it's unwise to make the log files world-readable, since
-    they might contain sensitive data.
+    The default permissions are either <literal>0600</literal>, meaning only the
+    server owner can read or write the log files or <literal>0640</literal>, that
+    allows any user in the same group can read the log files, based on the new
+    cluster created with <option>--allow-group-access</option> option of <command>initdb</command>
+    command. Note however that to make use of any setting other than default,
+    you'll need to alter <xref linkend="guc-log-directory"/> to store the files
+    somewhere outside the cluster data directory.

I would formulate that differently, by just adding an extra paragraph
to mention that using <literal>0640</literal> is recommended to be
compatible with initdb's --allow-group-access instead of sticking it
on the middle of the existing paragraph.

Thanks for the review.
I changed the log_file_mode doc patch as per your comment.

How about the attached?

And regarding current_logfiles permissions, I feel this file should have
permissions of data directory files as it is present in the data directory
whether it stores the information of log file, until this file is completely
removed with another approach to store the log file details.

I am not sure whether this has been already discussed or not? How about
using shared memory to store the log file names? So that we don't need
of this file?

Regards,
Haribabu Kommi
Fujitsu Australia

0001-log_file_mode-recommended-value-update.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Haribabu Kommi-2

On Mon, Feb 4, 2019 at 12:16 PM Haribabu Kommi <[hidden email]> wrote:

And regarding current_logfiles permissions, I feel this file should have
permissions of data directory files as it is present in the data directory
whether it stores the information of log file, until this file is completely
removed with another approach to store the log file details.

I am not sure whether this has been already discussed or not? How about
using shared memory to store the log file names? So that we don't need
of this file?

I checked the code why the current_logfiles is not implemented as shared memory
and found that the current syslogger doesn't attach to the shared memory of the
postmaster. To support storing the current_logfiles in shared memory, the syslogger
process also needs to attach to the shared memory, this seems to be a new infrastructure
change.

In case if we are not going to change the permissions of the file to group access mode
instead of if we strict with log_file_mode, I just tried the attached patch of moving the
current_logfiles patch to the log_directory. The only drawback of this approach, is incase
if the user changes the log_directory, the current_logfiles is present in the old log_directory.
I don't see that as a problem.

comments?

Regards,
Haribabu Kommi
Fujitsu Australia

0001-Move-the-current_logfiles-file-into-log_directory.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Michael Paquier-2
On Tue, Feb 26, 2019 at 12:22:53PM +1100, Haribabu Kommi wrote:
> I checked the code why the current_logfiles is not implemented as
> shared memory and found that the current syslogger doesn't attach to
> the shared memory of the postmaster. To support storing the
> current_logfiles in shared memory, the syslogger process also needs
> to attach to the shared memory, this seems to be a new
> infrastructure change.

I don't think you can do that anyway and we should not do it.  Shared
memory can be reset after a backend exits abnormally, but the
syslogger lives across that.  What you sent upthread to improve the
documentation is in my opinion sufficient:
https://www.postgresql.org/message-id/CAJrrPGe-v2_LMFD9nHrBEjJy3vVOKJwY3w_h+Fs2nxCJg3PbaA@...

I would not have split the paragraph you broke into two, but instead
just add this part in-between:
+       <para>
+        Permissions <literal>0640</literal> are recommended to be compatible with
+        <application>initdb</application> option <option>--allow-group-access</option>.
+       </para>
Any objections in doing that?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Robert Haas
On Tue, Mar 12, 2019 at 2:03 AM Michael Paquier <[hidden email]> wrote:

> On Tue, Feb 26, 2019 at 12:22:53PM +1100, Haribabu Kommi wrote:
> > I checked the code why the current_logfiles is not implemented as
> > shared memory and found that the current syslogger doesn't attach to
> > the shared memory of the postmaster. To support storing the
> > current_logfiles in shared memory, the syslogger process also needs
> > to attach to the shared memory, this seems to be a new
> > infrastructure change.
>
> I don't think you can do that anyway and we should not do it.  Shared
> memory can be reset after a backend exits abnormally, but the
> syslogger lives across that.

I think we should do what Haribabu proposed originally.  Moving
current_logfiles out of the data directory doesn't make sense,
because:

(1) If you're trying to find the log files, having a file that
contains their pathnames in the place where those files are does not
help you.  Having such a file in the known location, namely the data
directory, does.

(2) Someone might have logs from multiple PostgreSQL clusters in the
same external log directory, but there can only be one file named
current_logfiles.

(3) Someone might store PostgreSQL log files in the same directory as
non-PostgreSQL log files, and having a file called current_logfiles
floating around will be confusingly ambiguous.

On the other hand, changing the file to have the same permissions as
everything else in the data directory has basically no disadvantages.
I agree with Stephen's analysis that a file containing the names of
the current log files is not itself a log file.  Tom's idea that
making the permissions consistent with everything else in the data
directory would "break its only use-case" seems completely wrong.
Anybody who has permission to read the log files but not the data
directory will presumably hit the directory-level permissions on
$PGDATA before the issue of the permissions on current_logfiles() per
se become relevant, except in corner cases that I don't care about.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Michael Paquier-2
On Tue, Mar 12, 2019 at 04:08:53PM -0400, Robert Haas wrote:
> Anybody who has permission to read the log files but not the data
> directory will presumably hit the directory-level permissions on
> $PGDATA before the issue of the permissions on current_logfiles() per
> se become relevant, except in corner cases that I don't care about.

Sane deployments normally split the log directory and the main data
folder into separate partitions, and use an absolute path for
log_directory.  So, FWIW, I can live with the original proposal as
well.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: current_logfiles not following group access and instead follows log_file_mode permissions

Haribabu Kommi-2
In reply to this post by Michael Paquier-2

On Tue, Mar 12, 2019 at 5:03 PM Michael Paquier <[hidden email]> wrote:
On Tue, Feb 26, 2019 at 12:22:53PM +1100, Haribabu Kommi wrote:
> I checked the code why the current_logfiles is not implemented as
> shared memory and found that the current syslogger doesn't attach to
> the shared memory of the postmaster. To support storing the
> current_logfiles in shared memory, the syslogger process also needs
> to attach to the shared memory, this seems to be a new
> infrastructure change.

I don't think you can do that anyway and we should not do it.  Shared
memory can be reset after a backend exits abnormally, but the
syslogger lives across that.  What you sent upthread to improve the
documentation is in my opinion sufficient:
https://www.postgresql.org/message-id/CAJrrPGe-v2_LMFD9nHrBEjJy3vVOKJwY3w_h+Fs2nxCJg3PbaA@...

I would not have split the paragraph you broke into two, but instead
just add this part in-between:
+       <para>
+        Permissions <literal>0640</literal> are recommended to be compatible with
+        <application>initdb</application> option <option>--allow-group-access</option>.
+       </para>
Any objections in doing that?

If I remember correctly, in one of the mails, you mentioned that having a separate
para is better. Attached is the updated patch as per your suggestion.

IMO, this update is just a recommendation to the user, and sometimes it is still
possible that there may be strict permissions for the log file even the data directory
is allowed for the group access. So I feel it is still better to update the permissions
of the current_logfiles to the database files permissions than log file permissions.

Regards,
Haribabu Kommi
Fujitsu Australia

0001-log_file_mode-recommended-value-update_v2.patch (2K) Download Attachment
12