Обсуждение: 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.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.