Reopen logfile on SIGHUP
Large percentage of postgres installations, for example PGDG packages
for Debian/Ubuntu,
assume by default that log management will be handled by extrernals
tools such as logrotate.
Unfortunately such tools have no way to tell postgres to reopen log file
after rotation
and forced to use copy-truncate strategy that leads to a loss of log
messages which is unacceptable.
Small patch in the attachment implements logfile reopeninig on SIGHUP.
It only affects the file accessed by logging collector, which name you
can check with pg_current_logfile().
I hope you will find this feature useful.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
reopen_logfile_on_SIGHUP_v0.patchtext/x-patch; name=reopen_logfile_on_SIGHUP_v0.patchDownload+18-0
Anastasia Lubennikova <a.lubennikova@postgrespro.ru> writes:
Large percentage of postgres installations, for example PGDG packages
for Debian/Ubuntu, assume by default that log management will be
handled by extrernals tools such as logrotate.Unfortunately such tools have no way to tell postgres to reopen log
file after rotation and forced to use copy-truncate strategy that
leads to a loss of log messages which is unacceptable.Small patch in the attachment implements logfile reopeninig on SIGHUP.
It only affects the file accessed by logging collector, which name you
can check with pg_current_logfile().I hope you will find this feature useful.
+1 for the feature, but:
syslogFile = logfile_open(last_file_name, "a", false);
This will cause a fatal error if opening the logfile fails for any
reason (even transient errors like ENFILE/EMFILE). There is already the
logfile_rotate() function that can reopen log files safely based on time
and date limits. I'd suggest extending that by adding a config option
that controls whether to always reopen the log file on SIGHUP.
- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl
On 27 February 2018 at 14:41, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
Small patch in the attachment implements logfile reopeninig on SIGHUP.
It only affects the file accessed by logging collector, which name you can
check with pg_current_logfile().
HUP will cause Postgres to reload its config files. That seems like a
fine time to reopen the log files as well but it would be nice if
there was also some way to get it to *just* do that and not reload the
config files.
I wonder if it would be easiest to just have the syslogger watch for
some other signal as well (I'm guessing the the syslogger doesn't use
relcache invalidations so it could reuse USR1 for example). That would
be a bit inconvenient as the admins would have to find the syslogger
and deliver the signal directly, rather than through the postmaster
but it would be pretty easy for them.
--
greg
On 2018-02-27 22:32:41 +0000, Greg Stark wrote:
On 27 February 2018 at 14:41, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:Small patch in the attachment implements logfile reopeninig on SIGHUP.
It only affects the file accessed by logging collector, which name you can
check with pg_current_logfile().HUP will cause Postgres to reload its config files. That seems like a
fine time to reopen the log files as well but it would be nice if
there was also some way to get it to *just* do that and not reload the
config files.
Is that an actually important thing to be able to do?
I wonder if it would be easiest to just have the syslogger watch for
some other signal as well (I'm guessing the the syslogger doesn't use
relcache invalidations so it could reuse USR1 for example). That would
be a bit inconvenient as the admins would have to find the syslogger
and deliver the signal directly, rather than through the postmaster
but it would be pretty easy for them.
-many. We have been "signal starved" a number of times, and definitely
shouldn't waste one on this.
- Andres
Greg Stark <stark@mit.edu> writes:
On 27 February 2018 at 14:41, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:Small patch in the attachment implements logfile reopeninig on SIGHUP.
It only affects the file accessed by logging collector, which name you can
check with pg_current_logfile().
HUP will cause Postgres to reload its config files. That seems like a
fine time to reopen the log files as well but it would be nice if
there was also some way to get it to *just* do that and not reload the
config files.
There's already a pretty substantial amount of logic in syslogger.c
to decide whether to force a rotation if any of the logging collection
parameters changed. I don't especially like the proposed patch, aside
from its lack of error handling, because it is completely disconnected
from that logic and thus is likely to produce unnecessary thrashing
of the output file.
I wonder if it would be easiest to just have the syslogger watch for
some other signal as well (I'm guessing the the syslogger doesn't use
relcache invalidations so it could reuse USR1 for example). That would
be a bit inconvenient as the admins would have to find the syslogger
and deliver the signal directly, rather than through the postmaster
but it would be pretty easy for them.
It already does treat SIGUSR1 as a log rotation request. Apparently
the point of this patch is that some people don't find that easy enough
to use, which is fair, because finding out the collector's PID from
outside isn't very easy.
regards, tom lane
Andres Freund <andres@anarazel.de> writes:
On 2018-02-27 22:32:41 +0000, Greg Stark wrote:
On 27 February 2018 at 14:41, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:Small patch in the attachment implements logfile reopeninig on SIGHUP.
HUP will cause Postgres to reload its config files. That seems like a
fine time to reopen the log files as well but it would be nice if
there was also some way to get it to *just* do that and not reload the
config files.
Is that an actually important thing to be able to do?
Yeah, after further consideration I'm having a hard time seeing the point
of this patch. The syslogger already has plenty sufficient knobs for
controlling when it rotates its output file. If you're not using those,
I think the answer is to start using them, not to make the syslogger's
behavior even more complicated so you can avoid learning about them.
IOW, I think a fair response to this is "if you're using logrotate with
Postgres, you're doing it wrong". That was of some use back before we
spent so much sweat on the syslogger, but it's not a reasonable setup
today.
There'd be a point to this perhaps in configurations *not* using the
syslogger, but it's patching the wrong place for that case. (I'm
not sure there is a right place, unfortunately --- we don't have any
good way to redirect postmaster stderr after launch, since so many
processes would have to individually redirect.)
regards, tom lane
On Tue, Feb 27, 2018 at 4:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IOW, I think a fair response to this is "if you're using logrotate with
Postgres, you're doing it wrong". That was of some use back before we
spent so much sweat on the syslogger, but it's not a reasonable setup
today.
A couple of weeks ago a message was posted to general [1] in which I
concluded the desired behavior is not supported natively. I'm curious
whether better advice than mine can be given ...
/messages/by-id/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@mail.gmail.com
David J.
On 2018-02-27 16:20:28 -0700, David G. Johnston wrote:
On Tue, Feb 27, 2018 at 4:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IOW, I think a fair response to this is "if you're using logrotate with
Postgres, you're doing it wrong". That was of some use back before we
spent so much sweat on the syslogger, but it's not a reasonable setup
today.A couple of weeks ago a message was posted to general [1] in which I
concluded the desired behavior is not supported natively. I'm curious
whether better advice than mine can be given .../messages/by-id/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@mail.gmail.com
That link appears to be broken. Real one
/messages/by-id/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@mail.gmail.com
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Tue, Feb 27, 2018 at 4:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IOW, I think a fair response to this is "if you're using logrotate with
Postgres, you're doing it wrong". That was of some use back before we
spent so much sweat on the syslogger, but it's not a reasonable setup
today.
A couple of weeks ago a message was posted to general [1] in which I
concluded the desired behavior is not supported natively. I'm curious
whether better advice than mine can be given ...
/messages/by-id/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@mail.gmail.com
The particular behavior that guy wanted would require some new %-escape
in the log_filename parameter. Essentially we'd need to keep an
increasing sequence counter for log files and have it wrap around at some
user-specified count (5 in his example), then add a %-escape to include
the counter value in the generated filename. It's not an unreasonable
idea, if somebody wanted to code it up.
regards, tom lane
On Tue, Feb 27, 2018 at 05:52:20PM -0500, Tom Lane wrote:
It already does treat SIGUSR1 as a log rotation request. Apparently
the point of this patch is that some people don't find that easy enough
to use, which is fair, because finding out the collector's PID from
outside isn't very easy.
True enough. The syslogger does not show up in pg_stat_activity either,
so I think that being able to do so would help for this case.
--
Michael
If there is already SIGUSR1 for logfile reopening then shouldn`t
postmaster watch for it? Postmaster PID is easy to obtain.
On 02/28/2018 01:32 AM, Greg Stark wrote:
On 27 February 2018 at 14:41, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:Small patch in the attachment implements logfile reopeninig on SIGHUP.
It only affects the file accessed by logging collector, which name you can
check with pg_current_logfile().HUP will cause Postgres to reload its config files. That seems like a
fine time to reopen the log files as well but it would be nice if
there was also some way to get it to *just* do that and not reload the
config files.I wonder if it would be easiest to just have the syslogger watch for
some other signal as well (I'm guessing the the syslogger doesn't use
relcache invalidations so it could reuse USR1 for example). That would
be a bit inconvenient as the admins would have to find the syslogger
and deliver the signal directly, rather than through the postmaster
but it would be pretty easy for them.
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On 2/27/18 8:54 PM, Michael Paquier wrote:
On Tue, Feb 27, 2018 at 05:52:20PM -0500, Tom Lane wrote:
It already does treat SIGUSR1 as a log rotation request. Apparently
the point of this patch is that some people don't find that easy enough
to use, which is fair, because finding out the collector's PID from
outside isn't very easy.True enough. The syslogger does not show up in pg_stat_activity either,
so I think that being able to do so would help for this case.
There does not seem to be any consensus on this patch so I'm marking it
Waiting on Author for the time being. At the end of the CF I'll mark it
Returned with Feedback if there is no further activity.
Regards,
--
-David
david@pgmasters.net
Hi Anastasia,
On 3/28/18 1:46 PM, David Steele wrote:
On 2/27/18 8:54 PM, Michael Paquier wrote:
On Tue, Feb 27, 2018 at 05:52:20PM -0500, Tom Lane wrote:
It already does treat SIGUSR1 as a log rotation request. Apparently
the point of this patch is that some people don't find that easy enough
to use, which is fair, because finding out the collector's PID from
outside isn't very easy.True enough. The syslogger does not show up in pg_stat_activity either,
so I think that being able to do so would help for this case.There does not seem to be any consensus on this patch so I'm marking it
Waiting on Author for the time being. At the end of the CF I'll mark it
Returned with Feedback if there is no further activity.
I have marked this entry Returned with Feedback since there has been no
further activity and no opinions to the contrary.
Regards,
--
-David
david@pgmasters.net
On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IOW, I think a fair response to this is "if you're using logrotate with
Postgres, you're doing it wrong".
Well, the original post says that this is how the PGDG RPMs are doing
it on Debian/Ubuntu. I wonder if that's due to some Debian/Ubuntu
policy or just a preference on the part of whoever did the packaging
work. Anyway it's a little hard to argue that the configuration is
insane when we're shipping it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IOW, I think a fair response to this is "if you're using logrotate with
Postgres, you're doing it wrong".
Well, the original post says that this is how the PGDG RPMs are doing
it on Debian/Ubuntu. I wonder if that's due to some Debian/Ubuntu
policy or just a preference on the part of whoever did the packaging
work. Anyway it's a little hard to argue that the configuration is
insane when we're shipping it.
We, as in the core project, are not shipping it. I'm also unclear
on why you want to exclude "fix the RPM packaging" as a reasonable
solution. It seems likely that some change in that packaging would
be necessary anyway, as it wouldn't know today about any signaling
method we might choose to adopt.
Having said that, I'm not averse to providing a solution if it's robust,
not too invasive and doesn't break other use-cases. So far we've not
seen a patch that meets those conditions.
regards, tom lane
On 04/10/2018 12:17 PM, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IOW, I think a fair response to this is "if you're using logrotate with
Postgres, you're doing it wrong".Well, the original post says that this is how the PGDG RPMs are doing
it on Debian/Ubuntu. I wonder if that's due to some Debian/Ubuntu
policy or just a preference on the part of whoever did the packaging
work. Anyway it's a little hard to argue that the configuration is
insane when we're shipping it.We, as in the core project, are not shipping it.
Well, yes we are at least from an external perception problem. The name
says it all, PGDG RPMs. They are either the official PostgreSQL.Org RPMs
or they aren't. If they aren't they shouldn't be called PGDG RPMs nor
should they be available from yum.postgresql.org and apt.postgresql.org
respectively.
Note: I am not advocating the removal of those packages. I am advocating
that the core project of PostgreSQL.Org in fact does ship those packages
and that is how people see it outside of our email silo.
JD
--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
*** A fault and talent of mine is to tell it exactly how it is. ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
***** Unless otherwise stated, opinions are my own. *****
On Tue, Apr 10, 2018 at 3:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We, as in the core project, are not shipping it.
+1 for what JD said on that subject.
I'm also unclear
on why you want to exclude "fix the RPM packaging" as a reasonable
solution.
Mostly because the complaint was about the *Debian* packaging. Other
than that, it's possible that that's the way forward.
It seems likely that some change in that packaging would
be necessary anyway, as it wouldn't know today about any signaling
method we might choose to adopt.Having said that, I'm not averse to providing a solution if it's robust,
not too invasive and doesn't break other use-cases. So far we've not
seen a patch that meets those conditions.
Fair enough.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
El 10/04/18 a las 22:40, Robert Haas escribió:
Having said that, I'm not averse to providing a solution if it's robust,
not too invasive and doesn't break other use-cases. So far we've not
seen a patch that meets those conditions.Fair enough.
Syslogger does already rotate logs properly on SIGHUP under some
conditions, so we can just change this to unconditional rotation.
Probably some people wouldn't want their logs to be rotated on SIGHUP,
so we could also add a GUC to control this. Please see the attached patch.
--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
rotate-on-sighup-v2.patchtext/x-patch; name=rotate-on-sighup-v2.patchDownload+36-0
Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> writes:
Syslogger does already rotate logs properly on SIGHUP under some
conditions, so we can just change this to unconditional rotation.
Probably some people wouldn't want their logs to be rotated on SIGHUP,
so we could also add a GUC to control this. Please see the attached patch.
I don't believe this meets the "not break other use-cases" requirement.
Point 1: I do not like a solution that presumes that some background
daemon is going to SIGHUP the postmaster whenever it feels like it.
That will break scenarios in which the DBA is in the midst of a set
of related configuration changes (either ALTER SYSTEM commands or
manual postgresql.conf edits) and doesn't want those changes applied
till she's done. So we need a mechanism that's narrowly targeted
to reopening the logfile, without SIGHUP'ing the entire database.
Point 2: Depending on how you've got the log filenames configured,
setting rotation_requested may result in a change in log filename, which
will be the wrong thing in some use-cases, particularly that of an
external logrotate daemon that only wishes you'd close and reopen your
file descriptor. This is a pre-existing issue with the SIGUSR1 code path,
which I think hasn't come up only because hardly anybody is using that.
If we're going to make it mainstream, we need to think harder about how
that ought to work.
Anastasia's original patch avoided the point-2 pitfall, but didn't
do anything about point 1.
BTW, another thing that needs to be considered is the interaction with
rotation_disabled. Right now we automatically drop that on SIGHUP, but
I'm unclear on whether it should be different for logrotate requests.
regards, tom lane
On 04/11/2018 12:00 AM, Tom Lane wrote:
Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> writes:
Syslogger does already rotate logs properly on SIGHUP under some
conditions, so we can just change this to unconditional rotation.
Probably some people wouldn't want their logs to be rotated on SIGHUP,
so we could also add a GUC to control this. Please see the attached patch.I don't believe this meets the "not break other use-cases" requirement.
Point 1: I do not like a solution that presumes that some background
daemon is going to SIGHUP the postmaster whenever it feels like it.
That will break scenarios in which the DBA is in the midst of a set
of related configuration changes (either ALTER SYSTEM commands or
manual postgresql.conf edits) and doesn't want those changes applied
till she's done. So we need a mechanism that's narrowly targeted
to reopening the logfile, without SIGHUP'ing the entire database.
If logging collector can reopen file on SIGUSR1, then maybe there should
be logging_collector.pid file in PGDATA, so external rotation tools can
get it without much trouble?
Point 2: Depending on how you've got the log filenames configured,
setting rotation_requested may result in a change in log filename, which
will be the wrong thing in some use-cases, particularly that of an
external logrotate daemon that only wishes you'd close and reopen your
file descriptor. This is a pre-existing issue with the SIGUSR1 code path,
which I think hasn't come up only because hardly anybody is using that.
If we're going to make it mainstream, we need to think harder about how
that ought to work.
External tools usually rely on logfile name staying the same. PGDG
distribution do it that way for sure.
Anastasia's original patch avoided the point-2 pitfall, but didn't
do anything about point 1.BTW, another thing that needs to be considered is the interaction with
rotation_disabled. Right now we automatically drop that on SIGHUP, but
I'm unclear on whether it should be different for logrotate requests.regards, tom lane
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
