Re: [Patch] remove duplicated smgrclose

On Sat, Mar 8, 2025 at 12:04 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Sun, Oct 27, 2024 at 12:05 PM Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
> >
> > On Wed, 14 Aug 2024 at 11:35, 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.
> > >
> > > 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
> >
> > Hi!
> > Looks like discussion on the subject is completed, and no open items
> > left, so I will try to mark commitfest [1] entry as Ready For
> > Committer.
> >
>
> I've looked at the patch and have some comments:
>
> The patch removes smgrclose() calls following smgrdounlinkall(), for example:
>
> --- a/src/backend/catalog/storage.c
> +++ b/src/backend/catalog/storage.c
> @@ -686,9 +686,6 @@ smgrDoPendingDeletes(bool isCommit)
> {
> smgrdounlinkall(srels, nrels, false);
>
> - for (int i = 0; i < nrels; i++)
> - smgrclose(srels[i]);
> -
> pfree(srels);
> }
> }
>
> While smgrdounlinkall() close the relation at smgr level as follow:
>
> /* Close the forks at smgr level */
> for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
> smgrsw[which].smgr_close(rels[i], forknum);
>
> smgrrelease(), called by smgrclose(), also does the same thing but
> does more things as follow:.
>
> void
> smgrrelease(SMgrRelation reln)
> {
> for (ForkNumber forknum = 0; forknum <= MAX_FORKNUM; forknum++)
> {
> smgrsw[reln->smgr_which].smgr_close(reln, forknum);
> reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber;
> }
> reln->smgr_targblock = InvalidBlockNumber;
> }
>
> Therefore, if we move such smgrclose() calls, we would end up missing
> some operations that are done in smgrrelease() but not in
> smgrdounlinkall(), no?

Yeah, we miss setting the smgr_targblock/smgr_cached_nblocks to
InvalidBlockNumber,
but in the smgrDoPendingDeletes case, SMgrRelation are freed after calling
smgrdounlinkall, so I guess it's ok not setting the InvalidBlockNumber?

I did a quick seach of smgrdounlinkall usage, SMgrRelation seems
not needed after the calling of smgrdounlinkall.

>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com

--
Regards
Junwang Zhao