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

On Thu, Nov 25, 2021 at 1:07 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Tue, Oct 5, 2021 at 7:07 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > Patch details:
> > 0001 to 0006 implements an approach1
> > 0007 removes the code of pg_class scanning and adds the directory scan.
> >
>
> I had a scan through the patches, though have not yet actually run any
> tests to try to better gauge their benefit.
> I do have some initial review comments though:
>
> 0003
>
> src/backend/commands/tablecmds.c
> (1) RelationCopyAllFork()
> In the following comment:
>
> +/*
> + * Copy source smgr all fork's data to the destination smgr.
> + */
>
> Shouldn't it say "smgr relation"?
> Also, you could additionally say ", using a specified fork data
> copying function." or something like that, to account for the
> additional argument.
>
>
> 0006
>
> src/backend/commands/dbcommands.c
> (1) function prototype location
>
> The following prototype is currently located in the "non-export
> function prototypes" section of the source file, but it's not static -
> shouldn't it be in dbcommands.h?
>
> +void RelationCopyStorageUsingBuffer(SMgrRelation src, SMgrRelation dst,
> +        ForkNumber forkNum, char relpersistence);
>
> (2) CreateDirAndVersionFile()
> Shouldn't the following code:
>
> + fd = OpenTransientFile(versionfile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
> + if (fd < 0 && errno == EEXIST && isRedo)
> +   fd = OpenTransientFile(versionfile, O_RDWR | PG_BINARY);
>
> actually be:
>
> + fd = OpenTransientFile(versionfile, O_WRONLY | O_CREAT | O_EXCL | PG_BINARY);
> + if (fd < 0 && errno == EEXIST && isRedo)
> +   fd = OpenTransientFile(versionfile, O_WRONLY | O_TRUNC | PG_BINARY);
>
> since we're only writing to that file descriptor and we want to
> truncate the file if it already exists.
>
> The current comment says "... open it in the write mode.", but should
> say "... open it in write mode."
>
> Also, shouldn't you be writing a newline (\n) after the
> PG_MAJORVERSION ? (compare with code in initdb.c)
>
> (3) GetDatabaseRelationList()
> Shouldn't:
>
> +  if (PageIsNew(page) || PageIsEmpty(page))
> +    continue;
>
> be:
>
> +  if (PageIsNew(page) || PageIsEmpty(page))
> +  {
> +    UnlockReleaseBuffer(buf);
> +    continue;
> +  }
>
> ?
>
> Also, in the following code:
>
> +  if (rnodelist == NULL)
> +    rnodelist = list_make1(relinfo);
> +  else
> +    rnodelist = lappend(rnodelist, relinfo);
>
> it should really be "== NIL" rather than "== NULL".
> But in any case, that code can just be:
>
>    rnodelist = lappend(rnodelist, relinfo);
>
> because lappend() will create a list if the first arg is NIL.
>
> (4) RelationCopyStorageUsingBuffer()
>
> In the function comments, IMO it is better to use "APIs" instead of "apis".
>
> Also, better to use "get" instead of "got" in the following comment:
>
> +  /* If we got a cancel signal during the copy of the data, quit */

Thanks for the review and many valuable comments, I have fixed all of
them except this comment (/* If we got a cancel signal during the copy
of the data, quit */) because this looks fine to me.  0007, I have
dropped from the patchset for now.  I have also included fixes for
comments given by John.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com