Re: [Patch] remove duplicated smgrclose - Mailing list pgsql-hackers

Hi,

On Sun, Oct 27, 2024 at 12:05 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> On Wed, 14 Aug 2024 at 11:35, Steven Niu <niushiji@gmail.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@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
>
> 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?

Regards,

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