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

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 */


0007

(I think I prefer the first approach rather than this 2nd approach)

src/backend/commands/dbcommands.c
(1) createdb()
pfree(srcpath) seems to be missing, in the case that CopyDatabase() gets called.

(2) GetRelfileNodeFromFileName()
%s in sscanf() allows an unbounded read and is considered potentially
dangerous (allows buffer overflow), especially here where
FORKNAMECHARS is so small.

+  nmatch = sscanf(filename, "%u_%s", &relfilenode, forkname);

how about using the following instead in this case:

+  nmatch = sscanf(filename, "%u_%4s", &relfilenode, forkname);

?

(even if there were > 4 chars after the underscore, it would still
match and InvalidOid would be returned because nmatch==2)

Regards,
Greg Nancarrow
Fujitsu Australia