Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints - Mailing list pgsql-hackers

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 <dilipbalaut@gmail.com> wrote:
>
> 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