Thread: 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
Attachment
> 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
Attachment
> 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
Attachment
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 instigatinga 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
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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