Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

On Fri, Mar 22, 2019 at 12:19 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> I've looked at the patch and have comments and questions.
>
> + /*
> + * While we are holding the lock on the page, check if all tuples
> + * in the page are marked frozen at insertion. We can safely mark
> + * such page all-visible and set visibility map bits too.
> + */
> + if (CheckPageIsAllFrozen(relation, buffer))
> + PageSetAllVisible(page);
> +
> + MarkBufferDirty(buffer);
>
> Maybe we don't need to mark the buffer dirty if the page is not set
> all-visible.
>
>
Yeah, makes sense. Fixed.

> If we have CheckAndSetPageAllVisible() for only this
> situation we would rather need to check that the VM status of the page
> should be 0 and then set two flags to the page? The 'flags' will
> always be (VISIBILITYMAP_ALL_FROZEN | VISIBILITYMAP_ALL_VISIBLE) in
> copy freeze case. I'm confused that this function has both code that
> assumes some special situations and code that can be used in generic
> situations.
>

If a second COPY FREEZE is run within the same transaction and if starts
inserting into the page used by the previous COPY FREEZE, then the page
will already be marked all-visible/all-frozen. So we can skip repeating the
operation again. While it's quite unlikely that someone will do that and I
can't think of a situation where only one of those flags will be set, I
don't see a harm in keeping the code as is. This code is borrowed from
vacuumlazy.c and at some point we can even move it to some common location.

> Perhaps we can add some tests for this feature to pg_visibility module.
>
>
That's a good idea. Please see if the tests included in the attached patch
are enough.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services