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

Here are few more review comments:

1) It seems that we are not freeing the memory allocated for buf.data in
CreateDirAndVersionFile().

--

+ */
+static void
+CreateDirAndVersionFile(char *dbpath, Oid dbid, Oid tsid, bool isRedo)
+{

2) Do we need to pass dbpath here? I mean why not reconstruct it from dbid
and tsid.

--

3) Not sure if this point has already been discussed, Will we be able to
recover the data when wal_level is set to minimal because the following
condition would be false with this wal level.

+ use_wal = XLogIsNeeded() &&
+ (relpersistence == RELPERSISTENCE_PERMANENT || copying_initfork);

--
With Regards,
Ashutosh Sharma.

On Mon, Dec 6, 2021 at 9:12 AM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>
wrote:

> On Fri, Dec 3, 2021 at 8:28 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
>> On Fri, Dec 3, 2021 at 7:38 PM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>
>> wrote:
>> >
>> > I see that this patch is reducing the database creation time by almost
>> 3-4 times provided that the template database has some user data in it.
>> However, there are couple of points to be noted:
>>
>> Thanks a lot for looking into the patches.
>> >
>> > 1) It makes the crash recovery a bit slower than before if the crash
>> has occurred after the execution of a create database statement. Moreover,
>> if the template database size is big, it might even generate a lot of WAL
>> files which the user needs to be aware of.
>>
>> Yes it will but actually that is the only correct way to do it, in
>> current we are just logging the WAL as copying the source directory to
>> destination directory without really noting down exactly what we
>> wanted to copy, so we are force to do the checkpoint right after
>> create database because in crash recovery we can not actually replay
>> that WAL. Because WAL just say copy the source to destination so it
>> is very much possible that at the DO time source directory had some
>> different content than the REDO time so this would have created the
>> inconsistencies in the crash recovery so to avoid this bug they force
>> the checkpoint so now also if you do force checkpoint then again crash
>> recovery will be equally fast. So I would not say that we have made
>> crash recovery slow but we have removed some bugs and with that now we
>> don't need to force the checkpoint. Also note that in current code
>> even with force checkpoint the bug is not completely avoided in all
>> the cases, check below comments from the code[1].
>>
>> > 2) This will put a lot of load on the first checkpoint that will occur
>> after creating the database statement. I will experiment around this to see
>> if this has any side effects.
>>
>> But now a checkpoint can happen at its own need and there is no need
>> to force a checkpoint like it was before patch.
>>
>> So the major goal of this patch is 1) Correctly WAL log the create
>> database which is hack in the current system, 2) Avoid force
>> checkpoints, 3) We copy page by page so it will support TDE because if
>> the source and destination database has different encryption then we
>> can reencrypt the page before copying to destination database, which
>> is not possible in current system as we are copying directory 4) Now
>> the new database pages will get the latest LSN which is the correct
>> things earlier new database pages were getting copied directly with
>> old LSN only.
>>
>
> OK. Understood, thanks.!
>
> --
> With Regards,
> Ashutosh Sharma.
>