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

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: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:

/messages/by-id/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 see the 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) wsc ON 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:

/messages/by-id/CAGECzQR2r-rHFLQr5AonFehVP8DiFH+==R2yqdBvunYnwxsXNA@mail.gmail.com
/messages/by-id/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:

/messages/by-id/CAGECzQR2r-rHFLQr5AonFehVP8DiFH+==R2yqdBvunYnwxsXNA@mail.gmail.com
/messages/by-id/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]/messages/by-id/Zow-DBaDY2IzAzA2@nathan. Do we actually need to look at pmState? Or could we just skip
it if the context is <= PGC_S_ARGV?

[0]: /messages/by-id/Zow-DBaDY2IzAzA2@nathan

--
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