Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

On Sat, Jun 29, 2019 at 12:56 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:

> On Thu, Jun 27, 2019 at 11:02 AM Pavan Deolasee
> <pavan.deolasee@gmail.com> wrote:
> >
> >>> On 2019-04-07 18:04:27 -0700, Andres Freund wrote:
> >>> > Here's a *prototype* patch for this.  It only implements what I
> >>> > described for heap_multi_insert, not for plain inserts. I wanted to
> see
> >>> > what others think before investing additional time.
> >>>
> >>> Pavan, are you planning to work on this for v13 CF1? Or have you lost
> >>> interest on the topic?
> >>
> >>
> >> Yes, I plan to work on it.
> >>
> >
> > I am sorry, but I am not able to find time to get back to this because
> of other high priority items. If it still remains unaddressed in the next
> few weeks, I will pick it up again. But for now, I am happy if someone
> wants to pick and finish the work.
> >
>
> Fair enough, I have marked the entry [1] in the coming CF as "Returned
> with Feedback".  I hope that is okay with you.
>
> [1] - https://commitfest.postgresql.org/23/2009/
>
>

Hi,

As Pavan mentioned in the last email that he is no longer interested in
that, so I want to
take the lead and want to finish that. It is a bug and needs to be fixed.
I have rebased and the patch with the latest master and added some test
cases (borrowed from Pavan's patch), and did some performance testing with
a table size of
700MB (10Millions rows)

COPY WITH FREEZE took 21406.692ms and VACUUM took 2478.666ms
COPY WITH FREEZE took 23095.985ms and VACUUM took 26.309ms

The performance decrease in copy with the patch is only 7%, but we get
quite adequate performance in VACUUM. In any case, this is a bug fix, so we
can ignore
the performance hit.

There are two issues left to address.

1 - Andres: It only implements what I described for heap_multi_insert, not
for plain inserts.
I wanted to see what others think before investing additional time.

In which condition we need that for plain inserts?

2 - Andres:  I think we should remove the WAL logging from
visibilitymap_set(), and
move it to a separate, heap specific, function. Right now different
tableams e.g. would have to reimplement visibilitymap_set(), so that's a
second need to separate that functionality.  Let me try to come up with
a proposal.




-- 
Ibrar Ahmed