Re: DropRelFileLocatorBuffers
At Tue, 12 Jul 2022 10:30:20 -0400, Robert Haas wrote in > On Tue, Jul 12, 2022 at 12:07 AM Dilip Kumar 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
Re: DropRelFileLocatorBuffers
On Tue, Jul 12, 2022 at 12:07 AM Dilip Kumar 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
Re: DropRelFileLocatorBuffers
On Tue, Jul 12, 2022 at 7:55 AM Kyotaro Horiguchi wrote: > > At Mon, 11 Jul 2022 13:51:12 -0400, Robert Haas wrote > in > > On Fri, Jul 8, 2022 at 1:59 AM Kyotaro Horiguchi > > 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
Re: DropRelFileLocatorBuffers
At Mon, 11 Jul 2022 13:51:12 -0400, Robert Haas wrote
in
> On Fri, Jul 8, 2022 at 1:59 AM Kyotaro Horiguchi
> 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
>From dbb3dbebbea6aae29588bfccdbda28d0cf15a6fe Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Fri, 8 Jul 2022 13:20:31 +0900
Subject: [PATCH v2] Rename some *RelFileLocator* functions
A recent commit b0a55e4329 changed the term RelFileNode to
RelFileLocator. Accordingly the names of some functions were
changed. However, since RelFileLocator doesn't fully cover what
RelFileNode represented, some of the function names have gotten less
intuitive.
This commit replaces all uses of RelFileLocator in function names with
Relation because that uses of RelFileLocator actually mean storage
object.
---
src/backend/storage/buffer/bufmgr.c | 63 +--
src/backend/storage/buffer/localbuf.c | 16 +++
src/backend/storage/smgr/smgr.c | 4 +-
src/include/storage/buf_internals.h | 7 +--
src/include/storage/bufmgr.h | 10 ++---
5 files changed, 50 insertions(+), 50 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index e257ae23e4..c7d7abcd73 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -121,7 +121,7 @@ typedef struct CkptTsStatus
* Type for array used to sort SMgrRelations
*
* FlushRelationsAllBuffers shares the same comparator function with
- * DropRelFileLocatorsAllBuffers. Pointer to this struct and RelFileLocator must be
+ * DropRelationsAllBuffers. Pointer to this struct and RelFileLocator must be
* compatible.
*/
typedef struct SMgrSortArray
@@ -483,10 +483,10 @@ static BufferDesc *BufferAlloc(SMgrRelation smgr,
BufferAccessStrategy strategy,
bool *foundPtr);
static void FlushBuffer(BufferDesc *buf, SMgrRelation reln);
-static void FindAndDropRelFileLocatorBuffers(RelFileLocator rlocator,
- ForkNumber forkNum,
- BlockNumber nForkBlock,
- BlockNumber firstDelBlock);
+static void FindAndDropRelationBuffers(RelFileLocator rlocator,
+ ForkNumber forkNum,
+ BlockNumber nForkBlock,
+ BlockNumber firstDelBlock);
static void RelationCopyStorageUsingBuffer(Relation src, Relation dst,
ForkNumber forkNum,
bool isunlogged);
@@ -3026,7 +3026,7 @@ BufferGetLSNAtomic(Buffer buffer)
}
/* -
- * DropRelFileLocatorBuffers
+ * DropRelationBuffers
*
* This function removes from the buffer pool all the pages of the
* specified relation forks that have block numbers >= firstDelBlock.
@@ -3047,8 +3047,8 @@ BufferGetLSNAtomic(Buffer buffer)
*
*/
void
-DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
- int nforks, BlockNumber *firstDelBlock)
+DropRelationBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
+ int nforks, BlockNumber *firstDelBlock)
{
int i;
int j;
@@ -3064,8 +3064,8 @@ DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
if (rlocator.backend == MyBackendId)
{
for (j = 0; j < nforks; j++)
-DropRelFileLocatorLocalBuffers(rlocator.locator, forkNum[j],
- firstDelBlock[j]);
+DropRelationLocalBuffers(rlocator.locator, forkNum[j],
+ firstDelBlock[j]);
}
return;
}
@@ -3115,8 +3115,8 @@ DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
{
for (j = 0; j < nforks; j++)
- FindAndDropRelFileLocatorBuffers(rlocator.locator, forkNum[j],
- nForkBlock[j], firstDelBloc
Re: DropRelFileLocatorBuffers
On Fri, Jul 8, 2022 at 1:59 AM Kyotaro Horiguchi 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
Re: DropRelFileLocatorBuffers
,10 @@ static BufferDesc *BufferAlloc(SMgrRelation smgr,
BufferAccessStrategy strategy,
bool *foundPtr);
static void FlushBuffer(BufferDesc *buf, SMgrRelation reln);
-static void FindAndDropRelFileLocatorBuffers(RelFileLocator rlocator,
- ForkNumber forkNum,
- BlockNumber nForkBlock,
- BlockNumber firstDelBlock);
+static void FindAndDropRelFileBuffers(RelFileLocator rlocator,
+ ForkNumber forkNum,
+ BlockNumber nForkBlock,
+ BlockNumber firstDelBlock);
static void RelationCopyStorageUsingBuffer(Relation src, Relation dst,
ForkNumber forkNum,
bool isunlogged);
@@ -3026,7 +3026,7 @@ BufferGetLSNAtomic(Buffer buffer)
}
/* -
- * DropRelFileLocatorBuffers
+ * DropRelFileBuffers
*
* This function removes from the buffer pool all the pages of the
* specified relation forks that have block numbers >= firstDelBlock.
@@ -3047,8 +3047,8 @@ BufferGetLSNAtomic(Buffer buffer)
*
*/
void
-DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
- int nforks, BlockNumber *firstDelBlock)
+DropRelFileBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
+ int nforks, BlockNumber *firstDelBlock)
{
int i;
int j;
@@ -3064,8 +3064,8 @@ DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
if (rlocator.backend == MyBackendId)
{
for (j = 0; j < nforks; j++)
-DropRelFileLocatorLocalBuffers(rlocator.locator, forkNum[j],
- firstDelBlock[j]);
+DropRelFileLocalBuffers(rlocator.locator, forkNum[j],
+ firstDelBlock[j]);
}
return;
}
@@ -3115,8 +3115,8 @@ DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
{
for (j = 0; j < nforks; j++)
- FindAndDropRelFileLocatorBuffers(rlocator.locator, forkNum[j],
- nForkBlock[j], firstDelBlock[j]);
+ FindAndDropRelFileBuffers(rlocator.locator, forkNum[j],
+ nForkBlock[j], firstDelBlock[j]);
return;
}
@@ -3162,16 +3162,15 @@ DropRelFileLocatorBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
}
/* -
- * DropRelFileLocatorsAllBuffers
+ * DropRelFilesAllBuffers
*
* This function removes from the buffer pool all the pages of all
* forks of the specified relations. It's equivalent to calling
- * DropRelFileLocatorBuffers once per fork per relation with
- * firstDelBlock = 0.
- *
+ * DropRelFileBuffers once per fork per relation with firstDelBlock = 0.
+ *
*/
void
-DropRelFileLocatorsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
+DropRelFilesAllBuffers(SMgrRelation *smgr_reln, int nlocators)
{
int i;
int j;
@@ -3194,7 +3193,7 @@ DropRelFileLocatorsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
if (RelFileLocatorBackendIsTemp(smgr_reln[i]->smgr_rlocator))
{
if (smgr_reln[i]->smgr_rlocator.backend == MyBackendId)
-DropRelFileLocatorAllLocalBuffers(smgr_reln[i]->smgr_rlocator.locator);
+DropRelFileAllLocalBuffers(smgr_reln[i]->smgr_rlocator.locator);
}
else
rels[n++] = smgr_reln[i];
@@ -3219,7 +3218,7 @@ DropRelFileLocatorsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
/*
* We can avoid scanning the entire buffer pool if we know the exact size
- * of each of the given relation forks. See DropRelFileLocatorBuffers.
+ * of each of the given relation forks. See DropRelFileBuffers.
*/
for (i = 0; i < n && cached; i++)
{
@@ -3257,8 +3256,8 @@ DropRelFileLocatorsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
continue;
/* drop all the buffers for a particular relation fork */
-FindAndDropRelFileLocatorBuffers(rels[i]->smgr_rlocator.locator,
- j, block[i][j], 0);
+FindAndDropRelFileBuffers(rels[i]->smgr_rlocator.locator,
+ j, block[i][j], 0);
}
}
@@ -3291,7 +3290,7 @@ DropRelFileLocatorsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
uint32 buf_state;
/*
- * As in DropRelFileLocatorBuffers, an unlocked precheck should be
+ * As in DropRelFileBuffers, an unlocked precheck should be
* safe and saves some cycles.
*/
@@ -3331,7 +3330,7 @@ DropRelFileLocatorsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
}
/* -
- * FindAndDropRelFileLocatorBuffers
+ * FindAndDropRelFileBuffers
*
* This function performs look up in BufMapping table and removes from the
* buffer pool all the pages of the specified relation fork that has block
@@ -3340,9 +
Re: DropRelFileLocatorBuffers
At Thu, 7 Jul 2022 21:13:59 -0400, Robert Haas wrote in > On Thu, Jul 7, 2022 at 8:22 PM Kyotaro Horiguchi > 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
Re: DropRelFileLocatorBuffers
On Thu, Jul 7, 2022 at 8:22 PM Kyotaro Horiguchi 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
Re: DropRelFileLocatorBuffers
At Thu, 7 Jul 2022 08:36:14 -0400, Robert Haas wrote in > On Thu, Jul 7, 2022 at 4:44 AM Kyotaro Horiguchi > 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
Re: DropRelFileLocatorBuffers
On Thu, Jul 7, 2022 at 4:44 AM Kyotaro Horiguchi 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
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
