Subject: Re: [Patch] remove duplicated smgrclose

From: Junwang Zhao <[email-masked]>
Date: Fri, 9 Aug 2024 18:19:11 +0800

On Fri, Aug 9, 2024 at 5:20 PM Kirill Reshke <[email-masked]> wrote:
>
> On Thu, 1 Aug 2024 at 17:32, Junwang Zhao <[email-masked]> wrote:
> >
> > Hi Steven,
> >
> > On Wed, Jul 31, 2024 at 11:16 AM Steven Niu <[email-masked]> 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

---

From: Steven Niu <[email-masked]>
Date: Mon, 12 Aug 2024 18:11:42 +0800

Kirill,

Good catch!
I will split the patch into two to cover both cases.

Thanks,
Steven

---

From: Steven Niu <[email-masked]>
Date: Wed, 14 Aug 2024 14:35:39 +0800

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

---

From: Junwang Zhao <[email-masked]>
Date: Thu, 15 Aug 2024 18:03:45 +0800

On Wed, Aug 14, 2024 at 2:35 PM Steven Niu <[email-masked]> 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 ;)

-- 
Regards
Junwang Zhao

---

From: Steven Niu <[email-masked]>
Date: Fri, 16 Aug 2024 13:16:33 +0800

> >>> Steven, could you do this?

OK， thanks for your kind help.

Steven

---

From: Cary Huang <[email-masked]>
Date: Fri, 23 Aug 2024 21:15:29 +0000

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

---

From: Kirill Reshke <[email-masked]>
Date: Mon, 28 Oct 2024 00:05:11 +0500

>>> Steven, could you do this?

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

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 7 Mar 2025 20:04:19 -0800

Hi,

> left, so I will try to mark commitfest [1] entry as Ready For

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

---

From: Junwang Zhao <[email-masked]>
Date: Sat, 8 Mar 2025 17:05:40 +0800

On Sat, Mar 8, 2025 at 12:04 PM Masahiko Sawada <[email-masked]> wrote:
> does more things as follow:.
>
> void

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
Junwang Zhao

---

From: Junwang Zhao <[email-masked]>
Date: Sat, 8 Mar 2025 17:37:42 +0800

Hi Masahiko,

> 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
Junwang Zhao

---

From: Masahiko Sawada <[email-masked]>
Date: Sat, 8 Mar 2025 01:45:58 -0800

> So I agree with you that we would end up missing some operations with

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,

---

From: Steven Niu <[email-masked]>
Date: Mon, 10 Mar 2025 18:07:34 +0800

> some operations that are done in smgrrelease() but not in

 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
<https://www.postgresql.org/message-id/attachment/163268/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.

Best Regards,
Steven

---

From: Junwang Zhao <[email-masked]>
Date: Tue, 11 Mar 2025 09:38:13 +0800

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

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

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.

-- 
Regards
Junwang Zhao

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 11 Mar 2025 15:31:42 -0700

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

---

From: Steven Niu <[email-masked]>
Date: Thu, 13 Mar 2025 16:36:58 +0800

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

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 25 Mar 2025 22:43:05 -0700

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

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