incorrect handling of the timeout in pg_receivexlog
Hi,
When I compiled HEAD with --disable-integer-datetimes and tested
pg_receivexlog, I encountered unexpected replication timeout. As
far as I read the pg_receivexlog code, the cause of this problem is
that pg_receivexlog handles the standby message timeout incorrectly
in --disable-integer-datetimes. The attached patch fixes this problem.
Comments?
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
timeout_handling_v1.patchtext/x-diff; charset=US-ASCII; name=timeout_handling_v1.patchDownload
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 8ca3882..af4add0 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -62,7 +62,7 @@ open_walfile(XLogRecPtr startpoint, uint32 timeline, char *basedir, char *namebu
f = open(fn, O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR);
if (f == -1)
{
- fprintf(stderr, _("%s: Could not open WAL segment %s: %s\n"),
+ fprintf(stderr, _("%s: could not open WAL segment %s: %s\n"),
progname, fn, strerror(errno));
return -1;
}
@@ -190,6 +190,24 @@ localGetCurrentTimestamp(void)
}
/*
+ * Local version of TimestampDifferenceExceeds(), since we are not
+ * linked with backend code.
+ */
+static bool
+localTimestampDifferenceExceeds(TimestampTz start_time,
+ TimestampTz stop_time,
+ int msec)
+{
+ TimestampTz diff = stop_time - start_time;
+
+#ifdef HAVE_INT64_TIMESTAMP
+ return (diff >= msec * INT64CONST(1000));
+#else
+ return (diff * 1000.0 >= msec);
+#endif
+}
+
+/*
* Receive a log stream starting at the specified position.
*
* If sysidentifier is specified, validate that both the system
@@ -301,7 +319,8 @@ ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
*/
now = localGetCurrentTimestamp();
if (standby_message_timeout > 0 &&
- last_status < now - standby_message_timeout * 1000000)
+ localTimestampDifferenceExceeds(last_status, now,
+ standby_message_timeout * 1000))
{
/* Time to send feedback! */
char replybuf[sizeof(StandbyReplyMessage) + 1];
On Tue, Feb 7, 2012 at 2:58 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Hi,
When I compiled HEAD with --disable-integer-datetimes and tested
pg_receivexlog, I encountered unexpected replication timeout. As
far as I read the pg_receivexlog code, the cause of this problem is
that pg_receivexlog handles the standby message timeout incorrectly
in --disable-integer-datetimes. The attached patch fixes this problem.
Comments?
receivelog.c
-------
timeout.tv_sec = last_status + standby_message_timeout - now - 1;
if (timeout.tv_sec <= 0)
-------
Umm.. the above code also handles the timestamp incorrectly. ISTM that the
root cause of these problems is that receivelog.c uses TimestampTz. What
about changing receivelog.c so that it uses time_t instead of TimestampTz?
Which would make the code simpler, I think.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On 07.02.2012 09:03, Fujii Masao wrote:
On Tue, Feb 7, 2012 at 2:58 PM, Fujii Masao<masao.fujii@gmail.com> wrote:
When I compiled HEAD with --disable-integer-datetimes and tested
pg_receivexlog, I encountered unexpected replication timeout. As
far as I read the pg_receivexlog code, the cause of this problem is
that pg_receivexlog handles the standby message timeout incorrectly
in --disable-integer-datetimes. The attached patch fixes this problem.
Comments?receivelog.c
-------
timeout.tv_sec = last_status + standby_message_timeout - now - 1;
if (timeout.tv_sec<= 0)
-------Umm.. the above code also handles the timestamp incorrectly. ISTM that the
root cause of these problems is that receivelog.c uses TimestampTz.
Yep. While localGetCurrentTimestamp() returns a TimestampTz and handles
float timestamps correctly, the caller just assigns the result to a
int64 variable, assuming --enable-integer-datetimes.
What about changing receivelog.c so that it uses time_t instead of
TimestampTz? Which would make the code simpler, I think.
Hmm, that would reduce granularity to seconds. The --statusint option is
given in seconds, but it would be good to have more precision in the
calculations to avoid rounding errors.
But actually, if the purpose of the --statusint option is to avoid
disconnection because of exceeding the server's replication_timeout, one
second granularity just isn't enough to be begin with.
replication_timeout is given in milliseconds, so if you set
replication_timeout=900ms in the server, there is no way to make
pg_basebackup/pg_receivexlog to reply in time.
So, --statusint needs to be in milliseconds. And while we're at it, how
difficult would be to ask the server for the current value of
replication_timeout, and set --statusint automatically based on that? Or
perhaps mark replication_timeout as GUC_REPORT. It is rather fiddly that
depending on a server setting, you need to pass an option in the client
or it will just silently fail with no indication of what the problem is.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Tue, Feb 7, 2012 at 10:31, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
On 07.02.2012 09:03, Fujii Masao wrote:
On Tue, Feb 7, 2012 at 2:58 PM, Fujii Masao<masao.fujii@gmail.com> wrote:
When I compiled HEAD with --disable-integer-datetimes and tested
pg_receivexlog, I encountered unexpected replication timeout. As
far as I read the pg_receivexlog code, the cause of this problem is
that pg_receivexlog handles the standby message timeout incorrectly
in --disable-integer-datetimes. The attached patch fixes this problem.
Comments?receivelog.c
-------
timeout.tv_sec = last_status + standby_message_timeout - now - 1;
if (timeout.tv_sec<= 0)
-------Umm.. the above code also handles the timestamp incorrectly. ISTM that the
root cause of these problems is that receivelog.c uses TimestampTz.Yep. While localGetCurrentTimestamp() returns a TimestampTz and handles
float timestamps correctly, the caller just assigns the result to a int64
variable, assuming --enable-integer-datetimes.
Ugh. Indeed.
What about changing receivelog.c so that it uses time_t instead of
TimestampTz? Which would make the code simpler, I think.Hmm, that would reduce granularity to seconds. The --statusint option is
given in seconds, but it would be good to have more precision in the
calculations to avoid rounding errors.But actually, if the purpose of the --statusint option is to avoid
disconnection because of exceeding the server's replication_timeout, one
second granularity just isn't enough to be begin with. replication_timeout
is given in milliseconds, so if you set replication_timeout=900ms in the
server, there is no way to make pg_basebackup/pg_receivexlog to reply in
time.So, --statusint needs to be in milliseconds. And while we're at it, how
difficult would be to ask the server for the current value of
replication_timeout, and set --statusint automatically based on that? Or
perhaps mark replication_timeout as GUC_REPORT. It is rather fiddly that
depending on a server setting, you need to pass an option in the client or
it will just silently fail with no indication of what the problem is.
We can't really ask for it easily, since we're on a replication
connection. Unless we add that to the walsender grammar, but that
would make it impossible to use the receivexlog stuff against a 9.1
server (which I think still works, though I haven't tested it in a
while).
Do we have a facility to make it a GUC_REPORT but only for walsender
connections? It seems like a very unnecessary thing to have it sent
out over every single connection, since it would only be useful in a
very small subset of them.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 07.02.2012 11:35, Magnus Hagander wrote:
On Tue, Feb 7, 2012 at 10:31, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:So, --statusint needs to be in milliseconds. And while we're at it, how
difficult would be to ask the server for the current value of
replication_timeout, and set --statusint automatically based on that? Or
perhaps mark replication_timeout as GUC_REPORT. It is rather fiddly that
depending on a server setting, you need to pass an option in the client or
it will just silently fail with no indication of what the problem is.We can't really ask for it easily, since we're on a replication
connection. Unless we add that to the walsender grammar, but that
would make it impossible to use the receivexlog stuff against a 9.1
server (which I think still works, though I haven't tested it in a
while).
You could put a version-check there, and only send the command if
connected to a 9.2 server.
Do we have a facility to make it a GUC_REPORT but only for walsender
connections?
There's no such facility at the moment.
It seems like a very unnecessary thing to have it sent out over every
single connection, since it would only be useful in a very small
subset of them.
True, and conversely, many of the current GUC_REPORT settings don't
apply to replication clients at all. Like standard_conforming_strings
and DateStyle.
I think we need another flag for settings that should be sent to
replication clients. GUC_REPORT_REPLICATION? Some settings would have
both GUC_REPORT and GUC_REPORT_REPLICATION, while others would have only
one of them.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Tue, Feb 7, 2012 at 10:55, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
On 07.02.2012 11:35, Magnus Hagander wrote:
On Tue, Feb 7, 2012 at 10:31, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:So, --statusint needs to be in milliseconds. And while we're at it, how
difficult would be to ask the server for the current value of
replication_timeout, and set --statusint automatically based on that? Or
perhaps mark replication_timeout as GUC_REPORT. It is rather fiddly that
depending on a server setting, you need to pass an option in the client
or
it will just silently fail with no indication of what the problem is.We can't really ask for it easily, since we're on a replication
connection. Unless we add that to the walsender grammar, but that
would make it impossible to use the receivexlog stuff against a 9.1
server (which I think still works, though I haven't tested it in a
while).You could put a version-check there, and only send the command if connected
to a 9.2 server.
True..
Do we have a facility to make it a GUC_REPORT but only for walsender
connections?There's no such facility at the moment.
It seems like a very unnecessary thing to have it sent out over every
single connection, since it would only be useful in a very small
subset of them.True, and conversely, many of the current GUC_REPORT settings don't apply to
replication clients at all. Like standard_conforming_strings and DateStyle.
Right. But it's less important there, since the replication
connections will in any reasonable configuration be only a tiny part
of the connections.
I think we need another flag for settings that should be sent to replication
clients. GUC_REPORT_REPLICATION? Some settings would have both GUC_REPORT
and GUC_REPORT_REPLICATION, while others would have only one of them.
Yup, seems like the cleanest solution.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Tue, Feb 7, 2012 at 7:06 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Tue, Feb 7, 2012 at 10:55, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:On 07.02.2012 11:35, Magnus Hagander wrote:
On Tue, Feb 7, 2012 at 10:31, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:So, --statusint needs to be in milliseconds. And while we're at it, how
Attached patch does that and fixes the problem caused under
--disable-integer-datetimes.
difficult would be to ask the server for the current value of
replication_timeout, and set --statusint automatically based on that? Or
perhaps mark replication_timeout as GUC_REPORT. It is rather fiddly that
depending on a server setting, you need to pass an option in the client
or
it will just silently fail with no indication of what the problem is.We can't really ask for it easily, since we're on a replication
connection. Unless we add that to the walsender grammar, but that
would make it impossible to use the receivexlog stuff against a 9.1
server (which I think still works, though I haven't tested it in a
while).You could put a version-check there, and only send the command if connected
to a 9.2 server.True..
Do we have a facility to make it a GUC_REPORT but only for walsender
connections?There's no such facility at the moment.
It seems like a very unnecessary thing to have it sent out over every
single connection, since it would only be useful in a very small
subset of them.True, and conversely, many of the current GUC_REPORT settings don't apply to
replication clients at all. Like standard_conforming_strings and DateStyle.Right. But it's less important there, since the replication
connections will in any reasonable configuration be only a tiny part
of the connections.I think we need another flag for settings that should be sent to replication
clients. GUC_REPORT_REPLICATION? Some settings would have both GUC_REPORT
and GUC_REPORT_REPLICATION, while others would have only one of them.Yup, seems like the cleanest solution.
Agreed. But when replication_timeout is changed by SIGHUP in the server,
there would be a delay before pg_receivexlog receives the parameter
change packet and changes the standby message interval based on the
new value of replication_timeout. Which might cause unexpected replication
timeout. So the user-settable interval (i.e., --statusint) is still useful for
people who want to avoid such fragility, even if we will support the auto-
tuning of the interval.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
timeout_handling_v2.patchtext/x-diff; charset=US-ASCII; name=timeout_handling_v2.patchDownload
*** a/doc/src/sgml/ref/pg_basebackup.sgml
--- b/doc/src/sgml/ref/pg_basebackup.sgml
***************
*** 335,341 **** PostgreSQL documentation
<term><option>--statusint=<replaceable class="parameter">interval</replaceable></option></term>
<listitem>
<para>
! Specifies the number of seconds between status packets sent back to the
server. This is required when streaming the transaction log (using
<literal>--xlog=stream</literal>) if replication timeout is configured
on the server, and allows for easier monitoring. The default value is
--- 335,341 ----
<term><option>--statusint=<replaceable class="parameter">interval</replaceable></option></term>
<listitem>
<para>
! Specifies the number of milliseconds between status packets sent back to the
server. This is required when streaming the transaction log (using
<literal>--xlog=stream</literal>) if replication timeout is configured
on the server, and allows for easier monitoring. The default value is
*** a/doc/src/sgml/ref/pg_receivexlog.sgml
--- b/doc/src/sgml/ref/pg_receivexlog.sgml
***************
*** 108,114 **** PostgreSQL documentation
<term><option>--statusint=<replaceable class="parameter">interval</replaceable></option></term>
<listitem>
<para>
! Specifies the number of seconds between status packets sent back to the
server. This is required if replication timeout is configured on the
server, and allows for easier monitoring. The default value is
10 seconds.
--- 108,114 ----
<term><option>--statusint=<replaceable class="parameter">interval</replaceable></option></term>
<listitem>
<para>
! Specifies the number of milliseconds between status packets sent back to the
server. This is required if replication timeout is configured on the
server, and allows for easier monitoring. The default value is
10 seconds.
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***************
*** 46,52 **** int compresslevel = 0;
bool includewal = false;
bool streamwal = false;
bool fastcheckpoint = false;
! int standby_message_timeout = 10; /* 10 sec = default */
/* Progress counters */
static uint64 totalsize;
--- 46,52 ----
bool includewal = false;
bool streamwal = false;
bool fastcheckpoint = false;
! int standby_message_timeout = 10 * 1000; /* 10 sec = default */
/* Progress counters */
static uint64 totalsize;
***************
*** 118,124 **** usage(void)
printf(_(" --help show this help, then exit\n"));
printf(_(" --version output version information, then exit\n"));
printf(_("\nConnection options:\n"));
! printf(_(" -s, --statusint=INTERVAL time between status packets sent to server (in seconds)\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
--- 118,124 ----
printf(_(" --help show this help, then exit\n"));
printf(_(" --version output version information, then exit\n"));
printf(_("\nConnection options:\n"));
! printf(_(" -s, --statusint=INTERVAL time between status packets sent to server (in milliseconds)\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
*** a/src/bin/pg_basebackup/pg_receivexlog.c
--- b/src/bin/pg_basebackup/pg_receivexlog.c
***************
*** 36,42 ****
/* Global options */
char *basedir = NULL;
int verbose = 0;
! int standby_message_timeout = 10; /* 10 sec = default */
volatile bool time_to_abort = false;
--- 36,42 ----
/* Global options */
char *basedir = NULL;
int verbose = 0;
! int standby_message_timeout = 10 * 1000; /* 10 sec = default */
volatile bool time_to_abort = false;
***************
*** 59,65 **** usage(void)
printf(_(" -?, --help show this help, then exit\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_("\nConnection options:\n"));
! printf(_(" -s, --statusint=INTERVAL time between status packets sent to server (in seconds)\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
--- 59,65 ----
printf(_(" -?, --help show this help, then exit\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_("\nConnection options:\n"));
! printf(_(" -s, --statusint=INTERVAL time between status packets sent to server (in milliseconds)\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
*** a/src/bin/pg_basebackup/receivelog.c
--- b/src/bin/pg_basebackup/receivelog.c
***************
*** 62,68 **** open_walfile(XLogRecPtr startpoint, uint32 timeline, char *basedir, char *namebu
f = open(fn, O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR);
if (f == -1)
{
! fprintf(stderr, _("%s: Could not open WAL segment %s: %s\n"),
progname, fn, strerror(errno));
return -1;
}
--- 62,68 ----
f = open(fn, O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR);
if (f == -1)
{
! fprintf(stderr, _("%s: could not open WAL segment %s: %s\n"),
progname, fn, strerror(errno));
return -1;
}
***************
*** 190,195 **** localGetCurrentTimestamp(void)
--- 190,250 ----
}
/*
+ * Local version of TimestampDifference(), since we are not
+ * linked with backend code.
+ */
+ static void
+ localTimestampDifference(TimestampTz start_time, TimestampTz stop_time,
+ long *secs, int *microsecs)
+ {
+ TimestampTz diff = stop_time - start_time;
+
+ if (diff <= 0)
+ {
+ *secs = 0;
+ *microsecs = 0;
+ }
+ else
+ {
+ #ifdef HAVE_INT64_TIMESTAMP
+ *secs = (long) (diff / USECS_PER_SEC);
+ *microsecs = (int) (diff % USECS_PER_SEC);
+ #else
+ *secs = (long) diff;
+ *microsecs = (int) ((diff - *secs) * 1000000.0);
+ #endif
+ }
+ }
+
+ /*
+ * Local version of TimestampDifferenceExceeds(), since we are not
+ * linked with backend code.
+ */
+ static bool
+ localTimestampDifferenceExceeds(TimestampTz start_time,
+ TimestampTz stop_time,
+ int msec)
+ {
+ TimestampTz diff = stop_time - start_time;
+
+ #ifdef HAVE_INT64_TIMESTAMP
+ return (diff >= msec * INT64CONST(1000));
+ #else
+ return (diff * 1000.0 >= msec);
+ #endif
+ }
+
+ /*
+ * Local version of TimestampTzPlusMilliseconds(), since we are not
+ * linked with backend code.
+ */
+ #ifdef HAVE_INT64_TIMESTAMP
+ #define localTimestampTzPlusMilliseconds(tz,ms) ((tz) + ((ms) * (int64) 1000))
+ #else
+ #define localTimestampTzPlusMilliseconds(tz,ms) ((tz) + ((ms) / 1000.0))
+ #endif
+
+ /*
* Receive a log stream starting at the specified position.
*
* If sysidentifier is specified, validate that both the system
***************
*** 207,213 **** localGetCurrentTimestamp(void)
* indefinitely.
*
* standby_message_timeout controls how often we send a message
! * back to the master letting it know our progress, in seconds.
* This message will only contain the write location, and never
* flush or replay.
*
--- 262,268 ----
* indefinitely.
*
* standby_message_timeout controls how often we send a message
! * back to the master letting it know our progress, in milliseconds.
* This message will only contain the write location, and never
* flush or replay.
*
***************
*** 301,307 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
*/
now = localGetCurrentTimestamp();
if (standby_message_timeout > 0 &&
! last_status < now - standby_message_timeout * 1000000)
{
/* Time to send feedback! */
char replybuf[sizeof(StandbyReplyMessage) + 1];
--- 356,363 ----
*/
now = localGetCurrentTimestamp();
if (standby_message_timeout > 0 &&
! localTimestampDifferenceExceeds(last_status, now,
! standby_message_timeout))
{
/* Time to send feedback! */
char replybuf[sizeof(StandbyReplyMessage) + 1];
***************
*** 340,349 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
FD_SET(PQsocket(conn), &input_mask);
if (standby_message_timeout)
{
! timeout.tv_sec = last_status + standby_message_timeout - now - 1;
! if (timeout.tv_sec <= 0)
! timeout.tv_sec = 1; /* Always sleep at least 1 sec */
! timeout.tv_usec = 0;
timeoutptr = &timeout;
}
else
--- 396,404 ----
FD_SET(PQsocket(conn), &input_mask);
if (standby_message_timeout)
{
! localTimestampDifference(now, localTimestampTzPlusMilliseconds(last_status,
! standby_message_timeout - 1),
! &timeout.tv_sec, (int *)&timeout.tv_usec);
timeoutptr = &timeout;
}
else
***************
*** 361,367 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
}
else if (r < 0)
{
! fprintf(stderr, _("%s: select() failed: %m\n"), progname);
return false;
}
/* Else there is actually data on the socket */
--- 416,422 ----
}
else if (r < 0)
{
! fprintf(stderr, _("%s: select() failed: %s\n"), progname, strerror(errno));
return false;
}
/* Else there is actually data on the socket */
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
On 07.02.2012 09:03, Fujii Masao wrote:
What about changing receivelog.c so that it uses time_t instead of
TimestampTz? Which would make the code simpler, I think.
Hmm, that would reduce granularity to seconds.
It also creates portability issues that I'd just as soon not deal with,
ie, time_t is not the same width on all platforms. (The integer vs
float TimestampTz issue is a kind of portability problem, but we've
already bought into the assumption that sender and receiver must be
built with the same choice, no?)
regards, tom lane
On 07.02.2012 16:55, Tom Lane wrote:
(The integer vs float TimestampTz issue is a kind of portability
problem, but we've already bought into the assumption that sender and
receiver must be built with the same choice, no?)
Hmm, true. In hindsight, I think that was a bad choice, but it's a bit
late to change that. pg_basebackup doesn't otherwise care about the
integer/float timestamps, but it does send a timestamp back to the
server. You won't be able to actually start up the database if the
config options don't match, but I think it would be good if
pg_basebackup still worked across platforms and versions. For example,
you might have a central backup server that calls pg_basebackup on
several database servers, running on different platforms.
In 9.0, the only field in the protocol that depends on timestamp format
is WalDataMessageHeader->sendTime. That goes from server to client, and
pg_basebackup/pg_receivexlog don't care about that. In 9.1 we introduced
StandbyReplyMessage->sendTime, which is sent from client to server, but
looking at the code it looks like the server doesn't use it for
anything. In 9.2, we added WalSndrMessage->sendTime, which is used by a
standby server to calculate how far behind the standby is.
I'm tempted to just change all of those TimestampTz fields to something
that's independent of integer/float timestamp setting, in 9.2. At a
quick glance, it seems that it wouldn't break anything.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Tue, Feb 7, 2012 at 17:29, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
On 07.02.2012 16:55, Tom Lane wrote:
(The integer vs float TimestampTz issue is a kind of portability
problem, but we've already bought into the assumption that sender and
receiver must be built with the same choice, no?)Hmm, true. In hindsight, I think that was a bad choice, but it's a bit late
to change that. pg_basebackup doesn't otherwise care about the integer/float
timestamps, but it does send a timestamp back to the server. You won't be
able to actually start up the database if the config options don't match,
but I think it would be good if pg_basebackup still worked across platforms
and versions. For example, you might have a central backup server that calls
pg_basebackup on several database servers, running on different platforms.In 9.0, the only field in the protocol that depends on timestamp format is
WalDataMessageHeader->sendTime. That goes from server to client, and
pg_basebackup/pg_receivexlog don't care about that. In 9.1 we introduced
StandbyReplyMessage->sendTime, which is sent from client to server, but
looking at the code it looks like the server doesn't use it for anything. In
9.2, we added WalSndrMessage->sendTime, which is used by a standby server to
calculate how far behind the standby is.I'm tempted to just change all of those TimestampTz fields to something
that's independent of integer/float timestamp setting, in 9.2. At a quick
glance, it seems that it wouldn't break anything.
In general, I think that would work. Since we can't replicate across
versions anyway.
Will it break using pg_basebackup 9.2 on a 9.1 server, though? that
would also be very useful in the scenario of the central server...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Wed, Feb 8, 2012 at 1:33 AM, Magnus Hagander <magnus@hagander.net> wrote:
On Tue, Feb 7, 2012 at 17:29, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:On 07.02.2012 16:55, Tom Lane wrote:
(The integer vs float TimestampTz issue is a kind of portability
problem, but we've already bought into the assumption that sender and
receiver must be built with the same choice, no?)Hmm, true. In hindsight, I think that was a bad choice, but it's a bit late
to change that. pg_basebackup doesn't otherwise care about the integer/float
timestamps, but it does send a timestamp back to the server. You won't be
able to actually start up the database if the config options don't match,
but I think it would be good if pg_basebackup still worked across platforms
and versions. For example, you might have a central backup server that calls
pg_basebackup on several database servers, running on different platforms.In 9.0, the only field in the protocol that depends on timestamp format is
WalDataMessageHeader->sendTime. That goes from server to client, and
pg_basebackup/pg_receivexlog don't care about that. In 9.1 we introduced
StandbyReplyMessage->sendTime, which is sent from client to server, but
looking at the code it looks like the server doesn't use it for anything. In
9.2, we added WalSndrMessage->sendTime, which is used by a standby server to
calculate how far behind the standby is.I'm tempted to just change all of those TimestampTz fields to something
that's independent of integer/float timestamp setting, in 9.2. At a quick
glance, it seems that it wouldn't break anything.
Agreed. If we'll have not pushed such change into 9.2, we would break
something later.
In general, I think that would work. Since we can't replicate across
versions anyway.Will it break using pg_basebackup 9.2 on a 9.1 server, though? that
would also be very useful in the scenario of the central server...
No unless I'm missing something. Because pg_basebackup doesn't use
any message which is defined in walprotocol.h if "-x stream" option is
not specified.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Tue, Feb 28, 2012 at 6:08 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2012 at 1:33 AM, Magnus Hagander <magnus@hagander.net> wrote:
Will it break using pg_basebackup 9.2 on a 9.1 server, though? that
would also be very useful in the scenario of the central server...No unless I'm missing something. Because pg_basebackup doesn't use
any message which is defined in walprotocol.h if "-x stream" option is
not specified.
No, this is not right at all :( Changing TimestampTz fields in 9.2 would break
that use case.
If we support that use case, pg_basebackup 9.2 must know which an integer
or a double is used for TimestampTz in 9.1 server. Otherwise pg_basebackup
cannot process a WAL data message proporly. But unfortunately there is no
way for pg_basebackup 9.2 to know that... 9.1 has no API to report the actual
datatype of its TimestampTz field.
One idea to support that use case is to add new command-line option into
pg_basebackup, which specifies the datatype of TimestampTz field. You can
use one pg_basebackup 9.2 executable on 9.1 server whether
--disable-integer-datetimes is specified or not. But I'm not really sure if it's
worth doing this, because ISTM that it's rare to build a server and a
client with
the different choice about TimestampTz datatype.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Argh. This thread appears to have been forgotten - sorry about that.
Given that we're taling about a potential protocol change, we really
should resolve this before we wrap beta, no?
On Thu, Mar 29, 2012 at 6:43 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Feb 28, 2012 at 6:08 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2012 at 1:33 AM, Magnus Hagander <magnus@hagander.net> wrote:
Will it break using pg_basebackup 9.2 on a 9.1 server, though? that
would also be very useful in the scenario of the central server...No unless I'm missing something. Because pg_basebackup doesn't use
any message which is defined in walprotocol.h if "-x stream" option is
not specified.No, this is not right at all :( Changing TimestampTz fields in 9.2 would break
that use case.If we support that use case, pg_basebackup 9.2 must know which an integer
or a double is used for TimestampTz in 9.1 server. Otherwise pg_basebackup
cannot process a WAL data message proporly. But unfortunately there is no
way for pg_basebackup 9.2 to know that... 9.1 has no API to report the actual
datatype of its TimestampTz field.One idea to support that use case is to add new command-line option into
pg_basebackup, which specifies the datatype of TimestampTz field. You can
use one pg_basebackup 9.2 executable on 9.1 server whether
--disable-integer-datetimes is specified or not. But I'm not really sure if it's
worth doing this, because ISTM that it's rare to build a server and a
client with
the different choice about TimestampTz datatype.
I think that's survivable - but what will things look like if they do
mismatch? Can we detect "abnormal values" somewhere and at least abort
in a controlled fashion saying "sorry, wrong build flags"?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Thu, May 10, 2012 at 3:04 PM, Magnus Hagander <magnus@hagander.net> wrote:
Argh. This thread appears to have been forgotten - sorry about that.
Given that we're taling about a potential protocol change, we really
should resolve this before we wrap beta, no?
Had a chat with Heikki about this, and we came to the conslusion that
we don't actually have to fix it befor ebeta. Because pg_basebackup is
going to have to consider 9.1 servers anyway, and we can just treat
9.2beta1 as being a 9.1 from this perspective.
We still have to fix it, but it' snot as urgent :-)
FWIW, the main plan we're onto is still to add the GUCs on new
connections to walsender, so we have something to work with...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Thu, May 10, 2012 at 4:43 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Thu, May 10, 2012 at 3:04 PM, Magnus Hagander <magnus@hagander.net> wrote:
Argh. This thread appears to have been forgotten - sorry about that.
Given that we're taling about a potential protocol change, we really
should resolve this before we wrap beta, no?Had a chat with Heikki about this, and we came to the conslusion that
we don't actually have to fix it befor ebeta. Because pg_basebackup is
going to have to consider 9.1 servers anyway, and we can just treat
9.2beta1 as being a 9.1 from this perspective.We still have to fix it, but it' snot as urgent :-)
FWIW, the main plan we're onto is still to add the GUCs on new
connections to walsender, so we have something to work with...
And taking this a step further - we *already* send these GUCs.
Previous references to us not doing that were incorrect :-)
So this should be a much easier fix than we thought. And can be done
entirely in pg_basebackup, meaning we don't need to worry about
beta...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Thu, May 10, 2012 at 11:51 PM, Magnus Hagander <magnus@hagander.net> wrote:
And taking this a step further - we *already* send these GUCs.
Previous references to us not doing that were incorrect :-)So this should be a much easier fix than we thought. And can be done
entirely in pg_basebackup, meaning we don't need to worry about
beta...
Sounds good!
Regards,
--
Fujii Masao
On Thursday, May 10, 2012, Fujii Masao wrote:
On Thu, May 10, 2012 at 11:51 PM, Magnus Hagander <magnus@hagander.net<javascript:;>>
wrote:And taking this a step further - we *already* send these GUCs.
Previous references to us not doing that were incorrect :-)So this should be a much easier fix than we thought. And can be done
entirely in pg_basebackup, meaning we don't need to worry about
beta...Sounds good!
Should we go down the easy way and just reject connections when the flag is
mismatching between the client and the server (trivial to do - see the
attached patch)? Or should we try to implement both floating point and
integer in pg_basebackup, and make it work in either case? (We'd still have
to reject it if pg_basebackup was compiled without support for int64 at
all, of course, but the large majority of cases will have integer
timestamps these days, but could be made to support both integer and float
for the trivial operations that pg_basebackup actually does).
How common *is* it to have a build that doesn't have integer timestamps
these days? Does any of the binary builds do that at all, for example? If
it's uncommon enough, I think we should just go with the easy way out...
//Magnus
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
validate_int_timestamps.patchtext/x-patch; charset=US-ASCII; name=validate_int_timestamps.patchDownload
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index b12932f..3a3217d 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -75,6 +75,7 @@ GetConnection(void)
const char **keywords;
const char **values;
char *password = NULL;
+ char *tmpparam;
if (dbhost)
argcount++;
@@ -157,6 +158,29 @@ GetConnection(void)
free(values);
free(keywords);
+ /*
+ * Ensure we have the same value of integer timestamps as the
+ * server we are connecting to.
+ */
+ tmpparam = PQparameterStatus(tmpconn, "integer_datetimes");
+ if (!tmpparam)
+ {
+ fprintf(stderr, _("%s: could not determine server setting for integer_datetimes\n"),
+ progname);
+ exit(1);
+ }
+
+#ifdef HAVE_INT64_TIMESTAMP
+ if (strcmp(tmpparam, "on") != 0)
+#else
+ if (strcmp(tmpparam, "off") != 0)
+#endif
+ {
+ fprintf(stderr, _("%s: integer_datetimes compile flag mismatch with server\n"),
+ progname);
+ exit(1);
+ }
+
/* Store the password for next run */
if (password)
dbpassword = password;
Magnus Hagander <magnus@hagander.net> writes:
How common *is* it to have a build that doesn't have integer timestamps
these days? Does any of the binary builds do that at all, for example? If
it's uncommon enough, I think we should just go with the easy way out...
+1 for just rejecting a mismatch.
regards, tom lane
On Sat, May 12, 2012 at 12:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
How common *is* it to have a build that doesn't have integer timestamps
these days? Does any of the binary builds do that at all, for example? If
it's uncommon enough, I think we should just go with the easy way out...+1 for just rejecting a mismatch.
Agreed.
Regards,
--
Fujii Masao
On Fri, May 11, 2012 at 11:43 PM, Magnus Hagander <magnus@hagander.net> wrote:
Should we go down the easy way and just reject connections when the flag is
mismatching between the client and the server (trivial to do - see the
attached patch)?
+ char *tmpparam;
You forgot to add "const" before "char", which causes a compile-time warning.
Regards,
--
Fujii Masao
On Mon, May 14, 2012 at 2:24 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, May 11, 2012 at 11:43 PM, Magnus Hagander <magnus@hagander.net> wrote:
Should we go down the easy way and just reject connections when the flag is
mismatching between the client and the server (trivial to do - see the
attached patch)?+ char *tmpparam;
You forgot to add "const" before "char", which causes a compile-time warning.
I went ahead and committed this, with this fix and a slight change to
the message text.
Hope that's OK with everyone...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, May 22, 2012 at 11:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, May 14, 2012 at 2:24 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, May 11, 2012 at 11:43 PM, Magnus Hagander <magnus@hagander.net> wrote:
Should we go down the easy way and just reject connections when the flag is
mismatching between the client and the server (trivial to do - see the
attached patch)?+ char *tmpparam;
You forgot to add "const" before "char", which causes a compile-time warning.
I went ahead and committed this, with this fix and a slight change to
the message text.
Thanks!
Hope that's OK with everyone...
What about calling PQfinish() before exit() to avoid "unexpected EOF
connection" error?
Patch attached.
Regards,
--
Fujii Masao
Attachments:
pqfinish_before_exit_v1.patchapplication/octet-stream; name=pqfinish_before_exit_v1.patchDownload
*** a/src/bin/pg_basebackup/streamutil.c
--- b/src/bin/pg_basebackup/streamutil.c
***************
*** 167,172 **** GetConnection(void)
--- 167,173 ----
{
fprintf(stderr, _("%s: could not determine server setting for integer_datetimes\n"),
progname);
+ PQfinish(tmpconn);
exit(1);
}
***************
*** 178,183 **** GetConnection(void)
--- 179,185 ----
{
fprintf(stderr, _("%s: integer_datetimes compile flag does not match server\n"),
progname);
+ PQfinish(tmpconn);
exit(1);
}
On Wed, May 23, 2012 at 8:11 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, May 22, 2012 at 11:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, May 14, 2012 at 2:24 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, May 11, 2012 at 11:43 PM, Magnus Hagander <magnus@hagander.net> wrote:
Should we go down the easy way and just reject connections when the flag is
mismatching between the client and the server (trivial to do - see the
attached patch)?+ char *tmpparam;
You forgot to add "const" before "char", which causes a compile-time warning.
I went ahead and committed this, with this fix and a slight change to
the message text.Thanks!
Hope that's OK with everyone...
What about calling PQfinish() before exit() to avoid "unexpected EOF
connection" error?
Patch attached.
Makes sense, applied.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Thu, May 24, 2012 at 4:52 AM, Magnus Hagander <magnus@hagander.net> wrote:
On Wed, May 23, 2012 at 8:11 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, May 22, 2012 at 11:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, May 14, 2012 at 2:24 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, May 11, 2012 at 11:43 PM, Magnus Hagander <magnus@hagander.net> wrote:
Should we go down the easy way and just reject connections when the flag is
mismatching between the client and the server (trivial to do - see the
attached patch)?+ char *tmpparam;
You forgot to add "const" before "char", which causes a compile-time warning.
I went ahead and committed this, with this fix and a slight change to
the message text.Thanks!
Hope that's OK with everyone...
What about calling PQfinish() before exit() to avoid "unexpected EOF
connection" error?
Patch attached.Makes sense, applied.
Thanks! So, let's go back to the original problem: pg_receivexlog
still doesn't work fine
under --disable-integer-datetimes. I previously posted the patch which
fixes that problem.
http://archives.postgresql.org/message-id/CAHGQGwFutqnFPBYcHUCuoy1zMVDXto=o4OgsjrBWxW4zj2TCSw@mail.gmail.com
Attached is the updated version of the patch. Comments?
Regards,
--
Fujii Masao
Attachments:
timeout_handling_v3.patchapplication/octet-stream; name=timeout_handling_v3.patchDownload
*** a/doc/src/sgml/ref/pg_basebackup.sgml
--- b/doc/src/sgml/ref/pg_basebackup.sgml
***************
*** 335,341 **** PostgreSQL documentation
<term><option>--statusint=<replaceable class="parameter">interval</replaceable></option></term>
<listitem>
<para>
! Specifies the number of seconds between status packets sent back to the
server. This is required when streaming the transaction log (using
<literal>--xlog=stream</literal>) if replication timeout is configured
on the server, and allows for easier monitoring. The default value is
--- 335,341 ----
<term><option>--statusint=<replaceable class="parameter">interval</replaceable></option></term>
<listitem>
<para>
! Specifies the number of milliseconds between status packets sent back to the
server. This is required when streaming the transaction log (using
<literal>--xlog=stream</literal>) if replication timeout is configured
on the server, and allows for easier monitoring. The default value is
*** a/doc/src/sgml/ref/pg_receivexlog.sgml
--- b/doc/src/sgml/ref/pg_receivexlog.sgml
***************
*** 108,114 **** PostgreSQL documentation
<term><option>--statusint=<replaceable class="parameter">interval</replaceable></option></term>
<listitem>
<para>
! Specifies the number of seconds between status packets sent back to the
server. This is required if replication timeout is configured on the
server, and allows for easier monitoring. The default value is
10 seconds.
--- 108,114 ----
<term><option>--statusint=<replaceable class="parameter">interval</replaceable></option></term>
<listitem>
<para>
! Specifies the number of milliseconds between status packets sent back to the
server. This is required if replication timeout is configured on the
server, and allows for easier monitoring. The default value is
10 seconds.
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***************
*** 46,52 **** int compresslevel = 0;
bool includewal = false;
bool streamwal = false;
bool fastcheckpoint = false;
! int standby_message_timeout = 10; /* 10 sec = default */
/* Progress counters */
static uint64 totalsize;
--- 46,52 ----
bool includewal = false;
bool streamwal = false;
bool fastcheckpoint = false;
! int standby_message_timeout = 10 * 1000; /* 10 sec = default */
/* Progress counters */
static uint64 totalsize;
***************
*** 118,124 **** usage(void)
printf(_(" --help show this help, then exit\n"));
printf(_(" --version output version information, then exit\n"));
printf(_("\nConnection options:\n"));
! printf(_(" -s, --statusint=INTERVAL time between status packets sent to server (in seconds)\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
--- 118,124 ----
printf(_(" --help show this help, then exit\n"));
printf(_(" --version output version information, then exit\n"));
printf(_("\nConnection options:\n"));
! printf(_(" -s, --statusint=INTERVAL time between status packets sent to server (in milliseconds)\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
*** a/src/bin/pg_basebackup/pg_receivexlog.c
--- b/src/bin/pg_basebackup/pg_receivexlog.c
***************
*** 36,42 ****
/* Global options */
char *basedir = NULL;
int verbose = 0;
! int standby_message_timeout = 10; /* 10 sec = default */
volatile bool time_to_abort = false;
--- 36,42 ----
/* Global options */
char *basedir = NULL;
int verbose = 0;
! int standby_message_timeout = 10 * 1000; /* 10 sec = default */
volatile bool time_to_abort = false;
***************
*** 59,65 **** usage(void)
printf(_(" -?, --help show this help, then exit\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_("\nConnection options:\n"));
! printf(_(" -s, --statusint=INTERVAL time between status packets sent to server (in seconds)\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
--- 59,65 ----
printf(_(" -?, --help show this help, then exit\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_("\nConnection options:\n"));
! printf(_(" -s, --statusint=INTERVAL time between status packets sent to server (in milliseconds)\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
*** a/src/bin/pg_basebackup/receivelog.c
--- b/src/bin/pg_basebackup/receivelog.c
***************
*** 62,68 **** open_walfile(XLogRecPtr startpoint, uint32 timeline, char *basedir, char *namebu
f = open(fn, O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR);
if (f == -1)
{
! fprintf(stderr, _("%s: Could not open WAL segment %s: %s\n"),
progname, fn, strerror(errno));
return -1;
}
--- 62,68 ----
f = open(fn, O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR);
if (f == -1)
{
! fprintf(stderr, _("%s: could not open WAL segment %s: %s\n"),
progname, fn, strerror(errno));
return -1;
}
***************
*** 196,201 **** localGetCurrentTimestamp(void)
--- 196,256 ----
}
/*
+ * Local version of TimestampDifference(), since we are not
+ * linked with backend code.
+ */
+ static void
+ localTimestampDifference(TimestampTz start_time, TimestampTz stop_time,
+ long *secs, int *microsecs)
+ {
+ TimestampTz diff = stop_time - start_time;
+
+ if (diff <= 0)
+ {
+ *secs = 0;
+ *microsecs = 0;
+ }
+ else
+ {
+ #ifdef HAVE_INT64_TIMESTAMP
+ *secs = (long) (diff / USECS_PER_SEC);
+ *microsecs = (int) (diff % USECS_PER_SEC);
+ #else
+ *secs = (long) diff;
+ *microsecs = (int) ((diff - *secs) * 1000000.0);
+ #endif
+ }
+ }
+
+ /*
+ * Local version of TimestampDifferenceExceeds(), since we are not
+ * linked with backend code.
+ */
+ static bool
+ localTimestampDifferenceExceeds(TimestampTz start_time,
+ TimestampTz stop_time,
+ int msec)
+ {
+ TimestampTz diff = stop_time - start_time;
+
+ #ifdef HAVE_INT64_TIMESTAMP
+ return (diff >= msec * INT64CONST(1000));
+ #else
+ return (diff * 1000.0 >= msec);
+ #endif
+ }
+
+ /*
+ * Local version of TimestampTzPlusMilliseconds(), since we are not
+ * linked with backend code.
+ */
+ #ifdef HAVE_INT64_TIMESTAMP
+ #define localTimestampTzPlusMilliseconds(tz,ms) ((tz) + ((ms) * (int64) 1000))
+ #else
+ #define localTimestampTzPlusMilliseconds(tz,ms) ((tz) + ((ms) / 1000.0))
+ #endif
+
+ /*
* Receive a log stream starting at the specified position.
*
* If sysidentifier is specified, validate that both the system
***************
*** 212,218 **** localGetCurrentTimestamp(void)
* indefinitely.
*
* standby_message_timeout controls how often we send a message
! * back to the master letting it know our progress, in seconds.
* This message will only contain the write location, and never
* flush or replay.
*
--- 267,273 ----
* indefinitely.
*
* standby_message_timeout controls how often we send a message
! * back to the master letting it know our progress, in milliseconds.
* This message will only contain the write location, and never
* flush or replay.
*
***************
*** 306,312 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
*/
now = localGetCurrentTimestamp();
if (standby_message_timeout > 0 &&
! last_status < now - standby_message_timeout * 1000000)
{
/* Time to send feedback! */
char replybuf[sizeof(StandbyReplyMessage) + 1];
--- 361,368 ----
*/
now = localGetCurrentTimestamp();
if (standby_message_timeout > 0 &&
! localTimestampDifferenceExceeds(last_status, now,
! standby_message_timeout))
{
/* Time to send feedback! */
char replybuf[sizeof(StandbyReplyMessage) + 1];
***************
*** 345,354 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
FD_SET(PQsocket(conn), &input_mask);
if (standby_message_timeout)
{
! timeout.tv_sec = last_status + standby_message_timeout - now - 1;
! if (timeout.tv_sec <= 0)
! timeout.tv_sec = 1; /* Always sleep at least 1 sec */
! timeout.tv_usec = 0;
timeoutptr = &timeout;
}
else
--- 401,409 ----
FD_SET(PQsocket(conn), &input_mask);
if (standby_message_timeout)
{
! localTimestampDifference(now, localTimestampTzPlusMilliseconds(last_status,
! standby_message_timeout - 1),
! &timeout.tv_sec, (int *)&timeout.tv_usec);
timeoutptr = &timeout;
}
else
***************
*** 366,372 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
}
else if (r < 0)
{
! fprintf(stderr, _("%s: select() failed: %m\n"), progname);
return false;
}
/* Else there is actually data on the socket */
--- 421,427 ----
}
else if (r < 0)
{
! fprintf(stderr, _("%s: select() failed: %s\n"), progname, strerror(errno));
return false;
}
/* Else there is actually data on the socket */
On Fri, May 25, 2012 at 7:56 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, May 24, 2012 at 4:52 AM, Magnus Hagander <magnus@hagander.net> wrote:
On Wed, May 23, 2012 at 8:11 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, May 22, 2012 at 11:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, May 14, 2012 at 2:24 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, May 11, 2012 at 11:43 PM, Magnus Hagander <magnus@hagander.net> wrote:
Should we go down the easy way and just reject connections when the flag is
mismatching between the client and the server (trivial to do - see the
attached patch)?+ char *tmpparam;
You forgot to add "const" before "char", which causes a compile-time warning.
I went ahead and committed this, with this fix and a slight change to
the message text.Thanks!
Hope that's OK with everyone...
What about calling PQfinish() before exit() to avoid "unexpected EOF
connection" error?
Patch attached.Makes sense, applied.
Thanks! So, let's go back to the original problem: pg_receivexlog
still doesn't work fine
under --disable-integer-datetimes. I previously posted the patch which
fixes that problem.
http://archives.postgresql.org/message-id/CAHGQGwFutqnFPBYcHUCuoy1zMVDXto=o4OgsjrBWxW4zj2TCSw@mail.gmail.comAttached is the updated version of the patch. Comments?
It contains a number of unrelated changes of %m -> %s - what's the
motivation for those?
You also removed the "safeguard" of always sleeping at least 1 second
- should we keep some level of safeguard there, even if it's not in
full seconds anymore?
Is the -1 sent into localTimestampDifference still relevent at all?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Tue, Jun 5, 2012 at 5:32 PM, Magnus Hagander <magnus@hagander.net> wrote:
It contains a number of unrelated changes of %m -> %s - what's the
motivation for those?
%m in fprintf() is glibc extension according to man page, so it's not portable
and should not be used, I think.
We discussed this before and reached consensus not to use %m :)
http://archives.postgresql.org/pgsql-hackers/2011-01/msg01674.php
You also removed the "safeguard" of always sleeping at least 1 second
- should we keep some level of safeguard there, even if it's not in
full seconds anymore?Is the -1 sent into localTimestampDifference still relevent at all?
No because that "safeguard" would mess up with a user who sets
replication_timeout to less than one second. Though I'm not sure
whether there is really any user who wants such too short timeout....
Regards,
--
Fujii Masao
On Tue, Jun 5, 2012 at 3:36 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Jun 5, 2012 at 5:32 PM, Magnus Hagander <magnus@hagander.net> wrote:
It contains a number of unrelated changes of %m -> %s - what's the
motivation for those?%m in fprintf() is glibc extension according to man page, so it's not portable
and should not be used, I think.We discussed this before and reached consensus not to use %m :)
http://archives.postgresql.org/pgsql-hackers/2011-01/msg01674.php
:-) there goes my memory.
That said, we're using %m in a fairly large number of places already,
but they're mostly in the backend. I guess we're safe there.
Anyway, +1 for making that change then, but I'll make it as a separate patch.
You also removed the "safeguard" of always sleeping at least 1 second
- should we keep some level of safeguard there, even if it's not in
full seconds anymore?Is the -1 sent into localTimestampDifference still relevent at all?
No because that "safeguard" would mess up with a user who sets
replication_timeout to less than one second. Though I'm not sure
whether there is really any user who wants such too short timeout....
Right - I meant we might want to adjust the safeguad. Assuming <1 sec
is reasonable, maybe cap it at 100ms or so?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
On Tue, Jun 5, 2012 at 3:36 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
We discussed this before and reached consensus not to use %m :)
http://archives.postgresql.org/pgsql-hackers/2011-01/msg01674.php
:-) there goes my memory.
That said, we're using %m in a fairly large number of places already,
but they're mostly in the backend. I guess we're safe there.
It should only appear in elog/ereport calls; if there are any in bare
printfs, they are wrong, just as Fujii-san says. Frontend or backend
doesn't matter.
regards, tom lane
On Tue, Jun 5, 2012 at 10:39 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Tue, Jun 5, 2012 at 3:36 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Jun 5, 2012 at 5:32 PM, Magnus Hagander <magnus@hagander.net> wrote:
You also removed the "safeguard" of always sleeping at least 1 second
- should we keep some level of safeguard there, even if it's not in
full seconds anymore?Is the -1 sent into localTimestampDifference still relevent at all?
No because that "safeguard" would mess up with a user who sets
replication_timeout to less than one second. Though I'm not sure
whether there is really any user who wants such too short timeout....Right - I meant we might want to adjust the safeguad. Assuming <1 sec
is reasonable, maybe cap it at 100ms or so?
On second thought, the status packet interval doesn't need to be given
in milliseconds at all. As I said, which would mess up with a user who sets
replication_timeout to less than 1 sec. But since wal_receiver_status_interval
is given in seconds, we've already messed up with them even if we've
changed pg_receivexlog so that its status interval can be given in
milliseconds.
We received no complaints about wal_receiver_status_interval so far, so
I think there is still no need to allow pg_receivexlog --statusint to be set
to less than 1 sec. Thought?
Regards,
--
Fujii Masao
On Tue, Jun 5, 2012 at 4:28 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Jun 5, 2012 at 10:39 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Tue, Jun 5, 2012 at 3:36 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Jun 5, 2012 at 5:32 PM, Magnus Hagander <magnus@hagander.net> wrote:
You also removed the "safeguard" of always sleeping at least 1 second
- should we keep some level of safeguard there, even if it's not in
full seconds anymore?Is the -1 sent into localTimestampDifference still relevent at all?
No because that "safeguard" would mess up with a user who sets
replication_timeout to less than one second. Though I'm not sure
whether there is really any user who wants such too short timeout....Right - I meant we might want to adjust the safeguad. Assuming <1 sec
is reasonable, maybe cap it at 100ms or so?On second thought, the status packet interval doesn't need to be given
in milliseconds at all. As I said, which would mess up with a user who sets
replication_timeout to less than 1 sec. But since wal_receiver_status_interval
is given in seconds, we've already messed up with them even if we've
changed pg_receivexlog so that its status interval can be given in
milliseconds.We received no complaints about wal_receiver_status_interval so far, so
I think there is still no need to allow pg_receivexlog --statusint to be set
to less than 1 sec. Thought?
Works for me. We still need a (reworked) patch, though, right? We just
move where the move between seconds and milliseconds happens?
I definitely don't think we need subsecond granularity in the user
facing number. Even a second is pretty short. (We do need to retain
the ability to set it to 0 = off of course).
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Tue, Jun 5, 2012 at 11:42 PM, Magnus Hagander <magnus@hagander.net> wrote:
Works for me. We still need a (reworked) patch, though, right? We just
move where the move between seconds and milliseconds happens?
Attached is the updated version of the patch.
I definitely don't think we need subsecond granularity in the user
facing number. Even a second is pretty short.
Yep.
(We do need to retain the ability to set it to 0 = off of course).
Yep, a value of zero disables the status updates, and the patch adds
that explanation into the document of pg_basebackup and pg_receivexlog.
Regards,
--
Fujii Masao
Attachments:
timeout_handling_v4.patchapplication/octet-stream; name=timeout_handling_v4.patchDownload
*** a/doc/src/sgml/ref/pg_basebackup.sgml
--- b/doc/src/sgml/ref/pg_basebackup.sgml
***************
*** 338,345 **** PostgreSQL documentation
Specifies the number of seconds between status packets sent back to the
server. This is required when streaming the transaction log (using
<literal>--xlog=stream</literal>) if replication timeout is configured
! on the server, and allows for easier monitoring. The default value is
! 10 seconds.
</para>
</listitem>
</varlistentry>
--- 338,345 ----
Specifies the number of seconds between status packets sent back to the
server. This is required when streaming the transaction log (using
<literal>--xlog=stream</literal>) if replication timeout is configured
! on the server, and allows for easier monitoring. A value of zero disables
! the status updates completely. The default value is 10 seconds.
</para>
</listitem>
</varlistentry>
*** a/doc/src/sgml/ref/pg_receivexlog.sgml
--- b/doc/src/sgml/ref/pg_receivexlog.sgml
***************
*** 129,136 **** PostgreSQL documentation
<para>
Specifies the number of seconds between status packets sent back to the
server. This is required if replication timeout is configured on the
! server, and allows for easier monitoring. The default value is
! 10 seconds.
</para>
</listitem>
</varlistentry>
--- 129,136 ----
<para>
Specifies the number of seconds between status packets sent back to the
server. This is required if replication timeout is configured on the
! server, and allows for easier monitoring. A value of zero disables the
! status updates completely. The default value is 10 seconds.
</para>
</listitem>
</varlistentry>
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***************
*** 46,52 **** int compresslevel = 0;
bool includewal = false;
bool streamwal = false;
bool fastcheckpoint = false;
! int standby_message_timeout = 10; /* 10 sec = default */
/* Progress counters */
static uint64 totalsize;
--- 46,52 ----
bool includewal = false;
bool streamwal = false;
bool fastcheckpoint = false;
! int standby_message_timeout = 10 * 1000; /* 10 sec = default */
/* Progress counters */
static uint64 totalsize;
***************
*** 1311,1317 **** main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
! standby_message_timeout = atoi(optarg);
if (standby_message_timeout < 0)
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
--- 1311,1317 ----
dbgetpassword = 1;
break;
case 's':
! standby_message_timeout = atoi(optarg) * 1000;
if (standby_message_timeout < 0)
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
*** a/src/bin/pg_basebackup/pg_receivexlog.c
--- b/src/bin/pg_basebackup/pg_receivexlog.c
***************
*** 40,46 ****
char *basedir = NULL;
int verbose = 0;
int noloop = 0;
! int standby_message_timeout = 10; /* 10 sec = default */
volatile bool time_to_abort = false;
--- 40,46 ----
char *basedir = NULL;
int verbose = 0;
int noloop = 0;
! int standby_message_timeout = 10 * 1000; /* 10 sec = default */
volatile bool time_to_abort = false;
***************
*** 356,362 **** main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
! standby_message_timeout = atoi(optarg);
if (standby_message_timeout < 0)
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
--- 356,362 ----
dbgetpassword = 1;
break;
case 's':
! standby_message_timeout = atoi(optarg) * 1000;
if (standby_message_timeout < 0)
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
*** a/src/bin/pg_basebackup/receivelog.c
--- b/src/bin/pg_basebackup/receivelog.c
***************
*** 62,68 **** open_walfile(XLogRecPtr startpoint, uint32 timeline, char *basedir, char *namebu
f = open(fn, O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR);
if (f == -1)
{
! fprintf(stderr, _("%s: Could not open WAL segment %s: %s\n"),
progname, fn, strerror(errno));
return -1;
}
--- 62,68 ----
f = open(fn, O_WRONLY | O_CREAT | PG_BINARY, S_IRUSR | S_IWUSR);
if (f == -1)
{
! fprintf(stderr, _("%s: could not open WAL segment %s: %s\n"),
progname, fn, strerror(errno));
return -1;
}
***************
*** 196,201 **** localGetCurrentTimestamp(void)
--- 196,256 ----
}
/*
+ * Local version of TimestampDifference(), since we are not
+ * linked with backend code.
+ */
+ static void
+ localTimestampDifference(TimestampTz start_time, TimestampTz stop_time,
+ long *secs, int *microsecs)
+ {
+ TimestampTz diff = stop_time - start_time;
+
+ if (diff <= 0)
+ {
+ *secs = 0;
+ *microsecs = 0;
+ }
+ else
+ {
+ #ifdef HAVE_INT64_TIMESTAMP
+ *secs = (long) (diff / USECS_PER_SEC);
+ *microsecs = (int) (diff % USECS_PER_SEC);
+ #else
+ *secs = (long) diff;
+ *microsecs = (int) ((diff - *secs) * 1000000.0);
+ #endif
+ }
+ }
+
+ /*
+ * Local version of TimestampDifferenceExceeds(), since we are not
+ * linked with backend code.
+ */
+ static bool
+ localTimestampDifferenceExceeds(TimestampTz start_time,
+ TimestampTz stop_time,
+ int msec)
+ {
+ TimestampTz diff = stop_time - start_time;
+
+ #ifdef HAVE_INT64_TIMESTAMP
+ return (diff >= msec * INT64CONST(1000));
+ #else
+ return (diff * 1000.0 >= msec);
+ #endif
+ }
+
+ /*
+ * Local version of TimestampTzPlusMilliseconds(), since we are not
+ * linked with backend code.
+ */
+ #ifdef HAVE_INT64_TIMESTAMP
+ #define localTimestampTzPlusMilliseconds(tz,ms) ((tz) + ((ms) * (int64) 1000))
+ #else
+ #define localTimestampTzPlusMilliseconds(tz,ms) ((tz) + ((ms) / 1000.0))
+ #endif
+
+ /*
* Receive a log stream starting at the specified position.
*
* If sysidentifier is specified, validate that both the system
***************
*** 306,312 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
*/
now = localGetCurrentTimestamp();
if (standby_message_timeout > 0 &&
! last_status < now - standby_message_timeout * 1000000)
{
/* Time to send feedback! */
char replybuf[sizeof(StandbyReplyMessage) + 1];
--- 361,368 ----
*/
now = localGetCurrentTimestamp();
if (standby_message_timeout > 0 &&
! localTimestampDifferenceExceeds(last_status, now,
! standby_message_timeout))
{
/* Time to send feedback! */
char replybuf[sizeof(StandbyReplyMessage) + 1];
***************
*** 345,354 **** ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos, uint32 timeline, char *sysi
FD_SET(PQsocket(conn), &input_mask);
if (standby_message_timeout)
{
! timeout.tv_sec = last_status + standby_message_timeout - now - 1;
if (timeout.tv_sec <= 0)
timeout.tv_sec = 1; /* Always sleep at least 1 sec */
- timeout.tv_usec = 0;
timeoutptr = &timeout;
}
else
--- 401,411 ----
FD_SET(PQsocket(conn), &input_mask);
if (standby_message_timeout)
{
! localTimestampDifference(now, localTimestampTzPlusMilliseconds(last_status,
! standby_message_timeout - 1),
! &timeout.tv_sec, (int *)&timeout.tv_usec);
if (timeout.tv_sec <= 0)
timeout.tv_sec = 1; /* Always sleep at least 1 sec */
timeoutptr = &timeout;
}
else
On Wed, Jun 6, 2012 at 8:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Jun 5, 2012 at 11:42 PM, Magnus Hagander <magnus@hagander.net> wrote:
Works for me. We still need a (reworked) patch, though, right? We just
move where the move between seconds and milliseconds happens?Attached is the updated version of the patch.
Thanks.
I definitely don't think we need subsecond granularity in the user
facing number. Even a second is pretty short.Yep.
(We do need to retain the ability to set it to 0 = off of course).
Yep, a value of zero disables the status updates, and the patch adds
that explanation into the document of pg_basebackup and pg_receivexlog.
Applied, with some small modifications. For example, you don't need a
frontend-specific copy of #define's that are in the backend, since
those don't require linking to the backend, just the #include.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/