On Sat, Mar 12, 2022 at 1:35 AM Nathan Bossart <nathandbossart@gmail.com> 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 to do it there?
>
> + "logical rewrite mapping file(s) removed/synced=" UINT64_FORMAT ", size=%zu bytes,
time=%ld.%03ds, cutoff LSN=%X/%X",
>
> Can we split out the "removed" versus "synced" files? Otherwise, I think
> this is basically just telling us how many files are in the mappings
> directory.
>
> + CheckpointStats.repl_snap_cutoff_lsn = cutoff;
>
> I have the same note for this one as I have for repl_map_cutoff_lsn.
Thanks for reviewing this. I agree with all of the above suggestions
and incorporated them in the v2 patch.
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 doesn't make sense to emit the stats there and cause
server log to grow unnecessarily. Having said that, I added a note
there to change it whenever logical decoding on standbys is supported.
Please review v2.
Regards,
Bharath Rupireddy.