On Mon, Mar 10, 2025 at 3:08 AM Steven Niu <niush...@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
