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:
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. 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. -- Further, the code changes in the patch looks good. I just have few comments: +void +LockRelationId(LockRelId *relid, LOCKMODE lockmode) +{ + LOCKTAG tag; + LOCALLOCK *locallock; + LockAcquireResult res; + + SET_LOCKTAG_RELATION(tag, relid->dbId, relid->relId); Should there be an assertion statement here to ensure that relid->dbid and relid->relid is valid? -- if (info == XLOG_DBASE_CREATE) { xl_dbase_create_rec *xlrec = (xl_dbase_create_rec *) XLogRecGetData(record); - char *src_path; - char *dst_path; - struct stat st; - - src_path = GetDatabasePath(xlrec->src_db_id, xlrec->src_tablespace_id); - dst_path = GetDatabasePath(xlrec->db_id, xlrec->tablespace_id); + char *dbpath; - /* - * Our theory for replaying a CREATE is to forcibly drop the target - * subdirectory if present, then re-copy the source data. This may be - * more work than needed, but it is simple to implement. - */ - if (stat(dst_path, &st) == 0 && S_ISDIR(st.st_mode)) - { - if (!rmtree(dst_path, true)) - /* If this failed, copydir() below is going to error. */ - ereport(WARNING, - (errmsg("some useless files may be left behind in old database directory \"%s\"", - dst_path))); - } I think this is a significant change and probably needs some kind of explanation/comments as-in why we are just creating a dir and copying the version file when replaying create database operation. Earlier, this meant replaying the complete create database operation, that doesn't seem to be the case now. -- Have you intentionally skipped pg_internal.init file from being copied to the target database? -- With Regards, Ashutosh Sharma. On Thu, Dec 2, 2021 at 7:20 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > On Wed, Dec 1, 2021 at 6:04 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > Thanks a lot for testing this. From the error, it seems like some of > > the old buffer w.r.t. the previous tablespace is not dropped after the > > movedb. Actually, we are calling DropDatabaseBuffers() after copying > > to a new tablespace and dropping all the buffers of this database > > w.r.t the old tablespace. But seems something is missing, I will > > reproduce this and try to fix it by tomorrow. I will also fix the > > other review comments raised by you in the previous mail. > > Okay, I got the issue, basically we are dropping the database buffers > but not unregistering the existing sync request for database buffers > w.r.t old tablespace. Attached patch fixes that. I also had to extend > ForgetDatabaseSyncRequests so that we can delete the sync request of > the database for the particular tablespace so added another patch for > the same (0006). > > I will test the performance scenario next week, which is suggested by John. > > > -- > Regards, > Dilip Kumar > EnterpriseDB: http://www.enterprisedb.com >