Обсуждение: Should we say "wal_level = logical" instead of "wal_level >= logical"

Hi Hackers,

While reviewing another LR thread [1] it saw multiple wal_level
messages that seem more complex than necessary.

Background: There are three wal_level values [2], ranked as:
minimal < replica < logical

Notice there is no wal_level "above" logical. Despite this, there are
a number of places in the code and messages that say >= logical
instead of just = logical. Here is a list:

heapam.c
- * This is only used in wal_level >= WAL_LEVEL_LOGICAL, and only for catalog

xlog.c
- * This returns true if wal_level >= logical and we are inside a valid

tablecmds.c
- /* should only get here if wal_level >= logical */

slot.c
- appendStringInfoString(&err_detail, _("Logical decoding on standby
requires \"wal_level\" >= \"logical\" on the primary server."));

decode.c
- errmsg("logical decoding on standby requires \"wal_level\" >=
\"logical\" on the primary")));

logical.c
- errmsg("logical decoding requires \"wal_level\" >= \"logical\"")));
- errmsg("logical decoding on standby requires \"wal_level\" >=
\"logical\" on the primary")));

slotsync.c
- * Logical slot sync/creation requires wal_level >= logical.
- errmsg("replication slot synchronization requires \"wal_level\" >=
\"logical\""));

standby.c
- if (wal_level >= WAL_LEVEL_LOGICAL && isCatalogRel)
- if (wal_level >= WAL_LEVEL_LOGICAL)
pg_createsubscriber.c
- pg_log_error("publisher requires \"wal_level\" >= \"logical\"");

xlog.h
- #define XLogLogicalInfoActive() (wal_level >= WAL_LEVEL_LOGICAL)

035_standby_logical_decoding.pl
- # We are not able to read from the slot as it requires wal_level >=
logical on the primary server
- "logical decoding on standby requires \"wal_level\" >= \"logical\"
on the primary"

~~~

IMO all these error messages appear more complicated than necessary.

Isn't it simpler to say:
"requires \"wal_level\" = \"logical\" ..."

Instead of:
"requires \"wal_level\" >= \"logical\" ..."

~~~

FYI, there is other code that does not account for anything "above"
logical. e.g.
if (wal_level != WAL_LEVEL_LOGICAL)

~~~

I propose to write a patch to change all those user-facing messages.

What about changing the code/comments as well?

Thoughts?

======
[1] https://www.postgresql.org/message-id/CAD21AoAQJ0LuRYuj0g8-uB9Qtns88HK_TVdoa5jmX3ZPBK9gvw%40mail.gmail.com
[2] https://www.postgresql.org/docs/current/runtime-config-wal.html

Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Oct 16, 2025 at 5:36 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Hackers,
>
> While reviewing another LR thread [1] it saw multiple wal_level
> messages that seem more complex than necessary.
>
> Background: There are three wal_level values [2], ranked as:
> minimal < replica < logical
>
> Notice there is no wal_level "above" logical. Despite this, there are
> a number of places in the code and messages that say >= logical
> instead of just = logical. Here is a list:
>
> heapam.c
> - * This is only used in wal_level >= WAL_LEVEL_LOGICAL, and only for catalog
>
> xlog.c
> - * This returns true if wal_level >= logical and we are inside a valid
>
> tablecmds.c
> - /* should only get here if wal_level >= logical */
>
> slot.c
> - appendStringInfoString(&err_detail, _("Logical decoding on standby
> requires \"wal_level\" >= \"logical\" on the primary server."));
>
> decode.c
> - errmsg("logical decoding on standby requires \"wal_level\" >=
> \"logical\" on the primary")));
>
> logical.c
> - errmsg("logical decoding requires \"wal_level\" >= \"logical\"")));
> - errmsg("logical decoding on standby requires \"wal_level\" >=
> \"logical\" on the primary")));
>
> slotsync.c
> - * Logical slot sync/creation requires wal_level >= logical.
> - errmsg("replication slot synchronization requires \"wal_level\" >=
> \"logical\""));
>
> standby.c
> - if (wal_level >= WAL_LEVEL_LOGICAL && isCatalogRel)
> - if (wal_level >= WAL_LEVEL_LOGICAL)
> pg_createsubscriber.c
> - pg_log_error("publisher requires \"wal_level\" >= \"logical\"");
>
> xlog.h
> - #define XLogLogicalInfoActive() (wal_level >= WAL_LEVEL_LOGICAL)
>
> 035_standby_logical_decoding.pl
> - # We are not able to read from the slot as it requires wal_level >=
> logical on the primary server
> - "logical decoding on standby requires \"wal_level\" >= \"logical\"
> on the primary"
>
> ~~~
>
> IMO all these error messages appear more complicated than necessary.
>
> Isn't it simpler to say:
> "requires \"wal_level\" = \"logical\" ..."
>
> Instead of:
> "requires \"wal_level\" >= \"logical\" ..."
>
> ~~~
>
> FYI, there is other code that does not account for anything "above"
> logical. e.g.
> if (wal_level != WAL_LEVEL_LOGICAL)
>
> ~~~
>
> I propose to write a patch to change all those user-facing messages.
>
> What about changing the code/comments as well?
>
> Thoughts?

I think the reason it's phrased like that is because wal_level is
cumulative - higher wal_levels are assumed to cover all the functions
(and information) covered by lower wal levels.  By phrasing it as
wal_level >= logical we are being future-proof. If we add another
level, we won't need to change documentation or error messages. But
you are right it might confuse users because they won't see anything
higher than logical. [1] is possibly changing that property of
wal_level values or at least threatening to change it and we don't
know what the next wal_level would be and whether it will continue to
cover lower levels. But maybe users are already used to that phrasing
since it seems be there for some time and without complaints. Did we
always use that wording for example, did pre-logical wal_level
messages also used wal_level >= replica?

--
Best Wishes,
Ashutosh Bapat

On Thu, Oct 16, 2025 at 2:02 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
>
> I think the reason it's phrased like that is because wal_level is
> cumulative - higher wal_levels are assumed to cover all the functions
> (and information) covered by lower wal levels.  By phrasing it as
> wal_level >= logical we are being future-proof. If we add another
> level, we won't need to change documentation or error messages. But
> you are right it might confuse users because they won't see anything
> higher than logical. [1] is possibly changing that property of
> wal_level values or at least threatening to change it and we don't
> know what the next wal_level would be and whether it will continue to
> cover lower levels. But maybe users are already used to that phrasing
> since it seems be there for some time and without complaints. Did we
> always use that wording for example, did pre-logical wal_level
> messages also used wal_level >= replica?
>

Thanks for your feedback. Yes, it seems there was always the intent
that each wal_level is a superset of the previous one.

E.g. 15 years ago, when there was only:

typedef enum WalLevel
{
WAL_LEVEL_MINIMAL = 0,
WAL_LEVEL_ARCHIVE,
WAL_LEVEL_HOT_STANDBY
} WalLevel;

Still, the code used '>=' to compare the WAL_LEVEL_HOT_STANDBY:
#define XLogStandbyInfoActive() (wal_level >= WAL_LEVEL_HOT_STANDBY)

~~~

So let's turn this thread upside-down and say, if we are going to
assume always that...
minimal < replica < logical < (some future level)

...then let's be consistent about that everywhere in the
code/comments. Specifically, there are a few places where the source
is saying (wal_level != logical) instead of (wal_level < logical), or
(wal_level == logical) instead of (wal_level >= logical).

Please see the attached patch to address these.

~~~

I would have liked to take this a step further and replace:
if (strcmp(wal_level, "logical") != 0)

with
(get_wal_level_from_str(wal_level) > WAL_LEVEL_LOGICAL)

But AFAICT, adding that new function (similar to
get_wal_level_string(int wal_level) in xlogdesc.c) and exposing
WalLevel, would require some header restructuring. I think that the
result would be good, but I'm not sure it is worth the effort.

~~~

Do you have thoughts about the patch?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Oct 20, 2025 at 3:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
> Do you have thoughts about the patch?

I agree with the rationale that Ashutosh states but I don't see a
strong need to patch the code to make this a 100% invariable rule. (Of
course, someone else may disagree, which is fine.)

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Oct 21, 2025 at 2:11 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Oct 20, 2025 at 3:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > Do you have thoughts about the patch?
>
> I agree with the rationale that Ashutosh states but I don't see a
> strong need to patch the code to make this a 100% invariable rule. (Of
> course, someone else may disagree, which is fine.)
>

In case it makes any difference...

The codebase already follows this rule in 95% of cases. The patch
simply corrects a couple of inconsistencies that appeared to be
accidental oversights.

======
Kind Regards,
Peter Smith
Fujitsu Australia

On Wed, Oct 22, 2025 at 4:49 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Oct 21, 2025 at 2:11 AM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Mon, Oct 20, 2025 at 3:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > Do you have thoughts about the patch?
> >
> > I agree with the rationale that Ashutosh states but I don't see a
> > strong need to patch the code to make this a 100% invariable rule. (Of
> > course, someone else may disagree, which is fine.)
> >
>
> In case it makes any difference...
>
> The codebase already follows this rule in 95% of cases. The patch
> simply corrects a couple of inconsistencies that appeared to be
> accidental oversights.

I think we should accept comment-only changes in the patch. With those
changes comments are consistent with the code; otherwise code-readers
will get confused. I don't have a strong opinion about the comment +
code changes though. They may wait till changes in [1] get committed.
As Robert said, we may not want that to be an invariable rule.

--
Best Wishes,
Ashutosh Bapat

On Fri, Oct 24, 2025 at 9:09 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Oct 22, 2025 at 4:49 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Tue, Oct 21, 2025 at 2:11 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > >
> > > On Mon, Oct 20, 2025 at 3:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > > Do you have thoughts about the patch?
> > >
> > > I agree with the rationale that Ashutosh states but I don't see a
> > > strong need to patch the code to make this a 100% invariable rule. (Of
> > > course, someone else may disagree, which is fine.)
> > >
> >
> > In case it makes any difference...
> >
> > The codebase already follows this rule in 95% of cases. The patch
> > simply corrects a couple of inconsistencies that appeared to be
> > accidental oversights.
>
> I think we should accept comment-only changes in the patch. With those
> changes comments are consistent with the code; otherwise code-readers
> will get confused. I don't have a strong opinion about the comment +
> code changes though. They may wait till changes in [1] get committed.
> As Robert said, we may not want that to be an invariable rule.
>

OK, here is the same patch split into comment-only changes, and code-changes.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

> On Oct 27, 2025, at 10:59, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Oct 24, 2025 at 9:09 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
>>
>> On Wed, Oct 22, 2025 at 4:49 AM Peter Smith <smithpb2250@gmail.com> wrote:
>>>
>>> On Tue, Oct 21, 2025 at 2:11 AM Robert Haas <robertmhaas@gmail.com> wrote:
>>>>
>>>> On Mon, Oct 20, 2025 at 3:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
>>>>> Do you have thoughts about the patch?
>>>>
>>>> I agree with the rationale that Ashutosh states but I don't see a
>>>> strong need to patch the code to make this a 100% invariable rule. (Of
>>>> course, someone else may disagree, which is fine.)
>>>>
>>>
>>> In case it makes any difference...
>>>
>>> The codebase already follows this rule in 95% of cases. The patch
>>> simply corrects a couple of inconsistencies that appeared to be
>>> accidental oversights.
>>
>> I think we should accept comment-only changes in the patch. With those
>> changes comments are consistent with the code; otherwise code-readers
>> will get confused. I don't have a strong opinion about the comment +
>> code changes though. They may wait till changes in [1] get committed.
>> As Robert said, we may not want that to be an invariable rule.
>>
>
> OK, here is the same patch split into comment-only changes, and code-changes.
>
> ======
> Kind Regards,
> Peter Smith.
> Fujitsu Australia
> <v2-0002-fix-wal_level-equality-code.patch><v2-0001-fix-wal_level-equality-comments.patch>

I think 0001 basically good. A tiny comment is that, in inval.c, "wal_level>=logical” doesn’t have white-spaces around
“=“,while in the other two files, they have. So maybe all add white-spaces around “=“ here. 

For 0002, I have a fixed feeling.

This change is okay to me:
```
-    if (wal_level != WAL_LEVEL_LOGICAL)
+    if (wal_level < WAL_LEVEL_LOGICAL)
```

But I really don’t like the error message changes:
```
     if (nslots_on_old > 0 && strcmp(wal_level, "logical") != 0)
-        pg_fatal("\"wal_level\" must be \"logical\" but is set to \"%s\"",
+        pg_fatal("\"wal_level\" must be \"logical\" or higher but is set to \"%s\"",
```
And
```
-HINT:  Set "wal_level" to "logical" before creating subscriptions.
+HINT:  Set "wal_level" >= "logical" before creating subscriptions.
```

Which will really make end users confused. I believe end users don’t care about so-called future extensions, they only
needaccurate information. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Wed, Oct 29, 2025 at 8:10 PM Chao Li <li.evan.chao@gmail.com> wrote:
...
> I think 0001 basically good. A tiny comment is that, in inval.c, "wal_level>=logical” doesn’t have white-spaces
around“=“, while in the other two files, they have. So maybe all add white-spaces around “=“ here. 
>
> For 0002, I have a fixed feeling.
>
> This change is okay to me:
> ```
> -       if (wal_level != WAL_LEVEL_LOGICAL)
> +       if (wal_level < WAL_LEVEL_LOGICAL)
> ```
>
> But I really don’t like the error message changes:
> ```
>         if (nslots_on_old > 0 && strcmp(wal_level, "logical") != 0)
> -               pg_fatal("\"wal_level\" must be \"logical\" but is set to \"%s\"",
> +               pg_fatal("\"wal_level\" must be \"logical\" or higher but is set to \"%s\"",
> ```
> And
> ```
> -HINT:  Set "wal_level" to "logical" before creating subscriptions.
> +HINT:  Set "wal_level" >= "logical" before creating subscriptions.
> ```
>
> Which will really make end users confused. I believe end users don’t care about so-called future extensions, they
onlyneed accurate information. 
>

Hi Chao.

Thanks for your review comments. Here are the v3* patches.

* Patch 0001 - Fixed spaces per suggestion.

* Patch 0002 - Unchanged. For now, this patch 0002 is mostly only a
placeholder until Sawada-San's patch [1] is pushed, and then I will
revisit it. There is lots of overlap, so perhaps much of it will be
made redundant.

======
[1] https://www.postgresql.org/message-id/flat/CAD21AoAtqpZW%3DzC57qxEFbBCVhJ2kF2HXmuUT3w_tHGZCYmpnw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia