Thread: 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
Attachment
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
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.
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 ... > > https://www.postgresql.org/message-id/flat/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_%2Bc8NxJccCBHw%40mail.gmail.com#CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_+c8NxJccCBHw@mail.gmail.com That link appears to be broken. Real one https://www.postgresql.org/message-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 ... > https://www.postgresql.org/message-id/flat/CAKoQ0XHAy9De1C8gxUWHSW6w5iKcqX03wyWGe_%2Bc8NxJccCBHw%40mail.gmail.com#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
Attachment
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
Attachment
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
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
Attachment
At Thu, 12 Apr 2018 17:23:42 +0300, Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> wrote in
<f9f32301-53b4-74cb-335a-c293911aed41@postgrespro.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
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 08dc9ba031..7cadd71af0 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -83,6 +83,7 @@ extern uint32 bootstrap_data_checksum_version;
#define RECOVERY_COMMAND_DONE "recovery.done"
#define PROMOTE_SIGNAL_FILE "promote"
#define FALLBACK_PROMOTE_SIGNAL_FILE "fallback_promote"
+#define LOGROTATE_SIGNAL_FILE "logrotate"
/* User-settable parameters */
@@ -12225,6 +12226,24 @@ CheckPromoteSignal(void)
return false;
}
+/*
+ * Check to see if a log rotate request has arrived. Should be
+ * called by postmaster after receiving SIGUSR1.
+ */
+bool
+CheckLogrotateSignal(void)
+{
+ struct stat stat_buf;
+
+ if (stat(LOGROTATE_SIGNAL_FILE, &stat_buf) == 0)
+ {
+ unlink(LOGROTATE_SIGNAL_FILE);
+ return true;
+ }
+
+ return false;
+}
+
/*
* Wake up startup process to replay newly arrived WAL, or to notice that
* failover has been requested.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index d948369f3e..d4f0f5c853 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5092,7 +5092,8 @@ sigusr1_handler(SIGNAL_ARGS)
signal_child(PgArchPID, SIGUSR1);
}
- if (CheckPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE) &&
+ if ((CheckLogrotateSignal() ||
+ CheckPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE)) &&
SysLoggerPID != 0)
{
/* Tell syslogger to rotate logfile */
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 143021de05..e4968f7b6f 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -59,6 +59,7 @@ typedef enum
STOP_COMMAND,
RESTART_COMMAND,
RELOAD_COMMAND,
+ LOGROTATE_COMMAND,
STATUS_COMMAND,
PROMOTE_COMMAND,
KILL_COMMAND,
@@ -100,6 +101,7 @@ static char version_file[MAXPGPATH];
static char pid_file[MAXPGPATH];
static char backup_file[MAXPGPATH];
static char promote_file[MAXPGPATH];
+static char logrotate_file[MAXPGPATH];
#ifdef WIN32
static DWORD pgctl_start_type = SERVICE_AUTO_START;
@@ -125,6 +127,7 @@ static void do_restart(void);
static void do_reload(void);
static void do_status(void);
static void do_promote(void);
+static void do_logrotate(void);
static void do_kill(pgpid_t pid);
static void print_msg(const char *msg);
static void adjust_data_dir(void);
@@ -1171,6 +1174,62 @@ do_promote(void)
print_msg(_("server promoting\n"));
}
+/*
+ * log rotate
+ */
+
+static void
+do_logrotate(void)
+{
+ FILE *logrotatefile;
+ pgpid_t pid;
+
+ pid = get_pgpid(false);
+
+ if (pid == 0) /* no pid file */
+ {
+ write_stderr(_("%s: PID file \"%s\" does not exist\n"), progname, pid_file);
+ write_stderr(_("Is server running?\n"));
+ exit(1);
+ }
+ else if (pid < 0) /* standalone backend, not postmaster */
+ {
+ pid = -pid;
+ write_stderr(_("%s: cannot promote server; "
+ "single-user server is running (PID: %ld)\n"),
+ progname, pid);
+ exit(1);
+ }
+
+ snprintf(logrotate_file, MAXPGPATH, "%s/logrotate", pg_data);
+
+ if ((logrotatefile = fopen(logrotate_file, "w")) == NULL)
+ {
+ write_stderr(_("%s: could not create log rotate signal file \"%s\": %s\n"),
+ progname, logrotate_file, strerror(errno));
+ exit(1);
+ }
+ if (fclose(logrotatefile))
+ {
+ write_stderr(_("%s: could not write log rotate signal file \"%s\": %s\n"),
+ progname, logrotate_file, strerror(errno));
+ exit(1);
+ }
+
+ sig = SIGUSR1;
+ if (kill((pid_t) pid, sig) != 0)
+ {
+ write_stderr(_("%s: could not send log rotate signal (PID: %ld): %s\n"),
+ progname, pid, strerror(errno));
+ if (unlink(logrotate_file) != 0)
+ write_stderr(_("%s: could not remove logrotate signal file \"%s\": %s\n"),
+ progname, logrotate_file, strerror(errno));
+ exit(1);
+ }
+
+ print_msg(_("commanded to rotate log file\n"));
+}
+
/*
* utility routines
@@ -2332,6 +2391,8 @@ main(int argc, char **argv)
ctl_command = RESTART_COMMAND;
else if (strcmp(argv[optind], "reload") == 0)
ctl_command = RELOAD_COMMAND;
+ else if (strcmp(argv[optind], "logrotate") == 0)
+ ctl_command = LOGROTATE_COMMAND;
else if (strcmp(argv[optind], "status") == 0)
ctl_command = STATUS_COMMAND;
else if (strcmp(argv[optind], "promote") == 0)
@@ -2442,6 +2503,9 @@ main(int argc, char **argv)
case PROMOTE_COMMAND:
do_promote();
break;
+ case LOGROTATE_COMMAND:
+ do_logrotate();
+ break;
case KILL_COMMAND:
do_kill(killproc);
break;
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 421ba6d775..31d1fb4d80 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -280,6 +280,7 @@ extern void GetNextXidAndEpoch(TransactionId *xid, uint32 *epoch);
extern void RemovePromoteSignalFiles(void);
extern bool CheckPromoteSignal(void);
+extern bool CheckLogrotateSignal(void);
extern void WakeupRecovery(void);
extern void SetWalWriterSleeping(bool sleeping);
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
Attachment
At Fri, 20 Apr 2018 21:51:49 +0300, Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> wrote in <c7cd74db-dde3-49e7-cdde-b909b0fdac0b@postgrespro.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. https://www.postgresql.org/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 diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml index c0bf632cd4..068aec7b47 100644 --- a/doc/src/sgml/maintenance.sgml +++ b/doc/src/sgml/maintenance.sgml @@ -930,10 +930,30 @@ analyze threshold = analyze base threshold + analyze scale factor * number of tu <para> Alternatively, you might prefer to use an external log rotation program if you have one that you are already using with other - server software. For example, the <application>rotatelogs</application> - tool included in the <productname>Apache</productname> distribution - can be used with <productname>PostgreSQL</productname>. To do this, - just pipe the server's + + server software. For example, <application>logrotate</application> can + also be used to collect log files produced by the built-in logging + collector. In this configuration, the location and names of the log files + are determined by logging collector, + and <application>logrotate</application> periodically archives these files. + There must be some way for <application>logrotate</application> to inform + the application that the log file it is using is going to be archived, and + that it must direct further output to a new file. This is commonly done + with a <literal>postrotate</literal> script that sends + a <literal>SIGHUP</literal> signal to the application, which then reopens + the log file. In <productname>PostgreSQL</productname>, this is achieved + using a + <literal>logrotate</literal> command + of <application>pg_ctl</application>. When the server receives this + command, it switches to the new log files according to its configuration + (see <xref linkend="runtime-config-logging-where"/>). If it is configured + to use a static file name, it will just reopen the file. + </para> + <para> + For another example, <application>logrotate</application> tool included in + the <productname>Apache</productname> distribution can also be used + with <productname>PostgreSQL</productname>. To do this, just pipe the + server's <systemitem>stderr</systemitem> output to the desired program. If you start the server with <command>pg_ctl</command>, then <systemitem>stderr</systemitem> @@ -958,23 +978,6 @@ pg_ctl start | rotatelogs /var/log/pgsql_log 86400 configured to work with log files from <application>syslog</application>. </para> - - <para> - An external log rotation tool, such as <application>logrotate</application>, can also - be used to collect log files produced by the built-in logging collector. In this - configuration, the location and names of the log files are determined by logging - collector, and <application>logrotate</application> periodically archives these files. - There must be some way for <application>logrotate</application> to inform the application - that the log file it is using is going to be archived, and that it must direct further - output to a new file. This is commonly done with a <literal>postrotate</literal> script - that sends a <literal>SIGHUP</literal> signal to the application, which then reopens the - log file. In <productname>PostgreSQL</productname>, this is achieved using a - <literal>logrotate</literal> command of <application>pg_ctl</application>. When the - server receives this command, it switches to the new log files according to its - configuration (see <xref linkend="runtime-config-logging-where"/>). If it is configured - to use a static file name, it will just reopen the file. - </para> - <para> On many systems, however, <application>syslog</application> is not very reliable, particularly with large log messages; it might truncate or drop messages diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c index 58b759f305..41f036faad 100644 --- a/src/backend/postmaster/syslogger.c +++ b/src/backend/postmaster/syslogger.c @@ -388,7 +388,7 @@ SysLoggerMain(int argc, char *argv[]) { /* * Force rotation when both values are zero. It means the request - * was sent by pg_rotate_logfile. + * was sent by pg_rotate_logfile or pg_ctl logrotate. */ if (!time_based_rotation && size_rotation_for == 0) size_rotation_for = LOG_DESTINATION_STDERR | LOG_DESTINATION_CSVLOG;
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
Attachment
Here is a documentation update from Liudmila Mantrova. -- Alexander Kuzmenkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
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.kuzmenkov@postgrespro.ru> wrote in
<a9d2ad92-b4f9-42fb-77eb-f3085b964705@postgrespro.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
From bee1093cc6c0361c138ebaa4920b7f62fdc4959c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 9 Aug 2018 16:09:46 +0900
Subject: [PATCH 1/2] New pg_ctl subcommand "logrotate"
We are able to force log file rotation by calling pg_rotate_logfile()
SQL function or by sending SIGUSR1 directly to syslogger. External
tools that don't want to log in as superuser needs to know the PID of
syslogger.
This patch provides a means cause log rotation by pg_ctl.
---
src/backend/postmaster/postmaster.c | 6 ++-
src/backend/postmaster/syslogger.c | 36 ++++++++++++++-
src/bin/pg_ctl/pg_ctl.c | 89 ++++++++++++++++++++++++++++++++-----
src/include/postmaster/syslogger.h | 3 ++
4 files changed, 120 insertions(+), 14 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a4b53b33cd..84928a3017 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1268,6 +1268,9 @@ PostmasterMain(int argc, char *argv[])
*/
RemovePromoteSignalFiles();
+ /* Do the same for logrotate signal file */
+ RemoveLogrotateSignalFiles();
+
/* Remove any outdated file holding the current log filenames. */
if (unlink(LOG_METAINFO_DATAFILE) < 0 && errno != ENOENT)
ereport(LOG,
@@ -5100,7 +5103,8 @@ sigusr1_handler(SIGNAL_ARGS)
signal_child(PgArchPID, SIGUSR1);
}
- if (CheckPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE) &&
+ if ((CheckLogrotateSignal() ||
+ CheckPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE)) &&
SysLoggerPID != 0)
{
/* Tell syslogger to rotate logfile */
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 58b759f305..d4c41c9b7b 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -56,6 +56,9 @@
*/
#define READ_BUF_SIZE (2 * PIPE_CHUNK_SIZE)
+/* Log rotation signal file path, relative to $PGDATA */
+#define LOGROTATE_SIGNAL_FILE "logrotate"
+
/*
* GUC parameters. Logging_collector cannot be changed after postmaster
@@ -384,11 +387,18 @@ SysLoggerMain(int argc, char *argv[])
}
}
+ /*
+ * logrotate signal file alone doesn't cause rotation. We remove the
+ * file without setting rotation_requested if any.
+ */
+ if (CheckLogrotateSignal())
+ unlink(LOGROTATE_SIGNAL_FILE);
+
if (rotation_requested)
{
/*
* Force rotation when both values are zero. It means the request
- * was sent by pg_rotate_logfile.
+ * was sent by pg_rotate_logfile or pg_ctl logrotate.
*/
if (!time_based_rotation && size_rotation_for == 0)
size_rotation_for = LOG_DESTINATION_STDERR | LOG_DESTINATION_CSVLOG;
@@ -1422,6 +1432,30 @@ update_metainfo_datafile(void)
* --------------------------------
*/
+/*
+ * Check to see if a log rotation request has arrived. Should be
+ * called by postmaster after receiving SIGUSR1.
+ */
+bool
+CheckLogrotateSignal(void)
+{
+ struct stat stat_buf;
+
+ if (stat(LOGROTATE_SIGNAL_FILE, &stat_buf) == 0)
+ return true;
+
+ return false;
+}
+
+/*
+ * Remove the file signaling a log rotateion request.
+ */
+void
+RemoveLogrotateSignalFiles(void)
+{
+ unlink(LOGROTATE_SIGNAL_FILE);
+}
+
/* SIGHUP: set flag to reload config file */
static void
sigHupHandler(SIGNAL_ARGS)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index ed2396aa6c..ae8fe75c1a 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -59,6 +59,7 @@ typedef enum
STOP_COMMAND,
RESTART_COMMAND,
RELOAD_COMMAND,
+ LOGROTATE_COMMAND,
STATUS_COMMAND,
PROMOTE_COMMAND,
KILL_COMMAND,
@@ -100,6 +101,7 @@ static char version_file[MAXPGPATH];
static char pid_file[MAXPGPATH];
static char backup_file[MAXPGPATH];
static char promote_file[MAXPGPATH];
+static char logrotate_file[MAXPGPATH];
#ifdef WIN32
static DWORD pgctl_start_type = SERVICE_AUTO_START;
@@ -125,6 +127,7 @@ static void do_restart(void);
static void do_reload(void);
static void do_status(void);
static void do_promote(void);
+static void do_logrotate(void);
static void do_kill(pgpid_t pid);
static void print_msg(const char *msg);
static void adjust_data_dir(void);
@@ -1171,6 +1174,62 @@ do_promote(void)
print_msg(_("server promoting\n"));
}
+/*
+ * log rotate
+ */
+
+static void
+do_logrotate(void)
+{
+ FILE *logrotatefile;
+ pgpid_t pid;
+
+ pid = get_pgpid(false);
+
+ if (pid == 0) /* no pid file */
+ {
+ write_stderr(_("%s: PID file \"%s\" does not exist\n"), progname, pid_file);
+ write_stderr(_("Is server running?\n"));
+ exit(1);
+ }
+ else if (pid < 0) /* standalone backend, not postmaster */
+ {
+ pid = -pid;
+ write_stderr(_("%s: cannot rotate log file; "
+ "single-user server is running (PID: %ld)\n"),
+ progname, pid);
+ exit(1);
+ }
+
+ snprintf(logrotate_file, MAXPGPATH, "%s/logrotate", pg_data);
+
+ if ((logrotatefile = fopen(logrotate_file, "w")) == NULL)
+ {
+ write_stderr(_("%s: could not create log rotation signal file \"%s\": %s\n"),
+ progname, logrotate_file, strerror(errno));
+ exit(1);
+ }
+ if (fclose(logrotatefile))
+ {
+ write_stderr(_("%s: could not write log rotation signal file \"%s\": %s\n"),
+ progname, logrotate_file, strerror(errno));
+ exit(1);
+ }
+
+ sig = SIGUSR1;
+ if (kill((pid_t) pid, sig) != 0)
+ {
+ write_stderr(_("%s: could not send log rotation signal (PID: %ld): %s\n"),
+ progname, pid, strerror(errno));
+ if (unlink(logrotate_file) != 0)
+ write_stderr(_("%s: could not remove log rotation signal file \"%s\": %s\n"),
+ progname, logrotate_file, strerror(errno));
+ exit(1);
+ }
+
+ print_msg(_("commanded to rotate log file\n"));
+}
+
/*
* utility routines
@@ -1912,19 +1971,20 @@ do_help(void)
{
printf(_("%s is a utility to initialize, start, stop, or control a PostgreSQL server.\n\n"), progname);
printf(_("Usage:\n"));
- printf(_(" %s init[db] [-D DATADIR] [-s] [-o OPTIONS]\n"), progname);
- printf(_(" %s start [-D DATADIR] [-l FILENAME] [-W] [-t SECS] [-s]\n"
- " [-o OPTIONS] [-p PATH] [-c]\n"), progname);
- printf(_(" %s stop [-D DATADIR] [-m SHUTDOWN-MODE] [-W] [-t SECS] [-s]\n"), progname);
- printf(_(" %s restart [-D DATADIR] [-m SHUTDOWN-MODE] [-W] [-t SECS] [-s]\n"
- " [-o OPTIONS] [-c]\n"), progname);
- printf(_(" %s reload [-D DATADIR] [-s]\n"), progname);
- printf(_(" %s status [-D DATADIR]\n"), progname);
- printf(_(" %s promote [-D DATADIR] [-W] [-t SECS] [-s]\n"), progname);
- printf(_(" %s kill SIGNALNAME PID\n"), progname);
+ printf(_(" %s init[db] [-D DATADIR] [-s] [-o OPTIONS]\n"), progname);
+ printf(_(" %s start [-D DATADIR] [-l FILENAME] [-W] [-t SECS] [-s]\n"
+ " [-o OPTIONS] [-p PATH] [-c]\n"), progname);
+ printf(_(" %s stop [-D DATADIR] [-m SHUTDOWN-MODE] [-W] [-t SECS] [-s]\n"), progname);
+ printf(_(" %s restart [-D DATADIR] [-m SHUTDOWN-MODE] [-W] [-t SECS] [-s]\n"
+ " [-o OPTIONS] [-c]\n"), progname);
+ printf(_(" %s reload [-D DATADIR] [-s]\n"), progname);
+ printf(_(" %s status [-D DATADIR]\n"), progname);
+ printf(_(" %s promote [-D DATADIR] [-W] [-t SECS] [-s]\n"), progname);
+ printf(_(" %s logrotate [-D DATADIR] [-s]\n"), progname);
+ printf(_(" %s kill SIGNALNAME PID\n"), progname);
#ifdef WIN32
- printf(_(" %s register [-D DATADIR] [-N SERVICENAME] [-U USERNAME] [-P PASSWORD]\n"
- " [-S START-TYPE] [-e SOURCE] [-W] [-t SECS] [-s] [-o OPTIONS]\n"), progname);
+ printf(_(" %s register [-D DATADIR] [-N SERVICENAME] [-U USERNAME] [-P PASSWORD]\n"
+ " [-S START-TYPE] [-e SOURCE] [-W] [-t SECS] [-s] [-o OPTIONS]\n"), progname);
printf(_(" %s unregister [-N SERVICENAME]\n"), progname);
#endif
@@ -2333,6 +2393,8 @@ main(int argc, char **argv)
ctl_command = RESTART_COMMAND;
else if (strcmp(argv[optind], "reload") == 0)
ctl_command = RELOAD_COMMAND;
+ else if (strcmp(argv[optind], "logrotate") == 0)
+ ctl_command = LOGROTATE_COMMAND;
else if (strcmp(argv[optind], "status") == 0)
ctl_command = STATUS_COMMAND;
else if (strcmp(argv[optind], "promote") == 0)
@@ -2443,6 +2505,9 @@ main(int argc, char **argv)
case PROMOTE_COMMAND:
do_promote();
break;
+ case LOGROTATE_COMMAND:
+ do_logrotate();
+ break;
case KILL_COMMAND:
do_kill(killproc);
break;
diff --git a/src/include/postmaster/syslogger.h b/src/include/postmaster/syslogger.h
index b35fadc1bd..3fcb26cdb8 100644
--- a/src/include/postmaster/syslogger.h
+++ b/src/include/postmaster/syslogger.h
@@ -87,6 +87,9 @@ extern void write_syslogger_file(const char *buffer, int count, int dest);
extern void SysLoggerMain(int argc, char *argv[]) pg_attribute_noreturn();
#endif
+extern bool CheckLogrotateSignal(void);
+extern void RemoveLogrotateSignalFiles(void);
+
/*
* Name of files saving meta-data information about the log
* files currently in use by the syslogger
--
2.16.3
From f772f45007bb6247a266fe39f0dab318b6459565 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 9 Aug 2018 16:10:35 +0900
Subject: [PATCH 2/2] Documentation for pg_ctl logrotate
---
doc/src/sgml/maintenance.sgml | 34 ++++++++++++++++++++++++++++++++--
doc/src/sgml/ref/pg_ctl-ref.sgml | 15 ++++++++++++++-
2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml
index 4a68ec3b40..fd538e1cce 100644
--- a/doc/src/sgml/maintenance.sgml
+++ b/doc/src/sgml/maintenance.sgml
@@ -932,8 +932,8 @@ analyze threshold = analyze base threshold + analyze scale factor * number of tu
program if you have one that you are already using with other
server software. For example, the <application>rotatelogs</application>
tool included in the <productname>Apache</productname> distribution
- can be used with <productname>PostgreSQL</productname>. To do this,
- just pipe the server's
+ can be used with <productname>PostgreSQL</productname>. One way to
+ do this is to pipe the server's
<systemitem>stderr</systemitem> output to the desired program.
If you start the server with
<command>pg_ctl</command>, then <systemitem>stderr</systemitem>
@@ -945,6 +945,36 @@ pg_ctl start | rotatelogs /var/log/pgsql_log 86400
</programlisting>
</para>
+ <para>
+ You can combine these approaches by setting up <application>logrotate</application>
+ to collect log files produced by <productname>PostgreSQL</productname> built-in logging
+ collector. In this case, the logging collector defines the names and
+ location of the log files, while <application>logrotate</application>
+ periodically archives these files. When initiating log rotation,
+ <application>logrotate</application> must ensure that the application
+ sends further output to the new file. This is commonly done with a
+ <literal>postrotate</literal> script that sends a <literal>SIGHUP</literal> signal to
+ the application, which then reopens the log file.
+ In <productname>PostgreSQL</productname>, you can run <application>pg_ctl</application>
+ with the <literal>logrotate</literal> option instead. When the server receives
+ this command, the server either switches to a new log file or reopens the
+ existing file, depending on the logging configuration
+ (see <xref linkend="runtime-config-logging-where"/>).
+ </para>
+
+ <note>
+ <para>
+ When using static log file names, the server might fail to reopen the log
+ file if the max open file limit is reached or a file table overflow occurs.
+ In this case, log messages are sent to the old log file until a
+ successful log rotation. If <application>logrotate</application> is
+ configured to compress the log file and delete it, the server may lose
+ the messages logged in this timeframe. To avoid this issue, you can
+ configure the logging collector to dynamically assign log file names
+ and use a <literal>prerotate</literal> script to ignore open log files.
+ </para>
+ </note>
+
<para>
Another production-grade approach to managing log output is to
send it to <application>syslog</application> and let
diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml
index 01a9ba4542..fbbcc7b8e7 100644
--- a/doc/src/sgml/ref/pg_ctl-ref.sgml
+++ b/doc/src/sgml/ref/pg_ctl-ref.sgml
@@ -97,12 +97,19 @@ PostgreSQL documentation
<arg choice="opt"><option>-s</option></arg>
</cmdsynopsis>
+ <cmdsynopsis>
+ <command>pg_ctl</command>
+ <arg choice="plain"><option>logrotate</option></arg>
+ <arg choice="opt"><option>-D</option> <replaceable>datadir</replaceable></arg>
+ <arg choice="opt"><option>-s</option></arg>
+ </cmdsynopsis>
+
<cmdsynopsis>
<command>pg_ctl</command>
<arg choice="plain"><option>kill</option></arg>
<arg choice="plain"><replaceable>signal_name</replaceable></arg>
<arg choice="plain"><replaceable>process_id</replaceable></arg>
- </cmdsynopsis>
+ </cmdsynopsis>
<para>On Microsoft Windows, also:</para>
@@ -226,6 +233,12 @@ PostgreSQL documentation
and begin read-write operations.
</para>
+ <para>
+ <option>logrotate</option> mode rotates the server log file.
+ For details on how to use this mode with external log rotation tools, see
+ <xref linkend="logfile-maintenance"/>.
+ </para>
+
<para>
<option>kill</option> mode sends a signal to a specified process.
This is primarily valuable on <productname>Microsoft Windows</productname>
--
2.16.3
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.kuzmenkov@postgrespro.ru> wrote in <5142559a-82e6-b3e4-d6ed-8fd2d240c77e@postgrespro.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
Attachment
On Tue, Aug 21, 2018 at 4:48 AM Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > At Fri, 10 Aug 2018 15:33:26 +0300, Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> wrote in <5142559a-82e6-b3e4-d6ed-8fd2d240c77e@postgrespro.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
Attachment
At Tue, 21 Aug 2018 11:27:46 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180821022745.GE2897@paquier.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.korotkov@postgrespro.ru> wrote in <CAPpHfduqEyyjLXCNx_t7K2ugCDGVW7WLKL+zrfDEd5wzkvmg-w@mail.gmail.com> > On Tue, Aug 21, 2018 at 4:48 AM Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > > > At Fri, 10 Aug 2018 15:33:26 +0300, Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> wrote in <5142559a-82e6-b3e4-d6ed-8fd2d240c77e@postgrespro.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.kyotaro@lab.ntt.co.jp> wrote: > At Tue, 28 Aug 2018 18:50:31 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in <CAPpHfduqEyyjLXCNx_t7K2ugCDGVW7WLKL+zrfDEd5wzkvmg-w@mail.gmail.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.korotkov@postgrespro.ru> wrote: > On Wed, Aug 29, 2018 at 5:05 AM Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > At Tue, 28 Aug 2018 18:50:31 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in <CAPpHfduqEyyjLXCNx_t7K2ugCDGVW7WLKL+zrfDEd5wzkvmg-w@mail.gmail.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
Attachment
Hello. At Thu, 30 Aug 2018 13:42:42 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in <CAPpHfdvkp2zua+PisUGBuogkDFe133eeaLg3BxeiqQU1U4m_-A@mail.gmail.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.kyotaro@lab.ntt.co.jp> wrote: > At Thu, 30 Aug 2018 13:42:42 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in <CAPpHfdvkp2zua+PisUGBuogkDFe133eeaLg3BxeiqQU1U4m_-A@mail.gmail.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.korotkov@postgrespro.ru> wrote in <CAPpHfdurtsskTTLZaoSTfNRswQaoLfSwMpWY+g+8fjZ9aF34Jw@mail.gmail.com> > On Thu, Aug 30, 2018 at 2:44 PM Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > At Thu, 30 Aug 2018 13:42:42 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in <CAPpHfdvkp2zua+PisUGBuogkDFe133eeaLg3BxeiqQU1U4m_-A@mail.gmail.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