Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

On Thu, Mar 10, 2022 at 7:22 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> 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?

I think here fatal didn't really mean the error level FATAL, that
means critical and I have seen it is used in other places also.  But I
really don't think we need this comments to removed to avoid any
confusion.

> ==
>
> +
> (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"?

Changed

> ==
>
> +               /*
> +                * 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?

Yeah now it is unreachable code so removed.

> ==
>
> +               /* 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?

Yeah I have changed the complete logic and removed the smgr_open for
both source and destination and moved inside
CreateAndCopyRelationData, please check the updated code.

> ==
>
> +       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.

Yeah make sense to optimize, with that we will not have to get the
buffer strategy so done.

> ==
>
> +       /* 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.

Done

> ==
>
> +      <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.

Done

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com