Re: Differential code coverage between 16 and HEAD

On Tue, 16 Apr 2024 at 10:57, Andres Freund <andres(at)anarazel(dot)de> wrote:
> I guess was thinking more about BumpIsEmpty() and BumpStats() then the "bogus"
> cases. But BumpIsEmpty() likely is unreachable as well.

The only call to MemoryContextIsEmpty() I see is AtSubCommit_Memory()
and it's on an aset.c context type. I see generation.c has the same
issue per [1].

> BumpStats() is
> reachable, but perhaps it's not worth it?
>
> BEGIN;
> DECLARE foo CURSOR FOR SELECT LEFT(a,10),b FROM (VALUES(REPEAT('a', 512 * 1024),1),(REPEAT('b', 512 * 1024),2)) v(a,b) ORDER BY v.a DESC;
> FETCH 1 FROM foo;
> SELECT * FROM pg_backend_memory_contexts WHERE name = 'Caller tuples';

I think primarily it's good to exercise that code just to make sure it
does not crash. Looking at the output of the above on my machine:

name | ident | parent | level | total_bytes |
total_nblocks | free_bytes | free_chunks | used_bytes
---------------+-------+----------------+-------+-------------+---------------+------------+-------------+------------
Caller tuples | | TupleSort sort | 6 | 1056848 |
3 | 8040 | 0 | 1048808
(1 row)

I'd say:

Name: stable
ident: stable
parent: stable
level: could change from a refactor of code
total_bytes: could be different on other platforms or dependent on
MEMORY_CONTEXT_CHECKING
total_nblocks: stable enough
free_bytes: could be different on other platforms or dependent on
MEMORY_CONTEXT_CHECKING
free_chunks: always 0
used_bytes: could be different on other platforms or dependent on
MEMORY_CONTEXT_CHECKING

I've attached a patch which includes your test with unstable columns
stripped out.

I cut the 2nd row down to just 512 bytes as I didn't see the need to
add two large datums. Annoyingly it still uses 3 blocks as I've opted
to do dlist_push_head(&set->blocks, &block->node); in BumpAllocLarge()
which is the block that's picked up again in BumpAlloc() per block =
dlist_container(BumpBlock, node, dlist_head_node(&set->blocks));
wonder if the large blocks should push tail instead.

> Hm, independent of this, seems a bit odd that we don't include the memory
> context type in pg_backend_memory_contexts?

That seems like useful information to include. It sure would be
useful to have in there to verify that I'm testing BumpStats(). I've
written a patch [2].

David

[1] https://coverage.postgresql.org/src/backend/utils/mmgr/generation.c.gcov.html#997
[2] https://postgr.es/m/CAApHDvrXX1OR09Zjb5TnB0AwCKze9exZN=9Nxxg1ZCVV8W-3BA@mail.gmail.com