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 >
