Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints - Mailing list pgsql-hackers
From | Dilip Kumar |
---|---|
Subject | Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints |
Date | |
Msg-id | CAFiTN-tqUW7jm6mfyjY2jT0Q_0bsOWPGoG6WG8yxNOr5EE0BBw@mail.gmail.com Whole thread Raw |
In response to | Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints (Ashutosh Sharma <ashu.coek88@gmail.com>) |
Responses |
Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
(Ashutosh Sharma <ashu.coek88@gmail.com>)
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: