Here are some review comments on the latest patch
(v11-0004-WAL-logged-CREATE-DATABASE.patch). I actually did the review
of the v10 patch but that applies for this latest version as well.
+ /* Now errors are fatal ... */
+ START_CRIT_SECTION();
Did you mean PANIC instead of FATAL?
==
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid create
strategy %s", strategy),
+ errhint("Valid strategies are
\"wal_log\", and \"file_copy\".")));
+ }
Should this be - "invalid createdb strategy" instead of "invalid
create strategy"?
==
+ /*
+ * In case of ALTER DATABASE SET TABLESPACE we don't need to do
+ * anything for the object which are not in the source
db's default
+ * tablespace. The source and destination dboid will be same in
+ * case of ALTER DATABASE SET TABLESPACE.
+ */
+ else if (src_dboid == dst_dboid)
+ continue;
+ else
+ dstrnode.spcNode = srcrnode.spcNode;
Is this change still required? Do we support the WAL_COPY strategy for
ALTER DATABASE?
==
+ /* Open the source and the destination relation at
smgr level. */
+ src_smgr = smgropen(srcrnode, InvalidBackendId);
+ dst_smgr = smgropen(dstrnode, InvalidBackendId);
+
+ /* Copy relation storage from source to the destination. */
+ CreateAndCopyRelationData(src_smgr, dst_smgr,
relinfo->relpersistence);
Do we need to do smgropen for destination relfilenode here? Aren't we
already doing that inside RelationCreateStorage?
==
+ use_wal = XLogIsNeeded() &&
+ (relpersistence == RELPERSISTENCE_PERMANENT ||
copying_initfork);
+
+ /* Get number of blocks in the source relation. */
+ nblocks = smgrnblocks(src, forkNum);
What if number of blocks in a source relation is ZERO? Should we check
for that and return immediately. We have already done smgrcreate.
==
+ /* We don't need to copy the shared objects to the target. */
+ if (classForm->reltablespace == GLOBALTABLESPACE_OID)
+ return NULL;
+
+ /*
+ * If the object doesn't have the storage then nothing to be
+ * done for that object so just ignore it.
+ */
+ if (!RELKIND_HAS_STORAGE(classForm->relkind))
+ return NULL;
We can probably club together above two if-checks.
==
+ <varlistentry>
+ <term><replaceable class="parameter">strategy</replaceable></term>
+ <listitem>
+ <para>
+ This is used for copying the database directory. Currently, we have
+ two strategies the <literal>WAL_LOG</literal> and the
+ <literal>FILE_COPY</literal>. If <literal>WAL_LOG</literal> strategy
+ is used then the database will be copied block by block and it will
+ also WAL log each copied block. Otherwise, if <literal>FILE_COPY
I think we need to mention the default strategy in the documentation page.
--
With Regards,
Ashutosh Sharma.
On Thu, Mar 10, 2022 at 4:32 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> On Wed, Mar 9, 2022 at 6:44 PM Robert Haas <robertmh...@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
