InvalidXLogRecPtr in docs
I found a term "InvalidXLogRecPtr" in 9.0 docs.
http://developer.postgresql.org/pgdocs/postgres/functions-admin.html#FUNCTIONS-RECOVERY-INFO-TABLE
| ... then the return value will be InvalidXLogRecPtr (0/0).
I think it should not appear in docs because it's a name for an internal
constant variable. I'd like to rewrite the description like:
... then the return value will be 0/0, that is never used in normal cases.
Comments?
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
On Wed, Jun 9, 2010 at 9:46 PM, Takahiro Itagaki
<itagaki.takahiro@oss.ntt.co.jp> wrote:
I found a term "InvalidXLogRecPtr" in 9.0 docs.
http://developer.postgresql.org/pgdocs/postgres/functions-admin.html#FUNCTIONS-RECOVERY-INFO-TABLE
| ... then the return value will be InvalidXLogRecPtr (0/0).I think it should not appear in docs because it's a name for an internal
constant variable. I'd like to rewrite the description like:... then the return value will be 0/0, that is never used in normal cases.
Comments?
Maybe we should be returning NULL instead of 0/0.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Jun 9, 2010 at 9:46 PM, Takahiro Itagaki
<itagaki.takahiro@oss.ntt.co.jp> wrote:I found a term "InvalidXLogRecPtr" in 9.0 docs.
http://developer.postgresql.org/pgdocs/postgres/functions-admin.html#FUNCTIONS-RECOVERY-INFO-TABLE
| ... then the return value will be InvalidXLogRecPtr (0/0).
Maybe we should be returning NULL instead of 0/0.
+1 for using NULL instead of an artificially chosen value, for both of
those functions.
regards, tom lane
On Thu, Jun 10, 2010 at 11:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Jun 9, 2010 at 9:46 PM, Takahiro Itagaki
<itagaki.takahiro@oss.ntt.co.jp> wrote:I found a term "InvalidXLogRecPtr" in 9.0 docs.
http://developer.postgresql.org/pgdocs/postgres/functions-admin.html#FUNCTIONS-RECOVERY-INFO-TABLE
| ... then the return value will be InvalidXLogRecPtr (0/0).Maybe we should be returning NULL instead of 0/0.
+1 for using NULL instead of an artificially chosen value, for both of
those functions.
Okay, the attached patch makes those functions return NULL in that case.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
recovery_funcs_return_null_v1.patchapplication/octet-stream; name=recovery_funcs_return_null_v1.patchDownload
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 13286,13292 **** postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
then this value will remain static at the value of the last WAL record
received and synced to disk during that recovery. When the server has
been started without a streaming recovery then the return value will be
! InvalidXLogRecPtr (0/0).
</entry>
</row>
<row>
--- 13286,13292 ----
then this value will remain static at the value of the last WAL record
received and synced to disk during that recovery. When the server has
been started without a streaming recovery then the return value will be
! <literal>NULL</>.
</entry>
</row>
<row>
***************
*** 13299,13305 **** postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
If recovery has completed then this value will remain static at
the value of the last WAL record applied during that recovery.
When the server has been started normally without a recovery
! then the return value will be InvalidXLogRecPtr (0/0).
</entry>
</row>
</tbody>
--- 13299,13305 ----
If recovery has completed then this value will remain static at
the value of the last WAL record applied during that recovery.
When the server has been started normally without a recovery
! then the return value will be <literal>NULL</>.
</entry>
</row>
</tbody>
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 8753,8765 **** Datum
pg_last_xlog_receive_location(PG_FUNCTION_ARGS)
{
XLogRecPtr recptr;
- char location[MAXFNAMELEN];
recptr = GetWalRcvWriteRecPtr();
! snprintf(location, sizeof(location), "%X/%X",
! recptr.xlogid, recptr.xrecoff);
! PG_RETURN_TEXT_P(cstring_to_text(location));
}
/*
--- 8753,8771 ----
pg_last_xlog_receive_location(PG_FUNCTION_ARGS)
{
XLogRecPtr recptr;
recptr = GetWalRcvWriteRecPtr();
! if (recptr.xlogid == 0 && recptr.xrecoff == 0)
! PG_RETURN_NULL();
! else
! {
! char location[MAXFNAMELEN];
!
! snprintf(location, sizeof(location), "%X/%X",
! recptr.xlogid, recptr.xrecoff);
! PG_RETURN_TEXT_P(cstring_to_text(location));
! }
}
/*
***************
*** 8774,8788 **** pg_last_xlog_replay_location(PG_FUNCTION_ARGS)
/* use volatile pointer to prevent code rearrangement */
volatile XLogCtlData *xlogctl = XLogCtl;
XLogRecPtr recptr;
- char location[MAXFNAMELEN];
SpinLockAcquire(&xlogctl->info_lck);
recptr = xlogctl->recoveryLastRecPtr;
SpinLockRelease(&xlogctl->info_lck);
! snprintf(location, sizeof(location), "%X/%X",
! recptr.xlogid, recptr.xrecoff);
! PG_RETURN_TEXT_P(cstring_to_text(location));
}
/*
--- 8780,8800 ----
/* use volatile pointer to prevent code rearrangement */
volatile XLogCtlData *xlogctl = XLogCtl;
XLogRecPtr recptr;
SpinLockAcquire(&xlogctl->info_lck);
recptr = xlogctl->recoveryLastRecPtr;
SpinLockRelease(&xlogctl->info_lck);
! if (recptr.xlogid == 0 && recptr.xrecoff == 0)
! PG_RETURN_NULL();
! else
! {
! char location[MAXFNAMELEN];
!
! snprintf(location, sizeof(location), "%X/%X",
! recptr.xlogid, recptr.xrecoff);
! PG_RETURN_TEXT_P(cstring_to_text(location));
! }
}
/*
On 10/06/10 05:56, Tom Lane wrote:
Robert Haas<robertmhaas@gmail.com> writes:
On Wed, Jun 9, 2010 at 9:46 PM, Takahiro Itagaki
<itagaki.takahiro@oss.ntt.co.jp> wrote:I found a term "InvalidXLogRecPtr" in 9.0 docs.
http://developer.postgresql.org/pgdocs/postgres/functions-admin.html#FUNCTIONS-RECOVERY-INFO-TABLE
| ... then the return value will be InvalidXLogRecPtr (0/0).Maybe we should be returning NULL instead of 0/0.
+1 for using NULL instead of an artificially chosen value, for both of
those functions.
Agreed, committed a patch to do that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On 10/06/10 09:42, Fujii Masao wrote:
On Thu, Jun 10, 2010 at 11:56 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Robert Haas<robertmhaas@gmail.com> writes:
On Wed, Jun 9, 2010 at 9:46 PM, Takahiro Itagaki
<itagaki.takahiro@oss.ntt.co.jp> wrote:I found a term "InvalidXLogRecPtr" in 9.0 docs.
http://developer.postgresql.org/pgdocs/postgres/functions-admin.html#FUNCTIONS-RECOVERY-INFO-TABLE
| ... then the return value will be InvalidXLogRecPtr (0/0).Maybe we should be returning NULL instead of 0/0.
+1 for using NULL instead of an artificially chosen value, for both of
those functions.Okay, the attached patch makes those functions return NULL in that case.
Ah, I just committed a patch to do the same, before seeing your email.
Thanks anyway.
BTW, the docs claim about pg_last_xlog_location() that "While streaming
replication is in progress this will increase monotonically." That's a
bit misleading: when the replication connection is broken for some
reason and we restart it, we begin streaming from the beginning of the
last WAL segment. So at that moment, pg_last_xlog_location() moves
backwards to the beginning of the WAL segment.
Should we:
1. Just document that,
2. Change pg_last_xlog_location() to not move backwards in that case, or
3. Change the behavior so that we start streaming at the exact byte
location where we left off?
I believe that starting from the beginning of the WAL segment is just
paranoia, to avoid creating a WAL file that's missing some data from the
beginning. Right?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Thu, Jun 10, 2010 at 4:07 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Ah, I just committed a patch to do the same, before seeing your email.
Thanks anyway.
Yeah, thanks a lot!
BTW, the docs claim about pg_last_xlog_location() that "While streaming
replication is in progress this will increase monotonically." That's a bit
misleading: when the replication connection is broken for some reason and we
restart it, we begin streaming from the beginning of the last WAL segment.
So at that moment, pg_last_xlog_location() moves backwards to the beginning
of the WAL segment.Should we:
1. Just document that,
2. Change pg_last_xlog_location() to not move backwards in that case, or
3. Change the behavior so that we start streaming at the exact byte location
where we left off?
I'm for 2 as follows.
diff --git a/src/backend/replication/walreceiver.c
b/src/backend/replication/walreceiver.c
index 26aeca6..f0fd813 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -524,7 +524,8 @@ XLogWalRcvFlush(void) /* Update shared-memory status */
SpinLockAcquire(&walrcv->mutex);
- walrcv->receivedUpto = LogstreamResult.Flush;
+ if (XLByteLT(walrcv->receivedUpto, LogstreamResult.Flush))
+ walrcv->receivedUpto = LogstreamResult.Flush;
SpinLockRelease(&walrcv->mutex);I believe that starting from the beginning of the WAL segment is just
paranoia, to avoid creating a WAL file that's missing some data from the
beginning. Right?
Only when the recovery starting record (i.e., the record at the checkpoint
redo location) is not found, we need to start replication from the beginning
of the segment, I think. That is, fetching_ckpt = true case in the following
code.
if (PrimaryConnInfo)
{
RequestXLogStreaming(
fetching_ckpt ? RedoStartLSN : *RecPtr,
PrimaryConnInfo);
continue;
}
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On 10/06/10 10:43, Fujii Masao wrote:
On Thu, Jun 10, 2010 at 4:07 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:BTW, the docs claim about pg_last_xlog_location() that "While streaming
replication is in progress this will increase monotonically." That's a bit
misleading: when the replication connection is broken for some reason and we
restart it, we begin streaming from the beginning of the last WAL segment.
So at that moment, pg_last_xlog_location() moves backwards to the beginning
of the WAL segment.Should we:
1. Just document that,
2. Change pg_last_xlog_location() to not move backwards in that case, or
3. Change the behavior so that we start streaming at the exact byte location
where we left off?I'm for 2 as follows.
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index 26aeca6..f0fd813 100644 --- a/src/backend/replication/walreceiver.c +++ b/src/backend/replication/walreceiver.c @@ -524,7 +524,8 @@ XLogWalRcvFlush(void)/* Update shared-memory status */ SpinLockAcquire(&walrcv->mutex); - walrcv->receivedUpto = LogstreamResult.Flush; + if (XLByteLT(walrcv->receivedUpto, LogstreamResult.Flush)) + walrcv->receivedUpto = LogstreamResult.Flush; SpinLockRelease(&walrcv->mutex);
That's not enough, because we set receivedUpto in RequestXlogStreaming()
already.
I believe that starting from the beginning of the WAL segment is just
paranoia, to avoid creating a WAL file that's missing some data from the
beginning. Right?Only when the recovery starting record (i.e., the record at the checkpoint
redo location) is not found, we need to start replication from the beginning
of the segment, I think. That is, fetching_ckpt = true case in the following
code.if (PrimaryConnInfo)
{
RequestXLogStreaming(
fetching_ckpt ? RedoStartLSN : *RecPtr,
PrimaryConnInfo);
continue;
}
Even then, we wouldn't need to start from the beginning of the WAL
segment AFAICS. The point is to start from the Redo pointer, not from
the checkpoint record, because as soon as we read the checkpoint record
we'll need to start applying WAL from the Redo pointer, which is
earlier. The WAL file boundaries don't come into play there.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Thu, Jun 10, 2010 at 5:04 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Should we:
1. Just document that,
2. Change pg_last_xlog_location() to not move backwards in that case, or
3. Change the behavior so that we start streaming at the exact byte
location
where we left off?I'm for 2 as follows.
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index 26aeca6..f0fd813 100644 --- a/src/backend/replication/walreceiver.c +++ b/src/backend/replication/walreceiver.c @@ -524,7 +524,8 @@ XLogWalRcvFlush(void)/* Update shared-memory status */ SpinLockAcquire(&walrcv->mutex); - walrcv->receivedUpto = LogstreamResult.Flush; + if (XLByteLT(walrcv->receivedUpto, LogstreamResult.Flush)) + walrcv->receivedUpto = LogstreamResult.Flush; SpinLockRelease(&walrcv->mutex);That's not enough, because we set receivedUpto in RequestXlogStreaming()
already.
Ah, you are right.
I believe that starting from the beginning of the WAL segment is just
paranoia, to avoid creating a WAL file that's missing some data from the
beginning. Right?Only when the recovery starting record (i.e., the record at the checkpoint
redo location) is not found, we need to start replication from the
beginning
of the segment, I think. That is, fetching_ckpt = true case in the
following
code.if (PrimaryConnInfo)
{
RequestXLogStreaming(
fetching_ckpt ? RedoStartLSN : *RecPtr,
PrimaryConnInfo);
continue;
}Even then, we wouldn't need to start from the beginning of the WAL segment
AFAICS. The point is to start from the Redo pointer, not from the checkpoint
record, because as soon as we read the checkpoint record we'll need to start
applying WAL from the Redo pointer, which is earlier. The WAL file
boundaries don't come into play there.
You mean that the WAL file containing the Redo pointer is guaranteed to exist
if we could read the checkpoint record, so we don't need to start from the
beginning of the segment? This is probably true. But what if we could not read
the checkpoint record? In this case, the WAL file containing the Redo pointer
also might not exist.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On 10/06/10 11:37, Fujii Masao wrote:
On Thu, Jun 10, 2010 at 5:04 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:I believe that starting from the beginning of the WAL segment is just
paranoia, to avoid creating a WAL file that's missing some data from the
beginning. Right?Only when the recovery starting record (i.e., the record at the checkpoint
redo location) is not found, we need to start replication from the
beginning
of the segment, I think. That is, fetching_ckpt = true case in the
following
code.if (PrimaryConnInfo)
{
RequestXLogStreaming(
fetching_ckpt ? RedoStartLSN : *RecPtr,
PrimaryConnInfo);
continue;
}Even then, we wouldn't need to start from the beginning of the WAL segment
AFAICS. The point is to start from the Redo pointer, not from the checkpoint
record, because as soon as we read the checkpoint record we'll need to start
applying WAL from the Redo pointer, which is earlier. The WAL file
boundaries don't come into play there.You mean that the WAL file containing the Redo pointer is guaranteed to exist
if we could read the checkpoint record, so we don't need to start from the
beginning of the segment? This is probably true. But what if we could not read
the checkpoint record? In this case, the WAL file containing the Redo pointer
also might not exist.
Oh, I think I understand the issue now: we need the header in the
beginning of the WAL segment to be valid, even if the first record we're
interested in is in the middle of the file. I missed that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
Even then, we wouldn't need to start from the beginning of the WAL
segment AFAICS. The point is to start from the Redo pointer, not from
the checkpoint record, because as soon as we read the checkpoint record
we'll need to start applying WAL from the Redo pointer, which is
earlier. The WAL file boundaries don't come into play there.
I don't believe it's a good idea to have SR not write full xlog segment
files. Consider for example the following scenario:
1. SR writes some xlog file from the middle.
2. Filesystem says "ah-hah, I know about sparse storage" and doesn't
allocate the first half of the file.
3. Failover: slave goes live.
4. xlog file gets recycled for re-use.
5. While reusing the file, we write into the first half ... or try to,
but there's no disk space.
6. PANIC.
There are probably some other good reasons not to allow incomplete
copies of WAL files to exist on the slave system, anyway.
I'm not sure if it's worth the trouble, or even a particularly smart
idea, to force the output of the status function to be monotonic
regardless of what happens underneath. I think removing that claim
from the docs altogether is the easiest answer.
regards, tom lane
On Thu, Jun 10, 2010 at 11:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
Even then, we wouldn't need to start from the beginning of the WAL
segment AFAICS. The point is to start from the Redo pointer, not from
the checkpoint record, because as soon as we read the checkpoint record
we'll need to start applying WAL from the Redo pointer, which is
earlier. The WAL file boundaries don't come into play there.I don't believe it's a good idea to have SR not write full xlog segment
files. Consider for example the following scenario:1. SR writes some xlog file from the middle.
2. Filesystem says "ah-hah, I know about sparse storage" and doesn't
allocate the first half of the file.
3. Failover: slave goes live.
4. xlog file gets recycled for re-use.
5. While reusing the file, we write into the first half ... or try to,
but there's no disk space.
6. PANIC.There are probably some other good reasons not to allow incomplete
copies of WAL files to exist on the slave system, anyway.I'm not sure if it's worth the trouble, or even a particularly smart
idea, to force the output of the status function to be monotonic
regardless of what happens underneath. I think removing that claim
from the docs altogether is the easiest answer.
We should
(1) just remove "While streaming replication is in progress this will
increase monotonically." from the description about
pg_last_xlog_receive_location()?
or
(2) add "But if streaming replication is restarted this will back off
to the beginning of current WAL file" into there?
I'm for (2) since it's more informative. Thought?
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On 15/06/10 08:23, Fujii Masao wrote:
On Thu, Jun 10, 2010 at 11:06 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
I'm not sure if it's worth the trouble, or even a particularly smart
idea, to force the output of the status function to be monotonic
regardless of what happens underneath. I think removing that claim
from the docs altogether is the easiest answer.We should
(1) just remove "While streaming replication is in progress this will
increase monotonically." from the description about
pg_last_xlog_receive_location()?or
(2) add "But if streaming replication is restarted this will back off
to the beginning of current WAL file" into there?I'm for (2) since it's more informative. Thought?
Something like (2) seems better, because even if we remove the note that
it increases monotonically, people might still assume that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Tue, Jun 15, 2010 at 2:41 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
On 15/06/10 08:23, Fujii Masao wrote:
On Thu, Jun 10, 2010 at 11:06 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
I'm not sure if it's worth the trouble, or even a particularly smart
idea, to force the output of the status function to be monotonic
regardless of what happens underneath. I think removing that claim
from the docs altogether is the easiest answer.We should
(1) just remove "While streaming replication is in progress this will
increase monotonically." from the description about
pg_last_xlog_receive_location()?or
(2) add "But if streaming replication is restarted this will back off
to the beginning of current WAL file" into there?I'm for (2) since it's more informative. Thought?
Something like (2) seems better, because even if we remove the note that it
increases monotonically, people might still assume that.
The attached patch adds the following:
-------------
But when streaming replication is
restarted this will back off to the replication starting position,
which typically indicates the beginning of the WAL file including the
record in the position which <function>pg_last_xlog_replay_location</>
points to at the moment.
-------------
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
pg_last_xlog_receive_location_doc_v1.patchapplication/octet-stream; name=pg_last_xlog_receive_location_doc_v1.patchDownload
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 13282,13288 **** postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
<entry><type>text</type></entry>
<entry>Get last transaction log location received and synced to disk by
streaming replication. While streaming replication is in progress
! this will increase monotonically. If recovevery has completed
this will remain static at the value of the last WAL record
received and synced to disk during recovery. If streaming replication
is disabled, or it has not yet started, the function returns NULL.
--- 13282,13292 ----
<entry><type>text</type></entry>
<entry>Get last transaction log location received and synced to disk by
streaming replication. While streaming replication is in progress
! this will increase monotonically. But when streaming replication is
! restarted this will back off to the replication starting position,
! which typically indicates the beginning of the WAL file including the
! record in the position which <function>pg_last_xlog_replay_location</>
! points to at the moment. If recovevery has completed
this will remain static at the value of the last WAL record
received and synced to disk during recovery. If streaming replication
is disabled, or it has not yet started, the function returns NULL.
On Tue, Jun 15, 2010 at 4:22 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Jun 15, 2010 at 2:41 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:On 15/06/10 08:23, Fujii Masao wrote:
On Thu, Jun 10, 2010 at 11:06 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
I'm not sure if it's worth the trouble, or even a particularly smart
idea, to force the output of the status function to be monotonic
regardless of what happens underneath. I think removing that claim
from the docs altogether is the easiest answer.We should
(1) just remove "While streaming replication is in progress this will
increase monotonically." from the description about
pg_last_xlog_receive_location()?or
(2) add "But if streaming replication is restarted this will back off
to the beginning of current WAL file" into there?I'm for (2) since it's more informative. Thought?
Something like (2) seems better, because even if we remove the note that it
increases monotonically, people might still assume that.The attached patch adds the following:
-------------
But when streaming replication is
restarted this will back off to the replication starting position,
which typically indicates the beginning of the WAL file including the
record in the position which <function>pg_last_xlog_replay_location</>
points to at the moment.
-------------
Applied with some additional wordsmithing.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company