Обсуждение: How about renaming XLogFileRead() to XLogFileOpenForRead() and XLogFileOpen() to XLogFileOpenForWrite()?

Hi All,

Currently, in postgres we have two different functions that are
specially used to open the WAL files for reading and writing purposes.
The first one is XLogFileOpen() that is just used to open the WAL file
so that we can write WAL data in it. And then we have another function
named XLogFileRead that does the same thing but is used when reading
the WAL files during recovery time. How about renaming the function
XLogFileRead to XLogFileOpenForRead and the other one can be renamed
to XLogFileOpenForWrite. I think it will make the function name more
clear and increase the readability. At least XlogFileRead doesn't look
good to me, from the function name it actually appears like we are
trying to read a WAL file here but actually we are opening it so that
it can be read by some other routine.

Also I see that we are passing emode to the XLogFileRead function
which is not being used anywhere in the function, so can we remove it?

--
With Regards,
Ashutosh Sharma.

On Tue, May 10, 2022 at 6:16 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi All,
>
> Currently, in postgres we have two different functions that are
> specially used to open the WAL files for reading and writing purposes.
> The first one is XLogFileOpen() that is just used to open the WAL file
> so that we can write WAL data in it. And then we have another function
> named XLogFileRead that does the same thing but is used when reading
> the WAL files during recovery time. How about renaming the function
> XLogFileRead to XLogFileOpenForRead and the other one can be renamed
> to XLogFileOpenForWrite. I think it will make the function name more
> clear and increase the readability. At least XlogFileRead doesn't look
> good to me, from the function name it actually appears like we are
> trying to read a WAL file here but actually we are opening it so that
> it can be read by some other routine.
>
> Also I see that we are passing emode to the XLogFileRead function
> which is not being used anywhere in the function, so can we remove it?

Renaming XLogFileOpen to XLogFileOpenForWrite while it uses O_RDWR,
not O_RDWR is sort of conflicting. Also, I'm concerned that
XLogFileOpen is an extern function, the external modules using it
might break. XLogFileRead uses O_RDONLY and is a static function, so
it might be okay to change the name, the only concern is that it
creates diff with the older versions as we usually don't backport
renaming functions or variables/code improvements/not-so-critical
changes.

Having said that, IMHO, the existing functions and their names look
fine to me (developers can read the function/function comments to
understand their usage though).

Regards,
Bharath Rupireddy.

On Tue, May 10, 2022 at 6:46 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, May 10, 2022 at 6:16 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > Hi All,
> >
> > Currently, in postgres we have two different functions that are
> > specially used to open the WAL files for reading and writing purposes.
> > The first one is XLogFileOpen() that is just used to open the WAL file
> > so that we can write WAL data in it. And then we have another function
> > named XLogFileRead that does the same thing but is used when reading
> > the WAL files during recovery time. How about renaming the function
> > XLogFileRead to XLogFileOpenForRead and the other one can be renamed
> > to XLogFileOpenForWrite. I think it will make the function name more
> > clear and increase the readability. At least XlogFileRead doesn't look
> > good to me, from the function name it actually appears like we are
> > trying to read a WAL file here but actually we are opening it so that
> > it can be read by some other routine.
> >
> > Also I see that we are passing emode to the XLogFileRead function
> > which is not being used anywhere in the function, so can we remove it?
>
> Renaming XLogFileOpen to XLogFileOpenForWrite while it uses O_RDWR,
> not O_RDWR is sort of conflicting. Also, I'm concerned that
> XLogFileOpen is an extern function, the external modules using it
> might break.

Why would the external modules open WAL files to perform write
operations? AFAIU from the code, this function is specifically written
to open WAL files during WAL write operation. So I don't see any
problem in renaming it to XLogFileOpenForWrite(). Infact as I said, it
does increase the readability. And likewise, XLogFileRead can be
renamed to XLogFileOpenForRead which seems you are okay with.

--
With Regards,
Ashutosh Sharma.