Thread: May be BUG. Periodic burst growth of the checkpoint_req counter on replica.

Hello!

Found a periodic spike growth of the checkpoint_req counter on replica by 20-30 units
after large insert (~350Mb) on master.
Reproduction on master and replica with default conf:
1) execute the command "insert into test values (generate_series(1,1E7));".
This leads to the table's growth by about 350Mb during about 15 secs (on my pc).
2)The wal records start coming to the replica, and when their number exceeds a certain limit, a request is emitted to
thecheckpointer process to create restartpoint on the replica and checkpoint_req is incremented. With default settings,
thislimit is 42 segments.
 
3) Restartpoint creation fails because a new restartpoint can only be created if the replica has received new WAL
recordsabout the checkpoint from the moment of the previous restartpoint. But there were no such records.
 
4) When the next WAL segment is received by replica, the next request is generated to create a restartpoint on the
replica,and so on.
 
5) Finally, a WAL record about the checkpoint arrives on the replica, restartpoint is created and the growth of
checkpoint_reqstops.
 
The described process can be observed in the log with additional debugging. See insert_1E7_once.log attached. This
log is for v13 but master has the same behavior.

Can we treat such behavior as a bug?
If so it seems possible to check if a creating of restartpoint is obviously impossible before sending request and don't
sendit at all if so.
 

The patch applied tries to fix it.

With best regards.
-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

At Tue, 6 Sep 2022 14:02:53 +0300, "Anton A. Melnikov" <aamelnikov@inbox.ru> wrote in 
> Can we treat such behavior as a bug?

Depends on how we see the counter value. I think this can be annoying
but not a bug. CreateRestartPoint already handles that case.

While standby is well catching up, startup may make requests once per
segment switch while primary is running the latest checkpoint since
standby won't receive a checkpoint record until the primary ends the
last checkpoint.

> If so it seems possible to check if a creating of restartpoint is
> obviously impossible before sending request and don't send it at all
> if so.
> 
> The patch applied tries to fix it.

It lets XLogPageRead run the same check with what CreateRestartPoint
does, so it basically works (it is forgetting a lock, though. The
reason for omitting the lock in CreateRestartPoint is that it knows
checkopinter is the only updator of the shared variable.). I'm not
sure I like that for the code duplication.


I'm not sure we need to fix that but if we do that, I would impletent
IsNewCheckPointWALRecs() using XLogCtl->RedoRecPtr and
XLogCtl->lastCheckPoint.redo instead since they are protected by the
same lock, and they work more correct way, that is, that can avoid
restartpoint requests while the last checkpoint is running.  And I
would rename it as RestartPointAvailable() or something like that.

Or I might want to add XLogRestartpointNeeded(readSegNo) to reduce the
required number of info_lck by reading XLogCtl members at once.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello!

Thank you very much for your feedback and essential remarks.

On 07.09.2022 10:39, Kyotaro Horiguchi wrote:
> 
> It lets XLogPageRead run the same check with what CreateRestartPoint
> does, so it basically works (it is forgetting a lock, though. The
> reason for omitting the lock in CreateRestartPoint is that it knows
> checkopinter is the only updator of the shared variable.). I'm not
> sure I like that for the code duplication.
> 
> I'm not sure we need to fix that but if we do that, I would impletent
> IsNewCheckPointWALRecs() using XLogCtl->RedoRecPtr and
> XLogCtl->lastCheckPoint.redo instead since they are protected by the
> same lock, and they work more correct way, that is, that can avoid
> restartpoint requests while the last checkpoint is running.  And I
> would rename it as RestartPointAvailable() or something like that.

Corrected patch is attached (v2-0001-Fix-burst-checkpoint_req-growth.patch).
The access to Controlfile was removed so lwlock seems to be not needed.
Some logic duplication is still present and and i'm not quite sure if
it's possible to get rid of it. Would be glad to any suggestions.

> Or I might want to add XLogRestartpointNeeded(readSegNo) to reduce the
> required number of info_lck by reading XLogCtl members at once.

If we place this check into the XLogCheckpointNeeded() this will lead to a double
take of info_lck in XLogPageRead() when the restartpoint request is forming.
As it's done now taking of info_lck will be more rarely.
It seems i probably didn't understand your idea, please clarify it for me.

> Depends on how we see the counter value. I think this can be annoying
> but not a bug. CreateRestartPoint already handles that case. 

Yes! It is in fact annoying as docs says that checkpoint_req counts
"the number of requested checkpoints that have been performed".
But really checkpoints_req counts both the number of checkpoints requests
and restartpoint ones which may not be performed and resources are not spent.
The second frightening factor is the several times faster growth
of the checkpoints_timed counter on the replica vs primary due to scheduling
replays in 15 second if an attempt to create the restartpoint failed.

Here is a patch that leaves all logic as is, but adds a stats about
restartpoints. (v1-0001-Add-restartpoint-stats.patch)
.
For instance, for the same period on primary with this patch:
# SELECT CURRENT_TIME; select * from pg_stat_bgwriter \gx
     current_time
--------------------
  00:19:15.794561+03
(1 row)

-[ RECORD 1 ]---------+-----------------------------
checkpoints_timed     | 4
checkpoints_req       | 10
restartpoints_timed   | 0
restartpoints_req     | 0
restartpoints_done    | 0

On replica:
# SELECT CURRENT_TIME; select * from pg_stat_bgwriter \gx
     current_time
--------------------
  00:19:11.363009+03
(1 row)

-[ RECORD 1 ]---------+------------------------------
checkpoints_timed     | 0
checkpoints_req       | 0
restartpoints_timed   | 42
restartpoints_req     | 67
restartpoints_done    | 10

Only the counters checkpoints_timed, checkpoints_req and restartpoints_done give
the indication of resource-intensive operations.
Without this patch, the user would see on the replica something like this:

checkpoints_timed     | 42
checkpoints_req       | 109


With best regards,

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 2022-09-19 01:29:21 +0300, Anton A. Melnikov wrote:
> Corrected patch is attached (v2-0001-Fix-burst-checkpoint_req-growth.patch).

This patch doesn't pass the main regression tests tests successfully:

https://cirrus-ci.com/task/5502700019253248

diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/rules.out
/tmp/cirrus-ci-build/build/testrun/regress/regress/results/rules.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/rules.out    2022-12-06 05:49:53.687424000 +0000
+++ /tmp/cirrus-ci-build/build/testrun/regress/regress/results/rules.out    2022-12-06 05:53:04.642690000 +0000
@@ -1816,6 +1816,9 @@
    FROM pg_stat_get_archiver() s(archived_count, last_archived_wal, last_archived_time, failed_count, last_failed_wal,
last_failed_time,stats_reset);
 
 pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
     pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
+    pg_stat_get_bgwriter_timed_restartpoints() AS restartpoints_timed,
+    pg_stat_get_bgwriter_requested_restartpoints() AS restartpoints_req,
+    pg_stat_get_bgwriter_performed_restartpoints() AS restartpoints_done,
     pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
     pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
     pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,

Greetings,

Andres Freund

Hello!

On 06.12.2022 21:44, Andres Freund wrote:
> Hi,
> 
> On 2022-09-19 01:29:21 +0300, Anton A. Melnikov wrote:
>> Corrected patch is attached (v2-0001-Fix-burst-checkpoint_req-growth.patch).
> 
> This patch doesn't pass the main regression tests tests successfully:
> 
> https://cirrus-ci.com/task/5502700019253248
> 
> diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/rules.out
/tmp/cirrus-ci-build/build/testrun/regress/regress/results/rules.out
> --- /tmp/cirrus-ci-build/src/test/regress/expected/rules.out    2022-12-06 05:49:53.687424000 +0000
> +++ /tmp/cirrus-ci-build/build/testrun/regress/regress/results/rules.out    2022-12-06 05:53:04.642690000 +0000
> @@ -1816,6 +1816,9 @@
>      FROM pg_stat_get_archiver() s(archived_count, last_archived_wal, last_archived_time, failed_count,
last_failed_wal,last_failed_time, stats_reset);
 
>   pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
>       pg_stat_get_bgwriter_requested_checkpoints() AS checkpoints_req,
> +    pg_stat_get_bgwriter_timed_restartpoints() AS restartpoints_timed,
> +    pg_stat_get_bgwriter_requested_restartpoints() AS restartpoints_req,
> +    pg_stat_get_bgwriter_performed_restartpoints() AS restartpoints_done,
>       pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
>       pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
>       pg_stat_get_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
> 
> Greetings,
> 
> Andres Freund

Thank you for pointing!

I didn't think that the patch tester would apply both patch variants simultaneously,
assuming that these are two different possible solutions of the problem.
But it's even good, let it check both at once!

There was an error in the second variant (Add-restartpoint-stats), i forgot to correct the rules.out.
So fixed the second variant and rebased the first one (Fix-burst-checkpoint_req-growth)
to the current master.


With the best wishes,

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello!


On 15.03.2023 21:29, Gregory Stark (as CFM) wrote:

> These patches that are "Needs Review" and have received no comments at
> all since before March 1st are these. If your patch is amongst this
> list I would suggest any of:
> 
> 1) Move it yourself to the next CF (or withdraw it)
> 2) Post to the list with any pending questions asking for specific
> feedback -- it's much more likely to get feedback than just a generic
> "here's a patch plz review"...
> 3) Mark it Ready for Committer and possibly post explaining the
> resolution to any earlier questions to make it easier for a committer
> to understand the state
>

There are two different patch variants and some discussion expected.
So moved them to the next CF.

With the best wishes!

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi, Anton!

On Thu, Mar 16, 2023 at 2:39 PM Anton A. Melnikov <aamelnikov@inbox.ru> wrote:
> On 15.03.2023 21:29, Gregory Stark (as CFM) wrote:
>
> > These patches that are "Needs Review" and have received no comments at
> > all since before March 1st are these. If your patch is amongst this
> > list I would suggest any of:
> >
> > 1) Move it yourself to the next CF (or withdraw it)
> > 2) Post to the list with any pending questions asking for specific
> > feedback -- it's much more likely to get feedback than just a generic
> > "here's a patch plz review"...
> > 3) Mark it Ready for Committer and possibly post explaining the
> > resolution to any earlier questions to make it easier for a committer
> > to understand the state
> >
>
> There are two different patch variants and some discussion expected.
> So moved them to the next CF.

Thank you for your detailed observation regarding the spike growth of
the checkpoint_req counter on the replica following a large insert
operation on the master.  After reviewing your description and the
log, I agree with Kyotaro Horiguchi that the behavior you've outlined,
though potentially perceived as annoying, does not constitute a bug in
the PostgreSQL.

After examining the second patch
("v2-0001-Add-restartpoint-stats.patch"), it appears that adding
additional statistics as outlined in the patch is the most suitable
approach to address the concerns raised. This solution provides more
visibility into the system's behavior without altering its core
mechanics. However, it's essential that this additional functionality
is accompanied by comprehensive documentation to ensure clear
understanding and ease of use by the PostgreSQL community.

Please consider expanding the documentation to include detailed
explanations of the new statistics and their implications in various
scenarios.

------
Regards,
Alexander Korotkov

Thanks for remarks!

On 28.11.2023 21:34, Alexander Korotkov wrote:
> After examining the second patch
> ("v2-0001-Add-restartpoint-stats.patch"), it appears that adding
> additional statistics as outlined in the patch is the most suitable
> approach to address the concerns raised. This solution provides more
> visibility into the system's behavior without altering its core
> mechanics.

Agreed. I left only this variant of the patch and rework it due to commit 96f05261.
So the new counters is in the pg_stat_checkpointer view now.
Please see the v3-0001-add-restartpoints-stats.patch attached.


> However, it's essential that this additional functionality
> is accompanied by comprehensive documentation to ensure clear
> understanding and ease of use by the PostgreSQL community.
> 
> Please consider expanding the documentation to include detailed
> explanations of the new statistics and their implications in various
> scenarios.

In the separate v3-0002-doc-for-restartpoints-stats.patch i added the definitions
of the new counters into the "28.2.15. pg_stat_checkpointer" section
and explanation of them with examples into the "30.5.WAL Configuration" one.

Would be glad for any comments and and concerns.

With the best wishes,

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sat, Dec 23, 2023 at 12:04 AM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
> On Mon, Dec 4, 2023 at 3:50 AM Anton A. Melnikov <a.melnikov@postgrespro.ru> wrote:
>>
>> Thanks for remarks!
>>
>> On 28.11.2023 21:34, Alexander Korotkov wrote:
>> > After examining the second patch
>> > ("v2-0001-Add-restartpoint-stats.patch"), it appears that adding
>> > additional statistics as outlined in the patch is the most suitable
>> > approach to address the concerns raised. This solution provides more
>> > visibility into the system's behavior without altering its core
>> > mechanics.
>>
>> Agreed. I left only this variant of the patch and rework it due to commit 96f05261.
>> So the new counters is in the pg_stat_checkpointer view now.
>> Please see the v3-0001-add-restartpoints-stats.patch attached.
>>
>>
>> > However, it's essential that this additional functionality
>> > is accompanied by comprehensive documentation to ensure clear
>> > understanding and ease of use by the PostgreSQL community.
>> >
>> > Please consider expanding the documentation to include detailed
>> > explanations of the new statistics and their implications in various
>> > scenarios.
>>
>> In the separate v3-0002-doc-for-restartpoints-stats.patch i added the definitions
>> of the new counters into the "28.2.15. pg_stat_checkpointer" section
>> and explanation of them with examples into the "30.5.WAL Configuration" one.
>>
>> Would be glad for any comments and and concerns.
>
>
> I made some grammar corrections to the docs and have written the commit message.
>
> I think this patch now looks good.  I'm going to push this if there are no objections.

Pushed!

------
Regards,
Alexander Korotkov

On 25.12.2023 02:38, Alexander Korotkov wrote:

> Pushed!

Thanks a lot!

With the best regards!

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Fri, Dec 22, 2023 at 11:04 PM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
>
> Hi, Anton!
>
> On Mon, Dec 4, 2023 at 3:50 AM Anton A. Melnikov <a.melnikov@postgrespro.ru> wrote:
>>
>> Thanks for remarks!
>>
>> On 28.11.2023 21:34, Alexander Korotkov wrote:
>> > After examining the second patch
>> > ("v2-0001-Add-restartpoint-stats.patch"), it appears that adding
>> > additional statistics as outlined in the patch is the most suitable
>> > approach to address the concerns raised. This solution provides more
>> > visibility into the system's behavior without altering its core
>> > mechanics.
>>
>> Agreed. I left only this variant of the patch and rework it due to commit 96f05261.
>> So the new counters is in the pg_stat_checkpointer view now.
>> Please see the v3-0001-add-restartpoints-stats.patch attached.
>>
>>
>> > However, it's essential that this additional functionality
>> > is accompanied by comprehensive documentation to ensure clear
>> > understanding and ease of use by the PostgreSQL community.
>> >
>> > Please consider expanding the documentation to include detailed
>> > explanations of the new statistics and their implications in various
>> > scenarios.
>>
>> In the separate v3-0002-doc-for-restartpoints-stats.patch i added the definitions
>> of the new counters into the "28.2.15. pg_stat_checkpointer" section
>> and explanation of them with examples into the "30.5.WAL Configuration" one.
>>
>> Would be glad for any comments and and concerns.
>
>
> I made some grammar corrections to the docs and have written the commit message.
>
> I think this patch now looks good.  I'm going to push this if there are no objections.

Per the docs, the sync_time, write_time and buffers_written only apply
to checkpoints, not restartpoints. Is this correct? AFAICT from a
quick look at the code they include both checkpoints and restartpoints
in which case I think the docs should be clarified to indicate that?
(Or if I'm wrong, and it doesn't include them, then shouldn't we have
separate counters for them?)

//Magnus

On Sat, Mar 9, 2024 at 4:38 PM Magnus Hagander <magnus@hagander.net> wrote:
> Per the docs, the sync_time, write_time and buffers_written only apply
> to checkpoints, not restartpoints. Is this correct? AFAICT from a
> quick look at the code they include both checkpoints and restartpoints
> in which case I think the docs should be clarified to indicate that?

Right, these fields work as before reflecting both checkpoints and
restartpoints.  Documentation said checkpoints implying restartpoints
as well.  Now that we distinguish stats for checkpoints and
restartpoints, we need to update the docs.  Please, check the patch
attached.

------
Regards,
Alexander Korotkov

On 11.03.2024 03:39, Alexander Korotkov wrote:
> Now that we distinguish stats for checkpoints and
> restartpoints, we need to update the docs.  Please, check the patch
> attached.

Maybe bring the pg_stat_get_checkpointer_buffers_written() description in consistent with these changes?
Like that:

--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5740 +5740 @@
-  descr => 'statistics: number of buffers written by the checkpointer',
+  descr => 'statistics: number of buffers written during checkpoints and restartpoints',

And after i took a fresh look at this patch i noticed a simple way to extract
write_time and sync_time counters for restartpoints too.

What do you think, is there a sense to do this?


With the best wishes,


-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Mar 11, 2024 at 5:43 AM Anton A. Melnikov
<a.melnikov@postgrespro.ru> wrote:
> On 11.03.2024 03:39, Alexander Korotkov wrote:
> > Now that we distinguish stats for checkpoints and
> > restartpoints, we need to update the docs.  Please, check the patch
> > attached.
>
> Maybe bring the pg_stat_get_checkpointer_buffers_written() description in consistent with these changes?
> Like that:
>
> --- a/src/include/catalog/pg_proc.dat
> +++ b/src/include/catalog/pg_proc.dat
> @@ -5740 +5740 @@
> -  descr => 'statistics: number of buffers written by the checkpointer',
> +  descr => 'statistics: number of buffers written during checkpoints and restartpoints',

This makes sense.  I've included this into the revised patch.

> And after i took a fresh look at this patch i noticed a simple way to extract
> write_time and sync_time counters for restartpoints too.
>
> What do you think, is there a sense to do this?

I'm not sure we need this.  The ways we trigger checkpoints and
restartpoints are different.  This is why we needed separate
statistics.  But the process of writing buffers is the same.

------
Regards,
Alexander Korotkov

On Mon, Mar 11, 2024 at 11:48 AM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
>
> On Mon, Mar 11, 2024 at 5:43 AM Anton A. Melnikov
> <a.melnikov@postgrespro.ru> wrote:
> > On 11.03.2024 03:39, Alexander Korotkov wrote:
> > > Now that we distinguish stats for checkpoints and
> > > restartpoints, we need to update the docs.  Please, check the patch
> > > attached.
> >
> > Maybe bring the pg_stat_get_checkpointer_buffers_written() description in consistent with these changes?
> > Like that:
> >
> > --- a/src/include/catalog/pg_proc.dat
> > +++ b/src/include/catalog/pg_proc.dat
> > @@ -5740 +5740 @@
> > -  descr => 'statistics: number of buffers written by the checkpointer',
> > +  descr => 'statistics: number of buffers written during checkpoints and restartpoints',
>
> This makes sense.  I've included this into the revised patch.

Pushed.

------
Regards,
Alexander Korotkov

On 14.03.2024 03:19, Alexander Korotkov wrote:
> 
> Pushed.
> 

Thanks!


-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2024/03/14 9:19, Alexander Korotkov wrote:
> On Mon, Mar 11, 2024 at 11:48 AM Alexander Korotkov
> <aekorotkov@gmail.com> wrote:
>>
>> On Mon, Mar 11, 2024 at 5:43 AM Anton A. Melnikov
>> <a.melnikov@postgrespro.ru> wrote:
>>> On 11.03.2024 03:39, Alexander Korotkov wrote:
>>>> Now that we distinguish stats for checkpoints and
>>>> restartpoints, we need to update the docs.  Please, check the patch
>>>> attached.
>>>
>>> Maybe bring the pg_stat_get_checkpointer_buffers_written() description in consistent with these changes?
>>> Like that:
>>>
>>> --- a/src/include/catalog/pg_proc.dat
>>> +++ b/src/include/catalog/pg_proc.dat
>>> @@ -5740 +5740 @@
>>> -  descr => 'statistics: number of buffers written by the checkpointer',
>>> +  descr => 'statistics: number of buffers written during checkpoints and restartpoints',
>>
>> This makes sense.  I've included this into the revised patch.
> 
> Pushed.

If I understand correctly, restartpoints_timed and restartpoints_done were
separated because a restartpoint can be skipped. restartpoints_timed counts
when a restartpoint is triggered by a timeout, whether it runs or not,
while restartpoints_done only tracks completed restartpoints.

Similarly, I believe checkpoints should be handled the same way.
Checkpoints can also be skipped when the system is idle, but currently,
num_timed counts even the skipped ones, despite its documentation stating
it's the "Number of scheduled checkpoints that have been performed."

Why not separate num_timed into something like checkpoints_timed and
checkpoints_done to reflect these different counters?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2024/09/16 23:30, Anton A. Melnikov wrote:
> +1
> This idea seems quite tenable to me.
> 
> There is a small clarification. Now if there were no skipped restartpoints then
> restartpoints_done will be equal to restartpoints_timed + restartpoints_req.
> Similar for checkpoints.
> So i tried to introduce num_done counter for checkpoints in the patch attached.

Thanks for the patch! I believe this change is targeted for v18. For v17, however,
we should update the description of num_timed in the documentation. Thought?
Here's a suggestion:

"Number of scheduled checkpoints due to timeout. Note that checkpoints may be
skipped if the server has been idle since the last one, and this value counts
both completed and skipped checkpoints."

Regarding the patch:
                  if (do_restartpoint)
                      PendingCheckpointerStats.restartpoints_performed++;
+                else
+                    PendingCheckpointerStats.num_performed++;

I expected the counter not to be incremented when a checkpoint is skipped,
but in this code, when a checkpoint is skipped, ckpt_performed is set to true,
triggering the counter increment. This seems wrong.


> I'm not sure should we include testing for the case when num_done is less than
> num_timed + num_requested to the regress tests. I haven't been able to get it in a short time yet.

I'm not sure if that test is really necessary...

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Sep 18, 2024 at 1:21 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2024/09/17 11:47, Fujii Masao wrote:
> >
> >
> > On 2024/09/16 23:30, Anton A. Melnikov wrote:
> >> +1
> >> This idea seems quite tenable to me.
> >>
> >> There is a small clarification. Now if there were no skipped restartpoints then
> >> restartpoints_done will be equal to restartpoints_timed + restartpoints_req.
> >> Similar for checkpoints.
> >> So i tried to introduce num_done counter for checkpoints in the patch attached.
> >
> > Thanks for the patch! I believe this change is targeted for v18. For v17, however,
> > we should update the description of num_timed in the documentation. Thought?
> > Here's a suggestion:
> >
> > "Number of scheduled checkpoints due to timeout. Note that checkpoints may be
> > skipped if the server has been idle since the last one, and this value counts
> > both completed and skipped checkpoints."
>
> Patch attached.
> Unless there are any objections, I plan to commit this and back-patch it to v17.

I've checked this patch, it looks good to me.

Generally, it looks like I should be in charge for this, given I've
committed previous patch by Anton.  Thank you for reacting here faster
than me.  Please, go ahead with the patch.

------
Regards,
Alexander Korotkov
Supabase

On 2024/09/18 21:22, Alexander Korotkov wrote:
>> Patch attached.
>> Unless there are any objections, I plan to commit this and back-patch it to v17.
> 
> I've checked this patch, it looks good to me.
> 
> Generally, it looks like I should be in charge for this, given I've
> committed previous patch by Anton.  Thank you for reacting here faster
> than me.  Please, go ahead with the patch.

Thanks for the review! Pushed!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2024/09/18 23:35, Anton A. Melnikov wrote:
> Fujii, Alexander thanks a lot!
> 
> On 17.09.2024 05:47, Fujii Masao wrote:
>>
>> Regarding the patch:
>>                   if (do_restartpoint)
>>                       PendingCheckpointerStats.restartpoints_performed++;
>> +                else
>> +                    PendingCheckpointerStats.num_performed++;
>>
>> I expected the counter not to be incremented when a checkpoint is skipped,
>> but in this code, when a checkpoint is skipped, ckpt_performed is set to true,
>> triggering the counter increment. This seems wrong.
> 
> Tried to fix it via returning bool value from the CreateCheckPoint()
> similarly to the CreateRestartPoint().
> 
> And slightly adjusted the patch so that it could be applied after yours.

Thanks for updating the patch!

-void
+bool
  CreateCheckPoint(int flags)

It would be helpful to explain the new return value in the comment
at the top of this function.

-                CreateCheckPoint(flags);
-                ckpt_performed = true;
+                ckpt_performed = CreateCheckPoint(flags);

This change could result in the next scheduled checkpoint being
triggered in 15 seconds if a checkpoint is skipped, which isn’t
the intended behavior.

-{ oid => '2769',
+{ oid => '6347',

I don't think that the existing functions need to be reassigned new OIDs.


Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 20.09.2024 19:19, Fujii Masao wrote:
> I've attached the updated version (0001.patch). I made some cosmetic changes,
> including reverting the switch in the entries for pg_stat_get_checkpointer_write_time
> and pg_stat_get_checkpointer_sync_time in pg_proc.dat, as I didn’t think
> that change was necessary. Could you please review the latest version?

Thanks for corrections!
All looks good for me.
As for switching in the pg_proc.dat entries the idea was to put them in order
so that the pg_stat_get_checkpointer* functions were grouped together.
I don't know if this is the common and accepted practice. Simply i like it better this way.
Sure, if you think it's unnecessary, let it stay as is with minimal diff.


> After we commit 0001.patch, how about applying 0002.patch, which updates
> the documentation for the pg_stat_checkpointer view to clarify what types
> of checkpoints and restartpoints each counter tracks?

I liked that the short definitions of the counters are now separated from
the description of its work features which are combined into one paragraph.
It seems to me that is much more logical and easier to understand.

In addition, checkpoints may be skipped due to "checkpoints are occurring
too frequently" error. Not sure, but maybe add this information to
the new description?

> In 0002.patch, I also modified the description of num_requested from
> "Number of backend requested checkpoints" to remove "backend," as it can
> be confusing since num_requested includes requests from sources other than
> the backend. Thought?

Agreed. E.g. from xlog. Then maybe changed it also in the function
descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested()
and pg_stat_get_checkpointer_restartpoints_requested().


Also checked v4 with the travis patch-tester. All is ok.

With the best wishes!

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2024/09/22 13:55, Anton A. Melnikov wrote:
> On 20.09.2024 19:19, Fujii Masao wrote:
>> I've attached the updated version (0001.patch). I made some cosmetic changes,
>> including reverting the switch in the entries for pg_stat_get_checkpointer_write_time
>> and pg_stat_get_checkpointer_sync_time in pg_proc.dat, as I didn’t think
>> that change was necessary. Could you please review the latest version?
> 
> Thanks for corrections!
> All looks good for me.

Thanks for the review! I've pushed the 0001 patch.


> As for switching in the pg_proc.dat entries the idea was to put them in order
> so that the pg_stat_get_checkpointer* functions were grouped together.
> I don't know if this is the common and accepted practice. Simply i like it better this way.
> Sure, if you think it's unnecessary, let it stay as is with minimal diff.

I understand your point, but I didn't made that change to keep the diff minimal,
which should make future back-patching easier.


>> After we commit 0001.patch, how about applying 0002.patch, which updates
>> the documentation for the pg_stat_checkpointer view to clarify what types
>> of checkpoints and restartpoints each counter tracks?
> 
> I liked that the short definitions of the counters are now separated from
> the description of its work features which are combined into one paragraph.
> It seems to me that is much more logical and easier to understand.

Thanks for the review!


> In addition, checkpoints may be skipped due to "checkpoints are occurring
> too frequently" error. Not sure, but maybe add this information to
> the new description?

 From what I can see in the code, that error message doesn’t seem to indicate
the checkpoint is being skipped. In fact, checkpoints are still happening
actually when that message appears. Am I misunderstanding something?


>> In 0002.patch, I also modified the description of num_requested from
>> "Number of backend requested checkpoints" to remove "backend," as it can
>> be confusing since num_requested includes requests from sources other than
>> the backend. Thought?
> 
> Agreed. E.g. from xlog. Then maybe changed it also in the function
> descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested()
> and pg_stat_get_checkpointer_restartpoints_requested().

Yes, good catch!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 30.09.2024 06:26, Fujii Masao wrote:
> Thanks for the review! I've pushed the 0001 patch.

Thanks a lot!

>> As for switching in the pg_proc.dat entries the idea was to put them in order
>> so that the pg_stat_get_checkpointer* functions were grouped together.
>> I don't know if this is the common and accepted practice. Simply i like it better this way.
>> Sure, if you think it's unnecessary, let it stay as is with minimal diff.
> 
> I understand your point, but I didn't made that change to keep the diff minimal,
> which should make future back-patching easier.

Agreed. Its quite reasonable. I've not take into account the backporting
possibility at all. This is of course wrong.

>> In addition, checkpoints may be skipped due to "checkpoints are occurring
>> too frequently" error. Not sure, but maybe add this information to
>> the new description?
> 
>  From what I can see in the code, that error message doesn’t seem to indicate
> the checkpoint is being skipped. In fact, checkpoints are still happening
> actually when that message appears. Am I misunderstanding something?

No, you are right! This is my oversight. I didn't notice that elevel is just a log
not a error. Thanks!


With the best wishes,
   

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2024/09/30 16:00, Anton A. Melnikov wrote:
> 
> On 30.09.2024 06:26, Fujii Masao wrote:
>> Thanks for the review! I've pushed the 0001 patch.
> 
> Thanks a lot!
> 
>>> As for switching in the pg_proc.dat entries the idea was to put them in order
>>> so that the pg_stat_get_checkpointer* functions were grouped together.
>>> I don't know if this is the common and accepted practice. Simply i like it better this way.
>>> Sure, if you think it's unnecessary, let it stay as is with minimal diff.
>>
>> I understand your point, but I didn't made that change to keep the diff minimal,
>> which should make future back-patching easier.
> 
> Agreed. Its quite reasonable. I've not take into account the backporting
> possibility at all. This is of course wrong.
> 
>>> In addition, checkpoints may be skipped due to "checkpoints are occurring
>>> too frequently" error. Not sure, but maybe add this information to
>>> the new description?
>>
>>  From what I can see in the code, that error message doesn’t seem to indicate
>> the checkpoint is being skipped. In fact, checkpoints are still happening
>> actually when that message appears. Am I misunderstanding something?
> 
> No, you are right! This is my oversight. I didn't notice that elevel is just a log
> not a error. Thanks!

Ok, so I pushed 0002.patch. Thanks for the review!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 08.10.2024 15:42, Fujii Masao wrote:
> On 2024/09/30 12:26, Fujii Masao wrote:
>>>> In 0002.patch, I also modified the description of num_requested from
>>>> "Number of backend requested checkpoints" to remove "backend," as it can
>>>> be confusing since num_requested includes requests from sources other than
>>>> the backend. Thought?
>>>
>>> Agreed. E.g. from xlog. Then maybe changed it also in the function
>>> descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested()
>>> and pg_stat_get_checkpointer_restartpoints_requested().
>>
>> Yes, good catch!
> 
> Patch attached.
  
Looked at the patch. Just in case, checked that neither
“backend completed” nor “backend requested” were found anywhere else.
All is ok for me.

Thanks a lot!

With the best wishes,

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2024/10/08 23:16, Anton A. Melnikov wrote:
> 
> On 08.10.2024 15:42, Fujii Masao wrote:
>> On 2024/09/30 12:26, Fujii Masao wrote:
>>>>> In 0002.patch, I also modified the description of num_requested from
>>>>> "Number of backend requested checkpoints" to remove "backend," as it can
>>>>> be confusing since num_requested includes requests from sources other than
>>>>> the backend. Thought?
>>>>
>>>> Agreed. E.g. from xlog. Then maybe changed it also in the function
>>>> descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested()
>>>> and pg_stat_get_checkpointer_restartpoints_requested().
>>>
>>> Yes, good catch!
>>
>> Patch attached.
> 
> Looked at the patch. Just in case, checked that neither
> “backend completed” nor “backend requested” were found anywhere else.
> All is ok for me.

Thanks for the review! Pushed.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 10.10.2024 18:14, Fujii Masao wrote:

> Thanks for the review! Pushed.

Thanks a lot!


With the best regards,

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company