Horton

Re: [Patch] remove duplicated smgrclose

Steven Niu <niushiji@gmail.com>

View thread
From: Steven Niu <niushiji@gmail.com>
To: Junwang Zhao <zhjwpku@gmail.com>
Cc: Kirill Reshke <reshkekirill@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-08-14T06:35:39Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Give SMgrRelation pointers a well-defined lifetime.

Attachments

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 <niushiji@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 <zhjwpku@gmail.com> 于2024年8月9日周五 18:19写道：
>
>> 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
>>
>