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

Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)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(dot)lubennikova(at)postgrespro(dot)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(dot)lubennikova(at)postgrespro(dot)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(at)mit(dot)edu> writes:
> On 27 February 2018 at 14:41, Anastasia Lubennikova
> <a(dot)lubennikova(at)postgrespro(dot)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(at)anarazel(dot)de> writes:
> On 2018-02-27 22:32:41 +0000, Greg Stark wrote:
>> On 27 February 2018 at 14:41, Anastasia Lubennikova
>> <a(dot)lubennikova(at)postgrespro(dot)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(at)sss(dot)pgh(dot)pa(dot)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 ...

/message-id/flat/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_%2Bc8NxJccCBHw%40mail(dot)gmail(dot)com#CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw(at)mail(dot)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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 ...
>
> /message-id/flat/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_%2Bc8NxJccCBHw%40mail(dot)gmail(dot)com#CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw(at)mail(dot)gmail(dot)com

That link appears to be broken. Real one
/message-id/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@mail.gmail.com

"David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> On Tue, Feb 27, 2018 at 4:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 ...

> /message-id/flat/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_%2Bc8NxJccCBHw%40mail(dot)gmail(dot)com#CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw(at)mail(dot)gmail(dot)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(dot)lubennikova(at)postgrespro(dot)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(at)pgmasters(dot)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(at)pgmasters(dot)net

On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
>> On Tue, Feb 27, 2018 at 6:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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

Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)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(dot)kuzmenkov(at)postgrespro(dot)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

On 11.04.2018 00:00, Tom Lane wrote:
> So we need a mechanism that's narrowly targeted
> to reopening the logfile, without SIGHUP'ing the entire database.

We can send SIGUSR1 to the syslogger process. To make its pid easier to
find out, it can be published in "$PGDATA/logging_collector.pid", as
suggested by Grigory. The attached patch does this. It also adds a brief
description of how to use this with logrotate.

> Point 2: Depending on how you've got the log filenames configured,
> setting rotation_requested may result in a change in log filename

If logrotate only needs the file to be reopened, syslogger's rotation
does just than when using a static log file name. I imagine logrotate
can be configured to do something useful with changing file names, too.
It is a matter of keeping the configuration of syslogger and logrotate
consistent.

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

The SIGUSR1 path is supposed to be used by automated tools. In a sense,
it is an automatic rotation, the difference being that it originates
from an external tool and not from syslogger itself. So, it sounds
plausible that the rotation request shouldn't touch the
rotation_disabled flag, and should be disabled by it, just like the
automatic rotation.

Still, this leads us to a scenario where we can lose logs:
1. postgres is configured to use a static file name. logrotate is
configured to move the file, send SIGUSR1 to postgres syslogger, gzip
the file and delete it.
2. logrotate starts the rotation. It moves the file and signals postgres
to reopen it.
3. postgres fails to reopen the file because there are too many files
open (ENFILE/EMFILE), which is a normal occurrence on heavily loaded
systems. Or it doesn't open the new file because the rotation_disable
flag is set. It continues logging to the old file.
4. logrotate has no way to detect this failure, so it gzips the file and
unlinks it.
5. postgres continues writing to the now unlinked file, and we lose an
arbitrary amount of logs until the next successful rotation.

With dynamic file names, logrotate can be told to skip open files, so
that it doesn't touch our log file if we haven't switched to the new
one. With a static file name, the log file is always open, so this
method doesn't work. I'm not sure how to make this work reliably.

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

At Thu, 12 Apr 2018 17:23:42 +0300, Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> wrote in <f9f32301-53b4-74cb-335a-c293911aed41(at)postgrespro(dot)ru>
> On 11.04.2018 00:00, Tom Lane wrote:
> > So we need a mechanism that's narrowly targeted
> > to reopening the logfile, without SIGHUP'ing the entire database.
>
> We can send SIGUSR1 to the syslogger process. To make its pid easier
> to find out, it can be published in "$PGDATA/logging_collector.pid",
> as suggested by Grigory. The attached patch does this. It also adds a
> brief description of how to use this with logrotate.

FWIW I'm not a fan of officially exposing logging collector PID
and let users send SIGUSR1 directly to the postmaster's internal
process. (It seems to me more unusual than pg_terminate_backed.)
We can provide a new command "pg_ctl logrotate" to hide the
details. (It cannot be executed by root, though.)

> > Point 2: Depending on how you've got the log filenames configured,
> > setting rotation_requested may result in a change in log filename
>
> If logrotate only needs the file to be reopened, syslogger's rotation
> does just than when using a static log file name. I imagine logrotate
> can be configured to do something useful with changing file names,
> too. It is a matter of keeping the configuration of syslogger and
> logrotate consistent.

Seems fine for me.

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

I feel the same, an explicit request from user ought to reset (or
ignore) it. (By the way, logrorate_disabled cannot be reset
without reloading config..)

> The SIGUSR1 path is supposed to be used by automated tools. In a
> sense, it is an automatic rotation, the difference being that it
> originates from an external tool and not from syslogger itself. So, it
> sounds plausible that the rotation request shouldn't touch the
> rotation_disabled flag, and should be disabled by it, just like the
> automatic rotation.
>
> Still, this leads us to a scenario where we can lose logs:
> 1. postgres is configured to use a static file name. logrotate is
> configured to move the file, send SIGUSR1 to postgres syslogger, gzip
> the file and delete it.
> 2. logrotate starts the rotation. It moves the file and signals
> postgres to reopen it.
> 3. postgres fails to reopen the file because there are too many files
> open (ENFILE/EMFILE), which is a normal occurrence on heavily loaded
> systems. Or it doesn't open the new file because the rotation_disable
> flag is set. It continues logging to the old file.
> 4. logrotate has no way to detect this failure, so it gzips the file
> and unlinks it.
> 5. postgres continues writing to the now unlinked file, and we lose an
> arbitrary amount of logs until the next successful rotation.
>
> With dynamic file names, logrotate can be told to skip open files, so
> that it doesn't touch our log file if we haven't switched to the new
> one. With a static file name, the log file is always open, so this
> method doesn't work. I'm not sure how to make this work reliably.

The loss is unavoidable by any means since logrotate works that
way by design. It doesn't care whether its peer did the work as
expected. Someone wants to avoid the loss can use copytruncate
for another kind of small loss that can happen at every rotation
time and we don't need to change anything in the case. Those who
want more reliability ought to use the PostgreSQL's genuine
logging mechanism:p

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 04/16/2018 05:54 AM, Kyotaro HORIGUCHI wrote:
> We can provide a new command "pg_ctl logrotate" to hide the
> details. (It cannot be executed by root, though.)

I like this approach.

I looked at the patch and changed some things:
- cleaned up the error messages
- moved checkLogrotateSignal to postmaster.c, since it has no reason to
be in xlog.c
- added some documentation I had from my older patch

Other than that, it looks good to me. The updated patch is attached.

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

At Fri, 20 Apr 2018 21:51:49 +0300, Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> wrote in <c7cd74db-dde3-49e7-cdde-b909b0fdac0b(at)postgrespro(dot)ru>
> On 04/16/2018 05:54 AM, Kyotaro HORIGUCHI wrote:
> > We can provide a new command "pg_ctl logrotate" to hide the
> > details. (It cannot be executed by root, though.)
>
> I like this approach.

Thanks. I found that the SIGUSR1 path is already ignoring
rotation_disabled so we don't need bother with the flag.

> I looked at the patch and changed some things:
> - cleaned up the error messages

Thanks for cleaning up crude copy'n pastes and wording.

> - moved checkLogrotateSignal to postmaster.c, since it has no reason to
> - be in xlog.c

Agreed. But

there seem to be no convention that static function starts with a
lower case letter. checkControlFile initMasks are minor instances
but..

> - added some documentation I had from my older patch
>
> Other than that, it looks good to me. The updated patch is attached.

Thanks for the documentation, but I see a description for the
same objective and different measure just above there.

/docs/devel/static/logfile-maintenance.html

> Alternatively, you might prefer to use an external log rotation
> program if you have one that you are already using with other
...
> rotation, the logrotate program can be configured to work with
> log files from syslog.

It seems that the additional description needs to be meld into
this at the first place? And some caveat may be needed on failure
cases. And in the attached the comment for "if
(rotateion_requested)" is edited.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 04/24/2018 06:09 AM, Kyotaro HORIGUCHI wrote:
>
> It seems that the additional description needs to be meld into
> this at the first place? And some caveat may be needed on failure
> cases.

That's right. I applied your diff and rewrote these paragraphs, adding
some words about the possible loss of messages and how to fix it. I also
removed the forgotten declaration of CheckLogrotateSignal from xlog.h.
The updated patch is attached.

We should probably have a commitfest entry for this, so here it is:
https://commitfest.postgresql.org/18/1622/

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello
Something was wrong? Attached file is empty.

regards, Sergei

On 04/25/2018 05:57 PM, Sergei Kornilov wrote:
> Attached file is empty.

My bad, here is the correct file.

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Here is a documentation update from Liudmila Mantrova.

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

I think the latest v4 patch addresses the concerns raised upthread. I'm
marking the commitfest entry as RFC.

--

Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello.

At Tue, 7 Aug 2018 16:36:34 +0300, Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> wrote in <a9d2ad92-b4f9-42fb-77eb-f3085b964705(at)postgrespro(dot)ru>
> I think the latest v4 patch addresses the concerns raised
> upthread. I'm marking the commitfest entry as RFC.

Thank you, but it is forgetting pg_ctl --help output. I added it
and moved logrotate entry in docs just above "kill" in
app-pg-ctl.html. Also added "-s" option.

Furthermore, I did the following things.

- Since I'm not sure unlink is signal-handler safe on all
supported platforms, I moved unlink() call out of
checkLogrotateSignal() to SysLoggerMain, just before rotating
log file.

- Refactored moving the main stuff to syslogger.c.

- The diff is splitted into two files and renamed. (but version
number continues to v5).

I'm not certain whether to allow the signal file alone cause
rotation, but this patch doesn't. Directly placed signal file is
to be removed without causing rotation.

Please find the attached.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 08/09/2018 10:33 AM, Kyotaro HORIGUCHI wrote:
>
> - Since I'm not sure unlink is signal-handler safe on all
> supported platforms, I moved unlink() call out of
> checkLogrotateSignal() to SysLoggerMain, just before rotating
> log file.

Which platforms specifically do you have in mind? unlink() is required
to be async-signal-safe by POSIX.1-2001, which is required by UNIX 03,
and these are rather old.
For UNIX 03-certified distributions, see this list:
http://www.opengroup.org/csq/search/t=XY1.html
For FreeBSD, unlink() was signal-safe at least in 4.0, which was
released in 2000
https://www.freebsd.org/cgi/man.cgi?query=sigaction&apropos=0&sektion=0&manpath=FreeBSD+4.0-RELEASE&arch=default&format=html
Debian 4.0, which was released in 2007 and had a 2.6 kernel, also claims
to have a signal-safe unlink():
https://www.freebsd.org/cgi/man.cgi?query=signal&apropos=0&sektion=0&manpath=Debian+4.0.9&arch=default&format=html

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

At Fri, 10 Aug 2018 15:33:26 +0300, Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> wrote in <5142559a-82e6-b3e4-d6ed-8fd2d240c77e(at)postgrespro(dot)ru>
> On 08/09/2018 10:33 AM, Kyotaro HORIGUCHI wrote:
> >
> > - Since I'm not sure unlink is signal-handler safe on all
> > supported platforms, I moved unlink() call out of
> > checkLogrotateSignal() to SysLoggerMain, just before rotating
> > log file.
>
> Which platforms specifically do you have in mind? unlink() is required
> to be async-signal-safe by POSIX.1-2001, which is required by UNIX 03,
> and these are rather old.

I suspect that something bad can happen on Windows. Another
reason for the movement I didn't mention was it is not necesarry
to be there. So I applied the principle that a signal handler
should be as small and simple as possible. As the result the
flow of logrotate signal handling becomes similar to that for
promote signal.

> For UNIX 03-certified distributions, see this list:
> http://www.opengroup.org/csq/search/t=XY1.html
> For FreeBSD, unlink() was signal-safe at least in 4.0, which was
> released in 2000
> https://www.freebsd.org/cgi/man.cgi?query=sigaction&apropos=0&sektion=0&manpath=FreeBSD+4.0-RELEASE&arch=default&format=html
> Debian 4.0, which was released in 2007 and had a 2.6 kernel, also
> claims to have a signal-safe unlink():
> https://www.freebsd.org/cgi/man.cgi?query=signal&apropos=0&sektion=0&manpath=Debian+4.0.9&arch=default&format=html

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Aug 21, 2018 at 09:26:54AM +0900, Kyotaro HORIGUCHI wrote:
> I suspect that something bad can happen on Windows.

[troll mode]
More and even worse things than that could happen on Windows.
[/troll mode]
--
Michael

On Tue, Aug 21, 2018 at 4:48 AM Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> At Fri, 10 Aug 2018 15:33:26 +0300, Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> wrote in <5142559a-82e6-b3e4-d6ed-8fd2d240c77e(at)postgrespro(dot)ru>
> > On 08/09/2018 10:33 AM, Kyotaro HORIGUCHI wrote:
> > >
> > > - Since I'm not sure unlink is signal-handler safe on all
> > > supported platforms, I moved unlink() call out of
> > > checkLogrotateSignal() to SysLoggerMain, just before rotating
> > > log file.
> >
> > Which platforms specifically do you have in mind? unlink() is required
> > to be async-signal-safe by POSIX.1-2001, which is required by UNIX 03,
> > and these are rather old.
>
> I suspect that something bad can happen on Windows. Another
> reason for the movement I didn't mention was it is not necesarry
> to be there. So I applied the principle that a signal handler
> should be as small and simple as possible. As the result the
> flow of logrotate signal handling becomes similar to that for
> promote signal.

I went through this thread. It started from discussion about changing
signal handling in syslogger, which has spotted set of problems.
However, now there is a patch which add new pg_ctl command, which
issues SIGUSR1 to syslogger. It seems that nobody in the thread
object against this feature.

I've revised this patch a bit. It appears to me that only postmaster
cares about logrotate file, while syslogger just handles SIGUSR1 as it
did before. So, I've moved code that deletes logrotate file into
postmaster.c.

Also I found that this new pg_ctl isn't covered with tests at all. So
I've added very simple tap tests, which ensures that when log file was
renamed, it reappeared again after pg_ctl logrotate. I wonder how
that would work on Windows. Thankfully commitfest.cputube.org have
Windows checking facility now.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

At Tue, 21 Aug 2018 11:27:46 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180821022745(dot)GE2897(at)paquier(dot)xyz>
> On Tue, Aug 21, 2018 at 09:26:54AM +0900, Kyotaro HORIGUCHI wrote:
> > I suspect that something bad can happen on Windows.
>
> [troll mode]
> More and even worse things than that could happen on Windows.
> [/troll mode]

I don't have a candy for you just now:p

Well, I take that as it's a kind of no-problem to remove files in
signal handlers on Windows.

thanks.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 28 Aug 2018 18:50:31 +0300, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote in <CAPpHfduqEyyjLXCNx_t7K2ugCDGVW7WLKL+zrfDEd5wzkvmg-w(at)mail(dot)gmail(dot)com>
> On Tue, Aug 21, 2018 at 4:48 AM Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >
> > At Fri, 10 Aug 2018 15:33:26 +0300, Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> wrote in <5142559a-82e6-b3e4-d6ed-8fd2d240c77e(at)postgrespro(dot)ru>
> > > On 08/09/2018 10:33 AM, Kyotaro HORIGUCHI wrote:
> > > >
> > > > - Since I'm not sure unlink is signal-handler safe on all
> > > > supported platforms, I moved unlink() call out of
> > > > checkLogrotateSignal() to SysLoggerMain, just before rotating
> > > > log file.
> > >
> > > Which platforms specifically do you have in mind? unlink() is required
> > > to be async-signal-safe by POSIX.1-2001, which is required by UNIX 03,
> > > and these are rather old.
> >
> > I suspect that something bad can happen on Windows. Another
> > reason for the movement I didn't mention was it is not necesarry
> > to be there. So I applied the principle that a signal handler
> > should be as small and simple as possible. As the result the
> > flow of logrotate signal handling becomes similar to that for
> > promote signal.
>
> I went through this thread. It started from discussion about changing
> signal handling in syslogger, which has spotted set of problems.
> However, now there is a patch which add new pg_ctl command, which
> issues SIGUSR1 to syslogger. It seems that nobody in the thread
> object against this feature.

Agreed.

> I've revised this patch a bit. It appears to me that only postmaster
> cares about logrotate file, while syslogger just handles SIGUSR1 as it
> did before. So, I've moved code that deletes logrotate file into
> postmaster.c.

As replied to Michael's commnet, I agree to the change.

> Also I found that this new pg_ctl isn't covered with tests at all. So
> I've added very simple tap tests, which ensures that when log file was
> renamed, it reappeared again after pg_ctl logrotate. I wonder how
> that would work on Windows. Thankfully commitfest.cputube.org have
> Windows checking facility now.

Thanks for the test. Documentaion and help message looks fine
including the changed ordering. (180 seconds retry may be a bit
too long but I'm fine with it.)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi!

On Wed, Aug 29, 2018 at 5:05 AM Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> At Tue, 28 Aug 2018 18:50:31 +0300, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote in <CAPpHfduqEyyjLXCNx_t7K2ugCDGVW7WLKL+zrfDEd5wzkvmg-w(at)mail(dot)gmail(dot)com>
> > Also I found that this new pg_ctl isn't covered with tests at all. So
> > I've added very simple tap tests, which ensures that when log file was
> > renamed, it reappeared again after pg_ctl logrotate. I wonder how
> > that would work on Windows. Thankfully commitfest.cputube.org have
> > Windows checking facility now.
>
> Thanks for the test. Documentaion and help message looks fine
> including the changed ordering. (180 seconds retry may be a bit
> too long but I'm fine with it.)

Thank you for the comments. My idea about retry logic was to provide
the similar behavior to poll_query_until().

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Wed, Aug 29, 2018 at 12:01 PM Alexander Korotkov
<a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> On Wed, Aug 29, 2018 at 5:05 AM Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > At Tue, 28 Aug 2018 18:50:31 +0300, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote in <CAPpHfduqEyyjLXCNx_t7K2ugCDGVW7WLKL+zrfDEd5wzkvmg-w(at)mail(dot)gmail(dot)com>
> > > Also I found that this new pg_ctl isn't covered with tests at all. So
> > > I've added very simple tap tests, which ensures that when log file was
> > > renamed, it reappeared again after pg_ctl logrotate. I wonder how
> > > that would work on Windows. Thankfully commitfest.cputube.org have
> > > Windows checking facility now.
> >
> > Thanks for the test. Documentaion and help message looks fine
> > including the changed ordering. (180 seconds retry may be a bit
> > too long but I'm fine with it.)
>
> Thank you for the comments. My idea about retry logic was to provide
> the similar behavior to poll_query_until().

It seems that http://commitfest.cputube.org/ runs only "make check" on
Windows. But my Postgres Pro colleagues checked that tests passed on
32-bit and 64-bit versions of Windows Server 2008. Also I made some
minor beautifications on code and documentation.

This patch seems to have good shape and generally being quite
harmless. Do we have any objections to committing this?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello.

At Thu, 30 Aug 2018 13:42:42 +0300, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote in <CAPpHfdvkp2zua+PisUGBuogkDFe133eeaLg3BxeiqQU1U4m_-A(at)mail(dot)gmail(dot)com>
> It seems that http://commitfest.cputube.org/ runs only "make check" on
> Windows. But my Postgres Pro colleagues checked that tests passed on
> 32-bit and 64-bit versions of Windows Server 2008. Also I made some
> minor beautifications on code and documentation.
>
> This patch seems to have good shape and generally being quite
> harmless. Do we have any objections to committing this?

I checked that on my Win7 box and worked. Of course I have no
objection.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Aug 30, 2018 at 2:44 PM Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> At Thu, 30 Aug 2018 13:42:42 +0300, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote in <CAPpHfdvkp2zua+PisUGBuogkDFe133eeaLg3BxeiqQU1U4m_-A(at)mail(dot)gmail(dot)com>
> > It seems that http://commitfest.cputube.org/ runs only "make check" on
> > Windows. But my Postgres Pro colleagues checked that tests passed on
> > 32-bit and 64-bit versions of Windows Server 2008. Also I made some
> > minor beautifications on code and documentation.
> >
> > This patch seems to have good shape and generally being quite
> > harmless. Do we have any objections to committing this?
>
> I checked that on my Win7 box and worked. Of course I have no
> objection.

So, pushed. Thank you.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

At Sat, 1 Sep 2018 19:52:16 +0300, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote in <CAPpHfdurtsskTTLZaoSTfNRswQaoLfSwMpWY+g+8fjZ9aF34Jw(at)mail(dot)gmail(dot)com>
> On Thu, Aug 30, 2018 at 2:44 PM Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > At Thu, 30 Aug 2018 13:42:42 +0300, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote in <CAPpHfdvkp2zua+PisUGBuogkDFe133eeaLg3BxeiqQU1U4m_-A(at)mail(dot)gmail(dot)com>
> > > It seems that http://commitfest.cputube.org/ runs only "make check" on
> > > Windows. But my Postgres Pro colleagues checked that tests passed on
> > > 32-bit and 64-bit versions of Windows Server 2008. Also I made some
> > > minor beautifications on code and documentation.
> > >
> > > This patch seems to have good shape and generally being quite
> > > harmless. Do we have any objections to committing this?
> >
> > I checked that on my Win7 box and worked. Of course I have no
> > objection.
>
> So, pushed. Thank you.

Thanks!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center