Обсуждение: 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

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
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 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

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