Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-09-27 Thread Bharath Rupireddy
On Mon, Sep 26, 2022 at 3:21 AM Justin Pryzby wrote: > > > This patch needs an update. It is failing on Windows for the test case > > "33/238 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade with an > > "exit status 2" error. > > > > The details are available on the cfbot.cputue.org. > >

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-09-25 Thread Justin Pryzby
On Sun, Sep 25, 2022 at 04:50:54PM -0400, Hamid Akhtar wrote: > On Sun, 28 Aug 2022 at 17:30, Nathan Bossart wrote: > > > On Thu, Aug 25, 2022 at 10:12:00AM +0530, Bharath Rupireddy wrote: > > > Please review the v12 patch attached. > > > > LGTM. I've marked this as ready-for-committer. > > >

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-09-25 Thread Hamid Akhtar
On Sun, 28 Aug 2022 at 17:30, Nathan Bossart wrote: > On Thu, Aug 25, 2022 at 10:12:00AM +0530, Bharath Rupireddy wrote: > > Please review the v12 patch attached. > > LGTM. I've marked this as ready-for-committer. > This patch needs an update. It is failing on Windows for the test case "33/238

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-08-28 Thread Nathan Bossart
On Thu, Aug 25, 2022 at 10:12:00AM +0530, Bharath Rupireddy wrote: > Please review the v12 patch attached. LGTM. I've marked this as ready-for-committer. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-08-25 Thread Kyotaro Horiguchi
At Wed, 24 Aug 2022 13:34:54 +0900, Michael Paquier wrote in > On Wed, Aug 24, 2022 at 10:48:01AM +0900, Kyotaro Horiguchi wrote: > > By the way, I think we use INSTR_TIME_* macros to do masure internal > > durations (mainly for the monotonic clock characteristics, and to > > reduce performance

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-08-24 Thread Bharath Rupireddy
On Tue, Aug 23, 2022 at 11:19 PM Nathan Bossart wrote: > > On Wed, Aug 17, 2022 at 11:17:24AM +0530, Bharath Rupireddy wrote: > > + "logical decoding file(s) > > processing time=%ld.%03d s", > > I would suggest shortening this to something like

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-08-23 Thread Michael Paquier
On Wed, Aug 24, 2022 at 10:48:01AM +0900, Kyotaro Horiguchi wrote: > By the way, I think we use INSTR_TIME_* macros to do masure internal > durations (mainly for the monotonic clock characteristics, and to > reduce performance degradation on Windows?). I'm not sure that's > crutial here but I

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-08-23 Thread Kyotaro Horiguchi
At Tue, 23 Aug 2022 10:49:40 -0700, Nathan Bossart wrote in > On Wed, Aug 17, 2022 at 11:17:24AM +0530, Bharath Rupireddy wrote: > > + "logical decoding file(s) > > processing time=%ld.%03d s", > > I would suggest shortening this to something like

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-08-23 Thread Nathan Bossart
On Wed, Aug 17, 2022 at 11:17:24AM +0530, Bharath Rupireddy wrote: > + "logical decoding file(s) > processing time=%ld.%03d s", I would suggest shortening this to something like "logical decoding processing" or "logical replication processing." >

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-08-16 Thread Bharath Rupireddy
On Wed, Aug 17, 2022 at 2:52 AM Nathan Bossart wrote: > > On Tue, Jul 19, 2022 at 05:29:10PM +0530, Bharath Rupireddy wrote: > > I've also added the total number of WAL files a checkpoint has > > processed (scanned the pg_wal directory) while removing old WAL files. > > This helps to estimate the

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-08-16 Thread Nathan Bossart
On Tue, Jul 19, 2022 at 05:29:10PM +0530, Bharath Rupireddy wrote: > I've also added the total number of WAL files a checkpoint has > processed (scanned the pg_wal directory) while removing old WAL files. > This helps to estimate the pg_wal disk space at the time of a > particular checkpoint,

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-07-19 Thread Bharath Rupireddy
On Tue, Apr 26, 2022 at 6:31 AM Michael Paquier wrote: > > On Mon, Apr 25, 2022 at 01:34:38PM -0700, Nathan Bossart wrote: > > I took another look at the example output, and I think I agree that logging > > the total time for logical decoding operations is probably the best path > > forward.

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-04-25 Thread Michael Paquier
On Mon, Apr 25, 2022 at 01:34:38PM -0700, Nathan Bossart wrote: > I took another look at the example output, and I think I agree that logging > the total time for logical decoding operations is probably the best path > forward. This information would be enough to clue an administrator into > the

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-04-25 Thread Nathan Bossart
On Thu, Mar 24, 2022 at 03:22:11PM +0530, Bharath Rupireddy wrote: >> > > > > Both seem still very long. I still am doubtful this level of detail >> > > > > is >> > > > > appropriate. Seems more like a thing for a tracepoint or such. How >> > > > > about just >> > > > > printing the time for the

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-24 Thread Bharath Rupireddy
On Thu, Mar 24, 2022 at 8:58 AM Bharath Rupireddy wrote: > > On Wed, Mar 23, 2022 at 10:16 AM Bharath Rupireddy > wrote: > > > > On Tue, Mar 22, 2022 at 8:12 PM Andres Freund wrote: > > > > Do you mean like this? > > > > ereport(LOG, > > > > /* translator: the placeholders show

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-23 Thread Bharath Rupireddy
On Wed, Mar 23, 2022 at 10:16 AM Bharath Rupireddy wrote: > > On Tue, Mar 22, 2022 at 8:12 PM Andres Freund wrote: > > > Do you mean like this? > > > ereport(LOG, > > > /* translator: the placeholders show checkpoint options */ > > > (errmsg("%s starting:%s%s%s%s%s%s%s%s", >

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-22 Thread Bharath Rupireddy
On Tue, Mar 22, 2022 at 8:12 PM Andres Freund wrote: > > Do you mean like this? > > ereport(LOG, > > /* translator: the placeholders show checkpoint options */ > > (errmsg("%s starting:%s%s%s%s%s%s%s%s", > > restartpoint ? _("restartpoint") :

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-22 Thread Andres Freund
Hi, On 2022-03-22 16:32:24 +0530, Bharath Rupireddy wrote: > On Tue, Mar 22, 2022 at 12:20 AM Andres Freund wrote: > > > Something like attached > > > v6-0001-Deduplicate-checkpoint-restartpoint-complete-mess.patch? > > > > Mostly. I don't see a reason for the use of the stringinfo. And I think

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-22 Thread Bharath Rupireddy
On Tue, Mar 22, 2022 at 12:20 AM Andres Freund wrote: > > Something like attached > > v6-0001-Deduplicate-checkpoint-restartpoint-complete-mess.patch? > > Mostly. I don't see a reason for the use of the stringinfo. And I think > LogCheckpointStart() should be dealt with similarly. > > I'd just

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-21 Thread Nathan Bossart
On Mon, Mar 21, 2022 at 11:27:15AM +0530, Bharath Rupireddy wrote: > On Sat, Mar 19, 2022 at 3:16 AM Nathan Bossart > wrote: >> /* buffer stats */ >> appendStringInfo(, "wrote %d buffers (%.1f%%); ", >> >>

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-21 Thread Andres Freund
Hi, On 2022-03-21 11:27:15 +0530, Bharath Rupireddy wrote: > On Sat, Mar 19, 2022 at 4:39 AM Andres Freund wrote: > > > > before we further change this (as done in this patch) we should deduplicate > > these huge statements in a separate patch / commit. > > Something like attached >

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-20 Thread Bharath Rupireddy
On Sat, Mar 19, 2022 at 4:39 AM Andres Freund wrote: > > before we further change this (as done in this patch) we should deduplicate > these huge statements in a separate patch / commit. Something like attached v6-0001-Deduplicate-checkpoint-restartpoint-complete-mess.patch? > As discussed in a

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-18 Thread Andres Freund
Hi, On 2022-03-18 13:32:52 +0530, Bharath Rupireddy wrote: > @@ -6115,46 +6116,81 @@ LogCheckpointEnd(bool restartpoint) > CheckpointStats.ckpt_sync_rels; > average_msecs = (long) ((average_sync_time + 999) / 1000); > > + initStringInfo(); > + > if

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-18 Thread Nathan Bossart
On Fri, Mar 18, 2022 at 01:32:52PM +0530, Bharath Rupireddy wrote: > Well, here's the v5 patch, sample output at [1]. Please have a look at it. I think this is on the right track, but I would even take it a step further by separating each group of information: if (restartpoint)

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-18 Thread Bharath Rupireddy
On Fri, Mar 18, 2022 at 11:11 AM Bharath Rupireddy wrote: > > On Fri, Mar 18, 2022 at 2:15 AM Nathan Bossart > wrote: > > > > On Thu, Mar 17, 2022 at 06:48:43PM +0530, Bharath Rupireddy wrote: > > > Personally, I tend to agree with v4-0001 (option (4)) or v4-0002 > > > (option (3)) than v4-0003

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-17 Thread Bharath Rupireddy
On Fri, Mar 18, 2022 at 2:15 AM Nathan Bossart wrote: > > On Thu, Mar 17, 2022 at 06:48:43PM +0530, Bharath Rupireddy wrote: > > Personally, I tend to agree with v4-0001 (option (4)) or v4-0002 > > (option (3)) than v4-0003 (option (1)) as it adds more unreadability > > with most of the code

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-17 Thread Nathan Bossart
On Thu, Mar 17, 2022 at 06:48:43PM +0530, Bharath Rupireddy wrote: > Personally, I tend to agree with v4-0001 (option (4)) or v4-0002 > (option (3)) than v4-0003 (option (1)) as it adds more unreadability > with most of the code duplicated creating more diff with previous > versions and

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-17 Thread Bharath Rupireddy
On Thu, Mar 17, 2022 at 12:12 AM Nathan Bossart wrote: > > On Wed, Mar 16, 2022 at 03:02:41PM +0530, Bharath Rupireddy wrote: > > On Mon, Mar 14, 2022 at 10:34 PM Nathan Bossart > > wrote: > >> I'm -1 on splitting these new statistics to separate LOGs. In addition to > >> making it more

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-16 Thread Nathan Bossart
On Wed, Mar 16, 2022 at 03:02:41PM +0530, Bharath Rupireddy wrote: > On Mon, Mar 14, 2022 at 10:34 PM Nathan Bossart > wrote: >> I'm -1 on splitting these new statistics to separate LOGs. In addition to >> making it more difficult to discover statistics for a given checkpoint, I >> think it

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-16 Thread Bharath Rupireddy
On Mon, Mar 14, 2022 at 10:34 PM Nathan Bossart wrote: > > > Attaching v3 patch which emits logs only when necessary and doesn't > > clutter the existing "checkpoint/restartpoint completed" message, see > > some sample logs at [1]. Please review it further. > > I'm okay with not adding these

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-16 Thread Bharath Rupireddy
On Wed, Mar 16, 2022 at 1:47 AM Nathan Bossart wrote: > > On Tue, Mar 15, 2022 at 11:04:26AM +0900, Michael Paquier wrote: > > On Mon, Mar 14, 2022 at 03:54:19PM +0530, Bharath Rupireddy wrote: > >> At times, the snapshot or mapping files can be large in number and one > >> some platforms it

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-15 Thread Nathan Bossart
On Tue, Mar 15, 2022 at 11:04:26AM +0900, Michael Paquier wrote: > On Mon, Mar 14, 2022 at 03:54:19PM +0530, Bharath Rupireddy wrote: >> At times, the snapshot or mapping files can be large in number and one >> some platforms it takes time for checkpoint to process all of them. >> Having the stats

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-14 Thread Michael Paquier
On Mon, Mar 14, 2022 at 03:54:19PM +0530, Bharath Rupireddy wrote: > At times, the snapshot or mapping files can be large in number and one > some platforms it takes time for checkpoint to process all of them. > Having the stats about them in server logs can help us better analyze > why checkpoint

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-14 Thread Nathan Bossart
On Mon, Mar 14, 2022 at 05:03:59PM +0530, Bharath Rupireddy wrote: > On Mon, Mar 14, 2022 at 1:33 PM Michael Paquier wrote: >> On Mon, Mar 14, 2022 at 10:54:56AM +0530, Bharath Rupireddy wrote: >> > Yes, this is a concern. Also, when there were no logical replication >> > slots on a plain server

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-14 Thread Bharath Rupireddy
On Mon, Mar 14, 2022 at 1:33 PM Michael Paquier wrote: > > On Mon, Mar 14, 2022 at 10:54:56AM +0530, Bharath Rupireddy wrote: > > Yes, this is a concern. Also, when there were no logical replication > > slots on a plain server or the server removed or cleaned up all the > > snapshot/mappings

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-14 Thread Bharath Rupireddy
On Mon, Mar 14, 2022 at 1:33 PM Michael Paquier wrote: > > On Mon, Mar 14, 2022 at 10:54:56AM +0530, Bharath Rupireddy wrote: > > Yes, this is a concern. Also, when there were no logical replication > > slots on a plain server or the server removed or cleaned up all the > > snapshot/mappings

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-14 Thread Michael Paquier
On Mon, Mar 14, 2022 at 10:54:56AM +0530, Bharath Rupireddy wrote: > Yes, this is a concern. Also, when there were no logical replication > slots on a plain server or the server removed or cleaned up all the > snapshot/mappings files, why would anyone want to have these messages > with all 0s in

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-13 Thread Bharath Rupireddy
On Mon, Mar 14, 2022 at 10:45 AM Michael Paquier wrote: > > On Sun, Mar 13, 2022 at 02:58:58PM -0700, Nathan Bossart wrote: > > On Sun, Mar 13, 2022 at 01:54:10PM +0530, Bharath Rupireddy wrote: > >> Another thing I added in v2 is to not emit snapshot and mapping files > >> stats in case of

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-13 Thread Michael Paquier
On Sun, Mar 13, 2022 at 02:58:58PM -0700, Nathan Bossart wrote: > On Sun, Mar 13, 2022 at 01:54:10PM +0530, Bharath Rupireddy wrote: >> Another thing I added in v2 is to not emit snapshot and mapping files >> stats in case of restartpoint as logical decoding isn't supported on >> standbys, so it

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-13 Thread Nathan Bossart
On Sun, Mar 13, 2022 at 01:54:10PM +0530, Bharath Rupireddy wrote: > Thanks for reviewing this. I agree with all of the above suggestions > and incorporated them in the v2 patch. Thanks for the new patch. > Another thing I added in v2 is to not emit snapshot and mapping files > stats in case of

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-13 Thread Bharath Rupireddy
On Sat, Mar 12, 2022 at 1:35 AM Nathan Bossart wrote: > > +CheckpointStats.repl_map_cutoff_lsn = cutoff; > > Could we set repl_map_cutoff_lsn closer to where it is calculated? Right > now, it's set at the bottom of the function just before the directory is > freed. Is there a strong reason

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2022-03-11 Thread Nathan Bossart
+CheckpointStats.repl_map_cutoff_lsn = cutoff; Could we set repl_map_cutoff_lsn closer to where it is calculated? Right now, it's set at the bottom of the function just before the directory is freed. Is there a strong reason to do it there? +"logical rewrite mapping

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2021-12-30 Thread Bharath Rupireddy
On Tue, Nov 30, 2021 at 6:24 AM Cary Huang wrote: > > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation:not tested >

Re: add checkpoint stats of snapshot and mapping files of pg_logical dir

2021-11-29 Thread Cary Huang
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:not tested Hi The patch applies and tests fine. I don't think there is any

add checkpoint stats of snapshot and mapping files of pg_logical dir

2021-10-29 Thread Bharath Rupireddy
Hi, At times, there can be many snapshot and mapping files under pg_logical dir that the checkpoint might have to delete/fsync based on the cutoff LSN which can increase the checkpoint time. The user/admin/developer might want to get an answer for "what's the total time spent in deleting these