Re: Should we say "wal_level = logical" instead of "wal_level >= logical"

On Thu, Oct 16, 2025 at 2:02 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)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