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

On Thu, Mar 10, 2022 at 7:22 PM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)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