Reopen logfile on SIGHUP

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Reopen logfile on SIGHUP

Anastasia Lubennikova
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


reopen_logfile_on_SIGHUP_v0.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Dagfinn Ilmari Mannsåker
Anastasia Lubennikova <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Greg Stark
In reply to this post by Anastasia Lubennikova
On 27 February 2018 at 14:41, Anastasia Lubennikova
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Andres Freund
On 2018-02-27 22:32:41 +0000, Greg Stark wrote:

> On 27 February 2018 at 14:41, Anastasia Lubennikova
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Tom Lane-2
In reply to this post by Greg Stark
Greg Stark <[hidden email]> writes:
> On 27 February 2018 at 14:41, Anastasia Lubennikova
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> writes:
> On 2018-02-27 22:32:41 +0000, Greg Stark wrote:
>> On 27 February 2018 at 14:41, Anastasia Lubennikova
>> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

David G Johnston
On Tue, Feb 27, 2018 at 4:12 PM, Tom Lane <[hidden email]> 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 ...


David J.

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Andres Freund
On 2018-02-27 16:20:28 -0700, David G. Johnston wrote:

> On Tue, Feb 27, 2018 at 4:12 PM, Tom Lane <[hidden email]> 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 ...
>
> https://www.postgresql.org/message-id/flat/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_%2Bc8NxJccCBHw%40mail.gmail.com#CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@...

That link appears to be broken. Real one
https://www.postgresql.org/message-id/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@...

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Tom Lane-2
In reply to this post by David G Johnston
"David G. Johnston" <[hidden email]> writes:
> On Tue, Feb 27, 2018 at 4:12 PM, Tom Lane <[hidden email]> 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 ...

> https://www.postgresql.org/message-id/flat/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_%2Bc8NxJccCBHw%40mail.gmail.com#CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@...

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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Michael Paquier-2
In reply to this post by Tom Lane-2
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

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

Re: Reopen logfile on SIGHUP

Grigory Smolkin
In reply to this post by Greg Stark
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
> <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

David Steele
In reply to this post by Michael Paquier-2
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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

David Steele
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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Robert Haas
In reply to this post by Tom Lane-2
On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Tom Lane-2
Robert Haas <[hidden email]> writes:
> On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Joshua D. Drake
On 04/10/2018 12:17 PM, Tom Lane wrote:

> Robert Haas <[hidden email]> writes:
>> On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <[hidden email]> 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.   *****

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Robert Haas
In reply to this post by Tom Lane-2
On Tue, Apr 10, 2018 at 3:17 PM, Tom Lane <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Alexander Kuzmenkov
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


rotate-on-sighup-v2.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Tom Lane-2
Alexander Kuzmenkov <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Reopen logfile on SIGHUP

Grigory Smolkin

On 04/11/2018 12:00 AM, Tom Lane wrote:

> Alexander Kuzmenkov <[hidden email]> 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


12
Previous Thread Next Thread