On Wed, Mar 9, 2022 at 6:44 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> > Right, infact now also if you see the logic, the
> > write_relmap_file_internal() is taking care of the actual update and
> > the write_relmap_file() is doing update + setting the global
> > variables. So yeah we can rename as you suggested in 0001 and here
> > also we can change the logic as you suggested that the recovery and
> > createdb will only call the first function which is just doing the
> > update.
>
> But I think we want the path construction to be managed by the
> function rather than the caller, too.
I have completely changed the logic for this refactoring. Basically,
write_relmap_file(), is already having parameters to control whether
to write wal, send inval and we are already passing the dbpath.
Instead of making a new function I just pass one additional parameter
to this function itself about whether we are creating a new map or not
and I think with that changes are very less and this looks cleaner to
me. Similarly for load_relmap_file() also I just had to pass the
dbpath and memory for destination map. Please have a look and let me
know your thoughts.
> Sure, I guess. It's just not obvious why the argument would actually
> need to be a function that copies storage, or why there's more than
> one way to copy storage. I'd rather keep all the code paths unified,
> if we can, and set behavior via flags or something, maybe. I'm not
> sure whether that's realistic, though.
I try considering that, I think it doesn't look good to make it flag
based, One of the main problem I noticed is that now for copying
either we need to call RelationCopyStorageis() or
RelationCopyStorageUsingBuffer() based on the input flag. But if we
move the main copy function to the storage.c then the storage.c will
have depedency on bufmgr functions because I don't think we can keep
RelationCopyStorageUsingBuffer() inside storage.c. So for now, I have
duplicated the loop which is already there in index_copy_data() and
heapam_relation_copy_data() and kept that in bufmgr.c and also moved
RelationCopyStorageUsingBuffer() into the bufmgr.c. I think bufmgr.c
is already having function which are dealing with smgr things so I
feel this is the right place for the function.
Other changes:
1. 0001 and 0002 are merged because now we are not really refactoring
these function and just passing the additioanl arguments to it make
sense to combine the changes.
2. Same with 0003, that now we are not refactoring existing functions
but providing new interfaces so merged it with the 0004 (which was
0006 previously)
I think we should also write the test cases for create database
strategy. But I do not see any test case for create database for
testing the existing options. So I am wondering whether we should add
the test case only for the new option we are providing or we should
create a separate path which tests the new option as well as the
existing options.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
