Re: Differential code coverage between 16 and HEAD

On Tue, 16 Apr 2024 at 10:57, Andres Freund <andres@anarazel.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