Thread: DropRelFileLocatorBuffers
Hello. While working on a patch, I met a function with the signature of: > DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum, > int nforks, BlockNumber *firstDelBlock) It was DropRelFileNodeBuffers(), which means "Drop buffers for a RelFileNode", where RelFileNode means a storage or a (set of) file(s). In that sense, "Drop buffers for a RelFile*Locator*" sounds a bit off to me. Isn't it better change the name? RelFileLocator doesn't look to be fit here. "DropRelFileBuffers" works better at least for me.. If it does, some other functions need the same amendment. Thought? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Jul 7, 2022 at 4:44 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > While working on a patch, I met a function with the signature of: > > > DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum, > > int nforks, BlockNumber *firstDelBlock) > > It was DropRelFileNodeBuffers(), which means "Drop buffers for a > RelFileNode", where RelFileNode means a storage or a (set of) file(s). > In that sense, "Drop buffers for a RelFile*Locator*" sounds a bit off > to me. Isn't it better change the name? RelFileLocator doesn't look > to be fit here. > > "DropRelFileBuffers" works better at least for me.. If it does, some > other functions need the same amendment. Have you looked at the commit message for https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b0a55e43299c4ea2a9a8c757f9c26352407d0ccc ? -- Robert Haas EDB: http://www.enterprisedb.com
At Thu, 7 Jul 2022 08:36:14 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > On Thu, Jul 7, 2022 at 4:44 AM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > While working on a patch, I met a function with the signature of: > > > > > DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum, > > > int nforks, BlockNumber *firstDelBlock) > > > > It was DropRelFileNodeBuffers(), which means "Drop buffers for a > > RelFileNode", where RelFileNode means a storage or a (set of) file(s). > > In that sense, "Drop buffers for a RelFile*Locator*" sounds a bit off > > to me. Isn't it better change the name? RelFileLocator doesn't look > > to be fit here. > > > > "DropRelFileBuffers" works better at least for me.. If it does, some > > other functions need the same amendment. > > Have you looked at the commit message for > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b0a55e43299c4ea2a9a8c757f9c26352407d0ccc > ? Thanks for the reply. Yes if it is "RelFileLocator when we're talking about all the things that are needed to locate a relation's files on disk,". I read this as RelFileLocator is a kind of pointer to files. I thought RelFileNode as a pointer as well as the storage itself. The difference of the two for me could be analogized as the difference between "DropFileBuffers" and "DropFileNameBuffers". I think the latter is usually spelled as "DropBuffersByFileNames" or such. Though, I don't want to keep fighting any further if others don't feel it uneasy ;) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Jul 7, 2022 at 8:22 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > Thanks for the reply. > > Yes if it is "RelFileLocator when we're talking about all the things > that are needed to locate a relation's files on disk,". I read this as > RelFileLocator is a kind of pointer to files. I thought RelFileNode > as a pointer as well as the storage itself. The difference of the two > for me could be analogized as the difference between "DropFileBuffers" > and "DropFileNameBuffers". I think the latter is usually spelled as > "DropBuffersByFileNames" or such. > > Though, I don't want to keep fighting any further if others don't feel > it uneasy ;) I wouldn't mind if we took "Locator" out of the name of that function and just called it DropRelFileBuffers or DropRelationBuffers or something. That would be shorter, and maybe more intuitive. I wasn't quite able to understand whether your original question was prompted by having missed the commit in question, or whether you disagreed with it, so that's why I asked whether you had seen the commit message. -- Robert Haas EDB: http://www.enterprisedb.com
At Thu, 7 Jul 2022 21:13:59 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > On Thu, Jul 7, 2022 at 8:22 PM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > Thanks for the reply. > > > > Yes if it is "RelFileLocator when we're talking about all the things > > that are needed to locate a relation's files on disk,". I read this as > > RelFileLocator is a kind of pointer to files. I thought RelFileNode > > as a pointer as well as the storage itself. The difference of the two > > for me could be analogized as the difference between "DropFileBuffers" > > and "DropFileNameBuffers". I think the latter is usually spelled as > > "DropBuffersByFileNames" or such. > > > > Though, I don't want to keep fighting any further if others don't feel > > it uneasy ;) > > I wouldn't mind if we took "Locator" out of the name of that function > and just called it DropRelFileBuffers or DropRelationBuffers or > something. That would be shorter, and maybe more intuitive. Thanks. Will propose that. > I wasn't quite able to understand whether your original question was > prompted by having missed the commit in question, or whether you > disagreed with it, so that's why I asked whether you had seen the > commit message. The commit message is very helpful to understand the aim of the patch. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Fri, 08 Jul 2022 11:52:45 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > > I wouldn't mind if we took "Locator" out of the name of that function > > and just called it DropRelFileBuffers or DropRelationBuffers or > > something. That would be shorter, and maybe more intuitive. > > Thanks. Will propose that. I thought for a moment that "Relation" sounded better but that naming is confusing in bufmgr.c, where functions take Relation and those take RelFileLocator exist together. So the (second) attached introduces "RelFile" to represent RelFileNode excluding RelFileLocator. The function CreateAndCopyRelationData exists since before b0a55e4329 but renamed since it takes RelFileLocators. While working on this, I found that the following coment is wrong. * FlushRelationsAllBuffers * * This function flushes out of the buffer pool all the pages of all * forks of the specified smgr relations. It's equivalent to calling * FlushRelationBuffers once per fork per relation. The relations are * assumed not to use local buffers. It is equivalent to calling FlushRelationBuffers "per relation". This is attached as the first patch, which could be thought as a separate patch. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Attachment
On Fri, Jul 8, 2022 at 1:59 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > I thought for a moment that "Relation" sounded better but that naming > is confusing in bufmgr.c, where functions take Relation and those take > RelFileLocator exist together. So the (second) attached introduces > "RelFile" to represent RelFileNode excluding RelFileLocator. > > The function CreateAndCopyRelationData exists since before b0a55e4329 > but renamed since it takes RelFileLocators. I'm not very sold on this. I think that the places where you've replaced RelFileLocator with just RelFile in various functions might be an improvement, but the places where you've replaced Relation with RelFile seem to me to be worse. I don't really see that there's anything wrong with names like CreateAndCopyRelationData or FlushRelationsAllBuffers, and in general I prefer function names that are made up of whole words rather than parts of words. > While working on this, I found that the following coment is wrong. > > * FlushRelationsAllBuffers > * > * This function flushes out of the buffer pool all the pages of all > * forks of the specified smgr relations. It's equivalent to calling > * FlushRelationBuffers once per fork per relation. The relations are > * assumed not to use local buffers. > > It is equivalent to calling FlushRelationBuffers "per relation". This > is attached as the first patch, which could be thought as a separate > patch. I committed this. -- Robert Haas EDB: http://www.enterprisedb.com
At Mon, 11 Jul 2022 13:51:12 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > On Fri, Jul 8, 2022 at 1:59 AM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > The function CreateAndCopyRelationData exists since before b0a55e4329 > > but renamed since it takes RelFileLocators. > > I'm not very sold on this. I think that the places where you've > replaced RelFileLocator with just RelFile in various functions might > be an improvement, but the places where you've replaced Relation with > RelFile seem to me to be worse. I don't really see that there's > anything wrong with names like CreateAndCopyRelationData or > FlushRelationsAllBuffers, and in general I prefer function names that > are made up of whole words rather than parts of words. Fair enough. My first thought was that Relation can represent both Relation and "RelFile" but in the patch I choosed to make distinction between them by associating respectively to the types of the primary parameter (Relation or RelFileLocator). So I'm fine with Relation instead since I see it more intuitive than RelFileLocator in the function names. The attached is that. > > While working on this, I found that the following coment is wrong. .. > I committed this. Thanks! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Attachment
On Tue, Jul 12, 2022 at 7:55 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Mon, 11 Jul 2022 13:51:12 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > > On Fri, Jul 8, 2022 at 1:59 AM Kyotaro Horiguchi > > <horikyota.ntt@gmail.com> wrote: > > > The function CreateAndCopyRelationData exists since before b0a55e4329 > > > but renamed since it takes RelFileLocators. > > > > I'm not very sold on this. I think that the places where you've > > replaced RelFileLocator with just RelFile in various functions might > > be an improvement, but the places where you've replaced Relation with > > RelFile seem to me to be worse. I don't really see that there's > > anything wrong with names like CreateAndCopyRelationData or > > FlushRelationsAllBuffers, and in general I prefer function names that > > are made up of whole words rather than parts of words. > > Fair enough. My first thought was that Relation can represent both > Relation and "RelFile" but in the patch I choosed to make distinction > between them by associating respectively to the types of the primary > parameter (Relation or RelFileLocator). So I'm fine with Relation > instead since I see it more intuitive than RelFileLocator in the > function names. > > The attached is that. I think the naming used in your patch looks better to me. So +1 for the change. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Tue, Jul 12, 2022 at 12:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > I think the naming used in your patch looks better to me. So +1 for the change. Committed. -- Robert Haas EDB: http://www.enterprisedb.com
At Tue, 12 Jul 2022 10:30:20 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > On Tue, Jul 12, 2022 at 12:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > I think the naming used in your patch looks better to me. So +1 for the change. > > Committed. Thank you, Robert and Dilip. regards. -- Kyotaro Horiguchi NTT Open Source Software Center