Junwang, Kirill, The split work has been done. I created a new patch for removing redundant smgrclose() function as attached. Please help review it.
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 >> >
v3-0001-remove-duplicated-smgrclose.patch
Description: Binary data
