Thread: [Patch] remove duplicated smgrclose

Hi Steven,

On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@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 Steven,

On Fri, Aug 2, 2024 at 12:12 PM Steven Niu <niushiji@gmail.com> wrote:
>
> Hi, Junwang,
>
> Thank you for the review and excellent summary in commit message!
>
> This is my first contribution to community, and not so familiar with the overall process.
> After reading the process again, it looks like that I'm not qualified to submit the patch to commitfest as I never
hadreviewed others' work.  :( 
> If so, could you please help to submit it to commitfest?
>

https://commitfest.postgresql.org/49/5149/

I can not find your profile on commitfest so I left the author as empty,
have you ever registered? If you have a account, you can put your
name in the Authors list.

> Best Regards,
> Steven
>
> Junwang Zhao <zhjwpku@gmail.com> 于2024年8月1日周四 20:32写道：
>>
>> Hi Steven,
>>
>> On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@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



--
Regards
Junwang Zhao

On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi Steven,
>
> On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <niushiji@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.

--
Best regards,
Kirill Reshke

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