From: | "Anton A(dot) Melnikov" <a(dot)melnikov(at)postgrespro(dot)ru> |
---|---|
To: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, daniel(at)yesql(dot)se |
Cc: | peter(at)eisentraut(dot)org, tgl(at)sss(dot)pgh(dot)pa(dot)us, robertmhaas(at)gmail(dot)com, michael(at)paquier(dot)xyz, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Use XLOG_CONTROL_FILE macro everywhere? |
Date: | 2024-09-03 09:02:26 |
Message-ID: | fc33823e-dbdf-4f31-b9aa-bf7f758019b9@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Use-XLOG_CONTROL_FILE-macro-everywhere-in-C-code.patch | text/x-patch | 9.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | jian he | 2024-09-03 09:05:02 | Re: pgsql: Add more SQL/JSON constructor functions |
Previous Message | Richard Guo | 2024-09-03 08:50:07 | Re: Remove no-op PlaceHolderVars |