Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2023-01-31 Thread vignesh C
On Thu, 8 Dec 2022 at 00:33, Andres Freund wrote: > > Hi, > > On 2022-11-04 09:25:52 +0100, Drouvot, Bertrand wrote: > > Please find attached a rebase in v7. > > cfbot complains that the docs don't build: > https://cirrus-ci.com/task/6694349031866368?logs=docs_build#L296 > > [03:24:27.317]

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-12-07 Thread Andres Freund
Hi, On 2022-11-04 09:25:52 +0100, Drouvot, Bertrand wrote: > Please find attached a rebase in v7. cfbot complains that the docs don't build: https://cirrus-ci.com/task/6694349031866368?logs=docs_build#L296 [03:24:27.317] ref/checkpoint.sgml:66: element para: validity error : Element para is

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-17 Thread Robert Haas
On Thu, Nov 17, 2022 at 11:24 AM Andres Freund wrote: > As an experiment, I added a progress report to BufferSync()'s first loop > (i.e. where it checks all buffers). On a 128GB shared_buffers cluster that > increases the time for a do-nothing checkpoint from ~235ms to ~280ms. If I > remove the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-17 Thread Tom Lane
Andres Freund writes: > I think pgstat_progress_start_command() needs the changecount stuff, as does > pgstat_progress_update_multi_param(). But for anything updating a single > parameter at a time it really doesn't do anything useful on a platform that > doesn't tear 64bit writes (so it could be

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-17 Thread Andres Freund
Hi, On 2022-11-17 09:03:32 -0500, Robert Haas wrote: > On Wed, Nov 16, 2022 at 2:52 PM Andres Freund wrote: > > I don't think we'd want every buffer write or whatnot go through the > > changecount mechanism, on some non-x86 platforms that could be noticable. > > But > > if we didn't stage the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-17 Thread Robert Haas
On Wed, Nov 16, 2022 at 2:52 PM Andres Freund wrote: > I don't think we'd want every buffer write or whatnot go through the > changecount mechanism, on some non-x86 platforms that could be noticable. But > if we didn't stage the stats updates locally I think we could make most of the > stats

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-17 Thread Bharath Rupireddy
On Thu, Nov 17, 2022 at 12:49 AM Robert Haas wrote: > > > I also think that a new pg_stat_checkpoint view is needed > > because, right now, the PgStat_CheckpointerStats stats are exposed via > > the pg_stat_bgwriter view, having a separate view for checkpoint stats > > is good here. > > Yep. On

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-16 Thread Andres Freund
Hi, On 2022-11-16 14:19:32 -0500, Robert Haas wrote: > I have never really understood why we drive background writer or > checkpointer statistics through the statistics collector. To some degree it is required for durability - the stats system needs to know how to write out those stats. But that

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-16 Thread Robert Haas
On Wed, Nov 16, 2022 at 5:32 AM Bharath Rupireddy wrote: > -1 for CheckpointerShmemStruct as it is being used for running > checkpoints and I don't think adding stats to it is a great idea. > Instead, extending PgStat_CheckpointerStats and using shared memory > stats for reporting progress/last

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-16 Thread Andres Freund
Hi, On 2022-11-16 16:01:55 +0530, Bharath Rupireddy wrote: > -1 for CheckpointerShmemStruct as it is being used for running > checkpoints and I don't think adding stats to it is a great idea. Why? Imo the data needed for progress reporting aren't really "stats". We'd not accumulate counters

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-16 Thread Bharath Rupireddy
On Wed, Nov 16, 2022 at 1:35 AM Robert Haas wrote: > > On Fri, Nov 4, 2022 at 4:27 AM Drouvot, Bertrand > wrote: > > Please find attached a rebase in v7. > > I don't think it's a good thing that this patch is using the > progress-reporting machinery. The point of that machinery is that we > want

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-15 Thread Andres Freund
Hi, On 2022-11-04 09:25:52 +0100, Drouvot, Bertrand wrote: > > @@ -7023,29 +7048,63 @@ static void > CheckPointGuts(XLogRecPtr checkPointRedo, int flags) > { > CheckPointRelationMap(); > + > + pgstat_progress_update_param(PROGRESS_CHECKPOINT_PHASE, > +

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-15 Thread Robert Haas
On Fri, Nov 4, 2022 at 4:27 AM Drouvot, Bertrand wrote: > Please find attached a rebase in v7. I don't think it's a good thing that this patch is using the progress-reporting machinery. The point of that machinery is that we want any backend to be able to report progress for any command it

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-15 Thread Nitin Jadhav
> v6 was not applying anymore, due to a change in > doc/src/sgml/ref/checkpoint.sgml done by b9eb0ff09e (Rename > pg_checkpointer predefined role to pg_checkpoint). > > Please find attached a rebase in v7. > > While working on this rebase, I also noticed that "pg_checkpointer" is > still mentioned

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-11-04 Thread Drouvot, Bertrand
Hi, On 7/28/22 11:38 AM, Nitin Jadhav wrote: To understand the performance effects of the above, I have taken the average of five checkpoints with the patch and without the patch in my environment. Here are the results. With patch: 269.65 s Without patch: 269.60 s Those look like timed

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-07-28 Thread Nitin Jadhav
> > To understand the performance effects of the above, I have taken the > > average of five checkpoints with the patch and without the patch in my > > environment. Here are the results. > > With patch: 269.65 s > > Without patch: 269.60 s > > Those look like timed checkpoints - if the checkpoints

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-07-06 Thread Andres Freund
Hi, On 2022-06-13 19:08:35 +0530, Nitin Jadhav wrote: > > Have you measured the performance effects of this? On fast storage with > > large > > shared_buffers I've seen these loops in profiles. It's probably fine, but > > it'd > > be good to verify that. > > To understand the performance

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-06-13 Thread Nitin Jadhav
> > Have you measured the performance effects of this? On fast storage with > > large > > shared_buffers I've seen these loops in profiles. It's probably fine, but > > it'd > > be good to verify that. > > I am wondering if we could make the function inlined at some point. > We could also play it

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-06-13 Thread Nitin Jadhav
> Have you measured the performance effects of this? On fast storage with large > shared_buffers I've seen these loops in profiles. It's probably fine, but it'd > be good to verify that. To understand the performance effects of the above, I have taken the average of five checkpoints with the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-06-06 Thread Nitin Jadhav
Hi, Here is the update patch which fixes the previous comments discussed in this thread. I am sorry for the long gap in the discussion. Kindly let me know if I have missed any of the comments or anything new. Thanks & Regards, Nitin Jadhav On Fri, Mar 18, 2022 at 4:52 PM Nitin Jadhav wrote: >

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-04-05 Thread Michael Paquier
On Fri, Mar 18, 2022 at 05:15:56PM -0700, Andres Freund wrote: > Have you measured the performance effects of this? On fast storage with large > shared_buffers I've seen these loops in profiles. It's probably fine, but it'd > be good to verify that. I am wondering if we could make the function

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-18 Thread Andres Freund
Hi, This is a long thread, sorry for asking if this has been asked before. On 2022-03-08 20:25:28 +0530, Nitin Jadhav wrote: >* Sort buffers that need to be written to reduce the likelihood of > random > @@ -2129,6 +2132,8 @@ BufferSync(int flags) > bufHdr =

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-18 Thread Nitin Jadhav
> > > I don't get it. The checkpoint flags and the view flags (set by > > > pgstat_progrss_update*) are different, so why can't we add this flag to > > > the > > > view flags? The fact that checkpointer.c doesn't update the passed flag > > > and > > > instead look in the shmem to see if

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-14 Thread Julien Rouhaud
On Mon, Mar 14, 2022 at 03:16:50PM +0530, Nitin Jadhav wrote: > > > I am not suggesting > > > removing the existing 'flags' field of pg_stat_progress_checkpoint > > > view and adding a new field 'throttled'. The content of the 'flags' > > > field remains the same. I was suggesting replacing the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-14 Thread Nitin Jadhav
> > I am not suggesting > > removing the existing 'flags' field of pg_stat_progress_checkpoint > > view and adding a new field 'throttled'. The content of the 'flags' > > field remains the same. I was suggesting replacing the 'next_flags' > > field with 'throttled' field since the new request with

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-11 Thread Julien Rouhaud
On Fri, Mar 11, 2022 at 04:59:11PM +0530, Nitin Jadhav wrote: > > That "throttled" flag should be the same as having or not a "force" in the > > flags. We should be consistent and report information the same way, so > > either > > a lot of flags (is_throttled, is_force...) or as now a single

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-11 Thread Nitin Jadhav
> > > > Ok. I agree that it is difficult to interpret it correctly. So even if > > say that a new checkpoint has been explicitly requested, the user may > > not understand that it affects current checkpoint behaviour unless the > > user knows the internals of the checkpoint. How about naming the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-11 Thread Julien Rouhaud
On Fri, Mar 11, 2022 at 02:41:23PM +0530, Nitin Jadhav wrote: > > Ok. I agree that it is difficult to interpret it correctly. So even if > say that a new checkpoint has been explicitly requested, the user may > not understand that it affects current checkpoint behaviour unless the > user knows the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-11 Thread Nitin Jadhav
> > I just wanted to avoid extra calculations just to show the progress in > > the view. Since it's a good metric, I have added an additional field > > named 'next_flags' to the view which holds all possible flag values of > > the next checkpoint. > > I still don't think that's ok. IIUC the only

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-11 Thread Nitin Jadhav
> > The current format matches with the server log message for the > > checkpoint start in LogCheckpointStart(). Just to be consistent, I > > have not changed the code. > > > > See below, how flags are shown in other sql functions like: > > ashu@postgres=# select * from

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-09 Thread Julien Rouhaud
On Tue, Mar 08, 2022 at 08:57:23PM +0530, Nitin Jadhav wrote: > > I just wanted to avoid extra calculations just to show the progress in > the view. Since it's a good metric, I have added an additional field > named 'next_flags' to the view which holds all possible flag values of > the next

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-09 Thread Ashutosh Sharma
On Tue, Mar 8, 2022 at 8:31 PM Nitin Jadhav wrote: > > > > [local]:5432 ashu@postgres=# select * from pg_stat_progress_checkpoint; > > > -[ RECORD 1 ]-+- > > > pid | 22043 > > > type | checkpoint > > > kind |

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-08 Thread Nitin Jadhav
> > > > > As mentioned upthread, there can be multiple backends that request a > > > > > checkpoint, so unless we want to store an array of pid we should > > > > > store a number > > > > > of backend that are waiting for a new checkpoint. > > > > It's a good metric to show in the view but the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-08 Thread Nitin Jadhav
> > [local]:5432 ashu@postgres=# select * from pg_stat_progress_checkpoint; > > -[ RECORD 1 ]-+- > > pid | 22043 > > type | checkpoint > > kind | immediate force wait requested time > > > > I think the output in the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-08 Thread Nitin Jadhav
> > 11) Can you be specific what are those "some operations" that forced a > > checkpoint? May be like, basebackup, createdb or something? > > + The checkpoint is started because some operation forced a > > checkpoint. > > > I will take care in the next patch. I feel mentioning/listing the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-07 Thread Nitin Jadhav
> + > + > + type text > + > + > + Type of checkpoint. See . > + > + > + > + > + > + kind text > + > + > + Kind of checkpoint. See . > + > + > > This looks a bit confusing. Two columns, one with the name

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-07 Thread Nitin Jadhav
> 1) Can we convert below into pgstat_progress_update_multi_param, just > to avoid function calls? > pgstat_progress_update_param(PROGRESS_CHECKPOINT_LSN, checkPoint.redo); > pgstat_progress_update_param(PROGRESS_CHECKPOINT_PHASE, > > 2) Why are we not having special phase for

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-04 Thread Ashutosh Sharma
Please don't mix comments from multiple reviewers into one thread. It's hard to understand which comments are mine or Julien's or from others. Can you please respond to the email from each of us separately with an inline response. That will be helpful to understand your thoughts on our review

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-04 Thread Nitin Jadhav
Thanks for reviewing. > 6) How about shutdown and end-of-recovery checkpoint? Are you planning > to have an ereport_startup_progress mechanism as 0002? I thought of including it earlier then I felt lets first make the current patch stable. Once all the fields are properly decided and the patch

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-03 Thread Julien Rouhaud
On Wed, Mar 2, 2022 at 7:15 PM Nitin Jadhav wrote: > > > > > As mentioned upthread, there can be multiple backends that request a > > > > checkpoint, so unless we want to store an array of pid we should store > > > > a number > > > > of backend that are waiting for a new checkpoint. > > It's a

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-03 Thread Ashutosh Sharma
Here are some of my review comments on the latest patch: + + + type text + + + Type of checkpoint. See . + + + + + + kind text + + + Kind of checkpoint. See . + + This looks a bit confusing. Two columns,

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-02 Thread Bharath Rupireddy
On Wed, Mar 2, 2022 at 4:45 PM Nitin Jadhav wrote: > > Also, how about special phases for SyncPostCheckpoint(), > > SyncPreCheckpoint(), InvalidateObsoleteReplicationSlots(), > > PreallocXlogFiles() (it currently pre-allocates only 1 WAL file, but > > it might be increase in future (?)),

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-02 Thread Nitin Jadhav
Thanks for reviewing. > > > I suggested upthread to store the starting timeline instead. This way > > > you can > > > deduce whether it's a restartpoint or a checkpoint, but you can also > > > deduce > > > other information, like what was the starting WAL. > > > > I don't understand why we

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-03-01 Thread Nitin Jadhav
> > 3) Why do we need this extra calculation for start_lsn? > > Do you ever see a negative LSN or something here? > > +('0/0'::pg_lsn + ( > > +CASE > > +WHEN (s.param3 < 0) THEN pow((2)::numeric, (64)::numeric) > > +ELSE (0)::numeric > > +END +

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-28 Thread Matthias van de Meent
On Sun, 27 Feb 2022 at 16:14, Bharath Rupireddy wrote: > 3) Why do we need this extra calculation for start_lsn? > Do you ever see a negative LSN or something here? > +('0/0'::pg_lsn + ( > +CASE > +WHEN (s.param3 < 0) THEN pow((2)::numeric, (64)::numeric) > +

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-28 Thread Julien Rouhaud
On Mon, Feb 28, 2022 at 06:03:54PM +0530, Bharath Rupireddy wrote: > On Mon, Feb 28, 2022 at 12:02 PM Julien Rouhaud wrote: > > > > I suggested upthread to store the starting timeline instead. This way you > > can > > deduce whether it's a restartpoint or a checkpoint, but you can also deduce >

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-28 Thread Bharath Rupireddy
On Mon, Feb 28, 2022 at 12:02 PM Julien Rouhaud wrote: > > Hi, > > On Mon, Feb 28, 2022 at 10:21:23AM +0530, Bharath Rupireddy wrote: > > > > Another thought for my review comment: > > > 1) Can't we use pg_is_in_recovery to determine if it's a restartpoint > > > or checkpoint instead of having a

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-27 Thread Julien Rouhaud
Hi, On Mon, Feb 28, 2022 at 10:21:23AM +0530, Bharath Rupireddy wrote: > > Another thought for my review comment: > > 1) Can't we use pg_is_in_recovery to determine if it's a restartpoint > > or checkpoint instead of having a new function > > pg_stat_get_progress_checkpoint_type? > > I don't

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-27 Thread Bharath Rupireddy
On Sun, Feb 27, 2022 at 8:44 PM Bharath Rupireddy wrote: > > On Fri, Feb 25, 2022 at 8:38 PM Nitin Jadhav > wrote: > > Had a quick look over the v3 patch. I'm not sure if it's the best way > to have pg_stat_get_progress_checkpoint_type, > pg_stat_get_progress_checkpoint_kind and >

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-27 Thread Bharath Rupireddy
On Fri, Feb 25, 2022 at 8:38 PM Nitin Jadhav wrote: Had a quick look over the v3 patch. I'm not sure if it's the best way to have pg_stat_get_progress_checkpoint_type, pg_stat_get_progress_checkpoint_kind and pg_stat_get_progress_checkpoint_start_time just for printing info in readable format in

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-25 Thread Julien Rouhaud
On Sat, Feb 26, 2022 at 02:30:36AM +0800, Julien Rouhaud wrote: > On Fri, Feb 25, 2022 at 06:49:42PM +0100, Matthias van de Meent wrote: > > > > The point I was trying to make was "If cps->ckpt_flags is > > CHECKPOINT_IMMEDIATE, we hurry up to start the new checkpoint that is > > actually

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-25 Thread Julien Rouhaud
On Fri, Feb 25, 2022 at 06:49:42PM +0100, Matthias van de Meent wrote: > > The point I was trying to make was "If cps->ckpt_flags is > CHECKPOINT_IMMEDIATE, we hurry up to start the new checkpoint that is > actually immediate". That doesn't mean that this checkpoint was > created with IMMEDIATE or

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-25 Thread Matthias van de Meent
On Fri, 25 Feb 2022 at 17:35, Julien Rouhaud wrote: > > On Fri, Feb 25, 2022 at 08:53:50PM +0530, Nitin Jadhav wrote: > > > > > > I'm not sure what Matthias meant, but as far as I know there's no > > > fundamental > > > difference between checkpoint with and without the CHECKPOINT_IMMEDIATE > >

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-25 Thread Julien Rouhaud
On Fri, Feb 25, 2022 at 08:53:50PM +0530, Nitin Jadhav wrote: > > > > I'm not sure what Matthias meant, but as far as I know there's no > > fundamental > > difference between checkpoint with and without the CHECKPOINT_IMMEDIATE > > flag, > > and there's also no scheduling for multiple

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-25 Thread Nitin Jadhav
> > Thank you Alvaro and Matthias for your views. I understand your point > > of not updating the progress-report flag here as it just checks > > whether the CHECKPOINT_IMMEDIATE is set or not and takes an action > > based on that but it doesn't change the checkpoint flags. I will > > modify the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-25 Thread Nitin Jadhav
> + if ((ckpt_flags & > +(CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_END_OF_RECOVERY)) == 0) > + { > > This code (present at multiple places) looks a little ugly to me, what > we can do instead is add a macro probably named IsShutdownCheckpoint() > which does the above check

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-25 Thread Nitin Jadhav
> Thank you Alvaro and Matthias for your views. I understand your point > of not updating the progress-report flag here as it just checks > whether the CHECKPOINT_IMMEDIATE is set or not and takes an action > based on that but it doesn't change the checkpoint flags. I will > modify the code but I

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-24 Thread Julien Rouhaud
Hi, On Fri, Feb 25, 2022 at 12:23:27AM +0530, Nitin Jadhav wrote: > > I think the change to ImmediateCheckpointRequested() makes no sense. > > Before this patch, that function merely inquires whether there's an > > immediate checkpoint queued. After this patch, it ... changes a > >

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-24 Thread Nitin Jadhav
> I think the change to ImmediateCheckpointRequested() makes no sense. > Before this patch, that function merely inquires whether there's an > immediate checkpoint queued. After this patch, it ... changes a > progress-reporting flag? I think it would make more sense to make the > progress-report

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-24 Thread Matthias van de Meent
On Wed, 23 Feb 2022 at 14:28, Nitin Jadhav wrote: > > Sharing the v2 patch. Kindly have a look and share your comments. Thanks for updating. > diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml With the new pg_stat_progress_checkpoint, you should also add a backreference

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-23 Thread Matthias van de Meent
On Wed, 23 Feb 2022 at 15:24, Nitin Jadhav wrote: > > > At least for pg_stat_progress_checkpoint, storing only a timestamp in > > the pg_stat storage (instead of repeatedly updating the field as a > > duration) seems to provide much more precise measures of 'time > > elapsed' for other sessions

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-23 Thread Justin Pryzby
+ Whenever the checkpoint operation is running, the + pg_stat_progress_checkpoint view will contain a + single row indicating the progress of the checkpoint. The tables below Maybe it should show a single row , unless the checkpointer isn't running at all (like in single user mode). +

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-23 Thread Alvaro Herrera
I think the change to ImmediateCheckpointRequested() makes no sense. Before this patch, that function merely inquires whether there's an immediate checkpoint queued. After this patch, it ... changes a progress-reporting flag? I think it would make more sense to make the progress-report flag

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-23 Thread Ashutosh Sharma
+ if ((ckpt_flags & +(CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_END_OF_RECOVERY)) == 0) + { This code (present at multiple places) looks a little ugly to me, what we can do instead is add a macro probably named IsShutdownCheckpoint() which does the above check and use it in

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-23 Thread Nitin Jadhav
> At least for pg_stat_progress_checkpoint, storing only a timestamp in > the pg_stat storage (instead of repeatedly updating the field as a > duration) seems to provide much more precise measures of 'time > elapsed' for other sessions if one step of the checkpoint is taking a > long time. I am

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-23 Thread Nitin Jadhav
> On what basis have you classified the above into the various types of > checkpoints? AFAIK, the first two types are based on what triggered > the checkpoint (whether it was the checkpoint_timeout or maz_wal_size > settings) while the third type indicates the force checkpoint that can > happen

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-23 Thread Nitin Jadhav
> I will make use of pgstat_progress_update_multi_param() in the next > patch to replace multiple calls to checkpoint_progress_update_param(). Fixed. --- > > The other progress tables use [type]_total as column names for counter > > targets (e.g. backup_total for backup_streamed, heap_blks_total

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-22 Thread Matthias van de Meent
On Tue, 22 Feb 2022 at 07:39, Nitin Jadhav wrote: > > > > Thank you for sharing the information. 'triggering backend PID' (int) > > > - can be stored without any problem. 'checkpoint or restartpoint?' > > > (boolean) - can be stored as a integer value like > > >

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-22 Thread Ashutosh Sharma
+/* Kinds of checkpoint (as advertised via PROGRESS_CHECKPOINT_KIND) */ +#define PROGRESS_CHECKPOINT_KIND_WAL0 +#define PROGRESS_CHECKPOINT_KIND_TIME 1 +#define PROGRESS_CHECKPOINT_KIND_FORCE 2 +#define PROGRESS_CHECKPOINT_KIND_UNKNOWN3 On

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-21 Thread Nitin Jadhav
> > Thank you for sharing the information. 'triggering backend PID' (int) > > - can be stored without any problem. 'checkpoint or restartpoint?' > > (boolean) - can be stored as a integer value like > > PROGRESS_CHECKPOINT_TYPE_CHECKPOINT(0) and > > PROGRESS_CHECKPOINT_TYPE_RESTARTPOINT(1).

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-18 Thread Julien Rouhaud
Hi, On Fri, Feb 18, 2022 at 08:07:05PM +0530, Nitin Jadhav wrote: > > The backend_pid contains a valid value only during > the CHECKPOINT command issued by the backend explicitly, otherwise the > value will be 0. We may have to add an additional field to > 'CheckpointerShmemStruct' to hold the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-18 Thread Nitin Jadhav
> > Okay. I feel this can be added as additional field but it will not > > replace backend_pid field as this represents the pid of the backend > > which triggered the current checkpoint. > > I don't think that's true. Requesting a checkpoint means telling the > checkpointer that it should wake up

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-17 Thread Julien Rouhaud
Hi, On Fri, Feb 18, 2022 at 12:20:26PM +0530, Nitin Jadhav wrote: > > > > If there's a checkpoint timed triggered and then someone calls > > pg_start_backup() which then wait for the end of the current checkpoint > > (possibly after changing the flags), I think the view should reflect that in > >

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-17 Thread Nitin Jadhav
> > > > Thank you for sharing the information. 'triggering backend PID' (int) > > > > - can be stored without any problem. > > > > > > There can be multiple processes triggering a checkpoint, or at least > > > wanting it > > > to happen or happen faster. > > > > Yes. There can be multiple

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-17 Thread Nitin Jadhav
> Interesting idea, and overall a nice addition to the > pg_stat_progress_* reporting infrastructure. > > Could you add your patch to the current commitfest at > https://commitfest.postgresql.org/37/? > > See below for some comments on the patch: Thanks you for reviewing. I have added it to the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-17 Thread Julien Rouhaud
Hi, On Thu, Feb 17, 2022 at 10:39:02PM +0530, Nitin Jadhav wrote: > > > Thank you for sharing the information. 'triggering backend PID' (int) > > > - can be stored without any problem. > > > > There can be multiple processes triggering a checkpoint, or at least > > wanting it > > to happen or

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-17 Thread Nitin Jadhav
> > Thank you for sharing the information. 'triggering backend PID' (int) > > - can be stored without any problem. > > There can be multiple processes triggering a checkpoint, or at least wanting > it > to happen or happen faster. Yes. There can be multiple processes but there will be one

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-17 Thread Matthias van de Meent
On Thu, 17 Feb 2022 at 07:56, Nitin Jadhav wrote: > > > Progress parameters are int64, so all of the new 'checkpoint start > > location' (lsn = uint64), 'triggering backend PID' (int), 'elapsed > > time' (store as start time in stat_progress, timestamp fits in 64 > > bits) and 'checkpoint or

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-17 Thread Julien Rouhaud
Hi, On Thu, Feb 17, 2022 at 12:26:07PM +0530, Nitin Jadhav wrote: > > Thank you for sharing the information. 'triggering backend PID' (int) > - can be stored without any problem. There can be multiple processes triggering a checkpoint, or at least wanting it to happen or happen faster. >

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-16 Thread Nitin Jadhav
> > The progress reporting mechanism of postgres uses the > > 'st_progress_param' array of 'PgBackendStatus' structure to hold the > > information related to the progress. There is a function > > 'pgstat_progress_update_param()' which takes 'index' and 'val' as > > arguments and updates the 'val'

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-16 Thread Matthias van de Meent
On Thu, 10 Feb 2022 at 07:53, Nitin Jadhav wrote: > > > > We need at least a trace of the number of buffers to sync (num_to_scan) > > > before the checkpoint start, instead of just emitting the stats at the > > > end. > > > > > > Bharat, it would be good to show the buffers synced counter and

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-16 Thread Matthias van de Meent
On Tue, 15 Feb 2022 at 13:16, Nitin Jadhav wrote: > > > Apart from above fields, I am planning to add few more fields to the > > view in the next patch. That is, process ID of the backend process > > which triggered a CHECKPOINT command, checkpoint start location, filed > > to indicate whether it

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-15 Thread Nitin Jadhav
> Apart from above fields, I am planning to add few more fields to the > view in the next patch. That is, process ID of the backend process > which triggered a CHECKPOINT command, checkpoint start location, filed > to indicate whether it is a checkpoint or restartpoint and elapsed > time of the

Re: Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-02-09 Thread Nitin Jadhav
> > We need at least a trace of the number of buffers to sync (num_to_scan) > > before the checkpoint start, instead of just emitting the stats at the end. > > > > Bharat, it would be good to show the buffers synced counter and the total > > buffers to sync, checkpointer pid, substep it is

Report checkpoint progress with pg_stat_progress_checkpoint (was: Report checkpoint progress in server logs)

2022-01-27 Thread Bharath Rupireddy
On Fri, Jan 21, 2022 at 11:07 AM Nitin Jadhav wrote: > > > I think the right choice to solve the *general* problem is the > > mentioned pg_stat_progress_checkpoints. > > > > We may want to *additionally* have the ability to log the progress > > specifically for the special cases when we're not