Re: Use XLOG_CONTROL_FILE macro everywhere?

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