Re: Minimal logical decoding on standbys

Hi,

On 2023-03-31 12:45:51 +0200, Drouvot, Bertrand wrote:
> On 3/31/23 6:33 AM, Andres Freund wrote:
> > Hi,
> >
> > On 2023-03-30 18:23:41 +0200, Drouvot, Bertrand wrote:
> > > On 3/30/23 9:04 AM, Andres Freund wrote:
> > > > I think this commit is ready to go. Unless somebody thinks differently, I
> > > > think I might push it tomorrow.
> > >
> > > Great! Once done, I'll submit a new patch so that GlobalVisTestFor() can make
> > > use of the heap relation in vacuumRedirectAndPlaceholder() (which will be possible
> > > once 0001 is committed).
> >
> > Unfortunately I did find an issue doing a pre-commit review of the patch.
> >
> > The patch adds VISIBILITYMAP_IS_CATALOG_REL to xl_heap_visible.flags - but it
> > does not remove the bit before calling visibilitymap_set().
> >
> > This ends up corrupting the visibilitymap, because the we'll set a bit for
> > another page.
> >
>
> Oh I see, I did not think about that (not enough experience in the VM area).
> Nice catch and thanks for pointing out!

I pushed a commit just adding an assertion that only valid bits are passed in.

> > I'm also thinking of splitting the patch into two. One patch to pass down the
> > heap relation into the new places, and another for the rest.
>
> I think that makes sense. I don't know how far you've work on the split but please
> find attached V54 doing such a split + implementing your VISIBILITYMAP_XLOG_VALID_BITS
> suggestion.

I pushed the pass-the-relation part. I removed an include of catalog.h that
was in the patch - I suspect it might have slipped in there from a later patch
in the series...

I was a bit bothered by using 'heap' instead of 'table' in so many places
(eventually we imo should standardize on the latter), but looking around the
changed places, heap was used for things like buffers etc. So I left it at
heap.

Glad we split 0001 - the rest is a lot easier to review.

Greetings,

Andres Freund