Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

On Tue, Jul 14, 2020 at 1:51 PM Anastasia Lubennikova
<a(dot)lubennikova(at)postgrespro(dot)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