Hi!
in v27-0001:
> Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
> > The last vacuum is expected to set vm bits, but the test doesn’t verify
> > that. Should we verify that like:
> > ```
> > evantest=# SELECT blkno, all_visible, all_frozen FROM
> > pg_visibility_map('test_vac_unmodified_heap');
> > blkno | all_visible | all_frozen
> > -------+-------------+------------
> > 0 | t | t
> > (1 row)
> I've done this. I've actually added three such verifications -- one
> after each step where the VM is expected to change. It shouldn't be
> very expensive, so I think it is okay. The way the test would fail if
> the buffer wasn't correctly dirtied is that it would assert out -- so
> the visibility map test wouldn't even have a chance to fail. But, I
> think it is also okay to confirm that the expected things are
> happening with the VM -- it just gives us extra coverage.
+1 on extra coverage. Should we also do sql-level check that the VM
indeed does not need to set PD_ALL_VISIBLE (check header bytes using
pageinspect?).
v27-0003 & v27-0004: I did not get the exact reason we introduced
`identify_and_fix_vm_corruption` in 0003 and moved code in 0004 to
another place. I can see we have this starting v25 of patch set. Well,
maybe this is not an issue at all...
in v27-0005. This patch changes code which is not exercised in
tests[0]. I spent some time understanding the conditions when we
entered this. There is a comment about non-finished relation
extension, but I got no success trying to reproduce this. I ended up
modifying code to lose PageSetAllVisible in proper places and running
vacuum. Looks like everything works as expected. I will spend some
more time on this, maybe I will be successful in writing an
injection-point-based TAP test which hits this...
[0]
https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html#1902
--
Best regards,
Kirill Reshke
