Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

On 01.07.2020 12:38, Daniel Gustafsson wrote:
> This patch incurs a compiler warning, which probably is quite simple to fix:
>
> heapam.c: In function ‘heap_multi_insert’:
> heapam.c:2349:4: error: ‘recptr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      visibilitymap_set(relation, BufferGetBlockNumber(buffer), buffer,
>      ^
> heapam.c:2136:14: note: ‘recptr’ was declared here
>     XLogRecPtr recptr;
>                ^
>
> Please fix and submit a new version, I'm marking the entry Waiting on Author in
> the meantime.
>
> cheers ./daniel
>
This patch looks very useful to me, so I want to pick it up.


The patch that fixes the compiler warning is in the attachment. Though, 
I'm not
entirely satisfied with this fix. Also, the patch contains some FIXME 
comments.
I'll test it more and send fixes this week.

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.

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);

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company