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

Few comments on the latest patch:

- /* We need to construct the pathname for this database */
- dbpath = GetDatabasePath(xlrec->dbid, xlrec->tsid);
+ if (xlrec->dbid != InvalidOid)
+ dbpath = GetDatabasePath(xlrec->dbid, xlrec->tsid);
+ else
+ dbpath = pstrdup("global");

Do we really need this change? Is GetDatabasePath() alone not capable
of handling it?

==

+static CreateDBRelInfo *ScanSourceDatabasePgClassTuple(HeapTupleData *tuple,
+
Oid tbid, Oid dbid,
+
char *srcpath);
+static List *ScanSourceDatabasePgClassPage(Page page, Buffer buf, Oid tbid,
+
Oid dbid, char *srcpath,
+
List *rnodelist, Snapshot snapshot);
+static List *ScanSourceDatabasePgClass(Oid srctbid, Oid srcdbid, char
*srcpath);

I think we can shorten these function names to probably
ScanSourceDBPgClassRel(), ScanSourceDBPgClassTuple() and likewise?

--
With Regards,
Ashutosh Sharma.

On Tue, Mar 15, 2022 at 3:24 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Mar 14, 2022 at 10:04 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> > I think it would make sense to have two different WAL records e.g.
> > XLOG_DBASE_CREATE_WAL_LOG and XLOG_DBASE_CREATE_FILE_COPY. Then it's
> > easy to see how this could be generalized to other strategies in the
> > future.
>
> Done that way. In dbase_desc(), for XLOG_DBASE_CREATE_FILE_COPY I
> have kept the older description i.e. "copy dir" and for
> XLOG_DBASE_CREATE_WAL_LOG it is "create dir", because logically the
> first one is actually copying and the second one is just creating the
> directory. Do you think we should be using "copy dir file_copy" and
> "copy dir wal_log" in the description as well?
>
> > On Mon, Mar 14, 2022 at 12:04 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > I was looking at 0001 and 0002 again and realized that I swapped the
> > > names load_relmap_file() and read_relmap_file() from what I should
> > > have done. Here's a revised version. With this, read_relmap_file() and
> > > write_relmap_file() become functions that just read and write the file
> > > without touching any global variables, and load_relmap_file() is the
> > > function that reads data from the file and puts it into a global
> > > variable, which seems more sensible than the way I had it before.
>
> Okay, I have included this patch and rebased other patches on top of that.
>
> > Regarding 0003 and 0005, I'm not a fan of 'bool isunlogged'. I think
> > 'bool permanent' would be better (note BM_PERMANENT). This would
> > involve reversing true and false.
>
> Okay changed.
>
> > Regarding 0005:
> >
> > + CREATEDB_WAL_LOG = 0,
> > + CREATEDB_FILE_COPY = 1
> >
> > I still think you don't need = 0 and = 1 here.
>
> Done
>
> > I'll probably go through and do a pass over the comments once you post
> > the next version of this. There seems to be work needed in a bunch of
> > places, but it probably makes more sense for me to go through and
> > adjust the things that seem to need it rather than listing a bunch of
> > changes for you to make.
>
> Sure, thanks.
>
> --
> Regards,
> Dilip Kumar
> EnterpriseDB: http://www.enterprisedb.com