On Tue, Jul 19, 2022 at 1:43 PM Kyotaro Horiguchi <horikyota....@gmail.com> wrote: > > At Tue, 19 Jul 2022 16:57:14 +0900 (JST), Kyotaro Horiguchi > <horikyota....@gmail.com> wrote in > > At Tue, 19 Jul 2022 16:02:26 +0900, Masahiko Sawada <sawada.m...@gmail.com> > > wrote in > > > On Tue, Jul 19, 2022 at 1:47 PM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > Good work. I wonder without comments this may create a problem in the > > > > future. OTOH, I don't see adding a check "catchange.xcnt > 0" before > > > > freeing the memory any less robust. Also, for consistency, we can use > > > > a similar check based on xcnt in the SnapBuildRestore to free the > > > > memory in the below code: > > > > + /* set catalog modifying transactions */ > > > > + if (builder->catchange.xip) > > > > + pfree(builder->catchange.xip); > > > > > > I would hesitate to add comments about preventing the particular > > > optimization. I think we do null-pointer-check-then-pfree many place. > > > It seems to me that checking the array length before memcpy is more > > > natural than checking both the array length and the array existence > > > before pfree. > > > > Anyway according to commit message of 46ab07ffda, POSIX forbits > > memcpy(NULL, NULL, 0). It seems to me that it is the cause of the > > false (or over) optimization. So if we add some comment, it would be > > for memcpy, not pfree.. > > For clarilty, I meant that I don't think we need that comment. >
Fair enough. I think commit 46ab07ffda clearly explains why it is a good idea to add a check as Sawada-San did in his latest version. I also agree that we don't any comment for this change. -- With Regards, Amit Kapila.
