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

On Thu, Mar 17, 2022 at 12:12 AM Nathan Bossart
<nathandbossart(at)gmail(dot)com> 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
> > <nathandbossart(at)gmail(dot)com> 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 actually adds even more bloat to the server log. If we are
> >> concerned about the amount of information in these LOGs, perhaps we should
> >> adjust the format to make it more human-readable.
> >
> > Below are the ways that I can think of. Thoughts?
>
> I think I prefer the first option. If these tasks don't do anything, we
> leave the stats out of the message. If they do some work, we add them.

Thanks Nathan.

> Below are the ways that I can think of. Thoughts?
>
> 1)
> if (restartpoint)
> {
> if (snap_files_rmvd > 0 && map_files_rmvd > 0)
> existing "restartpoint complete" message + "snapshot files
> removed cnt, time, cutoff lsn" + "mapping files removed cnt, time,
> cutoff lsn"
> else if (snap_files_rmvd > 0)
> existing "restartpoint complete" message + "snapshot files
> removed cnt, time, cutoff lsn"
> else if (map_files_rmvd > 0)
> existing "restartpoint complete" message + "mapping files removed
> cnt, time, cutoff lsn"
> else
> existing "restartpoint complete" message
> }
> else
> {
> same as above but with "checkpoint complete" instead of "restart complete"
> }
>
> 2) Use StringInfoData to prepare the message dynamically but this will
> create message translation problems.
>
> 3) Emit the "snapshot files removed cnt, time, cutoff lsn" and
> "mapping files removed cnt, time, cutoff lsn" at LOG level if
> (log_checkpoints) in CheckPointSnapBuild and
> CheckPointLogicalRewriteHeap respectively.
>
> 4) Have separate log messages in LogCheckpointEnd:
> if (snap_files_rmvd > 0)
> "snapshot files removed cnt, time, cutoff lsn"
> if (map_files_rmvd > 0)
> "mapping files removed cnt, time, cutoff lsn"
>
> if (restartpoint)
> existing "restartpoint complete" message
> else
> existing "checkpoint complete" message

FWIW, I'm attaching patches as follows:
v4-0001...patch implements option (4)
v4-0002...txt implements option (3)
v4-0003...txt implements option (1)

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 maintainability problems. Having said that, I will leave
it to the committer to decide on that.

Regards,
Bharath Rupireddy.