Thread: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal

Hi,

The documentation states that "WAL summarization cannot be enabled when wal_level is set to minimal." Therefore, at
startup,the postmaster checks these settings and exits with an error if they are not configured properly.
 

However, I found that summarize_wal can still be enabled while the server is running with wal_level=minimal. Please see
thefollowing example to cause this situation. I think this is a bug.
 


=# SHOW wal_level;
  wal_level
-----------
  minimal
(1 row)

=# SELECT * FROM pg_get_wal_summarizer_state();
  summarized_tli | summarized_lsn | pending_lsn | summarizer_pid
----------------+----------------+-------------+----------------
               0 | 0/0            | 0/0         |         (null)
(1 row)

=# ALTER SYSTEM SET summarize_wal TO on;
ALTER SYSTEM

=# SELECT pg_reload_conf();
  pg_reload_conf
----------------
  t
(1 row)

=# SELECT * FROM pg_get_wal_summarizer_state();
  summarized_tli | summarized_lsn | pending_lsn | summarizer_pid
----------------+----------------+-------------+----------------
               1 | 0/1492D80      | 0/1492DF8   |          12228
(1 row)


The attached patch adds a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal, fixing
theissue.
 

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Jul 03, 2024 at 11:08:48PM +0900, Fujii Masao wrote:
> +/*
> + * GUC check_hook for summarize_wal
> + */
> +bool
> +check_summarize_wal(bool *newval, void **extra, GucSource source)
> +{
> +    if (*newval && wal_level == WAL_LEVEL_MINIMAL)
> +    {
> +        GUC_check_errmsg("WAL cannot be summarized when \"wal_level\" is \"minimal\"");
> +        return false;
> +    }
> +    return true;
> +}

IME these sorts of GUC hooks that depend on the value of other GUCs tend to
be quite fragile.  This one might be okay because wal_level defaults to
'replica' and because there is an additional check in postmaster.c, but
that at least deserves a comment.

This sort of thing comes up enough that perhaps we should add a
better-supported way to deal with GUCs that depend on each other...

-- 
nathan

> On 3 Jul 2024, at 16:29, Nathan Bossart <nathandbossart@gmail.com> wrote:

> This sort of thing comes up enough that perhaps we should add a
> better-supported way to deal with GUCs that depend on each other...

+1

--
Daniel Gustafsson

On Wed, 3 Jul 2024 at 16:30, Nathan Bossart <nathandbossart@gmail.com> wrote:
> IME these sorts of GUC hooks that depend on the value of other GUCs tend to
> be quite fragile.  This one might be okay because wal_level defaults to
> 'replica' and because there is an additional check in postmaster.c, but
> that at least deserves a comment.

Yeah, this hook only works because wal_level isn't PGC_SIGHUP and
indeed because there's a check in postmaster.c. It now depends on the
ordering of these values in your config which place causes the error
message on startup.

This hits the already existing check:
summarize_wal = 'true'
wal_sumarizer = 'minimal'

This hits the new check:
summarize_wal = 'true'
wal_sumarizer = 'minimal'

And actually this would throw an error from the new check even though
the config is fine:

wal_sumarizer = 'minimal'
summarize_wal = 'true'
wal_sumarizer = 'logical'

> This sort of thing comes up enough that perhaps we should add a
> better-supported way to deal with GUCs that depend on each other...

+1. Sounds like we need a global GUC consistency check

Ugh copy paste mistake, this is what I meant

On Wed, 3 Jul 2024 at 16:45, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> This hits the already existing check:
> summarize_wal = 'true'
> wal_level = 'minimal'
>
> This hits the new check:
> summarize_wal = 'true'
> wal_level = 'minimal'
>
> And actually this would throw an error from the new check even though
> the config is fine:
>
> wal_level = 'minimal'
> summarize_wal = 'true'
> wal_level = 'logical'

On Wed, 3 Jul 2024 at 16:46, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> Ugh copy paste mistake, this is what I meant
>
> On Wed, 3 Jul 2024 at 16:45, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > This hits the already existing check:
> > summarize_wal = 'true'
> > wal_level = 'minimal'
> >
> > This hits the new check:
> > wal_level = 'minimal'
> > summarize_wal = 'true'
> >
> > And actually this would throw an error from the new check even though
> > the config is fine:
> >
> > wal_level = 'minimal'
> > summarize_wal = 'true'
> > wal_level = 'logical'

Okay... fixed one more copy paste mistake... (I blame end-of-day brain)

Nathan Bossart <nathandbossart@gmail.com> writes:
> This sort of thing comes up enough that perhaps we should add a
> better-supported way to deal with GUCs that depend on each other...

Yeah, a GUC check hook that tries to inspect the value of some
other GUC is generally going to create more problems than it
solves; we've learned that the hard way in the past.  We have
your patch to remove one instance of that on the CF queue:

https://www.postgresql.org/message-id/flat/ZnMr2k-Nk5vj7T7H@nathan

But that fix only works because those GUCs are PGC_POSTMASTER
and so we can perform a consistency check on them after GUC setup.

I'm not sure what a more general consistency check mechanism would
look like, but it would have to act at some other point than the
check_hooks do.

            regards, tom lane

On Wed, Jul 3, 2024 at 10:09 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> The documentation states that "WAL summarization cannot be enabled when wal_level is set to minimal." Therefore, at
startup,the postmaster checks these settings and exits with an error if they are not configured properly. 
>
> However, I found that summarize_wal can still be enabled while the server is running with wal_level=minimal. Please
seethe following example to cause this situation. I think this is a bug. 

Well, that's unfortunate. I suppose I got confused about whether
summarize_wal could be changed without a server restart.

I think the fix is probably not to cross-check the GUC values, but to
put something in the summarizer that prevents it from generating a
summary file if wal_level==minimal. Because an incremental backup
based on such summaries would be no good. I won't be working the next
couple of days due to the US holiday tomorrow, but I've made a note to
look into this more next week.

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

On 2024/07/03 23:29, Nathan Bossart wrote:
> On Wed, Jul 03, 2024 at 11:08:48PM +0900, Fujii Masao wrote:
>> +/*
>> + * GUC check_hook for summarize_wal
>> + */
>> +bool
>> +check_summarize_wal(bool *newval, void **extra, GucSource source)
>> +{
>> +    if (*newval && wal_level == WAL_LEVEL_MINIMAL)
>> +    {
>> +        GUC_check_errmsg("WAL cannot be summarized when \"wal_level\" is \"minimal\"");
>> +        return false;
>> +    }
>> +    return true;
>> +}
> 
> IME these sorts of GUC hooks that depend on the value of other GUCs tend to
> be quite fragile.  This one might be okay because wal_level defaults to
> 'replica' and because there is an additional check in postmaster.c, but
> that at least deserves a comment.

Yes, I didn't add a cross-check for wal_level and summarize_wal intentionally
because wal_level is a PGC_POSTMASTER option, and incorrect settings of them
are already checked at server startup. However, I agree that adding a comment
about this would be helpful.

BTW, another concern with summarize_wal is that it might not be enough to
just check the summarize_wal and wal_level settings. We also need to
ensure that WAL data generated with wal_level=minimal is not summarized.

For example, if wal_level is changed from minimal to replica or logical,
some old WAL data generated with wal_level=minimal might still exist in pg_wal.
In this case, if summarize_wal is enabled, the settings of wal_level and
summarize_wal is valid, but the started WAL summarizer might summarize
this old WAL data unexpectedly.

I haven't reviewed the code regarding how the WAL summarizer chooses
its starting point, so I'm not sure if there's a real risk of summarizing
WAL data generated with wal_level=minimal.


> This sort of thing comes up enough that perhaps we should add a
> better-supported way to deal with GUCs that depend on each other...

+1 for v18 or later. However, since the reported issue is in v17,
it needs to be addressed without such a improved check mechanism.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Jul 4, 2024 at 10:35 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> +1 for v18 or later. However, since the reported issue is in v17,
> it needs to be addressed without such a improved check mechanism.

Here is a draft patch for that. This is only lightly tested at this
point, so further testing would be greatly appreciated, as would code
review.

NOTE THAT this seems to require bumping XLOG_PAGE_MAGIC and
PG_CONTROL_VERSION, which I imagine won't be super-popular considering
we've already shipped beta2. However, I don't see another principled
way to fix the reported problem. We do currently log an
XLOG_PARAMETER_CHANGE message at startup when wal_level has changed,
but that's useless for this purpose. Consider that WAL summarization
on a standby can start from any XLOG_CHECKPOINT_SHUTDOWN or
XLOG_CHECKPOINT_REDO record, and the most recent XLOG_PARAMETER_CHANGE
may be arbitrarily far behind that point, and replay may be
arbitrarily far ahead of that point by which things may have changed
again. The only way to know for certain what wal_level was in effect
at the time one of those records was generated is if the record itself
contains that information, so that's what I did. If I'm reading the
code correctly, this doesn't increase the size of the WAL, because
XLOG_CHECKPOINT_REDO was previously storing a dummy integer, and
CheckPoint looks to have an alignment padding hole where I inserted
the new member. Even if the size did increase, I don't think it would
matter: these records aren't emitted frequently enough for it to be a
problem. But I think it doesn't. However, it does change the format of
WAL, and because the checkpoint data is also stored in the control
file, it changes the format of that, too.

If we really, really don't want to let those values change at this
point in the release cycle, then we could alternatively just document
the kinds of scenarios that this protects against as unsupported and
admonish people sternly not to do that kind of thing. I think it's
actually sort of rare, because you have to do something like: enable
WAL summarization, do some stuff, restart with wal_level=minimal and
summarize_wal=off, do some more stuff but not so much stuff that any
of the WAL you generate gets removed, then restart again with
wal_level=replica/logical and summarize_wal=on again, so that the WAL
generated at the lower WAL level gets summarized before it gets
removed. Then, an incremental backup that you took after doing that
dance against a prior backup taken before doing all that might get
confused about what to do. Although that does not sound like a
terribly likely or advisable sequence of steps, I bet some people will
do it and then it will be hard to figure out what went wrong (and even
after we do, it won't make those people whole). And on the flip side I
don't think that bumping XLOG_PAGE_MAGIC or PG_CONTROL_VERSION will
cause huge inconvenience, because people don't tend to put beta2 into
production -- the early adopters tend to go for .0 or .1, not betaX.

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

Here's v2.

Jakub Wartak pointed out to me off-list that this broke the case where
a chain of incrementals crosses a timeline switch. That made me
realize that I also need to add the WAL level to XLOG_END_OF_RECOVERY,
so this version does that.

I also forgot to mention that this patch changes behavior in the case
where you've been running with summarize_wal=off for a while and then
you turned it on. Previously, we'd start summarizing from the oldest
WAL record we could still read from pg_xlog. Now, we'll start
summarizing from the first checkpoint (or timeline switch) after that.
That's necessary, because when we read the oldest record available, we
can't know for sure what WAL level was used to generate it, so we have
to assume the worst case, i.e. minimal, and thus skip summarizing that
WAL. However, it's also harmless, because a WAL summary that covers
part of a checkpoint cycle is useless to us anyway. We need completely
WAL summaries from the start of the prior backup to the start of the
current one to be able to do an incremental backup, and the previous
backup and the current backup must have each started with a
checkpoint, so a summary covering part of a checkpoint cycle can never
make an incremental backup possible where it would not otherwise have
been possible.

One more thing I forgot to mention is that we can't fix this problem
by making summarize_wal PGC_POSTMASTER. That doesn't work because of
what is mentioned in the previous paragraph: when summarize_wal is
turned on it will go back and try to summarize any older WAL that is
still around: we need this infrastructure to know whether or not that
older WAL is safe to summarize. And I don't think we can remove the
behavior where we back up and try to summarize old WAL, either,
because then after a crash you'd always have a gap in your summary
files and you would have to take a new full backup afterwards, which
would suck. I continue to think that a lot of the value of this
feature is in making sure that it *always* works -- when you start to
add cases where full backups are required, this becomes a lot less
useful to the target audience for the feature, namely, people whose
databases are so large that full backups take an unreasonably long
time to complete.

...Robert

On 2024/07/10 2:57, Robert Haas wrote:
> Here's v2.

Thanks for the patch!

With v2 patch, I found a case where WAL data generated with
wal_level = minimal is summarized. In the steps below,
the hoge2_minimal table is created under wal_level = minimal,
but its block modification information is included in
the WAL summary files. I confirmed this by checking
the contents of WAL summary files using pg_wal_summary_contents().

Additionally, the hoge3_replica table created under
wal_level = replica is not summarized.

-------------------------------------------
initdb -D data
echo "wal_keep_size = '16GB'" >> data/postgresql.conf

pg_ctl -D data start
psql <<EOF
SHOW wal_level;
CREATE TABLE hoge1_replica AS SELECT n FROM generate_series(1, 100) n;
ALTER SYSTEM SET max_wal_senders TO 0;
ALTER SYSTEM SET wal_level TO 'minimal';
EOF

pg_ctl -D data restart
psql <<EOF
SHOW wal_level;
CREATE TABLE hoge2_minimal AS SELECT n FROM generate_series(1, 100) n;
ALTER SYSTEM SET wal_level TO 'replica';
EOF

pg_ctl -D data restart
psql <<EOF
SHOW wal_level;
CREATE TABLE hoge3_replica AS SELECT n FROM generate_series(1, 100) n;
CHECKPOINT;
CREATE TABLE hoge4_replica AS SELECT n FROM generate_series(1, 100) n;
CHECKPOINT;
ALTER SYSTEM SET summarize_wal TO on;
SELECT pg_reload_conf();
SELECT pg_sleep(5);
SELECT wsc.*, c.relname FROM pg_available_wal_summaries() JOIN LATERAL pg_wal_summary_contents(tli, start_lsn, end_lsn)
wscON true JOIN pg_class c ON wsc.relfilenode = c.relfilenode WHERE c.relname LIKE 'hoge%' ORDER BY c.relname;
 
EOF
-------------------------------------------

I believe this issue occurs when the server is shut down cleanly.
The shutdown-checkpoint record retains the wal_level value used
before the shutdown. If wal_level is changed after this,
the wal_level that indicated by the shutdown-checkpoint record
and that the WAL data generated afterwards depends on may not match.


I'm sure this patch is necessary as a safeguard for WAL summarization.
OTOH, I also think we should apply the patch I proposed earlier
in this thread, which prevents summarize_wal from being enabled
when wal_level is set to minimal. This way, if there's
a misconfiguration, users will see an error message and
can quickly identify and fix the issue. Thought?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> I believe this issue occurs when the server is shut down cleanly.
> The shutdown-checkpoint record retains the wal_level value used
> before the shutdown. If wal_level is changed after this,
> the wal_level that indicated by the shutdown-checkpoint record
> and that the WAL data generated afterwards depends on may not match.

Oh, that's a problem. I'll have to think about that.

> I'm sure this patch is necessary as a safeguard for WAL summarization.
> OTOH, I also think we should apply the patch I proposed earlier
> in this thread, which prevents summarize_wal from being enabled
> when wal_level is set to minimal. This way, if there's
> a misconfiguration, users will see an error message and
> can quickly identify and fix the issue. Thought?

I interpreted these emails as meaning that we should not proceed with
that approach:

https://www.postgresql.org/message-id/CAGECzQR2r-rHFLQr5AonFehVP8DiFH+==R2yqdBvunYnwxsXNA@mail.gmail.com
http://postgr.es/m/3253790.1720019802@sss.pgh.pa.us

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

On Wed, Jul 10, 2024 at 10:10:30AM -0400, Robert Haas wrote:
> On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> I'm sure this patch is necessary as a safeguard for WAL summarization.
>> OTOH, I also think we should apply the patch I proposed earlier
>> in this thread, which prevents summarize_wal from being enabled
>> when wal_level is set to minimal. This way, if there's
>> a misconfiguration, users will see an error message and
>> can quickly identify and fix the issue. Thought?
> 
> I interpreted these emails as meaning that we should not proceed with
> that approach:
> 
> https://www.postgresql.org/message-id/CAGECzQR2r-rHFLQr5AonFehVP8DiFH+==R2yqdBvunYnwxsXNA@mail.gmail.com
> http://postgr.es/m/3253790.1720019802@sss.pgh.pa.us

Yeah.  I initially thought this patch might be okay, at least as a stopgap,
but Jelte pointed out a case where it doesn't work, namely when you have
something like the following in the config file:

    wal_level = 'minimal'
    summarize_wal = 'true'
    wal_level = 'logical'

I'm not sure that's a dealbreaker for v17 if we can't come up with anything
else, but IMHO it at least deserves a loud comment and a plan for a better
solution in v18.

-- 
nathan

On Wed, 10 Jul 2024 at 16:18, Nathan Bossart <nathandbossart@gmail.com> wrote:
> Yeah.  I initially thought this patch might be okay, at least as a stopgap,
> but Jelte pointed out a case where it doesn't work, namely when you have
> something like the following in the config file:
>
>         wal_level = 'minimal'
>         summarize_wal = 'true'
>         wal_level = 'logical'

I think that issue can be solved fairly easily by making the guc
check_hook always pass during postmaster startup (by e.g. checking
pmState), and relying on the previous startup check instead during
startup.

On Wed, Jul 10, 2024 at 04:29:14PM +0200, Jelte Fennema-Nio wrote:
> On Wed, 10 Jul 2024 at 16:18, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Yeah.  I initially thought this patch might be okay, at least as a stopgap,
>> but Jelte pointed out a case where it doesn't work, namely when you have
>> something like the following in the config file:
>>
>>         wal_level = 'minimal'
>>         summarize_wal = 'true'
>>         wal_level = 'logical'
> 
> I think that issue can be solved fairly easily by making the guc
> check_hook always pass during postmaster startup (by e.g. checking
> pmState), and relying on the previous startup check instead during
> startup.

I was actually just thinking about doing something similar in a different
thread [0].  Do we actually need to look at pmState?  Or could we just skip
it if the context is <= PGC_S_ARGV?

[0] https://postgr.es/m/Zow-DBaDY2IzAzA2%40nathan

-- 
nathan

On Wed, 10 Jul 2024 at 16:46, Nathan Bossart <nathandbossart@gmail.com> wrote:
> Do we actually need to look at pmState?  Or could we just skip
> it if the context is <= PGC_S_ARGV?

I'm not 100% sure, but I think PGC_S_FILE would still be used when
postgresql.conf changes and on SIGHUP is sent. And we would want the
check_hook to be used then.

Jelte Fennema-Nio <postgres@jeltef.nl> writes:
> On Wed, 10 Jul 2024 at 16:18, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Yeah.  I initially thought this patch might be okay, at least as a stopgap,
>> but Jelte pointed out a case where it doesn't work, namely when you have
>> something like the following in the config file:
>>
>> wal_level = 'minimal'
>> summarize_wal = 'true'
>> wal_level = 'logical'

> I think that issue can be solved fairly easily by making the guc
> check_hook always pass during postmaster startup (by e.g. checking
> pmState), and relying on the previous startup check instead during
> startup.

Please, no.  We went through a ton of permutations of that kind of
idea years ago, when it first became totally clear that cross-checks
between GUCs do not work nicely if implemented in check_hooks.
(You can find all the things we tried in the commit log, although
I don't recall exactly when.)  A counter-example for what you just
said is when a configuration file like the above is loaded after
postmaster start.

If we want to solve this, let's actually solve it, perhaps by
inventing a "consistency check" mechanism that GUC applies after
it thinks it's reached a final set of GUC values.  I'm not very
clear on how outside checking code would be able to look at the
tentative rather than active values of the variables, but that
should be solvable.

            regards, tom lane

On Wed, Jul 10, 2024 at 11:11:13AM -0400, Tom Lane wrote:
> Please, no.  We went through a ton of permutations of that kind of
> idea years ago, when it first became totally clear that cross-checks
> between GUCs do not work nicely if implemented in check_hooks.
> (You can find all the things we tried in the commit log, although
> I don't recall exactly when.)

Understood.

> A counter-example for what you just
> said is when a configuration file like the above is loaded after
> postmaster start.

I haven't tested it, but from skimming around the code, it looks like
ProcessConfigFileInternal() would deduplicate any previous entries in the
file prior to applying the values and running the check hooks.  Else,
reloading a configuration file with multiple startup-only GUC entries could
fail, even without bogus GUC check hooks.

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> I haven't tested it, but from skimming around the code, it looks like
> ProcessConfigFileInternal() would deduplicate any previous entries in the
> file prior to applying the values and running the check hooks.  Else,
> reloading a configuration file with multiple startup-only GUC entries could
> fail, even without bogus GUC check hooks.

While it's been a little while since I actually traced the logic,
I believe the reason that case doesn't fail is this bit in
set_config_with_handle, about line 3477 as of HEAD:

        case PGC_POSTMASTER:
            if (context == PGC_SIGHUP)
            {
                /*
                 * We are re-reading a PGC_POSTMASTER variable from
                 * postgresql.conf.  We can't change the setting, so we should
                 * give a warning if the DBA tries to change it.  However,
                 * because of variant formats, canonicalization by check
                 * hooks, etc, we can't just compare the given string directly
                 * to what's stored.  Set a flag to check below after we have
                 * the final storable value.
                 */
                prohibitValueChange = true;
            }
            else if (context != PGC_POSTMASTER)
                // throw "cannot be changed now" error

            regards, tom lane

On 2024/07/10 23:18, Nathan Bossart wrote:
> On Wed, Jul 10, 2024 at 10:10:30AM -0400, Robert Haas wrote:
>> On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>> I'm sure this patch is necessary as a safeguard for WAL summarization.
>>> OTOH, I also think we should apply the patch I proposed earlier
>>> in this thread, which prevents summarize_wal from being enabled
>>> when wal_level is set to minimal. This way, if there's
>>> a misconfiguration, users will see an error message and
>>> can quickly identify and fix the issue. Thought?
>>
>> I interpreted these emails as meaning that we should not proceed with
>> that approach:
>>
>> https://www.postgresql.org/message-id/CAGECzQR2r-rHFLQr5AonFehVP8DiFH+==R2yqdBvunYnwxsXNA@mail.gmail.com
>> http://postgr.es/m/3253790.1720019802@sss.pgh.pa.us
> 
> Yeah.  I initially thought this patch might be okay, at least as a stopgap,
> but Jelte pointed out a case where it doesn't work, namely when you have
> something like the following in the config file:
> 
>     wal_level = 'minimal'
>     summarize_wal = 'true'
>     wal_level = 'logical'

Unless I'm mistaken, the patch works fine in this case. If the check_hook
triggered every time a parameter appears in the configuration file,
it would mistakenly detect wal_level=minimal and summarize_wal=on together
and raise an error. However, this isn't the case. The check_hook is
designed to trigger after duplicate parameters are deduplicated.
Am I missing something?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2024/07/11 0:44, Nathan Bossart wrote:
> On Wed, Jul 10, 2024 at 11:11:13AM -0400, Tom Lane wrote:
>> Please, no.  We went through a ton of permutations of that kind of
>> idea years ago, when it first became totally clear that cross-checks
>> between GUCs do not work nicely if implemented in check_hooks.
>> (You can find all the things we tried in the commit log, although
>> I don't recall exactly when.)
> 
> Understood.
> 
>> A counter-example for what you just
>> said is when a configuration file like the above is loaded after
>> postmaster start.
> 
> I haven't tested it, but from skimming around the code, it looks like
> ProcessConfigFileInternal() would deduplicate any previous entries in the
> file prior to applying the values and running the check hooks.  Else,
> reloading a configuration file with multiple startup-only GUC entries could
> fail, even without bogus GUC check hooks.

Yeah, I'm thinking the same.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Jul 10, 2024 at 11:54:38AM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> I haven't tested it, but from skimming around the code, it looks like
>> ProcessConfigFileInternal() would deduplicate any previous entries in the
>> file prior to applying the values and running the check hooks.  Else,
>> reloading a configuration file with multiple startup-only GUC entries could
>> fail, even without bogus GUC check hooks.
> 
> While it's been a little while since I actually traced the logic,
> I believe the reason that case doesn't fail is this bit in
> set_config_with_handle, about line 3477 as of HEAD:
> 
>         case PGC_POSTMASTER:
>             if (context == PGC_SIGHUP)
>             {
>                 /*
>                  * We are re-reading a PGC_POSTMASTER variable from
>                  * postgresql.conf.  We can't change the setting, so we should
>                  * give a warning if the DBA tries to change it.  However,
>                  * because of variant formats, canonicalization by check
>                  * hooks, etc, we can't just compare the given string directly
>                  * to what's stored.  Set a flag to check below after we have
>                  * the final storable value.
>                  */
>                 prohibitValueChange = true;
>             }
>             else if (context != PGC_POSTMASTER)
>                 // throw "cannot be changed now" error

That's what I thought at first, but then I saw this in
ProcessConfigFileInternal():

            /* If it's already marked, then this is a duplicate entry */
            if (record->status & GUC_IS_IN_FILE)
            {
                /*
                 * Mark the earlier occurrence(s) as dead/ignorable.  We could
                 * avoid the O(N^2) behavior here with some additional state,
                 * but it seems unlikely to be worth the trouble.
                 */
                ConfigVariable *pitem;

                for (pitem = head; pitem != item; pitem = pitem->next)
                {
                    if (!pitem->ignore &&
                        strcmp(pitem->name, item->name) == 0)
                        pitem->ignore = true;
                }
            }

-- 
nathan

On Thu, Jul 11, 2024 at 01:02:25AM +0900, Fujii Masao wrote:
> On 2024/07/10 23:18, Nathan Bossart wrote:
>> Yeah.  I initially thought this patch might be okay, at least as a stopgap,
>> but Jelte pointed out a case where it doesn't work, namely when you have
>> something like the following in the config file:
>> 
>>     wal_level = 'minimal'
>>     summarize_wal = 'true'
>>     wal_level = 'logical'
> 
> Unless I'm mistaken, the patch works fine in this case. If the check_hook
> triggered every time a parameter appears in the configuration file,
> it would mistakenly detect wal_level=minimal and summarize_wal=on together
> and raise an error. However, this isn't the case. The check_hook is
> designed to trigger after duplicate parameters are deduplicated.
> Am I missing something?

After further research, I think you are right about that.

-- 
nathan

On Wed, Jul 10, 2024 at 10:10 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > I believe this issue occurs when the server is shut down cleanly.
> > The shutdown-checkpoint record retains the wal_level value used
> > before the shutdown. If wal_level is changed after this,
> > the wal_level that indicated by the shutdown-checkpoint record
> > and that the WAL data generated afterwards depends on may not match.
>
> Oh, that's a problem. I'll have to think about that.

Here is an attempt at fixing this problem.

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

On Wed, Jul 10, 2024 at 11:11:13AM -0400, Tom Lane wrote:
> We went through a ton of permutations of that kind of
> idea years ago, when it first became totally clear that cross-checks
> between GUCs do not work nicely if implemented in check_hooks.
> (You can find all the things we tried in the commit log, although
> I don't recall exactly when.)

Do you remember the general timeframe or any of the GUCs involved?  I spent
some time searching through the commit log and mailing lists, but I've thus
far only found allusions to past bad experiences with such hooks.

-- 
nathan

On 2024/07/11 1:35, Robert Haas wrote:
> On Wed, Jul 10, 2024 at 10:10 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>> I believe this issue occurs when the server is shut down cleanly.
>>> The shutdown-checkpoint record retains the wal_level value used
>>> before the shutdown. If wal_level is changed after this,
>>> the wal_level that indicated by the shutdown-checkpoint record
>>> and that the WAL data generated afterwards depends on may not match.
>>
>> Oh, that's a problem. I'll have to think about that.
> 
> Here is an attempt at fixing this problem.

Thanks for updating the patch!

+     * fast_forward is normally false, but is true when we have encountered
+     * WAL generated with wal_level=minimal and are skipping over it without
+     * emitting summary files. In this case, summarized_tli and summarized_lsn
+     * will advance even though nothing is being written to disk, until we
+     * again reach a point where wal_level>minimal.
+     *
       * summarizer_pgprocno is the proc number of the summarizer process, if
       * one is running, or else INVALID_PROC_NUMBER.
       *
@@ -83,6 +89,7 @@ typedef struct
      TimeLineID    summarized_tli;
      XLogRecPtr    summarized_lsn;
      bool        lsn_is_exact;
+    bool        fast_forward;

It looks like the fast_forward field in WalSummarizerData is no longer necessary.

So far, I haven't found any other issues with the patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> It looks like the fast_forward field in WalSummarizerData is no longer necessary.
>
> So far, I haven't found any other issues with the patch.

Thanks for reviewing. Regarding fast_forward, I think I had the idea
in mind that perhaps it should be exposed by
pg_get_wal_summarizer_state(), but I didn't actually implement that.
Thinking about it again, I think maybe it's fine to just remove it
from the shared memory state, as this should be a rare scenario in
practice. What is your opinion?

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

On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> So far, I haven't found any other issues with the patch.

Here is a new version that removes the hunks you highlighted and also
adds a test case.

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

On Wed, Jul 10, 2024 at 01:41:41PM -0500, Nathan Bossart wrote:
> On Wed, Jul 10, 2024 at 11:11:13AM -0400, Tom Lane wrote:
>> We went through a ton of permutations of that kind of
>> idea years ago, when it first became totally clear that cross-checks
>> between GUCs do not work nicely if implemented in check_hooks.
>> (You can find all the things we tried in the commit log, although
>> I don't recall exactly when.)
> 
> Do you remember the general timeframe or any of the GUCs involved?  I spent
> some time searching through the commit log and mailing lists, but I've thus
> far only found allusions to past bad experiences with such hooks.

Could it be the effective_cache_size work from 2013-2014?

    https://www.postgresql.org/message-id/flat/CAHyXU0weDECnab1pypNum-dWGwjso_XMTY8-NvvzRphzM2Hv5A%40mail.gmail.com
    https://www.postgresql.org/message-id/flat/CAMkU%3D1zTMNZsnUV6L7aMvfJZfzjKbzAtuML3N35wyYaia9MJAw%40mail.gmail.com
    https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ee1e5662d8d8330726eaef7d3110cb7add24d058
    https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2850896961994aa0993b9e2ed79a209750181b8a
    https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=af930e606a3217db3909029c6c3f8d003ba70920

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=a16d421ca4fc639929bc964b2585e8382cf16e33;hp=08c8e8962f56c23c6799178d52d3b31350a0708f

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Jul 10, 2024 at 01:41:41PM -0500, Nathan Bossart wrote:
>> On Wed, Jul 10, 2024 at 11:11:13AM -0400, Tom Lane wrote:
>>> We went through a ton of permutations of that kind of
>>> idea years ago, when it first became totally clear that cross-checks
>>> between GUCs do not work nicely if implemented in check_hooks.

>> Do you remember the general timeframe or any of the GUCs involved?  I spent
>> some time searching through the commit log and mailing lists, but I've thus
>> far only found allusions to past bad experiences with such hooks.

> Could it be the effective_cache_size work from 2013-2014?

Yeah, that's what I was remembering.  It looks like my memory was
slightly faulty, in that what ee1e5662d tried to do was make the
default value of one GUC depend on the actual value of another one,
not implement a consistency check per se.  But the underlying problem
is the same: a check_hook can't assume it is seeing the appropriate
value of some other GUC, since a change of that one may be pending.

            regards, tom lane

On 2024/07/12 1:16, Robert Haas wrote:
> On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> It looks like the fast_forward field in WalSummarizerData is no longer necessary.
>>
>> So far, I haven't found any other issues with the patch.
> 
> Thanks for reviewing. Regarding fast_forward, I think I had the idea
> in mind that perhaps it should be exposed by
> pg_get_wal_summarizer_state(),

Understood.


> but I didn't actually implement that.
> Thinking about it again, I think maybe it's fine to just remove it
> from the shared memory state, as this should be a rare scenario in
> practice. What is your opinion?

I don't think it's a rare scenario since summarize_wal can be enabled
after starting the server with wal_level=minimal. Therefore, I believe
such a configuration should be prohibited using a GUC check hook,
as my patch does. Alternatively, we should at least report or
log something when summarize_wal is enabled but fast_forward is also
enabled, so users can easily detect or investigate this unexpected situation.
I'm not sure if exposing fast_forward is necessary for that or not...

Regarding pg_get_wal_summarizer_state(), it is documented that
summarized_lsn indicates the ending LSN of the last WAL summary file
written to disk. However, with the patch, summarized_lsn advances
even when fast_forward is enabled. The documentation should be updated,
or summarized_lsn should be changed so it doesn't advance while
fast_forward is enabled.


On 2024/07/12 3:00, Robert Haas wrote:
> On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> So far, I haven't found any other issues with the patch.
> 
> Here is a new version that removes the hunks you highlighted and also
> adds a test case.

Thanks for updating the patch! LGTM.

I have one small comment:

+# This test aims to validate that takeing an incremental backup fails when

"takeing" should be "taking"?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Sun, Jul 14, 2024 at 10:56 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> I don't think it's a rare scenario since summarize_wal can be enabled
> after starting the server with wal_level=minimal. Therefore, I believe
> such a configuration should be prohibited using a GUC check hook,
> as my patch does.

I guess I'm in the group of people who doesn't understand how this can
possibly work. There's no guarantee about the order in which GUC check
hooks are called, so you don't know if the value of the other variable
has already been set to the final value or not, which seems like a
fatal problem even if the code happens to work correctly as of today.
Even if you have such a guarantee, you can't prohibit a configuration
change at pg_ctl reload time: the server can refuse to start in case
of an invalid configuration, but a running server can't decide to shut
down or stop working at reload time.

> Alternatively, we should at least report or
> log something when summarize_wal is enabled but fast_forward is also
> enabled, so users can easily detect or investigate this unexpected situation.
> I'm not sure if exposing fast_forward is necessary for that or not...

To be honest, I'm uncomfortable with how much time is passing while we
debate these details. I feel like we need to get these WAL format
changes done sooner rather than later considering beta2 is already out
the door. Other changes like logging messages or not can be debated
once the basic fix is in. Even if they don't happen, nobody will have
a corrupted backup once the basic fix is done. At most they will be
confused about why some backup is failing.

> Regarding pg_get_wal_summarizer_state(), it is documented that
> summarized_lsn indicates the ending LSN of the last WAL summary file
> written to disk. However, with the patch, summarized_lsn advances
> even when fast_forward is enabled. The documentation should be updated,
> or summarized_lsn should be changed so it doesn't advance while
> fast_forward is enabled.

I think we need to advance summarized_lsn to get the proper behavior.
I can update the documentation.

> I have one small comment:
>
> +# This test aims to validate that takeing an incremental backup fails when
>
> "takeing" should be "taking"?

Will fix, thanks.

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

On Mon, Jul 15, 2024 at 02:30:42PM -0400, Robert Haas wrote:
> On Sun, Jul 14, 2024 at 10:56 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>> I don't think it's a rare scenario since summarize_wal can be enabled
>> after starting the server with wal_level=minimal. Therefore, I believe
>> such a configuration should be prohibited using a GUC check hook,
>> as my patch does.
> 
> I guess I'm in the group of people who doesn't understand how this can
> possibly work. There's no guarantee about the order in which GUC check
> hooks are called, so you don't know if the value of the other variable
> has already been set to the final value or not, which seems like a
> fatal problem even if the code happens to work correctly as of today.
> Even if you have such a guarantee, you can't prohibit a configuration
> change at pg_ctl reload time: the server can refuse to start in case
> of an invalid configuration, but a running server can't decide to shut
> down or stop working at reload time.

My understanding is that the correctness of this GUC check hook depends on
wal_level being a PGC_POSTMASTER GUC.  The check hook would always return
true during startup, and there'd be an additional cross-check in
PostmasterMain() that would fail startup if necessary.  After that point,
we know that wal_level cannot change, so the GUC check hook for
summarize_wal can depend on wal_level.  If it fails, my expectation would
be that the server would just ignore that change and continue.

-- 
nathan

On Mon, Jul 15, 2024 at 01:47:14PM -0500, Nathan Bossart wrote:
> On Mon, Jul 15, 2024 at 02:30:42PM -0400, Robert Haas wrote:
>> I guess I'm in the group of people who doesn't understand how this can
>> possibly work. There's no guarantee about the order in which GUC check
>> hooks are called, so you don't know if the value of the other variable
>> has already been set to the final value or not, which seems like a
>> fatal problem even if the code happens to work correctly as of today.
>> Even if you have such a guarantee, you can't prohibit a configuration
>> change at pg_ctl reload time: the server can refuse to start in case
>> of an invalid configuration, but a running server can't decide to shut
>> down or stop working at reload time.
> 
> My understanding is that the correctness of this GUC check hook depends on
> wal_level being a PGC_POSTMASTER GUC.  The check hook would always return
> true during startup, and there'd be an additional cross-check in
> PostmasterMain() that would fail startup if necessary.  After that point,
> we know that wal_level cannot change, so the GUC check hook for
> summarize_wal can depend on wal_level.  If it fails, my expectation would
> be that the server would just ignore that change and continue.

I should also note that since wal_level defaults to "replica", I don't
think we even need any extra "always return true on startup" logic.  If
wal_level is set prior to summarize_wal, the check hook will fail startup
as needed.  If summarize_wal is set first, the check hook will return true,
and we'll fall back on the PostmasterMain() check (that already exists).

In short, the original patch [0] seems like it should work in this
particular scenario, barring some corner case I haven't discovered.  That
being said, it's admittedly fragile and probably not a great precedent to
set.  I've been thinking about some ideas for more generic GUC dependency
tooling, but I don't have anything to share yet.

[0] https://postgr.es/m/attachment/161852/v1-0001-Prevent-summarize_wal-from-enabling-when-wal_leve.patch

-- 
nathan

On Mon, Jul 15, 2024 at 2:47 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> My understanding is that the correctness of this GUC check hook depends on
> wal_level being a PGC_POSTMASTER GUC.  The check hook would always return
> true during startup, and there'd be an additional cross-check in
> PostmasterMain() that would fail startup if necessary.  After that point,
> we know that wal_level cannot change, so the GUC check hook for
> summarize_wal can depend on wal_level.  If it fails, my expectation would
> be that the server would just ignore that change and continue.

But how do you know that, during startup, the setting for
summarize_wal is processed after the setting for wal_level?

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

On Mon, Jul 15, 2024 at 04:03:13PM -0400, Robert Haas wrote:
> On Mon, Jul 15, 2024 at 2:47 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> My understanding is that the correctness of this GUC check hook depends on
>> wal_level being a PGC_POSTMASTER GUC.  The check hook would always return
>> true during startup, and there'd be an additional cross-check in
>> PostmasterMain() that would fail startup if necessary.  After that point,
>> we know that wal_level cannot change, so the GUC check hook for
>> summarize_wal can depend on wal_level.  If it fails, my expectation would
>> be that the server would just ignore that change and continue.
> 
> But how do you know that, during startup, the setting for
> summarize_wal is processed after the setting for wal_level?

You don't, but the GUC check hook should always return true when
summarize_wal is processed first.  We'd rely on the PostmasterMain() check
to fail in that case.

-- 
nathan

On Mon, Jul 15, 2024 at 4:10 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> You don't, but the GUC check hook should always return true when
> summarize_wal is processed first.  We'd rely on the PostmasterMain() check
> to fail in that case.

OK, I see. So at startup time, the check hook might or might not catch
a configuration mismatch, but if it doesn't, then the PostmasterMain()
check will fire. Later, we'd have an unsolvable problem if both
wal_level and summarize_wal could change, but since wal_level can't
change after startup time, we only need to check summarize_wal, and it
can rely on the value of wal_level being correct.

TBH, I don't want to do that. I think it's too fragile. It's the sort
of thing that just barely works given the exact behavior of these
particular GUCs, but it relies on a bunch of subtle assumptions which
won't be evident to future readers of the code. People will very
possibly copy this barely-working code into other contexts where it
doesn't work at all, or they'll think the code implementing this is
buggy even if it isn't.

We don't really need that check for correctness here, and it arguably
even prohibits more than necessary - e.g. suppose you crash without
summarizing all the WAL and then restart with wal_level=minimal. It's
perfectly fine to run the summarizer and have it catch up with all
possible pre-crash summarization, but the proposed change would
prohibit it. Granted, the current check would require you to start
with summarize_wal=off and then enable it later to get that done, but
we could remove that check. Or maybe we should leave it alone, but my
point is: if we have a reasonable option to decouple the values of two
GUCs so that the legal values of one don't depend on each other, I
think that is always going to be better and simpler than trying to
implement cross-checks on the values for which we don't have proper
infrastructure.

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

On Tue, Jul 16, 2024 at 12:23:19PM -0400, Robert Haas wrote:
> TBH, I don't want to do that. I think it's too fragile. It's the sort
> of thing that just barely works given the exact behavior of these
> particular GUCs, but it relies on a bunch of subtle assumptions which
> won't be evident to future readers of the code. People will very
> possibly copy this barely-working code into other contexts where it
> doesn't work at all, or they'll think the code implementing this is
> buggy even if it isn't.

Agreed.  If there was really no other option, it would at the very least
need a humongous comment that explained why it worked in this specific case
and is unlikely to work in others.  But it sounds like we have another
choice... 

-- 
nathan

On 2024/07/17 1:30, Nathan Bossart wrote:
> On Tue, Jul 16, 2024 at 12:23:19PM -0400, Robert Haas wrote:
>> TBH, I don't want to do that. I think it's too fragile. It's the sort
>> of thing that just barely works given the exact behavior of these
>> particular GUCs, but it relies on a bunch of subtle assumptions which
>> won't be evident to future readers of the code. People will very
>> possibly copy this barely-working code into other contexts where it
>> doesn't work at all, or they'll think the code implementing this is
>> buggy even if it isn't.
> 
> Agreed.  If there was really no other option, it would at the very least
> need a humongous comment that explained why it worked in this specific case
> and is unlikely to work in others.  But it sounds like we have another
> choice...

I don't have another solution that can be pushed into v17. I understand
the risks raised so far, so I'm okay with just pushing the "fast_forward" patch.
It might be helpful to add a note to the summarize_wal documentation,
for example, "summarize_wal can be enabled after startup with wal_level = minimal,
but WAL generated at this level won't be summarized."?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Jul 16, 2024 at 1:16 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> I don't have another solution that can be pushed into v17. I understand
> the risks raised so far, so I'm okay with just pushing the "fast_forward" patch.
> It might be helpful to add a note to the summarize_wal documentation,
> for example, "summarize_wal can be enabled after startup with wal_level = minimal,
> but WAL generated at this level won't be summarized."?

Here is v5. This version differs from v4 in that it updates the
documentation for summarize_wal and pg_get_wal_summarizer_state(). The
previous version made no documentation changes.

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

On 2024/07/17 22:44, Robert Haas wrote:
> On Tue, Jul 16, 2024 at 1:16 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> I don't have another solution that can be pushed into v17. I understand
>> the risks raised so far, so I'm okay with just pushing the "fast_forward" patch.
>> It might be helpful to add a note to the summarize_wal documentation,
>> for example, "summarize_wal can be enabled after startup with wal_level = minimal,
>> but WAL generated at this level won't be summarized."?
> 
> Here is v5. This version differs from v4 in that it updates the
> documentation for summarize_wal and pg_get_wal_summarizer_state(). The
> previous version made no documentation changes.

Thanks for updating the patch! It looks good to me, except for one minor detail.

> +        reaches WAL not generated under <literal>wal_level=minimal</literal>,
> +        it will resume writing summaries to disk.

"WAL not generated under wal_level=minimal" sounds a bit confusing??
How about "WAL generated while wal_level is replica or higher" instead?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Jul 18, 2024 at 9:47 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> "WAL not generated under wal_level=minimal" sounds a bit confusing??
> How about "WAL generated while wal_level is replica or higher" instead?

Committed and back-patched to 17 with approximately that change, but
reworded slightly to make the tenses work.

I just realized that I should've created a CF entry for this so that
it went through CI before actually committing it. Let's see how long
it takes for me to get punished for that oversight...

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

On 2024/07/19 1:27, Robert Haas wrote:
> On Thu, Jul 18, 2024 at 9:47 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> "WAL not generated under wal_level=minimal" sounds a bit confusing??
>> How about "WAL generated while wal_level is replica or higher" instead?
> 
> Committed and back-patched to 17 with approximately that change, but
> reworded slightly to make the tenses work.

Many thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION