Hi,
On 2022-08-04 19:01:06 -0400, Tom Lane wrote:
> And while I'm piling on, how is this bit in RelationCopyStorageUsingBuffer
> not completely broken?
>
> /* Read block from source relation. */
> srcBuf = ReadBufferWithoutRelcache(src->rd_locator, forkNum, blkno,
> RBM_NORMAL, bstrategy_src,
> permanent);
> srcPage = BufferGetPage(srcBuf);
> if (PageIsNew(srcPage) || PageIsEmpty(srcPage))
> {
> ReleaseBuffer(srcBuf);
> continue;
> }
>
> /* Use P_NEW to extend the destination relation. */
> dstBuf = ReadBufferWithoutRelcache(dst->rd_locator, forkNum, P_NEW,
> RBM_NORMAL, bstrategy_dst,
> permanent);
>
> You can't skip pages just because they are empty. Well, maybe you could
> if you were doing something to ensure that you zero-fill the corresponding
> blocks on the destination side. But this isn't doing that. It's using
> P_NEW for dstBuf, which will have the effect of silently collapsing out
> such pages. Maybe in isolation a heap could withstand that, but its
> indexes won't be happy (and I guess t_ctid chain links won't either).
>
> I think you should just lose the if() stanza. There's no optimization to
> be had here that's worth any extra complication.
>
> (This seems worth fixing before beta3, as it looks like a rather
> nasty data corruption hazard.)
Ugh, yes. And even with this fixed I think this should grow at least an
assertion that the block numbers match, probably even an elog.
Greetings,
Andres
