Re: [Patch] remove duplicated smgrclose

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

Junwang Zhao <zhjwpku(at)gmail(dot)com> 于2024年8月9日周五 18:19写道：

> On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <reshkekirill(at)gmail(dot)com>
> wrote:
> >
> > On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> > >
> > > Hi Steven,
> > >
> > > On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji(at)gmail(dot)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
>