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