Re: Use read streams in pg_visibility

On Mon, Sep 09, 2024 at 06:25:07PM +0300, Nazir Bilal Yavuz wrote:
> On Thu, 5 Sept 2024 at 18:54, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Thu, Sep 05, 2024 at 03:59:53PM +0300, Nazir Bilal Yavuz wrote:
> > > On Wed, 4 Sept 2024 at 21:43, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > > > https://postgr.es/m/CAEudQAozv3wTY5TV2t29JcwPydbmKbiWQkZD42S2OgzdixPMDQ@mail.gmail.com
> > > > then observed that collect_corrupt_items() was now guaranteed to never detect
> > > > corruption. I have pushed revert ddfc556 of the pg_visibility.c changes. For
> > > > the next try, could you add that testing we discussed?
> > >
> > > Do you think that corrupting the visibility map and then seeing if
> > > pg_check_visible() and pg_check_frozen() report something is enough?
> >
> > I think so. Please check that it would have caught both the blkno bug and the
> > above bug.
>
> The test and updated patch files are attached. In that test I
> overwrite the visibility map file with its older state. Something like
> this:
>
> 1- Create the table, then run VACUUM FREEZE on the table.
> 2- Shutdown the server, create a copy of the vm file, restart the server.
> 3- Run the DELETE command on some $random_tuples.
> 4- Shutdown the server, overwrite the vm file with the #2 vm file,
> restart the server.
> 5- pg_check_visible and pg_check_frozen must report $random_tuples as corrupted.

Copying the vm file is a good technique. In my test runs, this does catch the
"never detect" bug, but it doesn't catch the blkno bug. Can you make it catch
both? It's possible my hand-patching to recreate the blkno bug is what went
wrong, so I'm attaching what I used. It consists of
v1-0002-Use-read-stream-in-pg_visibility-in-collect_corru.patch plus these
fixes for the "never detect" bug from your v6-0002:

--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -724,4 +724,4 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
{
- bool check_frozen = false;
- bool check_visible = false;
+ bool check_frozen = all_frozen;
+ bool check_visible = all_visible;
Buffer buffer;
@@ -757,5 +757,5 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
*/
- if (all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer))
+ if (check_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer))
check_frozen = false;
- if (all_visible && !VM_ALL_VISIBLE(rel, blkno, &vmbuffer))
+ if (check_visible && !VM_ALL_VISIBLE(rel, blkno, &vmbuffer))
check_visible = false;