On Fri, Dec 3, 2021 at 7:38 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> I see that this patch is reducing the database creation time by almost 3-4 times provided that the template database
hassome 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
databasestatement. Moreover, if the template database size is big, it might even generate a lot of WAL files which the
userneeds 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
experimentaround 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.
> Further, the code changes in the patch looks good. I just have few comments:
I will look into the other comments and get back to you, thanks.
[1]
* In PITR replay, the first of these isn't an issue, and the second
* is only a risk if the CREATE DATABASE and subsequent template
* database change both occur while a base backup is being taken.
* There doesn't seem to be much we can do about that except document
* it as a limitation.
*
* Perhaps if we ever implement CREATE DATABASE in a less cheesy way,
* we can avoid this.
*/
RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE | CHECKPOINT_WAIT);
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
