Thread: Second paragraph a little bit misleading

Second paragraph a little bit misleading

From
PG Doc comments form
Date:
The following documentation comment has been logged on the website:

Page: https://www.postgresql.org/docs/17/checksums.html
Description:

Hi there,

I think that the first sentence of the second paragraph in this page is a
little bit misleading. The first paragraph states that checksums are not
enabled by default. The first sentence in the second paragraph sounds like
it IS enabled by default when using initdb, but you have to pass -k
explicitly.

As far as I understood, it's a good idea to enable this and that is what the
beginning of the second paragraph means.

Maybe a sentence like "Checksums should normally be enabled when the cluster
is initialized using initdb" instead of "are" - or "It's recommended to
enable Checksums when initializing a cluster using initdb".

regards

Felix

Re: Second paragraph a little bit misleading

From
Bruce Momjian
Date:
On Tue, Feb 11, 2025 at 02:00:10PM +0000, PG Doc comments form wrote:
> The following documentation comment has been logged on the website:
> 
> Page: https://www.postgresql.org/docs/17/checksums.html
> Description:
> 
> Hi there,
> 
> I think that the first sentence of the second paragraph in this page is a
> little bit misleading. The first paragraph states that checksums are not
> enabled by default. The first sentence in the second paragraph sounds like
> it IS enabled by default when using initdb, but you have to pass -k
> explicitly.
> 
> As far as I understood, it's a good idea to enable this and that is what the
> beginning of the second paragraph means.
> 
> Maybe a sentence like "Checksums should normally be enabled when the cluster
> is initialized using initdb" instead of "are" - or "It's recommended to
> enable Checksums when initializing a cluster using initdb".

I can see your point.  Attached is a doc patch that improves it.  I
chose very simple language and plan to backpatch this to all supported
versions.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

Attachment

Re: Second paragraph a little bit misleading

From
Laurenz Albe
Date:
On Thu, 2025-02-20 at 11:38 -0500, Bruce Momjian wrote:
> On Tue, Feb 11, 2025 at 02:00:10PM +0000, PG Doc comments form wrote:
> > Page: https://www.postgresql.org/docs/17/checksums.html
> > 
> > I think that the first sentence of the second paragraph in this page is a
> > little bit misleading. The first paragraph states that checksums are not
> > enabled by default. The first sentence in the second paragraph sounds like
> > it IS enabled by default when using initdb, but you have to pass -k
> > explicitly.
> 
> I can see your point.  Attached is a doc patch that improves it.  I
> chose very simple language and plan to backpatch this to all supported
> versions.
>
> diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
> index 52b5b8f793b..705ca682777 100644
> --- a/doc/src/sgml/wal.sgml
> +++ b/doc/src/sgml/wal.sgml
> @@ -246,7 +246,7 @@
>    </para>
>  
>    <para>
> -   Checksums are normally enabled when the cluster is initialized using <link
> +   Checksums can be enabled when the cluster is initialized using <link
>     linkend="app-initdb-data-checksums"><application>initdb</application></link>.
>     They can also be enabled or disabled at a later time as an offline
>     operation. Data checksums are enabled or disabled at the full cluster

The change looks good for the back branches, but the default has changed
in v18: now checksums are the default.  So "can be enabled" doesn't sound
right for v18.  I'd leave "are normally enabled" in HEAD.

Yours,
Laurenz Albe

-- 

*E-Mail Disclaimer*
Der Inhalt dieser E-Mail ist ausschliesslich fuer den 
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat 
dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, 
dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder 
Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich 
in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.

*CONFIDENTIALITY NOTICE & DISCLAIMER
*This message and any attachment are 
confidential and may be privileged or otherwise protected from disclosure 
and solely for the use of the person(s) or entity to whom it is intended. 
If you have received this message in error and are not the intended 
recipient, please notify the sender immediately and delete this message and 
any attachment from your system. If you are not the intended recipient, be 
advised that any use of this message is prohibited and may be unlawful, and 
you must not copy this message or attachment or disclose the contents to 
any other person.



Re: Second paragraph a little bit misleading

From
Bruce Momjian
Date:
On Fri, Feb 21, 2025 at 08:25:56AM +0100, Laurenz Albe wrote:
> On Thu, 2025-02-20 at 11:38 -0500, Bruce Momjian wrote:
> > On Tue, Feb 11, 2025 at 02:00:10PM +0000, PG Doc comments form wrote:
> > > Page: https://www.postgresql.org/docs/17/checksums.html
> > > 
> > > I think that the first sentence of the second paragraph in this page is a
> > > little bit misleading. The first paragraph states that checksums are not
> > > enabled by default. The first sentence in the second paragraph sounds like
> > > it IS enabled by default when using initdb, but you have to pass -k
> > > explicitly.
> > 
> > I can see your point.  Attached is a doc patch that improves it.  I
> > chose very simple language and plan to backpatch this to all supported
> > versions.
> >
> > diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
> > index 52b5b8f793b..705ca682777 100644
> > --- a/doc/src/sgml/wal.sgml
> > +++ b/doc/src/sgml/wal.sgml
> > @@ -246,7 +246,7 @@
> >    </para>
> >  
> >    <para>
> > -   Checksums are normally enabled when the cluster is initialized using <link
> > +   Checksums can be enabled when the cluster is initialized using <link
> >     linkend="app-initdb-data-checksums"><application>initdb</application></link>.
> >     They can also be enabled or disabled at a later time as an offline
> >     operation. Data checksums are enabled or disabled at the full cluster
> 
> The change looks good for the back branches, but the default has changed
> in v18: now checksums are the default.  So "can be enabled" doesn't sound
> right for v18.  I'd leave "are normally enabled" in HEAD.

Yes, I was confused about that too, but I think we changed the code for
the development branch and if we decide to keep the new default, we will
change the docs later.  I didn't want to interfere with that.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.



Re: Second paragraph a little bit misleading

From
Laurenz Albe
Date:
On Fri, 2025-02-21 at 07:00 -0500, Bruce Momjian wrote:
> > > diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
> > > index 52b5b8f793b..705ca682777 100644
> > > --- a/doc/src/sgml/wal.sgml
> > > +++ b/doc/src/sgml/wal.sgml
> > > @@ -246,7 +246,7 @@
> > >     </para>
> > >  
> > >     <para>
> > > -   Checksums are normally enabled when the cluster is initialized using <link
> > > +   Checksums can be enabled when the cluster is initialized using <link
> > >      linkend="app-initdb-data-checksums"><application>initdb</application></link>.
> > >      They can also be enabled or disabled at a later time as an offline
> > >      operation. Data checksums are enabled or disabled at the full cluster
> >
> > The change looks good for the back branches, but the default has changed
> > in v18: now checksums are the default.  So "can be enabled" doesn't sound
> > right for v18.  I'd leave "are normally enabled" in HEAD.
>
> Yes, I was confused about that too, but I think we changed the code for
> the development branch and if we decide to keep the new default, we will
> change the docs later.  I didn't want to interfere with that.

Hmpf.  The documentation should always be in sync with the code, right?
So I think it should be left alone in HEAD, and if the checksum change
gets reverted, your patch should be applied to HEAD.

Yours,
Laurenz Albe

--

*E-Mail Disclaimer*
Der Inhalt dieser E-Mail ist ausschliesslich fuer den
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat
dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte,
dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder
Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich
in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.

*CONFIDENTIALITY NOTICE & DISCLAIMER
*This message and any attachment are
confidential and may be privileged or otherwise protected from disclosure
and solely for the use of the person(s) or entity to whom it is intended.
If you have received this message in error and are not the intended
recipient, please notify the sender immediately and delete this message and
any attachment from your system. If you are not the intended recipient, be
advised that any use of this message is prohibited and may be unlawful, and
you must not copy this message or attachment or disclose the contents to
any other person.



Re: Second paragraph a little bit misleading

From
Bruce Momjian
Date:
On Fri, Feb 21, 2025 at 01:42:09PM +0100, Laurenz Albe wrote:
> On Fri, 2025-02-21 at 07:00 -0500, Bruce Momjian wrote:
> > > > diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
> > > > index 52b5b8f793b..705ca682777 100644
> > > > --- a/doc/src/sgml/wal.sgml
> > > > +++ b/doc/src/sgml/wal.sgml
> > > > @@ -246,7 +246,7 @@
> > > >     </para>
> > > >   
> > > >     <para>
> > > > -   Checksums are normally enabled when the cluster is initialized using <link
> > > > +   Checksums can be enabled when the cluster is initialized using <link
> > > >      linkend="app-initdb-data-checksums"><application>initdb</application></link>.
> > > >      They can also be enabled or disabled at a later time as an offline
> > > >      operation. Data checksums are enabled or disabled at the full cluster
> > > 
> > > The change looks good for the back branches, but the default has changed
> > > in v18: now checksums are the default.  So "can be enabled" doesn't sound
> > > right for v18.  I'd leave "are normally enabled" in HEAD.
> > 
> > Yes, I was confused about that too, but I think we changed the code for
> > the development branch and if we decide to keep the new default, we will
> > change the docs later.  I didn't want to interfere with that.
> 
> Hmpf.  The documentation should always be in sync with the code, right?
> So I think it should be left alone in HEAD, and if the checksum change
> gets reverted, your patch should be applied to HEAD.

I see your point, and I now agree that the "Reliability" section was
just overlooked when the data checksum default was changed.  I made a
larger patch which improved the wording of data checksum mentions now
that it is the default in master.

I also fixed the Felix-reported problem in all the back branches through
14 --- PG 13 did not have the problem.

Patches attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

Attachment

Re: Second paragraph a little bit misleading

From
Laurenz Albe
Date:
On Fri, 2025-02-21 at 13:06 -0500, Bruce Momjian wrote:
> > Hmpf.  The documentation should always be in sync with the code, right?
> > So I think it should be left alone in HEAD, and if the checksum change
> > gets reverted, your patch should be applied to HEAD.
> 
> I see your point, and I now agree that the "Reliability" section was
> just overlooked when the data checksum default was changed.  I made a
> larger patch which improved the wording of data checksum mentions now
> that it is the default in master.
> 
> I also fixed the Felix-reported problem in all the back branches through
> 14 --- PG 13 did not have the problem.
> 
> Patches attached.

Thanks!  These patches look good to me.

Yours,
Laurenz Albe

-- 

*E-Mail Disclaimer*
Der Inhalt dieser E-Mail ist ausschliesslich fuer den 
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat 
dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, 
dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder 
Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich 
in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.

*CONFIDENTIALITY NOTICE & DISCLAIMER
*This message and any attachment are 
confidential and may be privileged or otherwise protected from disclosure 
and solely for the use of the person(s) or entity to whom it is intended. 
If you have received this message in error and are not the intended 
recipient, please notify the sender immediately and delete this message and 
any attachment from your system. If you are not the intended recipient, be 
advised that any use of this message is prohibited and may be unlawful, and 
you must not copy this message or attachment or disclose the contents to 
any other person.