On Fri, Aug 5, 2022 at 4:31 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> I wrote:
> > While I'm at it, I would like to strenuously object to the current
> > framing of CreateAndCopyRelationData as a general-purpose copying
> > mechanism.
>
> 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.)
Yeah this is broken.
--
Dilip
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
