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
