Horton

Thread

Commits

  1. Rename some functions to mention Relation instead of RelFileLocator.

  2. Fix mistake in comment.

  3. Change internal RelFileNode references to RelFileNumber or RelFileLocator.

  1. DropRelFileLocatorBuffers

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-07T08:44:36Z

    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

  2. Re: DropRelFileLocatorBuffers

    Robert Haas <robertmhaas@gmail.com> — 2022-07-07T12:36:14Z

    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

  3. Re: DropRelFileLocatorBuffers

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-08T00:22:38Z

    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

  4. Re: DropRelFileLocatorBuffers

    Robert Haas <robertmhaas@gmail.com> — 2022-07-08T01:13:59Z

    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

  5. Re: DropRelFileLocatorBuffers

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-08T02:52:45Z

    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

  6. Re: DropRelFileLocatorBuffers

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-08T05:59:51Z

    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

  7. Re: DropRelFileLocatorBuffers

    Robert Haas <robertmhaas@gmail.com> — 2022-07-11T17:51:12Z

    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

  8. Re: DropRelFileLocatorBuffers

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-12T02:25:31Z

    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

  9. Re: DropRelFileLocatorBuffers

    Dilip Kumar <dilipbalaut@gmail.com> — 2022-07-12T04:07:29Z

    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

  10. Re: DropRelFileLocatorBuffers

    Robert Haas <robertmhaas@gmail.com> — 2022-07-12T14:30:20Z

    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

  11. Re: DropRelFileLocatorBuffers

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-13T01:35:24Z

    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