On Mon, Mar 14, 2022 at 12:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Mar 14, 2022 at 7:51 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > Other than that, I have fixed some mistakes in comments and supported
> > tab completion for the new options.
>
> 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.
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.
Regarding 0004, I can't really see a reason for this function to take
a LockRelId as a parameter rather than two separate OIDs. I also can't
entirely see why it should be called LockRelationId. Maybe
LockRelationInDatabaseById(Oid dbid, Oid relid, LOCKMODE lockmode)?
Note that neither caller actually has a LockRelId available; both have
to construct one.
Regarding 0005:
+ CREATEDB_WAL_LOG = 0,
+ CREATEDB_FILE_COPY = 1
I still think you don't need = 0 and = 1 here.
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.
--
Robert Haas
EDB: http://www.enterprisedb.com
