Обсуждение: Add TLI number to name of files generated by pg_waldump --save-fullpage

Hi all,
(Fujii-san and David in CC.)

Fujii-san has reported on Twitter that we had better add the TLI
number to what pg_waldump --save-fullpage generates for the file names
of the blocks, as it could be possible that we overwrite some blocks.
This information can be added thanks to ws_tli, that tracks the TLI of
the opened segment.

Attached is a patch to fix this issue, adding an open item assigned to
me.  The file format is documented in the TAP test and the docs, the
two only places that would need a refresh.

Thoughts or comments?
--
Michael

At Tue, 27 Jun 2023 15:12:43 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> Hi all,
> (Fujii-san and David in CC.)
> 
> Fujii-san has reported on Twitter that we had better add the TLI
> number to what pg_waldump --save-fullpage generates for the file names
> of the blocks, as it could be possible that we overwrite some blocks.
> This information can be added thanks to ws_tli, that tracks the TLI of
> the opened segment.
> 
> Attached is a patch to fix this issue, adding an open item assigned to
> me.  The file format is documented in the TAP test and the docs, the
> two only places that would need a refresh.
> 
> Thoughts or comments?

It's sensible to add TLI to the file name. So +1 from me.

+# - Timeline number in hex format.

Arn't we reffering to it as "Timeline ID"? (I remember there was a
discussion about redefining the "timeline ID" to use non-orderable
IDs. That is, making it non-numbers.)

Otherwise it looks fine to me.


By the way, somewhat irrelevant to this patch, regading the the file
name for the output.


The file name was "LSNh-LSNl.spcOid.dbOid.relNumber.blk_forkname", but
the comment in the TAP script read as:

-# XXXXXXXX-XXXXXXXX.DBOID.TLOID.NODEOID.dd_fork with the components being:

which looks wrong. I'm not sure it is a business of this patch, though..

# Documentation looks coorect.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jun 27, 2023 at 03:44:04PM +0900, Kyotaro Horiguchi wrote:
> +# - Timeline number in hex format.
>
> Arn't we reffering to it as "Timeline ID"? (I remember there was a
> discussion about redefining the "timeline ID" to use non-orderable
> IDs. That is, making it non-numbers.)

Using ID is fine by me.

> The file name was "LSNh-LSNl.spcOid.dbOid.relNumber.blk_forkname", but
> the comment in the TAP script read as:
>
> -# XXXXXXXX-XXXXXXXX.DBOID.TLOID.NODEOID.dd_fork with the components being:
>
> which looks wrong. I'm not sure it is a business of this patch, though..

This part is getting changed here anyway, so improving it is fine by
me with the terms you are suggesting for these two 4-byte values in
this comment.
--
Michael

At Tue, 27 Jun 2023 15:58:38 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Tue, Jun 27, 2023 at 03:44:04PM +0900, Kyotaro Horiguchi wrote:
> > The file name was "LSNh-LSNl.spcOid.dbOid.relNumber.blk_forkname", but
> > the comment in the TAP script read as:
> > 
> > -# XXXXXXXX-XXXXXXXX.DBOID.TLOID.NODEOID.dd_fork with the components being:
> > 
> > which looks wrong. I'm not sure it is a business of this patch, though..
> 
> This part is getting changed here anyway, so improving it is fine by
> me with the terms you are suggesting for these two 4-byte values in
> this comment.

I meant that the name is structured as
TLIh-TLIl.<tablespace>.<database>.<relnumber>.<blk>._<fork>, which
appears to be inconsistent with the comment. (And I'm not sure what
"TLOID" is..)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Of course, it's wrong.

At Tue, 27 Jun 2023 16:39:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail
.com> wrote in 
> I meant that the name is structured as
- TLIh-TLIl.<tablespace>.<database>.<relnumber>.<blk>._<fork>, which
+ LSNh-LSNl.<tablespace>.<database>.<relnumber>.<blk>._<fork>, which

> appears to be inconsistent with the comment. (And I'm not sure what
> "TLOID" is..)
> 
> regards.
> 
> -- 
> Kyotaro Horiguchi
> NTT Open Source Software Center

On Tue, Jun 27, 2023 at 1:12 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> Hi all,
> (Fujii-san and David in CC.)
>
> Fujii-san has reported on Twitter that we had better add the TLI
> number to what pg_waldump --save-fullpage generates for the file names
> of the blocks, as it could be possible that we overwrite some blocks.
> This information can be added thanks to ws_tli, that tracks the TLI of
> the opened segment.
>
> Attached is a patch to fix this issue, adding an open item assigned to
> me.  The file format is documented in the TAP test and the docs, the
> two only places that would need a refresh.
>
> Thoughts or comments?

Patch looks good, but agreed that that comment should also be fixed.

Thanks!

David

On Tue, Jun 27, 2023 at 11:53:10AM -0500, David Christensen wrote:
> Patch looks good, but agreed that that comment should also be fixed.

Okay, thanks for checking!
--
Michael

On Tue, Jun 27, 2023 at 04:39:52PM +0900, Kyotaro Horiguchi wrote:
> I meant that the name is structured as
> TLIh-TLIl.<tablespace>.<database>.<relnumber>.<blk>._<fork>, which
> appears to be inconsistent with the comment. (And I'm not sure what
> "TLOID" is..)

Well, to be clear, it should not be TLIh-TLIl but LSNh-LSNl :)

I'm OK with these terms for the comments.  This is very internal
anyway so anybody using this feature should know what that means.

And yes, the order of the items is wrong, and I agree that TLOID is a
bit confusing once the TLI is added in the set.  I have just used
TBLSPCOID as term in the comment, and adjusted the XXX to be about the
LSN numbers.

Adjusted as per the v2 attached.
--
Michael

> Adjusted as per the v2 attached.

+1

At Tue, 27 Jun 2023 18:58:39 -0500, David Christensen <david.christensen@crunchydata.com> wrote in 
> > Adjusted as per the v2 attached.
> 
> +1

+1

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Jun 28, 2023 at 09:20:27AM +0900, Kyotaro Horiguchi wrote:
> At Tue, 27 Jun 2023 18:58:39 -0500, David Christensen <david.christensen@crunchydata.com> wrote in
>>> Adjusted as per the v2 attached.
>>
>> +1
>
> +1

Okay, cool.  Both of you seem happy with it, so I have applied it.
Thanks for the quick checks.
--
Michael