Horton

Thread

Commits

  1. Fix initialization of fake LSN for unlogged relations

  1. Fix of fake unlogged LSN initialization

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2019-10-19T05:03:00Z

    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

  2. Re: Fix of fake unlogged LSN initialization

    Michael Paquier <michael@paquier.xyz> — 2019-10-21T05:03:47Z

    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

  3. Re: Fix of fake unlogged LSN initialization

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-10-24T04:14:00Z

    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

  4. Re: Fix of fake unlogged LSN initialization

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-10-24T08:27:45Z

    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

  5. Re: Fix of fake unlogged LSN initialization

    Simon Riggs <simon@2ndquadrant.com> — 2019-10-24T10:57:33Z

    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

  6. Re: Fix of fake unlogged LSN initialization

    Michael Paquier <michael@paquier.xyz> — 2019-10-24T12:08:43Z

    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

  7. RE: Fix of fake unlogged LSN initialization

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2019-10-25T02:07:04Z

    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

  8. RE: Fix of fake unlogged LSN initialization

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2019-10-25T02:11:55Z

    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

  9. Re: Fix of fake unlogged LSN initialization

    Heikki Linnakangas <hlinnaka@iki.fi> — 2019-10-25T06:54:53Z

    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

  10. Re: Fix of fake unlogged LSN initialization

    Michael Paquier <michael@paquier.xyz> — 2019-10-25T06:55:32Z

    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

  11. Re: Fix of fake unlogged LSN initialization

    Michael Paquier <michael@paquier.xyz> — 2019-10-25T06:58:14Z

    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

  12. Re: Fix of fake unlogged LSN initialization

    Michael Paquier <michael@paquier.xyz> — 2019-10-27T04:58:50Z

    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

  13. Re: Fix of fake unlogged LSN initialization

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-10-27T06:55:07Z

    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

  14. Re: Fix of fake unlogged LSN initialization

    Michael Paquier <michael@paquier.xyz> — 2019-10-28T08:27:02Z

    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