Re: [Patch] remove duplicated smgrclose

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

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