Thanks Dilip for working on the review comments. I'll take a look at
the new version of patch and let you know my comments, if any.
--
With Regards,
Ashutosh Sharma.
On Thu, Mar 10, 2022 at 8:38 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> On Thu, Mar 10, 2022 at 7:22 PM Ashutosh Sharma <ashu.coe...@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
