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
