Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

Hi,

On 2019-04-05 09:20:36 +0530, Pavan Deolasee wrote:
> On Fri, Apr 5, 2019 at 9:05 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > I think the right approach would be to do all of this in heap_insert and
> > heap_multi_insert. Whenever starting to work on a page, if INSERT_FROZEN
> > is specified, remember whether it is either currently empty, or is
> > already marked as all-visible. If previously empty, mark it as all
> > visible at the end. If already all visible, there's no need to change
> > that. If not yet all-visible, no need to do anything, since it can't
> > have been inserted with COPY FREEZE.
>
>
> We're doing roughly the same. If we are running INSERT_FROZEN, whenever
> we're about to switch to a new page, we check if the previous page should
> be marked all-frozen and do it that way. The special code in copy.c is
> necessary to take care of the last page which we don't get to handle in the
> regular code path.

Well, it's not the same, because you need extra code from copy.c, extra
lock cycles, and extra WAL logging.

> Or are you suggesting that we don't even rescan the page for all-frozen
> tuples at the end and just simply mark it all-frozen at the start, when the
> first tuple is inserted and then don't touch the PD_ALL_VISIBLE/visibility
> map bit as we go on inserting more tuples in the page?

Correct. If done right that should be cheaper (no extra scans, less WAL
logging), without requiring some new dispatch logic from copy.c.

> Anyways, if major architectural changes are required then it's probably too
> late to consider this for PG12, even though it's more of a bug fix and a
> candidate for back-patching too.

Let's just see how bad it looks? I don't feel like we'd need to be super
strict about it. If it looks simple enough I'd e.g. be ok to merge this
soon after freeze, and backpatch around maybe 12.1 or such.

Greetings,

Andres Freund