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

On Wed, Aug 14, 2024 at 2:35 PM 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.

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@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



--
Regards
Junwang Zhao

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hi

the patch looks good to me as well. Calling smgrclose() right after calling smgrdounlinkall() does seem
unnecessary as it is already done inside smgrdounlinkall() as you mentioned. I checked the commit logs
and seems like the code has been like this for over 10 years. One difference is that smgrdounlinkall() does
not reset smgr_cached_nblocks and smgr_targblock but that does not seem to matter as it is about to
remove the physical files.

While leaving them like this does no harm because smgrclose() simply does nothing if the relation has already
been closed, it does look weird that the code tries to close the relation after smgrdounlinkall(), because the
physical files have just been deleted when smgrdounlinkall() completes, and we try to close something that
has been deleted ?!

---------------------------
Cary Huang
Highgo software Canada
www.highgo.ca

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.

[1] https://commitfest.postgresql.org/50/5149/
--
Best regards,
Kirill Reshke

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

On Sat, Mar 8, 2025 at 12:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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?

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

Hi Masahiko,

> > 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.
>

After a second look, I realize I'm wrong, it's that the pointers to SMgrRelation
are freed, not the SMgrRelation itself.

So I agree with you that we would end up missing some operations with
this patch.

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



-- 
Regards
Junwang Zhao

On Sat, Mar 8, 2025 at 1:37 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi Masahiko,
>
> > > 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.
> >
>
> After a second look, I realize I'm wrong, it's that the pointers to SMgrRelation
> are freed, not the SMgrRelation itself.
>
> So I agree with you that we would end up missing some operations with
> this patch.

Right. Also, I'm concerned that even if we could remove these
smgrclose() calls the benefit of removing these calls here would be
very small compared to the risk of changing the code.

Regards,

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

>
>  Hi, Masahiko
>
> Thanks for your comments! I understand your concern as you stated.
> However, my initial patch was split into two parts as Kirill suggested.
> This thread is about the first part. Another part is here: https://commitfest.postgresql.org/patch/5149/
> Or you can take a look at the v2-0001-remove-duplicated-smgrclose.patch in this thread for the complete change.
>
> I think either we review the v2-patch, or review the both 5149 and 5196 CFs, for my complete change.
> There should be no missing operations.

@@ -482,13 +482,11 @@ smgrdounlinkall(SMgrRelation *rels, int nrels,
bool isRedo)
  for (i = 0; i < nrels; i++)
  {
  RelFileLocatorBackend rlocator = rels[i]->smgr_rlocator;
- int which = rels[i]->smgr_which;

  rlocators[i] = rlocator;

  /* Close the forks at smgr level */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- smgrsw[which].smgr_close(rels[i], forknum);
+ smgrclose(rels[i]);
  }

Yeah, you are adjusting the behavior by moving the `smgrclose` operation
after the `smgrdounlinkall` to the `smgrdounlinkall` function itself.

Seems no missing operations in v2-patch. Thanks.

>
> Please let me know if you have more comments.
>
> Best Regards,
> Steven



-- 
Regards
Junwang Zhao

On Mon, Mar 10, 2025 at 3:08 AM Steven Niu <niushiji@gmail.com> wrote:
>
>
>
>  Hi, Masahiko
>
> Thanks for your comments! I understand your concern as you stated.
> However, my initial patch was split into two parts as Kirill suggested.
> This thread is about the first part. Another part is here: https://commitfest.postgresql.org/patch/5149/
> Or you can take a look at the v2-0001-remove-duplicated-smgrclose.patch in this thread for the complete change.
>
> I think either we review the v2-patch, or review the both 5149 and 5196 CFs, for my complete change.
> There should be no missing operations.
>
> Please let me know if you have more comments.

What is the main point of removing these duplication? While I can see
that in smgrDoPendingDeletes(), we do
'smgrsw[reln->smgr_which].smgr_close(reln, forknum);' twice for each
relation: in smgrdounlinkall() and in smgrrelease(), I'm not sure what
this change benefits to us. Particularly, we quick return from the
second mdclose() call as all segment files are already closed. Have
you experienced a case like where the system got stuck or got very
slower due to these duplicated calls?

Also, we might need to pay attention that with the patch
smgrdounlinkall() came to depend on smgrclose(). For example, the more
works we add in smgrclose() in the future, the more works
smgrdounlinkall() will do, which doesn't happen with the current code
as smgrdounlinkall() depends on mdclose().

Given that the patched codes doesn't do exactly the same things as
before (e.g, smgrdounlinkall() would end up resetting
reln->smgr_cached_nblocks[forknum] too), I think we need some reasons
for legitimating this change.

Regards,

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

在 2025/3/12 6:31, Masahiko Sawada 写道:
> On Mon, Mar 10, 2025 at 3:08 AM Steven Niu <niushiji@gmail.com> wrote:
>>
>>
>>
>>   Hi, Masahiko
>>
>> Thanks for your comments! I understand your concern as you stated.
>> However, my initial patch was split into two parts as Kirill suggested.
>> This thread is about the first part. Another part is here: https://commitfest.postgresql.org/patch/5149/
>> Or you can take a look at the v2-0001-remove-duplicated-smgrclose.patch in this thread for the complete change.
>>
>> I think either we review the v2-patch, or review the both 5149 and 5196 CFs, for my complete change.
>> There should be no missing operations.
>>
>> Please let me know if you have more comments.
> 
> What is the main point of removing these duplication? While I can see
> that in smgrDoPendingDeletes(), we do
> 'smgrsw[reln->smgr_which].smgr_close(reln, forknum);' twice for each
> relation: in smgrdounlinkall() and in smgrrelease(), I'm not sure what
> this change benefits to us. Particularly, we quick return from the
> second mdclose() call as all segment files are already closed. Have
> you experienced a case like where the system got stuck or got very
> slower due to these duplicated calls?
> 
> Also, we might need to pay attention that with the patch
> smgrdounlinkall() came to depend on smgrclose(). For example, the more
> works we add in smgrclose() in the future, the more works
> smgrdounlinkall() will do, which doesn't happen with the current code
> as smgrdounlinkall() depends on mdclose().
> 
> Given that the patched codes doesn't do exactly the same things as
> before (e.g, smgrdounlinkall() would end up resetting
> reln->smgr_cached_nblocks[forknum] too), I think we need some reasons
> for legitimating this change.
> 
> Regards,
> 


Hi, Masahiko

Thank you for your detailed review and valuable insights. I understand 
your concern about the immediate benefits of removing the duplicate 
smgr_close() call, especially since the second call effectively becomes 
a no-op due to the prior file closures. However, the primary intent of 
my change is not driven by performance improvements or addressing a 
specific issue, but rather by enhancing the code's structure and 
clarity. Having two "Close the forks at smgr level" operations might 
lead to confusion for readers of the code.


Additionally, the smgrclose() function not only closes the smgr but also 
resets smgr_cached_nblocks and smgr_targblock, making it a comprehensive 
operation. In the current implementation, the smgr is closed inside 
smgrdounlinkall(), while smgr_cached_nblocks and smgr_targblock are 
reset outside of it. This creates a fragmentation in the code logic, 
which could be streamlined.


Would it make sense to remove the smgr closing operation within 
smgrdounlinkall() and leave the rest of the code unchanged? This 
approach would eliminate the duplication while ensuring no operations 
are missed. I'd appreciate your thoughts on this suggestion.

Thanks,
Steven

On Thu, Mar 13, 2025 at 1:37 AM Steven Niu <niushiji@gmail.com> wrote:
>
>
>
> 在 2025/3/12 6:31, Masahiko Sawada 写道:
> > On Mon, Mar 10, 2025 at 3:08 AM Steven Niu <niushiji@gmail.com> wrote:
> >>
> >>
> >>
> >>   Hi, Masahiko
> >>
> >> Thanks for your comments! I understand your concern as you stated.
> >> However, my initial patch was split into two parts as Kirill suggested.
> >> This thread is about the first part. Another part is here: https://commitfest.postgresql.org/patch/5149/
> >> Or you can take a look at the v2-0001-remove-duplicated-smgrclose.patch in this thread for the complete change.
> >>
> >> I think either we review the v2-patch, or review the both 5149 and 5196 CFs, for my complete change.
> >> There should be no missing operations.
> >>
> >> Please let me know if you have more comments.
> >
> > What is the main point of removing these duplication? While I can see
> > that in smgrDoPendingDeletes(), we do
> > 'smgrsw[reln->smgr_which].smgr_close(reln, forknum);' twice for each
> > relation: in smgrdounlinkall() and in smgrrelease(), I'm not sure what
> > this change benefits to us. Particularly, we quick return from the
> > second mdclose() call as all segment files are already closed. Have
> > you experienced a case like where the system got stuck or got very
> > slower due to these duplicated calls?
> >
> > Also, we might need to pay attention that with the patch
> > smgrdounlinkall() came to depend on smgrclose(). For example, the more
> > works we add in smgrclose() in the future, the more works
> > smgrdounlinkall() will do, which doesn't happen with the current code
> > as smgrdounlinkall() depends on mdclose().
> >
> > Given that the patched codes doesn't do exactly the same things as
> > before (e.g, smgrdounlinkall() would end up resetting
> > reln->smgr_cached_nblocks[forknum] too), I think we need some reasons
> > for legitimating this change.
> >
> > Regards,
> >
>
>
> Hi, Masahiko
>
> Thank you for your detailed review and valuable insights. I understand
> your concern about the immediate benefits of removing the duplicate
> smgr_close() call, especially since the second call effectively becomes
> a no-op due to the prior file closures. However, the primary intent of
> my change is not driven by performance improvements or addressing a
> specific issue, but rather by enhancing the code's structure and
> clarity. Having two "Close the forks at smgr level" operations might
> lead to confusion for readers of the code.
>
>
> Additionally, the smgrclose() function not only closes the smgr but also
> resets smgr_cached_nblocks and smgr_targblock, making it a comprehensive
> operation. In the current implementation, the smgr is closed inside
> smgrdounlinkall(), while smgr_cached_nblocks and smgr_targblock are
> reset outside of it. This creates a fragmentation in the code logic,
> which could be streamlined.

I've investigated the code further. It seems like smgrclose() became
just an alias of smgrrelease() by commit 21d9c3ee4ef, making the
duplications of calling mdclose() for each relation and its fork by
smgrclose() and smgrunlinkall(). I'm hesitant with the first proposal
of removing smgrclose() calls following smgrunlinkall() because, as
you also pointed out, we would miss some operations (e.g. resetting
smgr_cached_nblocks), and it would create another coding convention
that we can omit smgrclose() only after smgrunlinkall(), causing the
code divergences, which might introduce another confusion.

> Would it make sense to remove the smgr closing operation within
> smgrdounlinkall() and leave the rest of the code unchanged?

If I understand your idea correctly, smgrdounlinkall() would end up
calling mdunlink() for each relation without mdclose(). I don't think
it's okay.

Regards,

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