Use XLOG_CONTROL_FILE macro everywhere?

On 19 Apr 2024, at 05:50, Anton A. Melnikov <a.melnikov@postgrespro.ru> wrote:

May be better use this macro everywhere in C code?

Off the cuff that seems to make sense, it does make it easier to grep for uses
of the control file.

--
Daniel Gustafsson

On Fri, Apr 19, 2024 at 10:12:14AM +0200, Daniel Gustafsson wrote:

Off the cuff that seems to make sense, it does make it easier to grep for uses
of the control file.

+1 for switching to the macro where we can.  Still, I don't see a
point in rushing and would just switch that once v18 opens up for
business. 
--
Michael

On 20 Apr 2024, at 01:23, Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Apr 19, 2024 at 10:12:14AM +0200, Daniel Gustafsson wrote:

Off the cuff that seems to make sense, it does make it easier to grep for uses
of the control file.

+1 for switching to the macro where we can. Still, I don't see a
point in rushing and would just switch that once v18 opens up for
business.

Absolutely, this is not fixing a defect so it's v18 material.

Anton: please register this patch in the Open commitfest to ensure it's not
forgotten about.

--
Daniel Gustafsson

On 20.04.2024 09:36, Daniel Gustafsson wrote:

Anton: please register this patch in the Open commitfest to ensure it's not
forgotten about.

Done.

Daniel and Michael thank you!

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 19.04.24 05:50, Anton A. Melnikov wrote:

There is a macro XLOG_CONTROL_FILE for control file name
defined in access/xlog_internal.h
And there are some places in code where this macro is used
like here
https://github.com/postgres/postgres/blob/84db9a0eb10dd1dbee6db509c0e427fa237177dc/src/bin/pg_resetwal/pg_resetwal.c#L588
or here
https://github.com/postgres/postgres/blob/84db9a0eb10dd1dbee6db509c0e427fa237177dc/src/common/controldata_utils.c#L214

But there are some other places where the control file
name is used as text string directly.

May be better use this macro everywhere in C code?

I don't know. I don't find XLOG_CONTROL_FILE to be a very intuitive
proxy for "pg_control".

On 24.04.2024 12:02, Peter Eisentraut wrote:

On 19.04.24 05:50, Anton A. Melnikov wrote:

May be better use this macro everywhere in C code?

I don't know.  I don't find XLOG_CONTROL_FILE to be a very intuitive proxy for "pg_control".

Then maybe replace XLOG_CONTROL_FILE with PG_CONTROL_FILE?

The PG_CONTROL_FILE_SIZE macro is already in the code.

With the best regards,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 24 Apr 2024, at 11:13, Anton A. Melnikov <a.melnikov@postgrespro.ru> wrote:

On 24.04.2024 12:02, Peter Eisentraut wrote:

On 19.04.24 05:50, Anton A. Melnikov wrote:

May be better use this macro everywhere in C code?

I don't know. I don't find XLOG_CONTROL_FILE to be a very intuitive proxy for "pg_control".

Maybe, but inconsistent usage is also unintuitive.

Then maybe replace XLOG_CONTROL_FILE with PG_CONTROL_FILE?

The PG_CONTROL_FILE_SIZE macro is already in the code.
With the best regards,

XLOG_CONTROL_FILE is close to two decades old so there might be extensions
using it (though the risk might be slim), perhaps using offering it as as well
as backwards-compatability is warranted should we introduce a new name?

--
Daniel Gustafsson

On 24.04.2024 12:19, Daniel Gustafsson wrote:

On 24 Apr 2024, at 11:13, Anton A. Melnikov <a.melnikov@postgrespro.ru> wrote:

On 24.04.2024 12:02, Peter Eisentraut wrote:

On 19.04.24 05:50, Anton A. Melnikov wrote:

May be better use this macro everywhere in C code?

I don't know. I don't find XLOG_CONTROL_FILE to be a very intuitive proxy for "pg_control".

Maybe, but inconsistent usage is also unintuitive.

Then maybe replace XLOG_CONTROL_FILE with PG_CONTROL_FILE?

The PG_CONTROL_FILE_SIZE macro is already in the code.
With the best regards,

XLOG_CONTROL_FILE is close to two decades old so there might be extensions
using it (though the risk might be slim), perhaps using offering it as as well
as backwards-compatability is warranted should we introduce a new name?

To ensure backward compatibility we can save the old macro like this:

#define XLOG_CONTROL_FILE "global/pg_control"
#define PG_CONTROL_FILE XLOG_CONTROL_FILE

With the best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Wed, Apr 24, 2024 at 12:32:46PM +0300, Anton A. Melnikov wrote:

To ensure backward compatibility we can save the old macro like this:

#define XLOG_CONTROL_FILE "global/pg_control"
#define PG_CONTROL_FILE XLOG_CONTROL_FILE

With the best wishes,

Not sure that I would bother with a second one. But, well, why not if
people want to rename it, as long as you keep compatibility.
--
Michael

On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 24, 2024 at 12:32:46PM +0300, Anton A. Melnikov wrote:

To ensure backward compatibility we can save the old macro like this:

#define XLOG_CONTROL_FILE "global/pg_control"
#define PG_CONTROL_FILE XLOG_CONTROL_FILE

With the best wishes,

Not sure that I would bother with a second one. But, well, why not if
people want to rename it, as long as you keep compatibility.

I vote for just standardizing on XLOG_CONTROL_FILE. That name seems
sufficiently intuitive to me, and I'd rather have one identifier for
this than two. It's simpler that way.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier <michael@paquier.xyz> wrote:

Not sure that I would bother with a second one. But, well, why not if
people want to rename it, as long as you keep compatibility.

I vote for just standardizing on XLOG_CONTROL_FILE. That name seems
sufficiently intuitive to me, and I'd rather have one identifier for
this than two. It's simpler that way.

+1. Back when we did the great xlog-to-wal renaming, we explicitly
agreed that we wouldn't change internal symbols referring to xlog.
It might or might not be appropriate to revisit that decision,
but I sure don't want to do it piecemeal, one symbol at a time.

Also, if we did rename this one, the logical choice would be
WAL_CONTROL_FILE not PG_CONTROL_FILE.

regards, tom lane

On 26.04.24 22:51, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier <michael@paquier.xyz> wrote:

Not sure that I would bother with a second one. But, well, why not if
people want to rename it, as long as you keep compatibility.

I vote for just standardizing on XLOG_CONTROL_FILE. That name seems
sufficiently intuitive to me, and I'd rather have one identifier for
this than two. It's simpler that way.

+1. Back when we did the great xlog-to-wal renaming, we explicitly
agreed that we wouldn't change internal symbols referring to xlog.
It might or might not be appropriate to revisit that decision,
but I sure don't want to do it piecemeal, one symbol at a time.

Also, if we did rename this one, the logical choice would be
WAL_CONTROL_FILE not PG_CONTROL_FILE.

My reasoning was mainly that I don't see pg_control as controlling just
the WAL. But I don't feel strongly about instigating a great renaming
here or something.

On 27 Apr 2024, at 11:12, Peter Eisentraut <peter@eisentraut.org> wrote:

On 26.04.24 22:51, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier <michael@paquier.xyz> wrote:

Not sure that I would bother with a second one. But, well, why not if
people want to rename it, as long as you keep compatibility.

I vote for just standardizing on XLOG_CONTROL_FILE. That name seems
sufficiently intuitive to me, and I'd rather have one identifier for
this than two. It's simpler that way.

+1. Back when we did the great xlog-to-wal renaming, we explicitly
agreed that we wouldn't change internal symbols referring to xlog.
It might or might not be appropriate to revisit that decision,
but I sure don't want to do it piecemeal, one symbol at a time.
Also, if we did rename this one, the logical choice would be
WAL_CONTROL_FILE not PG_CONTROL_FILE.

My reasoning was mainly that I don't see pg_control as controlling just the WAL. But I don't feel strongly about instigating a great renaming here or something.

Summarizing the thread it seems consensus is using XLOG_CONTROL_FILE
consistently as per the original patch.

A few comments on the patch though:

- * reads the data from $PGDATA/global/pg_control
+ * reads the data from $PGDATA/<control file>

I don't think this is an improvement, I'd leave that one as the filename
spelled out.

- "the \".old\" suffix from %s/global/pg_control.old.\n"
+ "the \".old\" suffix from %s/%s.old.\n"

Same with that change, not sure I think that makes reading the errormessage
code any easier.

--
Daniel Gustafsson

At Mon, 2 Sep 2024 15:44:45 +0200, Daniel Gustafsson <daniel@yesql.se> wrote in

Summarizing the thread it seems consensus is using XLOG_CONTROL_FILE
consistently as per the original patch.

A few comments on the patch though:

- * reads the data from $PGDATA/global/pg_control
+ * reads the data from $PGDATA/<control file>

I don't think this is an improvement, I'd leave that one as the filename
spelled out.

- "the \".old\" suffix from %s/global/pg_control.old.\n"
+ "the \".old\" suffix from %s/%s.old.\n"

Same with that change, not sure I think that makes reading the errormessage
code any easier.

I agree with the first point. In fact, I think it might be even better
to just write something like "reads the data from the control file" in
plain language rather than using the actual file name. As for the
second point, I'm fine either way, but if the main goal is to ensure
resilience against future changes to the value of XLOG_CONTROL_FILE,
then changing it makes sense. On the other hand, I don't see any
strong reasons not to change it. That said, I don't really expect the
value to change in the first place.

The following point caught my attention.

+++ b/src/backend/postmaster/postmaster.c

...

+#include "access/xlog_internal.h"

The name xlog_internal suggests that the file should only be included
by modules dealing with XLOG details, and postmaster.c doesn't seem to
fit that category. If the macro is used more broadly, it might be
better to move it to a more public location. However, following the
current discussion, if we decide to keep the macro's name as it is, it
would make more sense to keep it in its current location.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 03.09.2024 08:37, Kyotaro Horiguchi wrote:

At Mon, 2 Sep 2024 15:44:45 +0200, Daniel Gustafsson <daniel@yesql.se> wrote in

Summarizing the thread it seems consensus is using XLOG_CONTROL_FILE
consistently as per the original patch.

1)

A few comments on the patch though:

- * reads the data from $PGDATA/global/pg_control
+ * reads the data from $PGDATA/<control file>

I don't think this is an improvement, I'd leave that one as the filename
spelled out.

I agree with the first point. In fact, I think it might be even better
to just write something like "reads the data from the control file" in
plain language rather than using the actual file name.

Thanks for remarks! Agreed with both. Tried to fix in v2 attached.

2)

- "the \".old\" suffix from %s/global/pg_control.old.\n"
+ "the \".old\" suffix from %s/%s.old.\n"

Same with that change, not sure I think that makes reading the errormessage
code any easier.

As for the
second point, I'm fine either way, but if the main goal is to ensure
resilience against future changes to the value of XLOG_CONTROL_FILE,
then changing it makes sense. On the other hand, I don't see any
strong reasons not to change it. That said, I don't really expect the
value to change in the first place.

In v2 removed XLOG_CONTROL_FILE from args and used it directly in the string.
IMHO this makes the code more readable and will output the correct
text if the macro changes.

3)

The following point caught my attention.

+++ b/src/backend/postmaster/postmaster.c

...

+#include "access/xlog_internal.h"

The name xlog_internal suggests that the file should only be included
by modules dealing with XLOG details, and postmaster.c doesn't seem to
fit that category. If the macro is used more broadly, it might be
better to move it to a more public location. However, following the
current discussion, if we decide to keep the macro's name as it is, it
would make more sense to keep it in its current location.

Maybe include/access/xlogdefs.h would be a good place for this?
In v2 moved some definitions from xlog_internal to xlogdefs
and removed including xlog_internal.h from the postmaster.c.
Please, take a look on it.

With the best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

At Tue, 3 Sep 2024 12:02:26 +0300, "Anton A. Melnikov" <a.melnikov@postgrespro.ru> wrote in

In v2 removed XLOG_CONTROL_FILE from args and used it directly in the
string.
IMHO this makes the code more readable and will output the correct
text if the macro changes.

Yeah, I had this in my mind. Looks good to me.

3)

..

Maybe include/access/xlogdefs.h would be a good place for this?
In v2 moved some definitions from xlog_internal to xlogdefs
and removed including xlog_internal.h from the postmaster.c.
Please, take a look on it.

The change can help avoid disrupting existing users of the
macro. However, the file is documented as follows:

* Postgres write-ahead log manager record pointer and
* timeline number definitions

We could modify the file definition, but I'm not sure if that's the
best approach. Instead, I'd like to propose separating the file and
path-related definitions from xlog_internal.h, as shown in the
attached first patch. This change would allow some modules to include
files without unnecessary details.

The second file is your patch, adjusted based on the first patch.

I’d appreciate hearing from others on whether they find the first
patch worthwhile. If it’s not considered worthwhile, then I believe
having postmaster include xlog_internal.h would be the best approach.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 04.09.2024 11:09, Kyotaro Horiguchi wrote:

Instead, I'd like to propose separating the file and
path-related definitions from xlog_internal.h, as shown in the
attached first patch. This change would allow some modules to include
files without unnecessary details.

The second file is your patch, adjusted based on the first patch.

I’d appreciate hearing from others on whether they find the first
patch worthwhile. If it’s not considered worthwhile, then I believe
having postmaster include xlog_internal.h would be the best approach.

I really liked the idea of ​​extracting only the necessary and logically
complete part from xlog_internal.h.
But now the presence of macros related to the segment sizes
in the xlogfilepaths.h seems does not correspond to its name.
So i suggest further extract size-related definition and macros from
xlogfilepaths.h to xlogfilesize.h.

Here is a patch that tries to do this based on the your first patch.
Would be glad to hear your opinion.

With the best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Rebased all patches on the current master, renumbered
them to be linear and marked as v3 version.

With the best regards,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi!

* Patch needs rebase by CFbot

Rebased the patches onto the current master and marked them as v4.

Best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company