On Mon, Mar 14, 2022 at 12:04 PM Robert Haas <robertmh...@gmail.com> wrote: > On Mon, Mar 14, 2022 at 7:51 AM Dilip Kumar <dilipbal...@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
