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

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