Thread: Fix of fake unlogged LSN initialization
Hello, The attached trivial patch fixes the initialization of the fake unlogged LSN. Currently, BootstrapXLOG() in initdb setsthe initial fake unlogged LSN to FirstNormalUnloggedLSN (=1000), but the recovery and pg_resetwal sets it to 1. Thepatch 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
Attachment
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
Attachment
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
setsthe initial fake unlogged LSN to FirstNormalUnloggedLSN (=1000), but the recovery and pg_resetwal sets it to 1.
Thepatch 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?
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
On Fri, Oct 25, 2019 at 09:54:53AM +0300, Heikki Linnakangas wrote: > No particular reason, it's just a nice round value in decimal. Well: $ pg_controldata | grep -i fake Fake LSN counter for unlogged rels: 0/3E8 ;p -- Michael