Hi,
On 2024-04-15 15:36:04 -0400, Robert Haas wrote:
> On Sun, Apr 14, 2024 at 6:33 PM Andres Freund <andres@anarazel.de> wrote:
> > - Some of the new walsummary code could use more tests.
> > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/backup/walsummaryfuncs.c.gcov.html#L69
>
> So this is pg_wal_summary_contents() and
> pg_get_wal_summarizer_state(). I was reluctant to try to cover these
> because I thought it would be hard to get the tests to be stable. The
> difficulties in stabilizing src/bin/pg_walsummary/t/002_blocks.pl seem
> to demonstrate that this concern wasn't entire unfounded, but as far
> as I know that test is now stable, so we could probably use the same
> technique to test pg_wal_summary_contents(), maybe even as part of the
> same test case. I don't really know what a good test for
> pg_get_wal_summarizer_state() would look like, though.
I think even just reaching the code, without a lot of of verification of the
returned data, is better than not reaching the code at all. I.e. the test
could just check that the pid is set, the tli is right.
That'd also add at least some coverage of
https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/postmaster/walsummarizer.c.gcov.html#L433
> >
https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/bin/pg_combinebackup/pg_combinebackup.c.gcov.html#L424
>
> I guess we could test this by adding a tablespace, and a tablespace
> mapping, to one of the pg_combinebackup tests.
Seems worthwhile to me.
> > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/common/blkreftable.c.gcov.html#L790
>
> This is dead code. I thought we might need to use this as a way of
> managing memory pressure, but it didn't end up being needed. We could
> remove it, or mark it #if NOT_USED, or whatever.
Don't really have an opinion on that. How likely do you think we'll need it
going forward?
Note that I didn't look exhaustively through the coverage of the walsummarizer
code - I just looked at a few things that stood out. I looked for a few
minutes more:
- It seems worth explicitly covering the various
record types that walsummarizer needs to understand:
https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/postmaster/walsummarizer.c.gcov.html#L1184
i.e. XLOG_SMGR_TRUNCATE, XLOG_XACT_COMMIT_PREPARED, XLOG_XACT_ABORT, XLOG_XACT_ABORT_PREPARED.
- Another thing that looks to be not covered is dealing with
enabling/disabling summarize_wal, that also seems worth testing?
Greetings,
Andres Freund