On Tue, Jul 14, 2020 at 1:51 PM Anastasia Lubennikova <a.lubennik...@postgrespro.ru> wrote: > Questions from the first review pass: > > 1) Do we need XLH_INSERT_ALL_VISIBLE_SET ? IIUC, in the patch it is always > implied by XLH_INSERT_ALL_FROZEN_SET.
I agree that it looks unnecessary to have two separate bits. > 2) What does this comment mean? Does XXX refers to the lsn comparison? > Since it > is definitely necessary to update the VM. > > + * XXX: This seems entirely unnecessary? > + * > + * FIXME: Theoretically we should only do this after we've > + * modified the heap - but it's safe to do it here I think, > + * because this means that the page previously was empty. > + */ > + if (lsn > PageGetLSN(vmpage)) > + visibilitymap_set(reln, blkno, InvalidBuffer, lsn, > vmbuffer, > + InvalidTransactionId, vmbits); I wondered about that too. The comment which precedes it was, I believe, originally written by me, and copied here from heap_xlog_visible(). But it's not clear very good practice to just copy the comment like this. If the same logic applies, the code should say that we're doing the same thing here as in heap_xlog_visible() for consistency, or some such thing; after all, that's the primary place where that happens. But it looks like the XXX might have been added by a second person who thought that we didn't need this logic at all, and the FIXME by a third person who thought it was in the wrong place, so the whole thing is really confusing at this point. I'm pretty worried about this, too: + * FIXME: setting recptr here is a dirty dirty hack, to prevent + * visibilitymap_set() from WAL logging. That is indeed a dirty hack, and something needs to be done about it. I wonder if it was really all that smart to try to make the HEAP2_MULTI_INSERT do this instead of just issuing separate WAL records to mark it all-visible afterwards, but I don't see any reason why this can't be made to work. It needs substantially more polishing than it's had, though, I think. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
