Junwang Zhao <zhjw...@gmail.com> 于2024年8月15日周四 18:03写道：
> On Wed, Aug 14, 2024 at 2:35 PM Steven Niu <niush...@gmail.com> wrote: > > > > Junwang, Kirill, > > > > The split work has been done. I created a new patch for removing > redundant smgrclose() function as attached. > > Please help review it. > > Patch looks good, actually you can make the refactoring code as v3-0002-xxx > by using: > > git format-patch -2 -v 3 > > Another kind reminder: Do not top-post when you reply ;) > > > > > Thanks, > > Steven > > > > Steven Niu <niush...@gmail.com> 于2024年8月12日周一 18:11写道： > >> > >> Kirill, > >> > >> Good catch! > >> I will split the patch into two to cover both cases. > >> > >> Thanks, > >> Steven > >> > >> > >> Junwang Zhao <zhjw...@gmail.com> 于2024年8月9日周五 18:19写道： > >>> > >>> On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekir...@gmail.com> > wrote: > >>> > > >>> > On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjw...@gmail.com> wrote: > >>> > > > >>> > > Hi Steven, > >>> > > > >>> > > On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niush...@gmail.com> > wrote: > >>> > > > > >>> > > > Hello, hackers, > >>> > > > > >>> > > > I think there may be some duplicated codes. > >>> > > > Function smgrDoPendingDeletes() calls both smgrdounlinkall() and > smgrclose(). > >>> > > > But both functions would close SMgrRelation object, it's > dupliacted behavior? > >>> > > > > >>> > > > So I make this patch. Could someone take a look at it? > >>> > > > > >>> > > > Thanks for your help, > >>> > > > Steven > >>> > > > > >>> > > > From Highgo.com > >>> > > > > >>> > > > > >>> > > You change LGTM, but the patch seems not to be applied to HEAD, > >>> > > I generate the attached v2 using `git format` with some commit > message. > >>> > > > >>> > > -- > >>> > > Regards > >>> > > Junwang Zhao > >>> > > >>> > Hi all! > >>> > This change looks good to me. However, i have an objection to these > >>> > lines from v2: > >>> > > >>> > > /* Close the forks at smgr level */ > >>> > > - for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) > >>> > > - smgrsw[which].smgr_close(rels[i], forknum); > >>> > > + smgrclose(rels[i]); > >>> > > >>> > Why do we do this? This seems to be an unrelated change given thread > >>> > $subj. This is just a pure refactoring job, which deserves a separate > >>> > patch. There is similar coding in > >>> > smgrdestroy function: > >>> > > >>> > ``` > >>> > for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) > >>> > smgrsw[reln->smgr_which].smgr_close(reln, forknum); > >>> > ``` > >>> > > >>> > So, I feel like these two places should be either changed together or > >>> > not be altered at all. And is it definitely a separate change. > >>> > >>> Yeah, I tend to agree with you, maybe we should split the patch > >>> into two. > >>> > >>> Steven, could you do this? > >>> > >>> > > >>> > -- > >>> > Best regards, > >>> > Kirill Reshke > >>> > >>> > >>> > >>> -- > >>> Regards > >>> Junwang Zhao > > > > -- > Regards > Junwang Zhao > OK， thanks for your kind help. Steven
