Fix of fake unlogged LSN initialization
Hello,
The attached trivial patch fixes the initialization of the fake unlogged LSN. Currently, BootstrapXLOG() in initdb sets the initial fake unlogged LSN to FirstNormalUnloggedLSN (=1000), but the recovery and pg_resetwal sets it to 1. The patch modifies the latter two cases to match initdb.
I don't know if this do actual harm, because the description of FirstNormalUnloggedLSN doesn't give me any idea.
Regards
Takayuki Tsunakawa
Attachments:
init-fake-unlogged-lsn.patchapplication/octet-stream; name=init-fake-unlogged-lsn.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b602e28..f58fa0a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6687,7 +6687,7 @@ StartupXLOG(void)
if (ControlFile->state == DB_SHUTDOWNED)
XLogCtl->unloggedLSN = ControlFile->unloggedLSN;
else
- XLogCtl->unloggedLSN = 1;
+ XLogCtl->unloggedLSN = FirstNormalUnloggedLSN;
/*
* We must replay WAL entries using the same TimeLineID they were created
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index f85ea2d..b2f648f 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -700,7 +700,7 @@ GuessControlValues(void)
ControlFile.state = DB_SHUTDOWNED;
ControlFile.time = (pg_time_t) time(NULL);
ControlFile.checkPoint = ControlFile.checkPointCopy.redo;
- ControlFile.unloggedLSN = 1;
+ ControlFile.unloggedLSN = FirstNormalUnloggedLSN;
/* minRecoveryPoint, backupStartPoint and backupEndPoint can be left zero */
On Sat, Oct 19, 2019 at 05:03:00AM +0000, tsunakawa.takay@fujitsu.com wrote:
The attached trivial patch fixes the initialization of the fake
unlogged LSN. Currently, BootstrapXLOG() in initdb sets the initial
fake unlogged LSN to FirstNormalUnloggedLSN (=1000), but the
recovery and pg_resetwal sets it to 1. The patch modifies the
latter two cases to match initdb.I don't know if this do actual harm, because the description of
FirstNormalUnloggedLSN doesn't give me any idea.
From xlogdefs.h added by 9155580:
/*
* First LSN to use for "fake" LSNs.
*
* Values smaller than this can be used for special per-AM purposes.
*/
#define FirstNormalUnloggedLSN ((XLogRecPtr) 1000)
So it seems to me that you have caught a bug here, and that we had
better back-patch to v12 so as recovery and pg_resetwal don't mess up
with AMs using lower values than that.
--
Michael
At Mon, 21 Oct 2019 14:03:47 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Sat, Oct 19, 2019 at 05:03:00AM +0000, tsunakawa.takay@fujitsu.com wrote:
The attached trivial patch fixes the initialization of the fake
unlogged LSN. Currently, BootstrapXLOG() in initdb sets the initial
fake unlogged LSN to FirstNormalUnloggedLSN (=1000), but the
recovery and pg_resetwal sets it to 1. The patch modifies the
latter two cases to match initdb.I don't know if this do actual harm, because the description of
FirstNormalUnloggedLSN doesn't give me any idea.From xlogdefs.h added by 9155580:
/*
* First LSN to use for "fake" LSNs.
*
* Values smaller than this can be used for special per-AM purposes.
*/
#define FirstNormalUnloggedLSN ((XLogRecPtr) 1000)So it seems to me that you have caught a bug here, and that we had
better back-patch to v12 so as recovery and pg_resetwal don't mess up
with AMs using lower values than that.
+1
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Sat, Oct 19, 2019 at 3:18 PM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:
Hello,
The attached trivial patch fixes the initialization of the fake unlogged LSN. Currently, BootstrapXLOG() in initdb sets the initial fake unlogged LSN to FirstNormalUnloggedLSN (=1000), but the recovery and pg_resetwal sets it to 1. The patch modifies the latter two cases to match initdb.
I don't know if this do actual harm, because the description of FirstNormalUnloggedLSN doesn't give me any idea.
I have noticed that in StartupXlog also we reset it with 1, you might
want to fix that as well?
StartupXLOG
{
...
/*
* Initialize unlogged LSN. On a clean shutdown, it's restored from the
* control file. On recovery, all unlogged relations are blown away, so
* the unlogged LSN counter can be reset too.
*/
if (ControlFile->state == DB_SHUTDOWNED)
XLogCtl->unloggedLSN = ControlFile->unloggedLSN;
else
XLogCtl->unloggedLSN = 1;
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Mon, 21 Oct 2019 at 06:03, Michael Paquier <michael@paquier.xyz> wrote:
On Sat, Oct 19, 2019 at 05:03:00AM +0000, tsunakawa.takay@fujitsu.com
wrote:The attached trivial patch fixes the initialization of the fake
unlogged LSN. Currently, BootstrapXLOG() in initdb sets the initial
fake unlogged LSN to FirstNormalUnloggedLSN (=1000), but the
recovery and pg_resetwal sets it to 1. The patch modifies the
latter two cases to match initdb.I don't know if this do actual harm, because the description of
FirstNormalUnloggedLSN doesn't give me any idea.From xlogdefs.h added by 9155580:
/*
* First LSN to use for "fake" LSNs.
*
* Values smaller than this can be used for special per-AM purposes.
*/
#define FirstNormalUnloggedLSN ((XLogRecPtr) 1000)So it seems to me that you have caught a bug here, and that we had
better back-patch to v12 so as recovery and pg_resetwal don't mess up
with AMs using lower values than that.
I wonder why is that value 1000, rather than an aligned value or a whole
WAL page?
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Solutions for the Enterprise
On Thu, Oct 24, 2019 at 11:57:33AM +0100, Simon Riggs wrote:
I wonder why is that value 1000, rather than an aligned value or a whole
WAL page?
Good question. Heikki, why this choice?
--
Michael
From: Simon Riggs <simon@2ndquadrant.com>
From xlogdefs.h added by 9155580:
/*
* First LSN to use for "fake" LSNs.
*
* Values smaller than this can be used for special per-AM purposes.
*/
#define FirstNormalUnloggedLSN ((XLogRecPtr) 1000)
Yeah, I had seen it, but I didn't understand what kind of usage is assumed.
I wonder why is that value 1000, rather than an aligned value or a whole WAL
page?
I think that's because this fake LSN is not associated with the physical position of WAL records.
Regards
Takayuki Tsunakawa
From: Dilip Kumar <dilipbalaut@gmail.com>
I have noticed that in StartupXlog also we reset it with 1, you might
want to fix that as well?StartupXLOG
{
...
/*
* Initialize unlogged LSN. On a clean shutdown, it's restored from the
* control file. On recovery, all unlogged relations are blown away, so
* the unlogged LSN counter can be reset too.
*/
if (ControlFile->state == DB_SHUTDOWNED)
XLogCtl->unloggedLSN = ControlFile->unloggedLSN;
else
XLogCtl->unloggedLSN = 1;
Thanks for taking a look. I'm afraid my patch includes the fix for this part.
Regards
Takayuki Tsunakawa
On 24/10/2019 15:08, Michael Paquier wrote:
On Thu, Oct 24, 2019 at 11:57:33AM +0100, Simon Riggs wrote:
I wonder why is that value 1000, rather than an aligned value or a whole
WAL page?Good question. Heikki, why this choice?
No particular reason, it's just a nice round value in decimal.
- Heikki
On Thu, Oct 24, 2019 at 01:57:45PM +0530, Dilip Kumar wrote:
I have noticed that in StartupXlog also we reset it with 1, you might
want to fix that as well?
Tsunakawa-san's patch fixes that spot already. Grepping for
unloggedLSN in the code there is only pg_resetwal on top of what you
are mentioning here.
--
Michael
On Fri, Oct 25, 2019 at 02:07:04AM +0000, tsunakawa.takay@fujitsu.com wrote:
From: Simon Riggs <simon@2ndquadrant.com>
From xlogdefs.h added by 9155580:
/*
* First LSN to use for "fake" LSNs.
*
* Values smaller than this can be used for special per-AM purposes.
*/
#define FirstNormalUnloggedLSN ((XLogRecPtr) 1000)Yeah, I had seen it, but I didn't understand what kind of usage is assumed.
There is an explanation in the commit message of 9155580: that's to
make an interlocking logic in GiST able to work where a valid LSN
needs to be used. So a magic value was just wanted.
Your patch looks fine to me by the way after a second look, so I think
that we had better commit it and back-patch sooner than later. If
there are any objections or more comments, please feel free..
--
Michael
On Fri, Oct 25, 2019 at 02:11:55AM +0000, tsunakawa.takay@fujitsu.com wrote:
Thanks for taking a look. I'm afraid my patch includes the fix for this part.
Yes. And now this is applied and back-patched.
--
Michael
On Fri, Oct 25, 2019 at 7:42 AM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:
From: Dilip Kumar <dilipbalaut@gmail.com>
I have noticed that in StartupXlog also we reset it with 1, you might
want to fix that as well?StartupXLOG
{
...
/*
* Initialize unlogged LSN. On a clean shutdown, it's restored from the
* control file. On recovery, all unlogged relations are blown away, so
* the unlogged LSN counter can be reset too.
*/
if (ControlFile->state == DB_SHUTDOWNED)
XLogCtl->unloggedLSN = ControlFile->unloggedLSN;
else
XLogCtl->unloggedLSN = 1;Thanks for taking a look. I'm afraid my patch includes the fix for this part.
Oh, I missed that.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com