Re: [Patch] remove duplicated smgrclose

On Thu, Mar 13, 2025 at 1:37 AM Steven Niu <niushiji(at)gmail(dot)com> wrote:
>
>
>
> 在 2025/3/12 6:31, Masahiko Sawada 写道:
> > On Mon, Mar 10, 2025 at 3:08 AM Steven Niu <niushiji(at)gmail(dot)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