Re: [Patch] remove duplicated smgrclose

On Wed, Aug 14, 2024 at 2:35 PM Steven Niu <niushiji(at)gmail(dot)com> wrote:
>
> Junwang, Kirill,
>
> The split work has been done. I created a new patch for removing redundant smgrclose() function as attached.
> Please help review it.

Patch looks good, actually you can make the refactoring code as v3-0002-xxx
by using:

git format-patch -2 -v 3

Another kind reminder: Do not top-post when you reply ;)

>
> Thanks,
> Steven
>
> Steven Niu <niushiji(at)gmail(dot)com> 于2024年8月12日周一 18:11写道：
>>
>> 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

--
Regards
Junwang Zhao