Use XLOG_CONTROL_FILE macro everywhere?

Hello!

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?
The patch attached tries to do this.

Would be glad if take a look on it.

With the best regards,

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

> On 19 Apr 2024, at 05:50, Anton A. Melnikov <a(dot)melnikov(at)postgrespro(dot)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(at)paquier(dot)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(dot)melnikov(at)postgrespro(dot)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(dot)melnikov(at)postgrespro(dot)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(at)paquier(dot)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(at)gmail(dot)com> writes:
> On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier <michael(at)paquier(dot)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(at)gmail(dot)com> writes:
>> On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier <michael(at)paquier(dot)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(at)eisentraut(dot)org> wrote:
>
> On 26.04.24 22:51, Tom Lane wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Wed, Apr 24, 2024 at 8:04 PM Michael Paquier <michael(at)paquier(dot)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(at)yesql(dot)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(at)yesql(dot)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(dot)melnikov(at)postgrespro(dot)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

On 2025/03/28 13:09, Anton A. Melnikov wrote:
> Hi!
>
>> * Patch needs rebase by CFbot
>
> Rebased the patches onto the current master and marked them as v4.

I'm not sure there's clear consensus yet on the changes in the 0001 and
0002 patches, and it might not be worth rushing them in right before
the feature freeze. So for now, I reviewed and updated only the 0003 patch,
since there seems to be agreement on that one.

I've attached the updated version. It fixes a typo and replaces
the remaining hardcoded control file name with the XLOG_CONTROL_FILE macro.

How about committing the attached patch first? We can consider applying
the others later if consensus is reached.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hi!

On 04.04.2025 19:32, Fujii Masao wrote:
> I'm not sure there's clear consensus yet on the changes in the 0001 and
> 0002 patches, and it might not be worth rushing them in right before
> the feature freeze. So for now, I reviewed and updated only the 0003 patch,
> since there seems to be agreement on that one.
>
> I've attached the updated version. It fixes a typo and replaces
> the remaining hardcoded control file name with the XLOG_CONTROL_FILE macro.

Thanks! Looks good for me.
Maybe extend this to perl tests since there are several hardcoded names too?
The patch attached tries to do this.

> How about committing the attached patch first? We can consider applying
> the others later if consensus is reached

Yes, of cause. IMO, this is the best way now.

Best wishes,

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

On 2025/04/05 8:27, Anton A. Melnikov wrote:
> Hi!
>
> On 04.04.2025 19:32, Fujii Masao wrote:
>> I'm not sure there's clear consensus yet on the changes in the 0001 and
>> 0002 patches, and it might not be worth rushing them in right before
>> the feature freeze. So for now, I reviewed and updated only the 0003 patch,
>> since there seems to be agreement on that one.
>>
>> I've attached the updated version. It fixes a typo and replaces
>> the remaining hardcoded control file name with the XLOG_CONTROL_FILE macro.
>
> Thanks! Looks good for me.

Thanks for checking! Barring any objections, I'll go ahead and commit the patch.

> Maybe extend this to perl tests since there are several hardcoded names too?
> The patch attached tries to do this.

I had the same thought, i.e., we could use scan_server_header() to pull
XLOG_CONTROL_FILE from xlog_internal.h and replace the hardcoded "global/pg_control"
in the TAP tests. But for now, that feels like overkill, since the TAP tests already
have other hardcoded values like the WAL directory (pg_wal), and we haven't used
macros like XLOGDIR there either. Replacing just "global/pg_control" might feel
inconsistent, and handling the other cases properly would take more time beyond
feature freeze. So I'm thinking it's better to use XLOG_CONTROL_FILE only in
C code for now.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hi!

On 05.04.2025 06:11, Fujii Masao wrote:
>
> Thanks for checking! Barring any objections, I'll go ahead and commit the patch.
>

As for me all is ok. Thanks!

>> Maybe extend this to perl tests since there are several hardcoded names too?
>> The patch attached tries to do this.
>
> I had the same thought, i.e., we could use scan_server_header() to pull
> XLOG_CONTROL_FILE from xlog_internal.h and replace the hardcoded "global/pg_control"
> in the TAP tests. But for now, that feels like overkill, since the TAP tests already
> have other hardcoded values like the WAL directory (pg_wal), and we haven't used
> macros like XLOGDIR there either. Replacing just "global/pg_control" might feel
> inconsistent, and handling the other cases properly would take more time beyond
> feature freeze. So I'm thinking it's better to use XLOG_CONTROL_FILE only in
> C code for now.

Agreed. Also think that it is better to fix all such places at once and some later.

Best wishes!

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

On 2025/04/06 4:10, Anton A. Melnikov wrote:
> Hi!
>
> On 05.04.2025 06:11, Fujii Masao wrote:
>>
>> Thanks for checking! Barring any objections, I'll go ahead and commit the patch.
>>
>
> As for me all is ok. Thanks!

I've pushed the patch. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 07.04.2025 03:32, Fujii Masao wrote:

> I've pushed the patch. Thanks!

Thanks! Have a nice day!

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