Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

Hi,

On 2019-04-04 16:15:54 -0300, Alvaro Herrera wrote:
> On 2019-Apr-04, Andres Freund wrote:
> 
> > I'm totally not OK with this from a layering
> > POV. CheckAndSetAllVisibleBulkInsertState is entirely heap specific
> > (without being named such), whereas all the heap specific bits are
> > getting moved below tableam.
> 
> This is a fair complaint, but on the other hand the COPY changes for
> table AM are still being developed, so there's no ground on which to
> rebase this patch.  Do you have a timeline on getting the COPY one
> committed?

~2h. Just pondering the naming of some functions etc.  Don't think
there's a large interdependency though.

But even if tableam weren't committed, I'd still argue that it's
structurally done wrong in the patch right now.  FWIW, I actually think
this whole approach isn't quite right - this shouldn't be done as a
secondary action after we'd already inserted, with a separate
lock-unlock cycle etc.

Also, how is this code even close to correct?
CheckAndSetPageAllVisible() modifies the buffer in a crucial way, and
there's no WAL logging? Without even a comment arguing why that's OK (I
don't think it is)?

Greetings,

Andres Freund