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
