Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

On Fri, Aug 5, 2022 at 4:31 AM Tom Lane <tgl@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