Subject: Re: Proposal to add page headers to SLRU pages

From: "Andrey M. Borodin" <[email-masked]>
Date: Thu, 7 Dec 2023 13:19:12 +0300

Hi Yong!

+1 to the idea to protect SLRUs from corruption. I'm slightly leaning towards the idea of separating checksums from data pages, but anyway this checksums are better than no checksums.

> On 7 Dec 2023, at 10:06, Li, Yong <[email-masked]> wrote:
> 
> I am still working on patching the pg_upgrade.  Just love to hear your thoughts on the idea and the current patch.

FWIW you can take upgrade code from this patch [0] doing all the same stuff :)

Best regards, Andrey Borodin.

[0] https://www.postgresql.org/message-id/[email-masked]

---

From: Aleksander Alekseev <[email-masked]>
Date: Thu, 7 Dec 2023 17:16:59 +0300

Hi,

> FWIW you can take upgrade code from this patch [0] doing all the same stuff :)

Sounds like a half-measure to me. If we really want to go down this
rabbit hole IMO SLRU should be moved to shared buffers as proposed
elsewhere [1].

[1]: https://www.postgresql.org/message-id/[email-masked]

--
Best regards,
Aleksander Alekseev

---

From: Andrey Borodin <[email-masked]>
Date: Thu, 07 Dec 2023 22:32:20 +0500

07.12.2023, 19:17, "Aleksander Alekseev" <[email-masked]>:

Hi,

Thread that I cited stopped in 2018 for this exact reason. 5 years ago. Is this argument still valid?
Meanwhile checksums of buffer pages also reside on a page :)

---

From: "Debnath, Shawn" <[email-masked]>
Date: Thu, 7 Dec 2023 18:27:44 +0000

I would love to have seen more progress on the set of threads that proposed
the page header and integration of SLRU into buffer cache. The changes were
large, and unfortunately as a result, it didn't get the detailed review
that it needed. The complex nature of the feature allowed for more branches
to be split from the main thread with alternative approaches. Athough this is
great to see, it did result in the set of core requirements around LSN and
checksum tracking via page headers to not get into PG 16.

What is being proposed now is the simple and core functionality of introducing
page headers to SLRU pages while continuing to be in the SLRU cache. This
allows the whole project to be iterative and reviewers to better reason about
the smaller set of changes being introduced into the codebase.

Once the set of on-disk changes are in, we can follow up on optimizations.
It may be moving to buffer cache or reviewing Dilip's approach in [1], we
will have the option to be flexible in our approach.

[1] https://www.postgresql.org/message-id/flat/CAFiTN-vzDvNz=[email-masked]

Shawn

---

From: Robert Haas <[email-masked]>
Date: Thu, 7 Dec 2023 14:51:06 -0500

> will have the option to be flexible in our approach.

I basically agree with this. I don't think we should let the perfect
be the enemy of the good. Shooting down this patch because it doesn't
do everything that we want is a recipe for getting nothing done at
all.

That said, I don't think that the original post on this thread
provides a sufficiently clear and detailed motivation for making this
change. For this to eventually be committed, it's going to need (among
other things) a commit message that articulates a convincing rationale
for whatever changes it makes. Here's what the original email said:

> It adds a checksum to each SLRU page, tracks page LSN as if it is a standard page and eases future page enhancements.

Of those three things, in my opinion, the first is good and the other
two are too vague. I assume that most people who would be likely to
read a commit message would understand the value of pages having
checksums. But I can't immediately think of what the value of tracking
the page LSN as if it were a standard page might be, so that probably
needs more explanation. Similarly, at least one or two of the future
page enhancements that might be eased should be spelled out, and/or
the ways in which they would be made easier should be articulated.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: "Li, Yong" <[email-masked]>
Date: Fri, 8 Dec 2023 09:35:17 +0000

Given so many different approaches were discussed, I have started a wiki to record and collaborate all efforts towards SLRU improvements.  The wiki provides a concise overview of all the ideas discussed and can serve as a portal for all historical discussions.  Currently, the wiki summarizes four recent threads ranging from identifier format change to page header change, to moving SLRU into the main buffer pool, to reduce lock contention on SLRU latches.  We can keep the patch related discussions in this thread and use the wiki as a live document for larger scale collaborations.

The wiki page is here:  https://wiki.postgresql.org/wiki/SLRU_improvements

Regarding the benefits of this patch, here is a detailed explanation:

  1.  Checksum is added to each page, allowing us to verify if a page has been corrupted when read from the disk.
  2.  The ad-hoc LSN group structure is removed from the SLRU cache control data and is replaced by the page LSN in the page header. This allows us to use the same WAL protocol as used by pages in the main buffer pool: flush all redo logs up to the page LSN before flushing the page itself. If we move SLRU caches into the main buffer pool, this change fits naturally.
  3.  It leaves further optimizations open. We can continue to pursue the goal of moving SLRU into the main buffer pool, or we can follow the lock partition idea. This change by itself does not conflict with either proposal.

Also, the patch is now complete and is ready for review.  All check-world tests including tap tests passed with this patch.

Regards,
Yong

From: Robert Haas <[email-masked]>
Date: Friday, December 8, 2023 at 03:51
To: Debnath, Shawn <[email-masked]>
Cc: Andrey Borodin <[email-masked]>, PostgreSQL Hackers <[email-masked]>, Aleksander Alekseev <[email-masked]>, Li, Yong <[email-masked]>, Shyrabokau, Anton <[email-masked]>, Bagga, Rishu <[email-masked]>
Subject: Re: Proposal to add page headers to SLRU pages
External Email

--
Robert Haas
EDB: https://nam10.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.com%2F&data=05%7C01%7Cyoli%40ebay.com%7C2cad2fe1de8a40f3167608dbf75de73c%7C46326bff992841a0baca17c16c94ea99%7C0%7C0%7C638375754901646398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=hkccuGfpt1%2BKxuhk%2BJt%2F3HyYuJqQHYfizib76%2F9HtUU%3D&reserved=0<http://www.enterprisedb.com/>

---

From: "Bagga, Rishu" <[email-masked]>
Date: Tue, 19 Dec 2023 02:23:24 +0000

On Thu, Dec 8, 2023 at 1:36 AM Li, Yong <[email-masked]> wrote:

>Given so many different approaches were discussed, I have started a  
>wiki to record and collaborate all efforts towards SLRU 
>improvements.  The wiki provides a concise overview of all the ideas 
>discussed and can serve as a portal for all historical 
>discussions.  Currently, the wiki summarizes four recent threads 
>ranging from identifier format change to page header change, to moving 
>SLRU into the main buffer pool, to reduce lock contention on SLRU 
>latches.  We can keep the patch related discussions in this thread and 
>use the wiki as a live document for larger scale collaborations.

>The wiki page is 
>here:  https://wiki.postgresql.org/wiki/SLRU_improvements

>Regarding the benefits of this patch, here is a detailed explanation:

1.	Checksum is added to each page, allowing us to verify if a page has
	been corrupted when read from the disk.
2.	The ad-hoc LSN group structure is removed from the SLRU cache 
	control data and is replaced by the page LSN in the page header. 
	This allows us to use the same WAL protocol as used by pages in the 
	main buffer pool: flush all redo logs up to the page LSN before 
	flushing the page itself. If we move SLRU caches into the main 
	buffer pool, this change fits naturally.
3.	It leaves further optimizations open. We can continue to pursue the 
	goal of moving SLRU into the main buffer pool, or we can follow the 
	lock partition idea. This change by itself does not conflict with 
	either proposal.

>Also, the patch is now complete and is ready for review.  All check-
>world tests including tap tests passed with this patch. 

Hi Yong, 

I agree we should break the effort for the SLRU optimization into 
smaller chunks after having worked on some of the bigger patches and 
facing difficulty in making progress that way.

The patch looks mostly good to me; though one thing that I thought about 
differently with the upgrade portion is where we should keep the logic 
of re-writing the CLOG files.

There is a precedent introduced back in Postgres v9.6 in making on disk 
page format changes across different in visibility map: [1]

code comment: 
 * In versions of PostgreSQL prior to catversion 201603011, PostgreSQL's
 * visibility map included one bit per heap page; it now includes two.
 * When upgrading a cluster from before that time to a current PostgreSQL
 * version, we could refuse to copy visibility maps from the old cluster
 * to the new cluster; the next VACUUM would recreate them, but at the
 * price of scanning the entire table.  So, instead, we rewrite the old
 * visibility maps in the new format.  

This work is being done in file.c – it seems to me the proper way to 
proceed would be to continue writing on-disk upgrade logic here.

Besides that this looks good to me, would like to hear what others have to say.

Thanks, 

Rishu Bagga 

Amazon Web Services (AWS)

[1] https://github.com/postgres/postgres/commit/7087166a88fe0c04fc6636d0d6d6bea1737fc1fb

---

From: "Li, Yong" <[email-masked]>
Date: Tue, 19 Dec 2023 07:28:19 +0000

> Besides that this looks good to me, would like to hear what others have to say.

Thank you, Rishu for taking time to review the code.  I've updated the patch
and moved the on-disk upgrade logic to pg_upgrade/file.c.

I have also added this thread to the current Commitfest and hope this patch
will be  part of the 17 release.

The commitfest link:
https://commitfest.postgresql.org/46/4709/

Regards,
Yong,

---

From: Aleksander Alekseev <[email-masked]>
Date: Tue, 2 Jan 2024 14:35:31 +0300

Hi,

> https://commitfest.postgresql.org/46/4709/

Thanks for the updated patch.

cfbot seems to have some complaints regarding compiler warnings and
also building the patch on Windows:

http://cfbot.cputube.org/

-- 
Best regards,
Aleksander Alekseev

---

From: "Li, Yong" <[email-masked]>
Date: Thu, 4 Jan 2024 15:57:33 +0000

> On Jan 2, 2024, at 19:35, Aleksander Alekseev <[email-masked]> wrote:
>

Thanks for the information.  Here is the updated patch.

Regards,
Yong

---

From: "Li, Yong" <[email-masked]>
Date: Tue, 16 Jan 2024 09:12:16 +0000

Rebase the patch against the latest HEAD.

Regards,
Yong

---

From: "Li, Yong" <[email-masked]>
Date: Wed, 6 Mar 2024 12:01:46 +0000

Regards,
Yong

---

From: Jeff Davis <[email-masked]>
Date: Fri, 08 Mar 2024 13:22:30 -0800

> Rebase the patch against the latest HEAD.

The upgrade logic could use more comments explaining what's going on
and why. As I understand it, it's a one-time conversion that needs to
happen between 16 and 17. Is that right?

Was the way CLOG is upgraded already decided in some earlier
discussion?

Given that the CLOG is append-only and gets truncated occasionally, I
wonder whether we can just have some marker that xids before some
number are the old CLOG, and xids beyond that number are in the new
CLOG. I'm not necessarily suggesting that; just an idea.

Regards,
	Jeff Davis

---

From: "Li, Yong" <[email-masked]>
Date: Mon, 11 Mar 2024 10:01:42 +0000

> On Mar 9, 2024, at 05:22, Jeff Davis <[email-masked]> wrote:
> 
> External Email
> 

> In the new code we effectively store only one LSN per page, which I
> understand is strictly worse.  Maybe the idea of doing away with these
> LSN groups should be reconsidered ... unless I completely misunderstand
> the whole thing.
> 
> --
> Álvaro Herrera         PostgreSQL Developer  —

Thanks for the comments on LSN groups and pg_upgrade.

I have updated the patch to address both comments:
- The clog LSN group has been brought back.
  Now the page LSN on each clog page is used for honoring the write-ahead rule
  and it is always the highest LSN of all the LSN groups on the page.
  The LSN groups are used by TransactionIdGetStatus() as before.
- New comments have been added to pg_upgrade to mention the SLRU
  page header change as the reason for upgrading clog files.

Regards,
Yong

---

From: Jeff Davis <[email-masked]>
Date: Thu, 14 Mar 2024 15:27:24 -0700

>   The LSN groups are used by TransactionIdGetStatus() as before.

I like where this is going.

Álvaro, do you still see a problem with this approach?

>   page header change as the reason for upgrading clog files.

That seems reasonable, but were any alternatives discussed? Do we have
consensus that this is the right thing to do?

And if we use this approach, is there extra validation or testing that
can be done?

Regards,
	Jeff Davis

---

From: "Li, Yong" <[email-masked]>
Date: Tue, 19 Mar 2024 06:48:33 +0000

> consensus that this is the right thing to do?

In general, there are two approaches. Either we convert the existing clog files,
or we don’t.  The patch chooses to convert.

If we don’t, then the clog file code must be able to handle both formats. For,
XIDs in the range where the clog is written in the old format, segment and offset
computation must be done in one way, and for XIDs in a different range, it must
be computed in a different way.  To avoid changing the format in the middle of a
page, which must not happen, the new format must start from a clean page, 
possibly in a clean new segment.  If the database is extremely small and has only
a few transactions on the first page of clog, then we must either convert the whole
page (effectively the whole clog file), or we must skip the rest of the XIDs on the
page and ask the database to start from XIDs on the second page on restart.
Also, we need to consider where to store the cut-off XID and when to remove it.
All these details feel very complex and error prone to me.  Performing a one-time
conversion is the most efficient and straightforward approach to me. 

> And if we use this approach, is there extra validation or testing that

Unfortunately, the test requires a setup of two different versions of PG. I am not
aware of an existing test infrastructure which can run automated tests using two
PGs. I did manually verify the output of pg_upgrade. 

Regards,
Yong

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 10 Jun 2024 07:19:56 +0000

Hi,

> PGs. I did manually verify the output of pg_upgrade. 

I think there is something in t/002_pg_upgrade.pl (see src/bin/pg_upgrade/TESTING),
that could be used to run automated tests using an old and a current version.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Michael Paquier <[email-masked]>
Date: Mon, 10 Jun 2024 17:01:50 +0900

> that could be used to run automated tests using an old and a current version.

Cluster.pm relies on install_path for stuff, where it is possible to
create tests with multiple nodes pointing to different installation
paths.  This allows mixing nodes with different build options, or just
different major versions like pg_upgrade's perl tests.
--
Michael

---

From: "Li, Yong" <[email-masked]>
Date: Thu, 13 Jun 2024 08:41:23 +0000

> On Jun 10, 2024, at 16:01, Michael Paquier <[email-masked]> wrote:
> 
> External Email
> 
> From: Michael Paquier <[email-masked]>
> Date: June 10, 2024 at 16:01:50 GMT+8
> To: Bertrand Drouvot <[email-masked]>
> Cc: "Li, Yong" <[email-masked]>, Jeff Davis <[email-masked]>, Aleksander Alekseev <[email-masked]>, PostgreSQL Hackers <[email-masked]>, "Bagga, Rishu" <[email-masked]>, Robert Haas <[email-masked]>, Andrey Borodin <[email-masked]>, "Shyrabokau, Anton" <[email-masked]>
> 
> 
> —

Thanks for pointing	this out. Here is what I have tried:
1. Manually build and install PostgreSQL from the latest source code.
2. Following the instructions from src/bin/pg_upgrade to manually dump the regression database.
3. Apply the patch to the latest code, and build from the source.
4. Run “make check” by following the instructions from src/bin/pg_upgrade and setting up the olddump and oldinstall to point to the “old” installation used in step 2.

All tests pass.

Yong

---

From: Rishu Bagga <[email-masked]>
Date: Sat, 9 Nov 2024 09:32:14 -0800

> 4. Run “make check” by following the instructions from src/bin/pg_upgrade and setting up
> the olddump and oldinstall to point to the “old” installation used in step 2.

> All tests pass.

Hi all,

Following up on this. What remaining work do we need to do to get this in?

Thanks,

Rishu Bagga