Make use of assign_checkpoint_completion_target() to calculate CheckPointSegments correctly

Started by Bharath Rupireddyabout 3 years ago4 messages
#1Bharath Rupireddy
bharath.rupireddyforpostgres@gmail.com
1 attachment(s)

Hi,

It looks like assign_checkpoint_completion_target() is defined [1]commit 88e982302684246e8af785e78a467ac37c76dee9 Author: Heikki Linnakangas <heikki.linnakangas@iki.fi> Date: Mon Feb 23 18:53:02 2015 +0200,
but never used, because of which CheckPointSegments may miss to
account for changed checkpoint_completion_target. I'm attaching a tiny
patch to fix this.

Thoughts?

[1]: commit 88e982302684246e8af785e78a467ac37c76dee9 Author: Heikki Linnakangas <heikki.linnakangas@iki.fi> Date: Mon Feb 23 18:53:02 2015 +0200
commit 88e982302684246e8af785e78a467ac37c76dee9
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon Feb 23 18:53:02 2015 +0200

Replace checkpoint_segments with min_wal_size and max_wal_size.

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

Attachments:

v1-0001-Make-use-of-assign_checkpoint_completion_target-t.patchapplication/x-patch; name=v1-0001-Make-use-of-assign_checkpoint_completion_target-t.patchDownload
From ea9803c77b32b5d42ba7f5e1c8130f618b6322be Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 26 Dec 2022 12:21:56 +0000
Subject: [PATCH v1] Make use of assign_checkpoint_completion_target() to
 calculate CheckPointSegments correctly

---
 src/backend/utils/misc/guc_tables.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index a37c9f9844..948e1ae5aa 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3701,7 +3701,7 @@ struct config_real ConfigureNamesReal[] =
 		},
 		&CheckPointCompletionTarget,
 		0.9, 0.0, 1.0,
-		NULL, NULL, NULL
+		NULL, assign_checkpoint_completion_target, NULL
 	},
 
 	{
-- 
2.34.1

#2Michael Paquier
michael@paquier.xyz
In reply to: Bharath Rupireddy (#1)
Re: Make use of assign_checkpoint_completion_target() to calculate CheckPointSegments correctly

On Mon, Dec 26, 2022 at 06:12:34PM +0530, Bharath Rupireddy wrote:

It looks like assign_checkpoint_completion_target() is defined [1],
but never used, because of which CheckPointSegments may miss to
account for changed checkpoint_completion_target. I'm attaching a tiny
patch to fix this.

Thoughts?

Oops. It looks like you are right here. This would impact the
calculation of CheckPointSegments on reload when
checkpoint_completion_target is updated. This is wrong since we've
switched to max_wal_size as of 88e9823, so this had better be
backpatched all the way down.

Thoughts?
--
Michael

#3Bharath Rupireddy
bharath.rupireddyforpostgres@gmail.com
In reply to: Michael Paquier (#2)
Re: Make use of assign_checkpoint_completion_target() to calculate CheckPointSegments correctly

On Tue, Jan 17, 2023 at 12:31 PM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Dec 26, 2022 at 06:12:34PM +0530, Bharath Rupireddy wrote:

It looks like assign_checkpoint_completion_target() is defined [1],
but never used, because of which CheckPointSegments may miss to
account for changed checkpoint_completion_target. I'm attaching a tiny
patch to fix this.

Thoughts?

Oops. It looks like you are right here. This would impact the
calculation of CheckPointSegments on reload when
checkpoint_completion_target is updated. This is wrong since we've
switched to max_wal_size as of 88e9823, so this had better be
backpatched all the way down.

Thoughts?

+1 to backpatch as setting checkpoint_completion_target will not take
effect immediately.

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

#4Michael Paquier
michael@paquier.xyz
In reply to: Bharath Rupireddy (#3)
Re: Make use of assign_checkpoint_completion_target() to calculate CheckPointSegments correctly

On Tue, Jan 17, 2023 at 07:55:53PM +0530, Bharath Rupireddy wrote:

On Tue, Jan 17, 2023 at 12:31 PM Michael Paquier <michael@paquier.xyz> wrote:

Oops. It looks like you are right here. This would impact the
calculation of CheckPointSegments on reload when
checkpoint_completion_target is updated. This is wrong since we've
switched to max_wal_size as of 88e9823, so this had better be
backpatched all the way down.

Thoughts?

+1 to backpatch as setting checkpoint_completion_target will not take
effect immediately.

Okay, applied down to 11. I have double-checked the surroundings to
see if there was a similar mistake or something hiding around
CheckpointSegments but noticed nothing (also did some primary/standby
pgbench-ing with periodic reloads of checkpoint_completion_target,
while on it).
--
Michael