Re: [Patch] remove duplicated smgrclose

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.

--
Best regards,
Kirill Reshke