pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedAdd a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
44+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2024-07-18 402b586d0a 2b5819e2b4 Do not summarize WAL if generated with wal_level=minimal.
* Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
@ 2024-07-03 14:08 Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-03 19:13 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 44+ messages in thread
From: Fujii Masao @ 2024-07-03 14:08 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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 the following 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 the issue.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
v1-0001-Prevent-summarize_wal-from-enabling-when-wal_leve.patch (text/plain, 2.7K) download
From db4d879819417cade323249d6f68a1bef635857e Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 3 Jul 2024 20:17:42 +0900
Subject: [PATCH v1] Prevent summarize_wal from enabling when wal_level is
minimal.
WAL summarization should not be enabled if wal_level is set to minimal.
At startup, the postmaster checks these settings and exits with an error
if they are not configured properly. However, previously,
summarize_wal could be set to true after the server started with
wal_level = minimal, causing issues.
This commit adds a GUC check hook to ensure summarize_wal cannot
be enabled when wal_level is minimal.
---
src/backend/postmaster/walsummarizer.c | 16 +++++++++++++++-
src/backend/utils/misc/guc_tables.c | 2 +-
src/include/utils/guc_hooks.h | 1 +
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index 55dc2ea8f5..806daba5ea 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -44,7 +44,7 @@
#include "storage/proc.h"
#include "storage/procsignal.h"
#include "storage/shmem.h"
-#include "utils/guc.h"
+#include "utils/guc_hooks.h"
#include "utils/memutils.h"
#include "utils/wait_event.h"
@@ -1536,3 +1536,17 @@ MaybeRemoveOldWalSummaries(void)
}
}
}
+
+/*
+ * 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;
+}
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index d28b0bcb40..607e9856de 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -1781,7 +1781,7 @@ struct config_bool ConfigureNamesBool[] =
},
&summarize_wal,
false,
- NULL, NULL, NULL
+ check_summarize_wal, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 070d3f2a1a..83bf040ddf 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -138,6 +138,7 @@ extern bool check_ssl(bool *newval, void **extra, GucSource source);
extern bool check_stage_log_stats(bool *newval, void **extra, GucSource source);
extern bool check_subtrans_buffers(int *newval, void **extra,
GucSource source);
+extern bool check_summarize_wal(bool *newval, void **extra, GucSource source);
extern bool check_synchronous_standby_names(char **newval, void **extra,
GucSource source);
extern void assign_synchronous_standby_names(const char *newval, void *extra);
--
2.45.1
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-03 14:29 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-03 14:41 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Daniel Gustafsson <daniel@yesql.se>
2024-07-03 14:45 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-03 15:16 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
1 sibling, 4 replies; 44+ messages in thread
From: Nathan Bossart @ 2024-07-03 14:29 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-03 14:41 ` Daniel Gustafsson <daniel@yesql.se>
3 siblings, 0 replies; 44+ messages in thread
From: Daniel Gustafsson @ 2024-07-03 14:41 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-03 14:45 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-03 14:46 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
3 siblings, 1 reply; 44+ messages in thread
From: Jelte Fennema-Nio @ 2024-07-03 14:45 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-03 14:45 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
@ 2024-07-03 14:46 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-03 15:01 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
0 siblings, 1 reply; 44+ messages in thread
From: Jelte Fennema-Nio @ 2024-07-03 14:46 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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'
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-03 14:45 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-03 14:46 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
@ 2024-07-03 15:01 ` Jelte Fennema-Nio <postgres@jeltef.nl>
0 siblings, 0 replies; 44+ messages in thread
From: Jelte Fennema-Nio @ 2024-07-03 15:01 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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)
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-03 15:16 ` Tom Lane <tgl@sss.pgh.pa.us>
3 siblings, 0 replies; 44+ messages in thread
From: Tom Lane @ 2024-07-03 15:16 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-04 14:35 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
3 siblings, 1 reply; 44+ messages in thread
From: Fujii Masao @ 2024-07-04 14:35 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-08 19:37 ` Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-08 19:37 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
Attachments:
v1-0001-Do-not-summarize-WAL-if-generated-with-wal_level-.patch (application/octet-stream, 14.4K) download
From 9528afdedbd1dccb5f6717b8411bbdbefc0fc6b3 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Mon, 8 Jul 2024 15:14:57 -0400
Subject: [PATCH v1] Do not summarize WAL if generated with wal_level=minimal.
To do this, we must include the wal_level in the first WAL record
covered by each summary file; so add wal_level to struct Checkpoint
and the payload of XLOG_CHECKPOINT_REDO. This, in turn, requires
bumping XLOG_PAGE_MAGIC and, since the Checkpoint is also stored in
the control file, also PG_CONTROL_VERSION. It's not great to do
that so late in the release cycle, but the alternative seems to
ship v17 without robust protections against this scenario, which
could result in corrupted incremental backups.
Report by Fujii Masao.
---
src/backend/access/rmgrdesc/xlogdesc.c | 42 ++++--
src/backend/access/transam/xlog.c | 7 +-
src/backend/postmaster/walsummarizer.c | 178 +++++++++++++++++--------
src/include/access/xlog_internal.h | 2 +-
src/include/catalog/pg_control.h | 3 +-
5 files changed, 158 insertions(+), 74 deletions(-)
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index e455400716..f06f5c2461 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -33,6 +33,27 @@ const struct config_enum_entry wal_level_options[] = {
{NULL, 0, false}
};
+/*
+ * Find a string representation for wal_level
+ */
+static const char *
+get_wal_level_string(int wal_level)
+{
+ const struct config_enum_entry *entry;
+ const char *wal_level_str = "?";
+
+ for (entry = wal_level_options; entry->name; entry++)
+ {
+ if (entry->val == wal_level)
+ {
+ wal_level_str = entry->name;
+ break;
+ }
+ }
+
+ return wal_level_str;
+}
+
void
xlog_desc(StringInfo buf, XLogReaderState *record)
{
@@ -45,7 +66,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
- "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
+ "tli %u; prev tli %u; fpw %s; wal_level %s; xid %u:%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
@@ -53,6 +74,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
checkpoint->ThisTimeLineID,
checkpoint->PrevTimeLineID,
checkpoint->fullPageWrites ? "true" : "false",
+ get_wal_level_string(checkpoint->wal_level),
EpochFromFullTransactionId(checkpoint->nextXid),
XidFromFullTransactionId(checkpoint->nextXid),
checkpoint->nextOid,
@@ -95,20 +117,9 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
{
xl_parameter_change xlrec;
const char *wal_level_str;
- const struct config_enum_entry *entry;
memcpy(&xlrec, rec, sizeof(xl_parameter_change));
-
- /* Find a string representation for wal_level */
- wal_level_str = "?";
- for (entry = wal_level_options; entry->name; entry++)
- {
- if (entry->val == xlrec.wal_level)
- {
- wal_level_str = entry->name;
- break;
- }
- }
+ wal_level_str = get_wal_level_string(xlrec.wal_level);
appendStringInfo(buf, "max_connections=%d max_worker_processes=%d "
"max_wal_senders=%d max_prepared_xacts=%d "
@@ -150,7 +161,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
}
else if (info == XLOG_CHECKPOINT_REDO)
{
- /* No details to write out */
+ int wal_level;
+
+ memcpy(&wal_level, rec, sizeof(int));
+ appendStringInfo(buf, "wal_level %s", get_wal_level_string(wal_level));
}
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 33e27a6e72..fba015579e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6934,6 +6934,7 @@ CreateCheckPoint(int flags)
WALInsertLockAcquireExclusive();
checkPoint.fullPageWrites = Insert->fullPageWrites;
+ checkPoint.wal_level = wal_level;
if (shutdown)
{
@@ -6987,11 +6988,9 @@ CreateCheckPoint(int flags)
*/
if (!shutdown)
{
- int dummy = 0;
-
- /* Record must have payload to avoid assertion failure. */
+ /* Include WAL level in record for WAL summarizer's benefit. */
XLogBeginInsert();
- XLogRegisterData((char *) &dummy, sizeof(dummy));
+ XLogRegisterData((char *) &wal_level, sizeof(wal_level));
(void) XLogInsert(RM_XLOG_ID, XLOG_CHECKPOINT_REDO);
/*
diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index 55dc2ea8f5..c6fbf5b55f 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -71,6 +71,12 @@ typedef struct
* and so the LSN might point to the start of the next file even though
* that might happen to be in the middle of a WAL record.
*
+ * 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;
ProcNumber summarizer_pgprocno;
XLogRecPtr pending_lsn;
@@ -154,7 +161,8 @@ static void SummarizeSmgrRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
static void SummarizeXactRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
-static bool SummarizeXlogRecord(XLogReaderState *xlogreader);
+static bool SummarizeXlogRecord(XLogReaderState *xlogreader,
+ bool *new_fast_forward);
static int summarizer_read_local_xlog_page(XLogReaderState *state,
XLogRecPtr targetPagePtr,
int reqLen,
@@ -802,6 +810,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
char final_path[MAXPGPATH];
WalSummaryIO io;
BlockRefTable *brtab = CreateEmptyBlockRefTable();
+ bool fast_forward = true;
/* Initialize private data for xlogreader. */
private_data = (SummarizerReadLocalXLogPrivate *)
@@ -900,7 +909,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
int block_id;
char *errormsg;
XLogRecord *record;
- bool stop_requested = false;
+ uint8 rmid;
HandleWalSummarizerInterrupts();
@@ -969,56 +978,86 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
break;
}
- /* Special handling for particular types of WAL records. */
- switch (XLogRecGetRmid(xlogreader))
- {
- case RM_DBASE_ID:
- SummarizeDbaseRecord(xlogreader, brtab);
- break;
- case RM_SMGR_ID:
- SummarizeSmgrRecord(xlogreader, brtab);
- break;
- case RM_XACT_ID:
- SummarizeXactRecord(xlogreader, brtab);
- break;
- case RM_XLOG_ID:
- stop_requested = SummarizeXlogRecord(xlogreader);
- break;
- default:
- break;
- }
-
/*
- * If we've been told that it's time to end this WAL summary file, do
- * so. As an exception, if there's nothing included in this WAL
- * summary file yet, then stopping doesn't make any sense, and we
- * should wait until the next stop point instead.
+ * Certain types of records require special handling. Redo points and
+ * shutdown checkpoints trigger creation of new summary files and can
+ * also cause us to enter or exit "fast forward" mode. Other types of
+ * records can require special updates to the block reference table.
*/
- if (stop_requested && xlogreader->ReadRecPtr > summary_start_lsn)
+ rmid = XLogRecGetRmid(xlogreader);
+ if (rmid == RM_XLOG_ID)
{
- summary_end_lsn = xlogreader->ReadRecPtr;
- break;
+ bool new_fast_forward;
+
+ /*
+ * If we've already processed some WAL records when we hit a redo
+ * point or shutdown checkpoint, then we stop summarization before
+ * including this record in the current file, so that it will be
+ * the first record in the next file.
+ *
+ * When we hit one of those record types as the first record in a
+ * file, we adjust our notion of whether we're fast-forwarding.
+ * Any WAL generated with wal_level=minimal must be skipped
+ * without actually generating any summary file, because an
+ * incremental backup that crosses such WAL would be unsafe.
+ */
+ if (SummarizeXlogRecord(xlogreader, &new_fast_forward))
+ {
+ if (xlogreader->ReadRecPtr > summary_start_lsn)
+ {
+ summary_end_lsn = xlogreader->ReadRecPtr;
+ break;
+ }
+ else
+ fast_forward = new_fast_forward;
+ }
+ }
+ else if (!fast_forward)
+ {
+ /*
+ * This switch handles record types that require extra updates to
+ * the contents of the block reference table.
+ */
+ switch (rmid)
+ {
+ case RM_DBASE_ID:
+ SummarizeDbaseRecord(xlogreader, brtab);
+ break;
+ case RM_SMGR_ID:
+ SummarizeSmgrRecord(xlogreader, brtab);
+ break;
+ case RM_XACT_ID:
+ SummarizeXactRecord(xlogreader, brtab);
+ break;
+ }
}
- /* Feed block references from xlog record to block reference table. */
- for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
- block_id++)
+ /*
+ * If we're in fast-forward mode, we don't really need to do anything.
+ * Otherwise, feed block references from xlog record to block
+ * reference table.
+ */
+ if (!fast_forward)
{
- RelFileLocator rlocator;
- ForkNumber forknum;
- BlockNumber blocknum;
+ for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
+ block_id++)
+ {
+ RelFileLocator rlocator;
+ ForkNumber forknum;
+ BlockNumber blocknum;
- if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
- &forknum, &blocknum, NULL))
- continue;
+ if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
+ &forknum, &blocknum, NULL))
+ continue;
- /*
- * As we do elsewhere, ignore the FSM fork, because it's not fully
- * WAL-logged.
- */
- if (forknum != FSM_FORKNUM)
- BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
- blocknum);
+ /*
+ * As we do elsewhere, ignore the FSM fork, because it's not
+ * fully WAL-logged.
+ */
+ if (forknum != FSM_FORKNUM)
+ BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
+ blocknum);
+ }
}
/* Update our notion of where this summary file ends. */
@@ -1047,9 +1086,10 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
/*
* If a timeline switch occurs, we may fail to make any progress at all
* before exiting the loop above. If that happens, we don't write a WAL
- * summary file at all.
+ * summary file at all. We can also skip writing a file if we're in
+ * fast-forward mode.
*/
- if (summary_end_lsn > summary_start_lsn)
+ if (summary_end_lsn > summary_start_lsn && !fast_forward)
{
/* Generate temporary and final path name. */
snprintf(temp_path, MAXPGPATH,
@@ -1085,6 +1125,14 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
durable_rename(temp_path, final_path, ERROR);
}
+ /* If we skipped a non-zero amount of WAL, log a debug message. */
+ if (summary_end_lsn > summary_start_lsn && fast_forward)
+ ereport(DEBUG1,
+ errmsg("skipped summarizing WAL on TLI %u from %X/%X to %X/%X",
+ tli,
+ LSN_FORMAT_ARGS(summary_start_lsn),
+ LSN_FORMAT_ARGS(summary_end_lsn)));
+
return summary_end_lsn;
}
@@ -1263,22 +1311,44 @@ SummarizeXactRecord(XLogReaderState *xlogreader, BlockRefTable *brtab)
/*
* Special handling for WAL records with RM_XLOG_ID.
+ *
+ * The return value is true if WAL summarization should stop before this
+ * record and false otherwise. When the return value is true,
+ * *new_fast_forward indicates whether future processing should be done
+ * in fast forward mode (i.e. read WAL without emitting summaries) or not.
*/
static bool
-SummarizeXlogRecord(XLogReaderState *xlogreader)
+SummarizeXlogRecord(XLogReaderState *xlogreader, bool *new_fast_forward)
{
uint8 info = XLogRecGetInfo(xlogreader) & ~XLR_INFO_MASK;
+ int record_wal_level;
- if (info == XLOG_CHECKPOINT_REDO || info == XLOG_CHECKPOINT_SHUTDOWN)
+ if (info == XLOG_CHECKPOINT_REDO)
{
- /*
- * This is an LSN at which redo might begin, so we'd like
- * summarization to stop just before this WAL record.
- */
- return true;
+ /* Payload is wal_level at the time record was written. */
+ memcpy(&record_wal_level, XLogRecGetData(xlogreader), sizeof(int));
}
+ else if (info == XLOG_CHECKPOINT_SHUTDOWN)
+ {
+ CheckPoint rec_ckpt;
- return false;
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&rec_ckpt, XLogRecGetData(xlogreader), sizeof(CheckPoint));
+ record_wal_level = rec_ckpt.wal_level;
+ }
+ else
+ {
+ /* No special handling required. Return false. */
+ return false;
+ }
+
+ /*
+ * We only get here if this is an LSN at which redo might begin, and if
+ * that is the case, then we would like WAL summarization to stop just
+ * before this WAL record.
+ */
+ *new_fast_forward = (record_wal_level == WAL_LEVEL_MINIMAL);
+ return true;
}
/*
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 5161b72f28..f270065fd3 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -31,7 +31,7 @@
/*
* Each page of XLOG file has a header like this:
*/
-#define XLOG_PAGE_MAGIC 0xD115 /* can be used as WAL version indicator */
+#define XLOG_PAGE_MAGIC 0xD116 /* can be used as WAL version indicator */
typedef struct XLogPageHeaderData
{
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index a00606ffcd..e80ff8e414 100644
--- a/src/include/catalog/pg_control.h
+++ b/src/include/catalog/pg_control.h
@@ -22,7 +22,7 @@
/* Version identifier for this pg_control format */
-#define PG_CONTROL_VERSION 1300
+#define PG_CONTROL_VERSION 1700
/* Nonce key length, see below */
#define MOCK_AUTH_NONCE_LEN 32
@@ -40,6 +40,7 @@ typedef struct CheckPoint
TimeLineID PrevTimeLineID; /* previous TLI, if this record begins a new
* timeline (equals ThisTimeLineID otherwise) */
bool fullPageWrites; /* current full_page_writes */
+ int wal_level; /* current wal_level */
FullTransactionId nextXid; /* next free transaction ID */
Oid nextOid; /* next free OID */
MultiXactId nextMulti; /* next free MultiXactId */
--
2.39.3 (Apple Git-145)
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-09 17:57 ` Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-09 17:57 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
Attachments:
v2-0001-Do-not-summarize-WAL-if-generated-with-wal_level-.patch (application/octet-stream, 16.1K) download
From f51f2aba432afa49483bf8e7fd9f5aeb0dae83cc Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Tue, 9 Jul 2024 13:22:06 -0400
Subject: [PATCH v2] Do not summarize WAL if generated with wal_level=minimal.
To do this, we must include the wal_level in the first WAL record
covered by each summary file; so add wal_level to struct Checkpoint
and the payload of XLOG_CHECKPOINT_REDO and XLOG_END_OF_RECOVERY.
This, in turn, requires bumping XLOG_PAGE_MAGIC and, since the
Checkpoint is also stored in the control file, also
PG_CONTROL_VERSION. It's not great to do that so late in the release
cycle, but the alternative seems to ship v17 without robust
protections against this scenario, which could result in corrupted
incremental backups.
Report by Fujii Masao. Tested by Jakub Wartak.
---
src/backend/access/rmgrdesc/xlogdesc.c | 47 +++---
src/backend/access/transam/xlog.c | 8 +-
src/backend/postmaster/walsummarizer.c | 192 ++++++++++++++++++-------
src/include/access/xlog_internal.h | 3 +-
src/include/catalog/pg_control.h | 3 +-
5 files changed, 177 insertions(+), 76 deletions(-)
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index e455400716..363294d623 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -33,6 +33,27 @@ const struct config_enum_entry wal_level_options[] = {
{NULL, 0, false}
};
+/*
+ * Find a string representation for wal_level
+ */
+static const char *
+get_wal_level_string(int wal_level)
+{
+ const struct config_enum_entry *entry;
+ const char *wal_level_str = "?";
+
+ for (entry = wal_level_options; entry->name; entry++)
+ {
+ if (entry->val == wal_level)
+ {
+ wal_level_str = entry->name;
+ break;
+ }
+ }
+
+ return wal_level_str;
+}
+
void
xlog_desc(StringInfo buf, XLogReaderState *record)
{
@@ -45,7 +66,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
- "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
+ "tli %u; prev tli %u; fpw %s; wal_level %s; xid %u:%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
@@ -53,6 +74,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
checkpoint->ThisTimeLineID,
checkpoint->PrevTimeLineID,
checkpoint->fullPageWrites ? "true" : "false",
+ get_wal_level_string(checkpoint->wal_level),
EpochFromFullTransactionId(checkpoint->nextXid),
XidFromFullTransactionId(checkpoint->nextXid),
checkpoint->nextOid,
@@ -95,20 +117,9 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
{
xl_parameter_change xlrec;
const char *wal_level_str;
- const struct config_enum_entry *entry;
memcpy(&xlrec, rec, sizeof(xl_parameter_change));
-
- /* Find a string representation for wal_level */
- wal_level_str = "?";
- for (entry = wal_level_options; entry->name; entry++)
- {
- if (entry->val == xlrec.wal_level)
- {
- wal_level_str = entry->name;
- break;
- }
- }
+ wal_level_str = get_wal_level_string(xlrec.wal_level);
appendStringInfo(buf, "max_connections=%d max_worker_processes=%d "
"max_wal_senders=%d max_prepared_xacts=%d "
@@ -135,9 +146,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
xl_end_of_recovery xlrec;
memcpy(&xlrec, rec, sizeof(xl_end_of_recovery));
- appendStringInfo(buf, "tli %u; prev tli %u; time %s",
+ appendStringInfo(buf, "tli %u; prev tli %u; time %s; wal_level %s",
xlrec.ThisTimeLineID, xlrec.PrevTimeLineID,
- timestamptz_to_str(xlrec.end_time));
+ timestamptz_to_str(xlrec.end_time),
+ get_wal_level_string(xlrec.wal_level));
}
else if (info == XLOG_OVERWRITE_CONTRECORD)
{
@@ -150,7 +162,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
}
else if (info == XLOG_CHECKPOINT_REDO)
{
- /* No details to write out */
+ int wal_level;
+
+ memcpy(&wal_level, rec, sizeof(int));
+ appendStringInfo(buf, "wal_level %s", get_wal_level_string(wal_level));
}
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 33e27a6e72..636be5ca4d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6934,6 +6934,7 @@ CreateCheckPoint(int flags)
WALInsertLockAcquireExclusive();
checkPoint.fullPageWrites = Insert->fullPageWrites;
+ checkPoint.wal_level = wal_level;
if (shutdown)
{
@@ -6987,11 +6988,9 @@ CreateCheckPoint(int flags)
*/
if (!shutdown)
{
- int dummy = 0;
-
- /* Record must have payload to avoid assertion failure. */
+ /* Include WAL level in record for WAL summarizer's benefit. */
XLogBeginInsert();
- XLogRegisterData((char *) &dummy, sizeof(dummy));
+ XLogRegisterData((char *) &wal_level, sizeof(wal_level));
(void) XLogInsert(RM_XLOG_ID, XLOG_CHECKPOINT_REDO);
/*
@@ -7314,6 +7313,7 @@ CreateEndOfRecoveryRecord(void)
elog(ERROR, "can only be used to end recovery");
xlrec.end_time = GetCurrentTimestamp();
+ xlrec.wal_level = wal_level;
WALInsertLockAcquireExclusive();
xlrec.ThisTimeLineID = XLogCtl->InsertTimeLineID;
diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index 55dc2ea8f5..207b126983 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -71,6 +71,12 @@ typedef struct
* and so the LSN might point to the start of the next file even though
* that might happen to be in the middle of a WAL record.
*
+ * 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;
ProcNumber summarizer_pgprocno;
XLogRecPtr pending_lsn;
@@ -154,7 +161,8 @@ static void SummarizeSmgrRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
static void SummarizeXactRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
-static bool SummarizeXlogRecord(XLogReaderState *xlogreader);
+static bool SummarizeXlogRecord(XLogReaderState *xlogreader,
+ bool *new_fast_forward);
static int summarizer_read_local_xlog_page(XLogReaderState *state,
XLogRecPtr targetPagePtr,
int reqLen,
@@ -802,6 +810,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
char final_path[MAXPGPATH];
WalSummaryIO io;
BlockRefTable *brtab = CreateEmptyBlockRefTable();
+ bool fast_forward = true;
/* Initialize private data for xlogreader. */
private_data = (SummarizerReadLocalXLogPrivate *)
@@ -900,7 +909,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
int block_id;
char *errormsg;
XLogRecord *record;
- bool stop_requested = false;
+ uint8 rmid;
HandleWalSummarizerInterrupts();
@@ -969,56 +978,86 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
break;
}
- /* Special handling for particular types of WAL records. */
- switch (XLogRecGetRmid(xlogreader))
- {
- case RM_DBASE_ID:
- SummarizeDbaseRecord(xlogreader, brtab);
- break;
- case RM_SMGR_ID:
- SummarizeSmgrRecord(xlogreader, brtab);
- break;
- case RM_XACT_ID:
- SummarizeXactRecord(xlogreader, brtab);
- break;
- case RM_XLOG_ID:
- stop_requested = SummarizeXlogRecord(xlogreader);
- break;
- default:
- break;
- }
-
/*
- * If we've been told that it's time to end this WAL summary file, do
- * so. As an exception, if there's nothing included in this WAL
- * summary file yet, then stopping doesn't make any sense, and we
- * should wait until the next stop point instead.
+ * Certain types of records require special handling. Redo points and
+ * shutdown checkpoints trigger creation of new summary files and can
+ * also cause us to enter or exit "fast forward" mode. Other types of
+ * records can require special updates to the block reference table.
*/
- if (stop_requested && xlogreader->ReadRecPtr > summary_start_lsn)
+ rmid = XLogRecGetRmid(xlogreader);
+ if (rmid == RM_XLOG_ID)
{
- summary_end_lsn = xlogreader->ReadRecPtr;
- break;
+ bool new_fast_forward;
+
+ /*
+ * If we've already processed some WAL records when we hit a redo
+ * point or shutdown checkpoint, then we stop summarization before
+ * including this record in the current file, so that it will be
+ * the first record in the next file.
+ *
+ * When we hit one of those record types as the first record in a
+ * file, we adjust our notion of whether we're fast-forwarding.
+ * Any WAL generated with wal_level=minimal must be skipped
+ * without actually generating any summary file, because an
+ * incremental backup that crosses such WAL would be unsafe.
+ */
+ if (SummarizeXlogRecord(xlogreader, &new_fast_forward))
+ {
+ if (xlogreader->ReadRecPtr > summary_start_lsn)
+ {
+ summary_end_lsn = xlogreader->ReadRecPtr;
+ break;
+ }
+ else
+ fast_forward = new_fast_forward;
+ }
+ }
+ else if (!fast_forward)
+ {
+ /*
+ * This switch handles record types that require extra updates to
+ * the contents of the block reference table.
+ */
+ switch (rmid)
+ {
+ case RM_DBASE_ID:
+ SummarizeDbaseRecord(xlogreader, brtab);
+ break;
+ case RM_SMGR_ID:
+ SummarizeSmgrRecord(xlogreader, brtab);
+ break;
+ case RM_XACT_ID:
+ SummarizeXactRecord(xlogreader, brtab);
+ break;
+ }
}
- /* Feed block references from xlog record to block reference table. */
- for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
- block_id++)
+ /*
+ * If we're in fast-forward mode, we don't really need to do anything.
+ * Otherwise, feed block references from xlog record to block
+ * reference table.
+ */
+ if (!fast_forward)
{
- RelFileLocator rlocator;
- ForkNumber forknum;
- BlockNumber blocknum;
+ for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
+ block_id++)
+ {
+ RelFileLocator rlocator;
+ ForkNumber forknum;
+ BlockNumber blocknum;
- if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
- &forknum, &blocknum, NULL))
- continue;
+ if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
+ &forknum, &blocknum, NULL))
+ continue;
- /*
- * As we do elsewhere, ignore the FSM fork, because it's not fully
- * WAL-logged.
- */
- if (forknum != FSM_FORKNUM)
- BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
- blocknum);
+ /*
+ * As we do elsewhere, ignore the FSM fork, because it's not
+ * fully WAL-logged.
+ */
+ if (forknum != FSM_FORKNUM)
+ BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
+ blocknum);
+ }
}
/* Update our notion of where this summary file ends. */
@@ -1047,9 +1086,10 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
/*
* If a timeline switch occurs, we may fail to make any progress at all
* before exiting the loop above. If that happens, we don't write a WAL
- * summary file at all.
+ * summary file at all. We can also skip writing a file if we're in
+ * fast-forward mode.
*/
- if (summary_end_lsn > summary_start_lsn)
+ if (summary_end_lsn > summary_start_lsn && !fast_forward)
{
/* Generate temporary and final path name. */
snprintf(temp_path, MAXPGPATH,
@@ -1085,6 +1125,14 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
durable_rename(temp_path, final_path, ERROR);
}
+ /* If we skipped a non-zero amount of WAL, log a debug message. */
+ if (summary_end_lsn > summary_start_lsn && fast_forward)
+ ereport(DEBUG1,
+ errmsg("skipped summarizing WAL on TLI %u from %X/%X to %X/%X",
+ tli,
+ LSN_FORMAT_ARGS(summary_start_lsn),
+ LSN_FORMAT_ARGS(summary_end_lsn)));
+
return summary_end_lsn;
}
@@ -1263,22 +1311,58 @@ SummarizeXactRecord(XLogReaderState *xlogreader, BlockRefTable *brtab)
/*
* Special handling for WAL records with RM_XLOG_ID.
+ *
+ * The return value is true if WAL summarization should stop before this
+ * record and false otherwise. When the return value is true,
+ * *new_fast_forward indicates whether future processing should be done
+ * in fast forward mode (i.e. read WAL without emitting summaries) or not.
*/
static bool
-SummarizeXlogRecord(XLogReaderState *xlogreader)
+SummarizeXlogRecord(XLogReaderState *xlogreader, bool *new_fast_forward)
{
uint8 info = XLogRecGetInfo(xlogreader) & ~XLR_INFO_MASK;
+ int record_wal_level;
- if (info == XLOG_CHECKPOINT_REDO || info == XLOG_CHECKPOINT_SHUTDOWN)
+ if (info == XLOG_CHECKPOINT_REDO)
{
- /*
- * This is an LSN at which redo might begin, so we'd like
- * summarization to stop just before this WAL record.
- */
- return true;
+ /* Payload is wal_level at the time record was written. */
+ memcpy(&record_wal_level, XLogRecGetData(xlogreader), sizeof(int));
+ }
+ else if (info == XLOG_CHECKPOINT_SHUTDOWN)
+ {
+ CheckPoint rec_ckpt;
+
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&rec_ckpt, XLogRecGetData(xlogreader), sizeof(CheckPoint));
+ record_wal_level = rec_ckpt.wal_level;
+ }
+ else if (info == XLOG_END_OF_RECOVERY)
+ {
+ xl_end_of_recovery xlrec;
+
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&xlrec, XLogRecGetData(xlogreader), sizeof(xl_end_of_recovery));
+ record_wal_level = xlrec.wal_level;
+ }
+ else
+ {
+ /* No special handling required. Return false. */
+ return false;
}
- return false;
+ /*
+ * Redo can only begin at an XLOG_CHECKPOINT_REDO or
+ * XLOG_CHECKPOINT_SHUTDOWN record, so we want WAL summarization to begin
+ * at those points. Hence, when those records are encountered, return true,
+ * so that we stop just before summarizing either of those records.
+ *
+ * We also reach here if we just saw XLOG_END_OF_RECOVERY. That's not
+ * a place where recovery can start, but it is a place where we might need
+ * WAL summarization can start, because it's the first record written
+ * after a timeline switch.
+ */
+ *new_fast_forward = (record_wal_level == WAL_LEVEL_MINIMAL);
+ return true;
}
/*
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 5161b72f28..c6a91fb456 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -31,7 +31,7 @@
/*
* Each page of XLOG file has a header like this:
*/
-#define XLOG_PAGE_MAGIC 0xD115 /* can be used as WAL version indicator */
+#define XLOG_PAGE_MAGIC 0xD116 /* can be used as WAL version indicator */
typedef struct XLogPageHeaderData
{
@@ -302,6 +302,7 @@ typedef struct xl_end_of_recovery
TimestampTz end_time;
TimeLineID ThisTimeLineID; /* new TLI */
TimeLineID PrevTimeLineID; /* previous TLI we forked off from */
+ int wal_level;
} xl_end_of_recovery;
/*
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index a00606ffcd..e80ff8e414 100644
--- a/src/include/catalog/pg_control.h
+++ b/src/include/catalog/pg_control.h
@@ -22,7 +22,7 @@
/* Version identifier for this pg_control format */
-#define PG_CONTROL_VERSION 1300
+#define PG_CONTROL_VERSION 1700
/* Nonce key length, see below */
#define MOCK_AUTH_NONCE_LEN 32
@@ -40,6 +40,7 @@ typedef struct CheckPoint
TimeLineID PrevTimeLineID; /* previous TLI, if this record begins a new
* timeline (equals ThisTimeLineID otherwise) */
bool fullPageWrites; /* current full_page_writes */
+ int wal_level; /* current wal_level */
FullTransactionId nextXid; /* next free transaction ID */
Oid nextOid; /* next free OID */
MultiXactId nextMulti; /* next free MultiXactId */
--
2.39.3 (Apple Git-145)
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-10 05:56 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Fujii Masao @ 2024-07-10 05:56 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-10 14:10 ` Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 44+ messages in thread
From: Robert Haas @ 2024-07-10 14:10 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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...
http://postgr.es/m/3253790.1720019802@sss.pgh.pa.us
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-10 14:18 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 16:02 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
1 sibling, 2 replies; 44+ messages in thread
From: Nathan Bossart @ 2024-07-10 14:18 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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...
> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-10 14:29 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 14:46 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 15:11 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 44+ messages in thread
From: Jelte Fennema-Nio @ 2024-07-10 14:29 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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.
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
@ 2024-07-10 14:46 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 15:08 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
1 sibling, 1 reply; 44+ messages in thread
From: Nathan Bossart @ 2024-07-10 14:46 UTC (permalink / raw)
To: Jelte Fennema-Nio <postgres@jeltef.nl>; +Cc: Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 14:46 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-10 15:08 ` Jelte Fennema-Nio <postgres@jeltef.nl>
0 siblings, 0 replies; 44+ messages in thread
From: Jelte Fennema-Nio @ 2024-07-10 15:08 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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.
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
@ 2024-07-10 15:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 15:44 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 18:41 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 2 replies; 44+ messages in thread
From: Tom Lane @ 2024-07-10 15:11 UTC (permalink / raw)
To: Jelte Fennema-Nio <postgres@jeltef.nl>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 15:11 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-07-10 15:44 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 15:54 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 16:07 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
1 sibling, 2 replies; 44+ messages in thread
From: Nathan Bossart @ 2024-07-10 15:44 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 15:11 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 15:44 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-10 15:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 16:15 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 1 reply; 44+ messages in thread
From: Tom Lane @ 2024-07-10 15:54 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 15:11 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 15:44 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 15:54 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-07-10 16:15 ` Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 44+ messages in thread
From: Nathan Bossart @ 2024-07-10 16:15 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 15:11 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 15:44 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-10 16:07 ` Fujii Masao <masao.fujii@oss.nttdata.com>
1 sibling, 0 replies; 44+ messages in thread
From: Fujii Masao @ 2024-07-10 16:07 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 15:11 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-07-10 18:41 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-11 20:42 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 1 reply; 44+ messages in thread
From: Nathan Bossart @ 2024-07-10 18:41 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 15:11 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 18:41 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-11 20:42 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-11 21:46 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 44+ messages in thread
From: Nathan Bossart @ 2024-07-11 20:42 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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%40mai...
https://www.postgresql.org/message-id/flat/CAMkU%3D1zTMNZsnUV6L7aMvfJZfzjKbzAtuML3N35wyYaia9MJAw%40m...
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ee1e5662d8d8330726eaef7d3110cb7add24d...
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2850896961994aa0993b9e2ed79a209750181...
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=af930e606a3217db3909029c6c3f8d003ba70...
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=a16d421ca4fc639929bc964b2585e8382...
--
nathan
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 15:11 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 18:41 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-11 20:42 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-11 21:46 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 44+ messages in thread
From: Tom Lane @ 2024-07-11 21:46 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Jelte Fennema-Nio <postgres@jeltef.nl>; Robert Haas <robertmhaas@gmail.com>; Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-10 16:02 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 16:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 1 reply; 44+ messages in thread
From: Fujii Masao @ 2024-07-10 16:02 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
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...
>> 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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 16:02 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-10 16:29 ` Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 44+ messages in thread
From: Nathan Bossart @ 2024-07-10 16:29 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-10 16:35 ` Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
1 sibling, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-10 16:35 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
Attachments:
v3-0001-Do-not-summarize-WAL-if-generated-with-wal_level-.patch (application/octet-stream, 16.6K) download
From ab6f60502ce25b5bd90d0c4ebed3d1dae06a5813 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 10 Jul 2024 12:31:09 -0400
Subject: [PATCH v3] Do not summarize WAL if generated with wal_level=minimal.
To do this, we must include the wal_level in the first WAL record
covered by each summary file; so add wal_level to struct Checkpoint
and the payload of XLOG_CHECKPOINT_REDO and XLOG_END_OF_RECOVERY.
This, in turn, requires bumping XLOG_PAGE_MAGIC and, since the
Checkpoint is also stored in the control file, also
PG_CONTROL_VERSION. It's not great to do that so late in the release
cycle, but the alternative seems to ship v17 without robust
protections against this scenario, which could result in corrupted
incremental backups.
Report by Fujii Masao. Tested by Jakub Wartak and Fujii Masao.
---
src/backend/access/rmgrdesc/xlogdesc.c | 47 ++++--
src/backend/access/transam/xlog.c | 8 +-
src/backend/postmaster/walsummarizer.c | 203 ++++++++++++++++++-------
src/include/access/xlog_internal.h | 3 +-
src/include/catalog/pg_control.h | 3 +-
5 files changed, 188 insertions(+), 76 deletions(-)
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index e455400716..363294d623 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -33,6 +33,27 @@ const struct config_enum_entry wal_level_options[] = {
{NULL, 0, false}
};
+/*
+ * Find a string representation for wal_level
+ */
+static const char *
+get_wal_level_string(int wal_level)
+{
+ const struct config_enum_entry *entry;
+ const char *wal_level_str = "?";
+
+ for (entry = wal_level_options; entry->name; entry++)
+ {
+ if (entry->val == wal_level)
+ {
+ wal_level_str = entry->name;
+ break;
+ }
+ }
+
+ return wal_level_str;
+}
+
void
xlog_desc(StringInfo buf, XLogReaderState *record)
{
@@ -45,7 +66,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
- "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
+ "tli %u; prev tli %u; fpw %s; wal_level %s; xid %u:%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
@@ -53,6 +74,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
checkpoint->ThisTimeLineID,
checkpoint->PrevTimeLineID,
checkpoint->fullPageWrites ? "true" : "false",
+ get_wal_level_string(checkpoint->wal_level),
EpochFromFullTransactionId(checkpoint->nextXid),
XidFromFullTransactionId(checkpoint->nextXid),
checkpoint->nextOid,
@@ -95,20 +117,9 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
{
xl_parameter_change xlrec;
const char *wal_level_str;
- const struct config_enum_entry *entry;
memcpy(&xlrec, rec, sizeof(xl_parameter_change));
-
- /* Find a string representation for wal_level */
- wal_level_str = "?";
- for (entry = wal_level_options; entry->name; entry++)
- {
- if (entry->val == xlrec.wal_level)
- {
- wal_level_str = entry->name;
- break;
- }
- }
+ wal_level_str = get_wal_level_string(xlrec.wal_level);
appendStringInfo(buf, "max_connections=%d max_worker_processes=%d "
"max_wal_senders=%d max_prepared_xacts=%d "
@@ -135,9 +146,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
xl_end_of_recovery xlrec;
memcpy(&xlrec, rec, sizeof(xl_end_of_recovery));
- appendStringInfo(buf, "tli %u; prev tli %u; time %s",
+ appendStringInfo(buf, "tli %u; prev tli %u; time %s; wal_level %s",
xlrec.ThisTimeLineID, xlrec.PrevTimeLineID,
- timestamptz_to_str(xlrec.end_time));
+ timestamptz_to_str(xlrec.end_time),
+ get_wal_level_string(xlrec.wal_level));
}
else if (info == XLOG_OVERWRITE_CONTRECORD)
{
@@ -150,7 +162,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
}
else if (info == XLOG_CHECKPOINT_REDO)
{
- /* No details to write out */
+ int wal_level;
+
+ memcpy(&wal_level, rec, sizeof(int));
+ appendStringInfo(buf, "wal_level %s", get_wal_level_string(wal_level));
}
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 33e27a6e72..636be5ca4d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6934,6 +6934,7 @@ CreateCheckPoint(int flags)
WALInsertLockAcquireExclusive();
checkPoint.fullPageWrites = Insert->fullPageWrites;
+ checkPoint.wal_level = wal_level;
if (shutdown)
{
@@ -6987,11 +6988,9 @@ CreateCheckPoint(int flags)
*/
if (!shutdown)
{
- int dummy = 0;
-
- /* Record must have payload to avoid assertion failure. */
+ /* Include WAL level in record for WAL summarizer's benefit. */
XLogBeginInsert();
- XLogRegisterData((char *) &dummy, sizeof(dummy));
+ XLogRegisterData((char *) &wal_level, sizeof(wal_level));
(void) XLogInsert(RM_XLOG_ID, XLOG_CHECKPOINT_REDO);
/*
@@ -7314,6 +7313,7 @@ CreateEndOfRecoveryRecord(void)
elog(ERROR, "can only be used to end recovery");
xlrec.end_time = GetCurrentTimestamp();
+ xlrec.wal_level = wal_level;
WALInsertLockAcquireExclusive();
xlrec.ThisTimeLineID = XLogCtl->InsertTimeLineID;
diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index 55dc2ea8f5..663009cd24 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -71,6 +71,12 @@ typedef struct
* and so the LSN might point to the start of the next file even though
* that might happen to be in the middle of a WAL record.
*
+ * 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;
ProcNumber summarizer_pgprocno;
XLogRecPtr pending_lsn;
@@ -154,7 +161,8 @@ static void SummarizeSmgrRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
static void SummarizeXactRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
-static bool SummarizeXlogRecord(XLogReaderState *xlogreader);
+static bool SummarizeXlogRecord(XLogReaderState *xlogreader,
+ bool *new_fast_forward);
static int summarizer_read_local_xlog_page(XLogReaderState *state,
XLogRecPtr targetPagePtr,
int reqLen,
@@ -802,6 +810,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
char final_path[MAXPGPATH];
WalSummaryIO io;
BlockRefTable *brtab = CreateEmptyBlockRefTable();
+ bool fast_forward = true;
/* Initialize private data for xlogreader. */
private_data = (SummarizerReadLocalXLogPrivate *)
@@ -900,7 +909,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
int block_id;
char *errormsg;
XLogRecord *record;
- bool stop_requested = false;
+ uint8 rmid;
HandleWalSummarizerInterrupts();
@@ -969,56 +978,86 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
break;
}
- /* Special handling for particular types of WAL records. */
- switch (XLogRecGetRmid(xlogreader))
- {
- case RM_DBASE_ID:
- SummarizeDbaseRecord(xlogreader, brtab);
- break;
- case RM_SMGR_ID:
- SummarizeSmgrRecord(xlogreader, brtab);
- break;
- case RM_XACT_ID:
- SummarizeXactRecord(xlogreader, brtab);
- break;
- case RM_XLOG_ID:
- stop_requested = SummarizeXlogRecord(xlogreader);
- break;
- default:
- break;
- }
-
/*
- * If we've been told that it's time to end this WAL summary file, do
- * so. As an exception, if there's nothing included in this WAL
- * summary file yet, then stopping doesn't make any sense, and we
- * should wait until the next stop point instead.
+ * Certain types of records require special handling. Redo points and
+ * shutdown checkpoints trigger creation of new summary files and can
+ * also cause us to enter or exit "fast forward" mode. Other types of
+ * records can require special updates to the block reference table.
*/
- if (stop_requested && xlogreader->ReadRecPtr > summary_start_lsn)
+ rmid = XLogRecGetRmid(xlogreader);
+ if (rmid == RM_XLOG_ID)
{
- summary_end_lsn = xlogreader->ReadRecPtr;
- break;
+ bool new_fast_forward;
+
+ /*
+ * If we've already processed some WAL records when we hit a redo
+ * point or shutdown checkpoint, then we stop summarization before
+ * including this record in the current file, so that it will be
+ * the first record in the next file.
+ *
+ * When we hit one of those record types as the first record in a
+ * file, we adjust our notion of whether we're fast-forwarding.
+ * Any WAL generated with wal_level=minimal must be skipped
+ * without actually generating any summary file, because an
+ * incremental backup that crosses such WAL would be unsafe.
+ */
+ if (SummarizeXlogRecord(xlogreader, &new_fast_forward))
+ {
+ if (xlogreader->ReadRecPtr > summary_start_lsn)
+ {
+ summary_end_lsn = xlogreader->ReadRecPtr;
+ break;
+ }
+ else
+ fast_forward = new_fast_forward;
+ }
+ }
+ else if (!fast_forward)
+ {
+ /*
+ * This switch handles record types that require extra updates to
+ * the contents of the block reference table.
+ */
+ switch (rmid)
+ {
+ case RM_DBASE_ID:
+ SummarizeDbaseRecord(xlogreader, brtab);
+ break;
+ case RM_SMGR_ID:
+ SummarizeSmgrRecord(xlogreader, brtab);
+ break;
+ case RM_XACT_ID:
+ SummarizeXactRecord(xlogreader, brtab);
+ break;
+ }
}
- /* Feed block references from xlog record to block reference table. */
- for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
- block_id++)
+ /*
+ * If we're in fast-forward mode, we don't really need to do anything.
+ * Otherwise, feed block references from xlog record to block
+ * reference table.
+ */
+ if (!fast_forward)
{
- RelFileLocator rlocator;
- ForkNumber forknum;
- BlockNumber blocknum;
+ for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
+ block_id++)
+ {
+ RelFileLocator rlocator;
+ ForkNumber forknum;
+ BlockNumber blocknum;
- if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
- &forknum, &blocknum, NULL))
- continue;
+ if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
+ &forknum, &blocknum, NULL))
+ continue;
- /*
- * As we do elsewhere, ignore the FSM fork, because it's not fully
- * WAL-logged.
- */
- if (forknum != FSM_FORKNUM)
- BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
- blocknum);
+ /*
+ * As we do elsewhere, ignore the FSM fork, because it's not
+ * fully WAL-logged.
+ */
+ if (forknum != FSM_FORKNUM)
+ BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
+ blocknum);
+ }
}
/* Update our notion of where this summary file ends. */
@@ -1047,9 +1086,10 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
/*
* If a timeline switch occurs, we may fail to make any progress at all
* before exiting the loop above. If that happens, we don't write a WAL
- * summary file at all.
+ * summary file at all. We can also skip writing a file if we're in
+ * fast-forward mode.
*/
- if (summary_end_lsn > summary_start_lsn)
+ if (summary_end_lsn > summary_start_lsn && !fast_forward)
{
/* Generate temporary and final path name. */
snprintf(temp_path, MAXPGPATH,
@@ -1085,6 +1125,14 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
durable_rename(temp_path, final_path, ERROR);
}
+ /* If we skipped a non-zero amount of WAL, log a debug message. */
+ if (summary_end_lsn > summary_start_lsn && fast_forward)
+ ereport(DEBUG1,
+ errmsg("skipped summarizing WAL on TLI %u from %X/%X to %X/%X",
+ tli,
+ LSN_FORMAT_ARGS(summary_start_lsn),
+ LSN_FORMAT_ARGS(summary_end_lsn)));
+
return summary_end_lsn;
}
@@ -1263,22 +1311,69 @@ SummarizeXactRecord(XLogReaderState *xlogreader, BlockRefTable *brtab)
/*
* Special handling for WAL records with RM_XLOG_ID.
+ *
+ * The return value is true if WAL summarization should stop before this
+ * record and false otherwise. When the return value is true,
+ * *new_fast_forward indicates whether future processing should be done
+ * in fast forward mode (i.e. read WAL without emitting summaries) or not.
*/
static bool
-SummarizeXlogRecord(XLogReaderState *xlogreader)
+SummarizeXlogRecord(XLogReaderState *xlogreader, bool *new_fast_forward)
{
uint8 info = XLogRecGetInfo(xlogreader) & ~XLR_INFO_MASK;
+ int record_wal_level;
- if (info == XLOG_CHECKPOINT_REDO || info == XLOG_CHECKPOINT_SHUTDOWN)
+ if (info == XLOG_CHECKPOINT_REDO)
{
- /*
- * This is an LSN at which redo might begin, so we'd like
- * summarization to stop just before this WAL record.
- */
- return true;
+ /* Payload is wal_level at the time record was written. */
+ memcpy(&record_wal_level, XLogRecGetData(xlogreader), sizeof(int));
}
+ else if (info == XLOG_CHECKPOINT_SHUTDOWN)
+ {
+ CheckPoint rec_ckpt;
- return false;
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&rec_ckpt, XLogRecGetData(xlogreader), sizeof(CheckPoint));
+ record_wal_level = rec_ckpt.wal_level;
+ }
+ else if (info == XLOG_PARAMETER_CHANGE)
+ {
+ xl_parameter_change xlrec;
+
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&xlrec, XLogRecGetData(xlogreader),
+ sizeof(xl_parameter_change));
+ record_wal_level = xlrec.wal_level;
+ }
+ else if (info == XLOG_END_OF_RECOVERY)
+ {
+ xl_end_of_recovery xlrec;
+
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&xlrec, XLogRecGetData(xlogreader), sizeof(xl_end_of_recovery));
+ record_wal_level = xlrec.wal_level;
+ }
+ else
+ {
+ /* No special handling required. Return false. */
+ return false;
+ }
+
+ /*
+ * Redo can only begin at an XLOG_CHECKPOINT_REDO or
+ * XLOG_CHECKPOINT_SHUTDOWN record, so we want WAL summarization to begin
+ * at those points. Hence, when those records are encountered, return
+ * true, so that we stop just before summarizing either of those records.
+ *
+ * We also reach here if we just saw XLOG_END_OF_RECOVERY or
+ * XLOG_PARAMETER_CHANGE. These are not places where recovery can start,
+ * but they're still relevant here. A new timeline can begin with
+ * XLOG_END_OF_RECOVERY, so we need to confirm the WAL level at that point;
+ * and a restart can provoke XLOG_PARAMETER_CHANGE after an intervening
+ * change to postgresql.conf, which might force us to stop summarizing.
+ */
+ *new_fast_forward = (record_wal_level == WAL_LEVEL_MINIMAL);
+ return true;
}
/*
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 5161b72f28..c6a91fb456 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -31,7 +31,7 @@
/*
* Each page of XLOG file has a header like this:
*/
-#define XLOG_PAGE_MAGIC 0xD115 /* can be used as WAL version indicator */
+#define XLOG_PAGE_MAGIC 0xD116 /* can be used as WAL version indicator */
typedef struct XLogPageHeaderData
{
@@ -302,6 +302,7 @@ typedef struct xl_end_of_recovery
TimestampTz end_time;
TimeLineID ThisTimeLineID; /* new TLI */
TimeLineID PrevTimeLineID; /* previous TLI we forked off from */
+ int wal_level;
} xl_end_of_recovery;
/*
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index a00606ffcd..e80ff8e414 100644
--- a/src/include/catalog/pg_control.h
+++ b/src/include/catalog/pg_control.h
@@ -22,7 +22,7 @@
/* Version identifier for this pg_control format */
-#define PG_CONTROL_VERSION 1300
+#define PG_CONTROL_VERSION 1700
/* Nonce key length, see below */
#define MOCK_AUTH_NONCE_LEN 32
@@ -40,6 +40,7 @@ typedef struct CheckPoint
TimeLineID PrevTimeLineID; /* previous TLI, if this record begins a new
* timeline (equals ThisTimeLineID otherwise) */
bool fullPageWrites; /* current full_page_writes */
+ int wal_level; /* current wal_level */
FullTransactionId nextXid; /* next free transaction ID */
Oid nextOid; /* next free OID */
MultiXactId nextMulti; /* next free MultiXactId */
--
2.39.3 (Apple Git-145)
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-11 10:51 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 16:16 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 44+ messages in thread
From: Fujii Masao @ 2024-07-11 10:51 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-11 16:16 ` Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 44+ messages in thread
From: Robert Haas @ 2024-07-11 16:16 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-11 18:00 ` Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
1 sibling, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-11 18:00 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
Attachments:
v4-0001-Do-not-summarize-WAL-if-generated-with-wal_level-.patch (application/octet-stream, 19.2K) download
From fb614fd53666fd0a7e4fd96a4003e36e5d5a3f0c Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 11 Jul 2024 13:47:28 -0400
Subject: [PATCH v4] Do not summarize WAL if generated with wal_level=minimal.
To do this, we must include the wal_level in the first WAL record
covered by each summary file; so add wal_level to struct Checkpoint
and the payload of XLOG_CHECKPOINT_REDO and XLOG_END_OF_RECOVERY.
This, in turn, requires bumping XLOG_PAGE_MAGIC and, since the
Checkpoint is also stored in the control file, also
PG_CONTROL_VERSION. It's not great to do that so late in the release
cycle, but the alternative seems to ship v17 without robust
protections against this scenario, which could result in corrupted
incremental backups.
A side effect of this patch is that, when a server with
wal_level=replica is started with summarize_wal=on for the first time,
summarization will no longer begin with the oldest WAL that still
exists in pg_wal, but rather from the first checkpoint after that.
This change should be harmless, because a WAL summary for a partial
checkpoint cycle can never make an incremental backup possible when
it would otherwise not have been.
Report by Fujii Masao. Patch by me. Review and/or testing by Jakub
Wartak and Fujii Masao.
Discussion: http://postgr.es/m/6e30082e-041b-4e31-9633-95a66de76f5d@oss.nttdata.com
---
src/backend/access/rmgrdesc/xlogdesc.c | 47 +++--
src/backend/access/transam/xlog.c | 8 +-
src/backend/postmaster/walsummarizer.c | 196 +++++++++++++-----
src/bin/pg_combinebackup/meson.build | 1 +
.../t/007_wal_level_minimal.pl | 73 +++++++
src/include/access/xlog_internal.h | 3 +-
src/include/catalog/pg_control.h | 3 +-
7 files changed, 255 insertions(+), 76 deletions(-)
create mode 100644 src/bin/pg_combinebackup/t/007_wal_level_minimal.pl
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index e455400716..363294d623 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -33,6 +33,27 @@ const struct config_enum_entry wal_level_options[] = {
{NULL, 0, false}
};
+/*
+ * Find a string representation for wal_level
+ */
+static const char *
+get_wal_level_string(int wal_level)
+{
+ const struct config_enum_entry *entry;
+ const char *wal_level_str = "?";
+
+ for (entry = wal_level_options; entry->name; entry++)
+ {
+ if (entry->val == wal_level)
+ {
+ wal_level_str = entry->name;
+ break;
+ }
+ }
+
+ return wal_level_str;
+}
+
void
xlog_desc(StringInfo buf, XLogReaderState *record)
{
@@ -45,7 +66,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
- "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
+ "tli %u; prev tli %u; fpw %s; wal_level %s; xid %u:%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
@@ -53,6 +74,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
checkpoint->ThisTimeLineID,
checkpoint->PrevTimeLineID,
checkpoint->fullPageWrites ? "true" : "false",
+ get_wal_level_string(checkpoint->wal_level),
EpochFromFullTransactionId(checkpoint->nextXid),
XidFromFullTransactionId(checkpoint->nextXid),
checkpoint->nextOid,
@@ -95,20 +117,9 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
{
xl_parameter_change xlrec;
const char *wal_level_str;
- const struct config_enum_entry *entry;
memcpy(&xlrec, rec, sizeof(xl_parameter_change));
-
- /* Find a string representation for wal_level */
- wal_level_str = "?";
- for (entry = wal_level_options; entry->name; entry++)
- {
- if (entry->val == xlrec.wal_level)
- {
- wal_level_str = entry->name;
- break;
- }
- }
+ wal_level_str = get_wal_level_string(xlrec.wal_level);
appendStringInfo(buf, "max_connections=%d max_worker_processes=%d "
"max_wal_senders=%d max_prepared_xacts=%d "
@@ -135,9 +146,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
xl_end_of_recovery xlrec;
memcpy(&xlrec, rec, sizeof(xl_end_of_recovery));
- appendStringInfo(buf, "tli %u; prev tli %u; time %s",
+ appendStringInfo(buf, "tli %u; prev tli %u; time %s; wal_level %s",
xlrec.ThisTimeLineID, xlrec.PrevTimeLineID,
- timestamptz_to_str(xlrec.end_time));
+ timestamptz_to_str(xlrec.end_time),
+ get_wal_level_string(xlrec.wal_level));
}
else if (info == XLOG_OVERWRITE_CONTRECORD)
{
@@ -150,7 +162,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
}
else if (info == XLOG_CHECKPOINT_REDO)
{
- /* No details to write out */
+ int wal_level;
+
+ memcpy(&wal_level, rec, sizeof(int));
+ appendStringInfo(buf, "wal_level %s", get_wal_level_string(wal_level));
}
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 33e27a6e72..636be5ca4d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6934,6 +6934,7 @@ CreateCheckPoint(int flags)
WALInsertLockAcquireExclusive();
checkPoint.fullPageWrites = Insert->fullPageWrites;
+ checkPoint.wal_level = wal_level;
if (shutdown)
{
@@ -6987,11 +6988,9 @@ CreateCheckPoint(int flags)
*/
if (!shutdown)
{
- int dummy = 0;
-
- /* Record must have payload to avoid assertion failure. */
+ /* Include WAL level in record for WAL summarizer's benefit. */
XLogBeginInsert();
- XLogRegisterData((char *) &dummy, sizeof(dummy));
+ XLogRegisterData((char *) &wal_level, sizeof(wal_level));
(void) XLogInsert(RM_XLOG_ID, XLOG_CHECKPOINT_REDO);
/*
@@ -7314,6 +7313,7 @@ CreateEndOfRecoveryRecord(void)
elog(ERROR, "can only be used to end recovery");
xlrec.end_time = GetCurrentTimestamp();
+ xlrec.wal_level = wal_level;
WALInsertLockAcquireExclusive();
xlrec.ThisTimeLineID = XLogCtl->InsertTimeLineID;
diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index 55dc2ea8f5..04629c32d8 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -154,7 +154,8 @@ static void SummarizeSmgrRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
static void SummarizeXactRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
-static bool SummarizeXlogRecord(XLogReaderState *xlogreader);
+static bool SummarizeXlogRecord(XLogReaderState *xlogreader,
+ bool *new_fast_forward);
static int summarizer_read_local_xlog_page(XLogReaderState *state,
XLogRecPtr targetPagePtr,
int reqLen,
@@ -802,6 +803,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
char final_path[MAXPGPATH];
WalSummaryIO io;
BlockRefTable *brtab = CreateEmptyBlockRefTable();
+ bool fast_forward = true;
/* Initialize private data for xlogreader. */
private_data = (SummarizerReadLocalXLogPrivate *)
@@ -900,7 +902,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
int block_id;
char *errormsg;
XLogRecord *record;
- bool stop_requested = false;
+ uint8 rmid;
HandleWalSummarizerInterrupts();
@@ -969,56 +971,86 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
break;
}
- /* Special handling for particular types of WAL records. */
- switch (XLogRecGetRmid(xlogreader))
- {
- case RM_DBASE_ID:
- SummarizeDbaseRecord(xlogreader, brtab);
- break;
- case RM_SMGR_ID:
- SummarizeSmgrRecord(xlogreader, brtab);
- break;
- case RM_XACT_ID:
- SummarizeXactRecord(xlogreader, brtab);
- break;
- case RM_XLOG_ID:
- stop_requested = SummarizeXlogRecord(xlogreader);
- break;
- default:
- break;
- }
-
/*
- * If we've been told that it's time to end this WAL summary file, do
- * so. As an exception, if there's nothing included in this WAL
- * summary file yet, then stopping doesn't make any sense, and we
- * should wait until the next stop point instead.
+ * Certain types of records require special handling. Redo points and
+ * shutdown checkpoints trigger creation of new summary files and can
+ * also cause us to enter or exit "fast forward" mode. Other types of
+ * records can require special updates to the block reference table.
*/
- if (stop_requested && xlogreader->ReadRecPtr > summary_start_lsn)
+ rmid = XLogRecGetRmid(xlogreader);
+ if (rmid == RM_XLOG_ID)
{
- summary_end_lsn = xlogreader->ReadRecPtr;
- break;
+ bool new_fast_forward;
+
+ /*
+ * If we've already processed some WAL records when we hit a redo
+ * point or shutdown checkpoint, then we stop summarization before
+ * including this record in the current file, so that it will be
+ * the first record in the next file.
+ *
+ * When we hit one of those record types as the first record in a
+ * file, we adjust our notion of whether we're fast-forwarding.
+ * Any WAL generated with wal_level=minimal must be skipped
+ * without actually generating any summary file, because an
+ * incremental backup that crosses such WAL would be unsafe.
+ */
+ if (SummarizeXlogRecord(xlogreader, &new_fast_forward))
+ {
+ if (xlogreader->ReadRecPtr > summary_start_lsn)
+ {
+ summary_end_lsn = xlogreader->ReadRecPtr;
+ break;
+ }
+ else
+ fast_forward = new_fast_forward;
+ }
+ }
+ else if (!fast_forward)
+ {
+ /*
+ * This switch handles record types that require extra updates to
+ * the contents of the block reference table.
+ */
+ switch (rmid)
+ {
+ case RM_DBASE_ID:
+ SummarizeDbaseRecord(xlogreader, brtab);
+ break;
+ case RM_SMGR_ID:
+ SummarizeSmgrRecord(xlogreader, brtab);
+ break;
+ case RM_XACT_ID:
+ SummarizeXactRecord(xlogreader, brtab);
+ break;
+ }
}
- /* Feed block references from xlog record to block reference table. */
- for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
- block_id++)
+ /*
+ * If we're in fast-forward mode, we don't really need to do anything.
+ * Otherwise, feed block references from xlog record to block
+ * reference table.
+ */
+ if (!fast_forward)
{
- RelFileLocator rlocator;
- ForkNumber forknum;
- BlockNumber blocknum;
+ for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
+ block_id++)
+ {
+ RelFileLocator rlocator;
+ ForkNumber forknum;
+ BlockNumber blocknum;
- if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
- &forknum, &blocknum, NULL))
- continue;
+ if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
+ &forknum, &blocknum, NULL))
+ continue;
- /*
- * As we do elsewhere, ignore the FSM fork, because it's not fully
- * WAL-logged.
- */
- if (forknum != FSM_FORKNUM)
- BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
- blocknum);
+ /*
+ * As we do elsewhere, ignore the FSM fork, because it's not
+ * fully WAL-logged.
+ */
+ if (forknum != FSM_FORKNUM)
+ BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
+ blocknum);
+ }
}
/* Update our notion of where this summary file ends. */
@@ -1047,9 +1079,10 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
/*
* If a timeline switch occurs, we may fail to make any progress at all
* before exiting the loop above. If that happens, we don't write a WAL
- * summary file at all.
+ * summary file at all. We can also skip writing a file if we're in
+ * fast-forward mode.
*/
- if (summary_end_lsn > summary_start_lsn)
+ if (summary_end_lsn > summary_start_lsn && !fast_forward)
{
/* Generate temporary and final path name. */
snprintf(temp_path, MAXPGPATH,
@@ -1085,6 +1118,14 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
durable_rename(temp_path, final_path, ERROR);
}
+ /* If we skipped a non-zero amount of WAL, log a debug message. */
+ if (summary_end_lsn > summary_start_lsn && fast_forward)
+ ereport(DEBUG1,
+ errmsg("skipped summarizing WAL on TLI %u from %X/%X to %X/%X",
+ tli,
+ LSN_FORMAT_ARGS(summary_start_lsn),
+ LSN_FORMAT_ARGS(summary_end_lsn)));
+
return summary_end_lsn;
}
@@ -1263,22 +1304,69 @@ SummarizeXactRecord(XLogReaderState *xlogreader, BlockRefTable *brtab)
/*
* Special handling for WAL records with RM_XLOG_ID.
+ *
+ * The return value is true if WAL summarization should stop before this
+ * record and false otherwise. When the return value is true,
+ * *new_fast_forward indicates whether future processing should be done
+ * in fast forward mode (i.e. read WAL without emitting summaries) or not.
*/
static bool
-SummarizeXlogRecord(XLogReaderState *xlogreader)
+SummarizeXlogRecord(XLogReaderState *xlogreader, bool *new_fast_forward)
{
uint8 info = XLogRecGetInfo(xlogreader) & ~XLR_INFO_MASK;
+ int record_wal_level;
- if (info == XLOG_CHECKPOINT_REDO || info == XLOG_CHECKPOINT_SHUTDOWN)
+ if (info == XLOG_CHECKPOINT_REDO)
{
- /*
- * This is an LSN at which redo might begin, so we'd like
- * summarization to stop just before this WAL record.
- */
- return true;
+ /* Payload is wal_level at the time record was written. */
+ memcpy(&record_wal_level, XLogRecGetData(xlogreader), sizeof(int));
}
+ else if (info == XLOG_CHECKPOINT_SHUTDOWN)
+ {
+ CheckPoint rec_ckpt;
- return false;
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&rec_ckpt, XLogRecGetData(xlogreader), sizeof(CheckPoint));
+ record_wal_level = rec_ckpt.wal_level;
+ }
+ else if (info == XLOG_PARAMETER_CHANGE)
+ {
+ xl_parameter_change xlrec;
+
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&xlrec, XLogRecGetData(xlogreader),
+ sizeof(xl_parameter_change));
+ record_wal_level = xlrec.wal_level;
+ }
+ else if (info == XLOG_END_OF_RECOVERY)
+ {
+ xl_end_of_recovery xlrec;
+
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&xlrec, XLogRecGetData(xlogreader), sizeof(xl_end_of_recovery));
+ record_wal_level = xlrec.wal_level;
+ }
+ else
+ {
+ /* No special handling required. Return false. */
+ return false;
+ }
+
+ /*
+ * Redo can only begin at an XLOG_CHECKPOINT_REDO or
+ * XLOG_CHECKPOINT_SHUTDOWN record, so we want WAL summarization to begin
+ * at those points. Hence, when those records are encountered, return
+ * true, so that we stop just before summarizing either of those records.
+ *
+ * We also reach here if we just saw XLOG_END_OF_RECOVERY or
+ * XLOG_PARAMETER_CHANGE. These are not places where recovery can start,
+ * but they're still relevant here. A new timeline can begin with
+ * XLOG_END_OF_RECOVERY, so we need to confirm the WAL level at that point;
+ * and a restart can provoke XLOG_PARAMETER_CHANGE after an intervening
+ * change to postgresql.conf, which might force us to stop summarizing.
+ */
+ *new_fast_forward = (record_wal_level == WAL_LEVEL_MINIMAL);
+ return true;
}
/*
diff --git a/src/bin/pg_combinebackup/meson.build b/src/bin/pg_combinebackup/meson.build
index 1d4b9c218f..d871b2e3b8 100644
--- a/src/bin/pg_combinebackup/meson.build
+++ b/src/bin/pg_combinebackup/meson.build
@@ -34,6 +34,7 @@ tests += {
't/004_manifest.pl',
't/005_integrity.pl',
't/006_db_file_copy.pl',
+ 't/007_wal_level_minimal.pl',
],
}
}
diff --git a/src/bin/pg_combinebackup/t/007_wal_level_minimal.pl b/src/bin/pg_combinebackup/t/007_wal_level_minimal.pl
new file mode 100644
index 0000000000..86c750108f
--- /dev/null
+++ b/src/bin/pg_combinebackup/t/007_wal_level_minimal.pl
@@ -0,0 +1,73 @@
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+#
+# This test aims to validate that takeing an incremental backup fails when
+# wal_level has been changed to minimal between the full backup and the
+# attempted incremental backup.
+
+use strict;
+use warnings FATAL => 'all';
+use File::Compare;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+note "testing using mode $mode";
+
+# Set up a new database instance.
+my $node1 = PostgreSQL::Test::Cluster->new('node1');
+$node1->init(allows_streaming => 1);
+$node1->append_conf('postgresql.conf', <<EOM);
+summarize_wal = on
+wal_keep_size = '1GB'
+EOM
+$node1->start;
+
+# Create a table and insert a test row into it.
+$node1->safe_psql('postgres', <<EOM);
+CREATE TABLE mytable (a int, b text);
+INSERT INTO mytable VALUES (1, 'finch');
+EOM
+
+# Take a full backup.
+my $backup1path = $node1->backup_dir . '/backup1';
+$node1->command_ok(
+ [ 'pg_basebackup', '-D', $backup1path, '--no-sync', '-cfast' ],
+ "full backup");
+
+# Switch to wal_level=minimal, which also requires max_wal_senders=0 and
+# summarize_wal=off
+$node1->safe_psql('postgres', <<EOM);
+ALTER SYSTEM SET wal_level = minimal;
+ALTER SYSTEM SET max_wal_senders = 0;
+ALTER SYSTEM SET summarize_wal = off;
+EOM
+$node1->restart;
+
+# Insert a second row on the original node.
+$node1->safe_psql('postgres', <<EOM);
+INSERT INTO mytable VALUES (2, 'gerbil');
+EOM
+
+# Revert configuration changes
+$node1->safe_psql('postgres', <<EOM);
+ALTER SYSTEM RESET wal_level;
+ALTER SYSTEM RESET max_wal_senders;
+ALTER SYSTEM RESET summarize_wal;
+EOM
+$node1->restart;
+
+# Now take an incremental backup.
+my $backup2path = $node1->backup_dir . '/backup2';
+$node1->command_fails_like(
+ [
+ 'pg_basebackup', '-D', $backup2path, '--no-sync', '-cfast',
+ '--incremental', $backup1path . '/backup_manifest'
+ ],
+ qr/WAL summaries are required on timeline 1 from.*are incomplete/,
+ "incremental backup fails");
+
+# OK, that's all.
+done_testing();
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 5161b72f28..c6a91fb456 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -31,7 +31,7 @@
/*
* Each page of XLOG file has a header like this:
*/
-#define XLOG_PAGE_MAGIC 0xD115 /* can be used as WAL version indicator */
+#define XLOG_PAGE_MAGIC 0xD116 /* can be used as WAL version indicator */
typedef struct XLogPageHeaderData
{
@@ -302,6 +302,7 @@ typedef struct xl_end_of_recovery
TimestampTz end_time;
TimeLineID ThisTimeLineID; /* new TLI */
TimeLineID PrevTimeLineID; /* previous TLI we forked off from */
+ int wal_level;
} xl_end_of_recovery;
/*
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index a00606ffcd..e80ff8e414 100644
--- a/src/include/catalog/pg_control.h
+++ b/src/include/catalog/pg_control.h
@@ -22,7 +22,7 @@
/* Version identifier for this pg_control format */
-#define PG_CONTROL_VERSION 1300
+#define PG_CONTROL_VERSION 1700
/* Nonce key length, see below */
#define MOCK_AUTH_NONCE_LEN 32
@@ -40,6 +40,7 @@ typedef struct CheckPoint
TimeLineID PrevTimeLineID; /* previous TLI, if this record begins a new
* timeline (equals ThisTimeLineID otherwise) */
bool fullPageWrites; /* current full_page_writes */
+ int wal_level; /* current wal_level */
FullTransactionId nextXid; /* next free transaction ID */
Oid nextOid; /* next free OID */
MultiXactId nextMulti; /* next free MultiXactId */
--
2.39.3 (Apple Git-145)
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-15 02:56 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Fujii Masao @ 2024-07-15 02:56 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-15 18:30 ` Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-15 18:30 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-15 18:47 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 19:14 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 44+ messages in thread
From: Nathan Bossart @ 2024-07-15 18:47 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-15 19:14 ` Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 0 replies; 44+ messages in thread
From: Nathan Bossart @ 2024-07-15 19:14 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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.patc...
--
nathan
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-15 20:03 ` Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-15 20:03 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-15 20:10 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 16:23 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Nathan Bossart @ 2024-07-15 20:10 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-16 16:23 ` Robert Haas <robertmhaas@gmail.com>
2024-07-16 16:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-16 16:23 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 16:23 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-16 16:30 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 17:16 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 44+ messages in thread
From: Nathan Bossart @ 2024-07-16 16:30 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Fujii Masao <masao.fujii@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 16:23 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-16 16:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
@ 2024-07-16 17:16 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-17 13:44 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Fujii Masao @ 2024-07-16 17:16 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 16:23 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-16 16:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 17:16 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-17 13:44 ` Robert Haas <robertmhaas@gmail.com>
2024-07-18 13:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-17 13:44 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
Attachments:
v5-0001-Do-not-summarize-WAL-if-generated-with-wal_level-.patch (application/octet-stream, 21.8K) download
From 7a34a545529db17ff4ff4d6e76745c1a8b4e642f Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 17 Jul 2024 09:23:09 -0400
Subject: [PATCH v5] Do not summarize WAL if generated with wal_level=minimal.
To do this, we must include the wal_level in the first WAL record
covered by each summary file; so add wal_level to struct Checkpoint
and the payload of XLOG_CHECKPOINT_REDO and XLOG_END_OF_RECOVERY.
This, in turn, requires bumping XLOG_PAGE_MAGIC and, since the
Checkpoint is also stored in the control file, also
PG_CONTROL_VERSION. It's not great to do that so late in the release
cycle, but the alternative seems to ship v17 without robust
protections against this scenario, which could result in corrupted
incremental backups.
A side effect of this patch is that, when a server with
wal_level=replica is started with summarize_wal=on for the first time,
summarization will no longer begin with the oldest WAL that still
exists in pg_wal, but rather from the first checkpoint after that.
This change should be harmless, because a WAL summary for a partial
checkpoint cycle can never make an incremental backup possible when
it would otherwise not have been.
Report by Fujii Masao. Patch by me. Review and/or testing by Jakub
Wartak and Fujii Masao.
Discussion: http://postgr.es/m/6e30082e-041b-4e31-9633-95a66de76f5d@oss.nttdata.com
---
doc/src/sgml/config.sgml | 16 +-
doc/src/sgml/func.sgml | 10 +
src/backend/access/rmgrdesc/xlogdesc.c | 47 +++--
src/backend/access/transam/xlog.c | 8 +-
src/backend/postmaster/walsummarizer.c | 196 +++++++++++++-----
src/bin/pg_combinebackup/meson.build | 1 +
.../t/007_wal_level_minimal.pl | 73 +++++++
src/include/access/xlog_internal.h | 3 +-
src/include/catalog/pg_control.h | 3 +-
9 files changed, 276 insertions(+), 81 deletions(-)
create mode 100644 src/bin/pg_combinebackup/t/007_wal_level_minimal.pl
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b14c5d81a1..3dec0b7cfe 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4318,11 +4318,17 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
<listitem>
<para>
Enables the WAL summarizer process. Note that WAL summarization can
- be enabled either on a primary or on a standby. WAL summarization
- cannot be enabled when <varname>wal_level</varname> is set to
- <literal>minimal</literal>. This parameter can only be set in the
- <filename>postgresql.conf</filename> file or on the server command line.
- The default is <literal>off</literal>.
+ be enabled either on a primary or on a standby. This parameter can only
+ be set in the <filename>postgresql.conf</filename> file or on the server
+ command line. The default is <literal>off</literal>.
+ </para>
+ <para>
+ The server cannot be started with <literal>summarize_wal=on</literal>
+ if <literal>wal_level</literal> is set to <literal>minimal</literal>. If
+ <literal>summarize_wal=on</literal> is configured after server startup
+ while <literal>wal_level=minimal</literal>, the summarizer will run
+ but refuse to generate summary files for any WAL generated with
+ <literal>wal_level=minimal</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3f93c61aa3..0f14317979 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28001,6 +28001,16 @@ acl | {postgres=arwdDxtm/postgres,foo=r/postgres}
not running, it will be equal to <literal>summarized_lsn</literal>.
<literal>summarizer_pid</literal> is the PID of the WAL summarizer
process, if it is running, and otherwise NULL.
+ </para>
+ <para>
+ As a special exception, the WAL summarizer will refuse to generate
+ WAL summary files if run on WAL generated under
+ <literal>wal_level=minimal</literal>, since such summaries would be
+ unsafe to use as the basis for an incremental backup. In this case,
+ the fields above will continue to advance as if summaries were being
+ generated, but nothing will be written to disk. Once the summarizer
+ reaches WAL not generated under <literal>wal_level=minimal</literal>,
+ it will resume writing summaries to disk.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index e455400716..363294d623 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -33,6 +33,27 @@ const struct config_enum_entry wal_level_options[] = {
{NULL, 0, false}
};
+/*
+ * Find a string representation for wal_level
+ */
+static const char *
+get_wal_level_string(int wal_level)
+{
+ const struct config_enum_entry *entry;
+ const char *wal_level_str = "?";
+
+ for (entry = wal_level_options; entry->name; entry++)
+ {
+ if (entry->val == wal_level)
+ {
+ wal_level_str = entry->name;
+ break;
+ }
+ }
+
+ return wal_level_str;
+}
+
void
xlog_desc(StringInfo buf, XLogReaderState *record)
{
@@ -45,7 +66,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
- "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
+ "tli %u; prev tli %u; fpw %s; wal_level %s; xid %u:%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
@@ -53,6 +74,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
checkpoint->ThisTimeLineID,
checkpoint->PrevTimeLineID,
checkpoint->fullPageWrites ? "true" : "false",
+ get_wal_level_string(checkpoint->wal_level),
EpochFromFullTransactionId(checkpoint->nextXid),
XidFromFullTransactionId(checkpoint->nextXid),
checkpoint->nextOid,
@@ -95,20 +117,9 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
{
xl_parameter_change xlrec;
const char *wal_level_str;
- const struct config_enum_entry *entry;
memcpy(&xlrec, rec, sizeof(xl_parameter_change));
-
- /* Find a string representation for wal_level */
- wal_level_str = "?";
- for (entry = wal_level_options; entry->name; entry++)
- {
- if (entry->val == xlrec.wal_level)
- {
- wal_level_str = entry->name;
- break;
- }
- }
+ wal_level_str = get_wal_level_string(xlrec.wal_level);
appendStringInfo(buf, "max_connections=%d max_worker_processes=%d "
"max_wal_senders=%d max_prepared_xacts=%d "
@@ -135,9 +146,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
xl_end_of_recovery xlrec;
memcpy(&xlrec, rec, sizeof(xl_end_of_recovery));
- appendStringInfo(buf, "tli %u; prev tli %u; time %s",
+ appendStringInfo(buf, "tli %u; prev tli %u; time %s; wal_level %s",
xlrec.ThisTimeLineID, xlrec.PrevTimeLineID,
- timestamptz_to_str(xlrec.end_time));
+ timestamptz_to_str(xlrec.end_time),
+ get_wal_level_string(xlrec.wal_level));
}
else if (info == XLOG_OVERWRITE_CONTRECORD)
{
@@ -150,7 +162,10 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
}
else if (info == XLOG_CHECKPOINT_REDO)
{
- /* No details to write out */
+ int wal_level;
+
+ memcpy(&wal_level, rec, sizeof(int));
+ appendStringInfo(buf, "wal_level %s", get_wal_level_string(wal_level));
}
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 33e27a6e72..636be5ca4d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6934,6 +6934,7 @@ CreateCheckPoint(int flags)
WALInsertLockAcquireExclusive();
checkPoint.fullPageWrites = Insert->fullPageWrites;
+ checkPoint.wal_level = wal_level;
if (shutdown)
{
@@ -6987,11 +6988,9 @@ CreateCheckPoint(int flags)
*/
if (!shutdown)
{
- int dummy = 0;
-
- /* Record must have payload to avoid assertion failure. */
+ /* Include WAL level in record for WAL summarizer's benefit. */
XLogBeginInsert();
- XLogRegisterData((char *) &dummy, sizeof(dummy));
+ XLogRegisterData((char *) &wal_level, sizeof(wal_level));
(void) XLogInsert(RM_XLOG_ID, XLOG_CHECKPOINT_REDO);
/*
@@ -7314,6 +7313,7 @@ CreateEndOfRecoveryRecord(void)
elog(ERROR, "can only be used to end recovery");
xlrec.end_time = GetCurrentTimestamp();
+ xlrec.wal_level = wal_level;
WALInsertLockAcquireExclusive();
xlrec.ThisTimeLineID = XLogCtl->InsertTimeLineID;
diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index 55dc2ea8f5..04629c32d8 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -154,7 +154,8 @@ static void SummarizeSmgrRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
static void SummarizeXactRecord(XLogReaderState *xlogreader,
BlockRefTable *brtab);
-static bool SummarizeXlogRecord(XLogReaderState *xlogreader);
+static bool SummarizeXlogRecord(XLogReaderState *xlogreader,
+ bool *new_fast_forward);
static int summarizer_read_local_xlog_page(XLogReaderState *state,
XLogRecPtr targetPagePtr,
int reqLen,
@@ -802,6 +803,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
char final_path[MAXPGPATH];
WalSummaryIO io;
BlockRefTable *brtab = CreateEmptyBlockRefTable();
+ bool fast_forward = true;
/* Initialize private data for xlogreader. */
private_data = (SummarizerReadLocalXLogPrivate *)
@@ -900,7 +902,7 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
int block_id;
char *errormsg;
XLogRecord *record;
- bool stop_requested = false;
+ uint8 rmid;
HandleWalSummarizerInterrupts();
@@ -969,56 +971,86 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
break;
}
- /* Special handling for particular types of WAL records. */
- switch (XLogRecGetRmid(xlogreader))
- {
- case RM_DBASE_ID:
- SummarizeDbaseRecord(xlogreader, brtab);
- break;
- case RM_SMGR_ID:
- SummarizeSmgrRecord(xlogreader, brtab);
- break;
- case RM_XACT_ID:
- SummarizeXactRecord(xlogreader, brtab);
- break;
- case RM_XLOG_ID:
- stop_requested = SummarizeXlogRecord(xlogreader);
- break;
- default:
- break;
- }
-
/*
- * If we've been told that it's time to end this WAL summary file, do
- * so. As an exception, if there's nothing included in this WAL
- * summary file yet, then stopping doesn't make any sense, and we
- * should wait until the next stop point instead.
+ * Certain types of records require special handling. Redo points and
+ * shutdown checkpoints trigger creation of new summary files and can
+ * also cause us to enter or exit "fast forward" mode. Other types of
+ * records can require special updates to the block reference table.
*/
- if (stop_requested && xlogreader->ReadRecPtr > summary_start_lsn)
+ rmid = XLogRecGetRmid(xlogreader);
+ if (rmid == RM_XLOG_ID)
{
- summary_end_lsn = xlogreader->ReadRecPtr;
- break;
+ bool new_fast_forward;
+
+ /*
+ * If we've already processed some WAL records when we hit a redo
+ * point or shutdown checkpoint, then we stop summarization before
+ * including this record in the current file, so that it will be
+ * the first record in the next file.
+ *
+ * When we hit one of those record types as the first record in a
+ * file, we adjust our notion of whether we're fast-forwarding.
+ * Any WAL generated with wal_level=minimal must be skipped
+ * without actually generating any summary file, because an
+ * incremental backup that crosses such WAL would be unsafe.
+ */
+ if (SummarizeXlogRecord(xlogreader, &new_fast_forward))
+ {
+ if (xlogreader->ReadRecPtr > summary_start_lsn)
+ {
+ summary_end_lsn = xlogreader->ReadRecPtr;
+ break;
+ }
+ else
+ fast_forward = new_fast_forward;
+ }
+ }
+ else if (!fast_forward)
+ {
+ /*
+ * This switch handles record types that require extra updates to
+ * the contents of the block reference table.
+ */
+ switch (rmid)
+ {
+ case RM_DBASE_ID:
+ SummarizeDbaseRecord(xlogreader, brtab);
+ break;
+ case RM_SMGR_ID:
+ SummarizeSmgrRecord(xlogreader, brtab);
+ break;
+ case RM_XACT_ID:
+ SummarizeXactRecord(xlogreader, brtab);
+ break;
+ }
}
- /* Feed block references from xlog record to block reference table. */
- for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
- block_id++)
+ /*
+ * If we're in fast-forward mode, we don't really need to do anything.
+ * Otherwise, feed block references from xlog record to block
+ * reference table.
+ */
+ if (!fast_forward)
{
- RelFileLocator rlocator;
- ForkNumber forknum;
- BlockNumber blocknum;
+ for (block_id = 0; block_id <= XLogRecMaxBlockId(xlogreader);
+ block_id++)
+ {
+ RelFileLocator rlocator;
+ ForkNumber forknum;
+ BlockNumber blocknum;
- if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
- &forknum, &blocknum, NULL))
- continue;
+ if (!XLogRecGetBlockTagExtended(xlogreader, block_id, &rlocator,
+ &forknum, &blocknum, NULL))
+ continue;
- /*
- * As we do elsewhere, ignore the FSM fork, because it's not fully
- * WAL-logged.
- */
- if (forknum != FSM_FORKNUM)
- BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
- blocknum);
+ /*
+ * As we do elsewhere, ignore the FSM fork, because it's not
+ * fully WAL-logged.
+ */
+ if (forknum != FSM_FORKNUM)
+ BlockRefTableMarkBlockModified(brtab, &rlocator, forknum,
+ blocknum);
+ }
}
/* Update our notion of where this summary file ends. */
@@ -1047,9 +1079,10 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
/*
* If a timeline switch occurs, we may fail to make any progress at all
* before exiting the loop above. If that happens, we don't write a WAL
- * summary file at all.
+ * summary file at all. We can also skip writing a file if we're in
+ * fast-forward mode.
*/
- if (summary_end_lsn > summary_start_lsn)
+ if (summary_end_lsn > summary_start_lsn && !fast_forward)
{
/* Generate temporary and final path name. */
snprintf(temp_path, MAXPGPATH,
@@ -1085,6 +1118,14 @@ SummarizeWAL(TimeLineID tli, XLogRecPtr start_lsn, bool exact,
durable_rename(temp_path, final_path, ERROR);
}
+ /* If we skipped a non-zero amount of WAL, log a debug message. */
+ if (summary_end_lsn > summary_start_lsn && fast_forward)
+ ereport(DEBUG1,
+ errmsg("skipped summarizing WAL on TLI %u from %X/%X to %X/%X",
+ tli,
+ LSN_FORMAT_ARGS(summary_start_lsn),
+ LSN_FORMAT_ARGS(summary_end_lsn)));
+
return summary_end_lsn;
}
@@ -1263,22 +1304,69 @@ SummarizeXactRecord(XLogReaderState *xlogreader, BlockRefTable *brtab)
/*
* Special handling for WAL records with RM_XLOG_ID.
+ *
+ * The return value is true if WAL summarization should stop before this
+ * record and false otherwise. When the return value is true,
+ * *new_fast_forward indicates whether future processing should be done
+ * in fast forward mode (i.e. read WAL without emitting summaries) or not.
*/
static bool
-SummarizeXlogRecord(XLogReaderState *xlogreader)
+SummarizeXlogRecord(XLogReaderState *xlogreader, bool *new_fast_forward)
{
uint8 info = XLogRecGetInfo(xlogreader) & ~XLR_INFO_MASK;
+ int record_wal_level;
- if (info == XLOG_CHECKPOINT_REDO || info == XLOG_CHECKPOINT_SHUTDOWN)
+ if (info == XLOG_CHECKPOINT_REDO)
{
- /*
- * This is an LSN at which redo might begin, so we'd like
- * summarization to stop just before this WAL record.
- */
- return true;
+ /* Payload is wal_level at the time record was written. */
+ memcpy(&record_wal_level, XLogRecGetData(xlogreader), sizeof(int));
}
+ else if (info == XLOG_CHECKPOINT_SHUTDOWN)
+ {
+ CheckPoint rec_ckpt;
- return false;
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&rec_ckpt, XLogRecGetData(xlogreader), sizeof(CheckPoint));
+ record_wal_level = rec_ckpt.wal_level;
+ }
+ else if (info == XLOG_PARAMETER_CHANGE)
+ {
+ xl_parameter_change xlrec;
+
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&xlrec, XLogRecGetData(xlogreader),
+ sizeof(xl_parameter_change));
+ record_wal_level = xlrec.wal_level;
+ }
+ else if (info == XLOG_END_OF_RECOVERY)
+ {
+ xl_end_of_recovery xlrec;
+
+ /* Extract wal_level at time record was written from payload. */
+ memcpy(&xlrec, XLogRecGetData(xlogreader), sizeof(xl_end_of_recovery));
+ record_wal_level = xlrec.wal_level;
+ }
+ else
+ {
+ /* No special handling required. Return false. */
+ return false;
+ }
+
+ /*
+ * Redo can only begin at an XLOG_CHECKPOINT_REDO or
+ * XLOG_CHECKPOINT_SHUTDOWN record, so we want WAL summarization to begin
+ * at those points. Hence, when those records are encountered, return
+ * true, so that we stop just before summarizing either of those records.
+ *
+ * We also reach here if we just saw XLOG_END_OF_RECOVERY or
+ * XLOG_PARAMETER_CHANGE. These are not places where recovery can start,
+ * but they're still relevant here. A new timeline can begin with
+ * XLOG_END_OF_RECOVERY, so we need to confirm the WAL level at that point;
+ * and a restart can provoke XLOG_PARAMETER_CHANGE after an intervening
+ * change to postgresql.conf, which might force us to stop summarizing.
+ */
+ *new_fast_forward = (record_wal_level == WAL_LEVEL_MINIMAL);
+ return true;
}
/*
diff --git a/src/bin/pg_combinebackup/meson.build b/src/bin/pg_combinebackup/meson.build
index 1d4b9c218f..d871b2e3b8 100644
--- a/src/bin/pg_combinebackup/meson.build
+++ b/src/bin/pg_combinebackup/meson.build
@@ -34,6 +34,7 @@ tests += {
't/004_manifest.pl',
't/005_integrity.pl',
't/006_db_file_copy.pl',
+ 't/007_wal_level_minimal.pl',
],
}
}
diff --git a/src/bin/pg_combinebackup/t/007_wal_level_minimal.pl b/src/bin/pg_combinebackup/t/007_wal_level_minimal.pl
new file mode 100644
index 0000000000..900df6b286
--- /dev/null
+++ b/src/bin/pg_combinebackup/t/007_wal_level_minimal.pl
@@ -0,0 +1,73 @@
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+#
+# This test aims to validate that taking an incremental backup fails when
+# wal_level has been changed to minimal between the full backup and the
+# attempted incremental backup.
+
+use strict;
+use warnings FATAL => 'all';
+use File::Compare;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+note "testing using mode $mode";
+
+# Set up a new database instance.
+my $node1 = PostgreSQL::Test::Cluster->new('node1');
+$node1->init(allows_streaming => 1);
+$node1->append_conf('postgresql.conf', <<EOM);
+summarize_wal = on
+wal_keep_size = '1GB'
+EOM
+$node1->start;
+
+# Create a table and insert a test row into it.
+$node1->safe_psql('postgres', <<EOM);
+CREATE TABLE mytable (a int, b text);
+INSERT INTO mytable VALUES (1, 'finch');
+EOM
+
+# Take a full backup.
+my $backup1path = $node1->backup_dir . '/backup1';
+$node1->command_ok(
+ [ 'pg_basebackup', '-D', $backup1path, '--no-sync', '-cfast' ],
+ "full backup");
+
+# Switch to wal_level=minimal, which also requires max_wal_senders=0 and
+# summarize_wal=off
+$node1->safe_psql('postgres', <<EOM);
+ALTER SYSTEM SET wal_level = minimal;
+ALTER SYSTEM SET max_wal_senders = 0;
+ALTER SYSTEM SET summarize_wal = off;
+EOM
+$node1->restart;
+
+# Insert a second row on the original node.
+$node1->safe_psql('postgres', <<EOM);
+INSERT INTO mytable VALUES (2, 'gerbil');
+EOM
+
+# Revert configuration changes
+$node1->safe_psql('postgres', <<EOM);
+ALTER SYSTEM RESET wal_level;
+ALTER SYSTEM RESET max_wal_senders;
+ALTER SYSTEM RESET summarize_wal;
+EOM
+$node1->restart;
+
+# Now take an incremental backup.
+my $backup2path = $node1->backup_dir . '/backup2';
+$node1->command_fails_like(
+ [
+ 'pg_basebackup', '-D', $backup2path, '--no-sync', '-cfast',
+ '--incremental', $backup1path . '/backup_manifest'
+ ],
+ qr/WAL summaries are required on timeline 1 from.*are incomplete/,
+ "incremental backup fails");
+
+# OK, that's all.
+done_testing();
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 5161b72f28..c6a91fb456 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -31,7 +31,7 @@
/*
* Each page of XLOG file has a header like this:
*/
-#define XLOG_PAGE_MAGIC 0xD115 /* can be used as WAL version indicator */
+#define XLOG_PAGE_MAGIC 0xD116 /* can be used as WAL version indicator */
typedef struct XLogPageHeaderData
{
@@ -302,6 +302,7 @@ typedef struct xl_end_of_recovery
TimestampTz end_time;
TimeLineID ThisTimeLineID; /* new TLI */
TimeLineID PrevTimeLineID; /* previous TLI we forked off from */
+ int wal_level;
} xl_end_of_recovery;
/*
diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
index a00606ffcd..e80ff8e414 100644
--- a/src/include/catalog/pg_control.h
+++ b/src/include/catalog/pg_control.h
@@ -22,7 +22,7 @@
/* Version identifier for this pg_control format */
-#define PG_CONTROL_VERSION 1300
+#define PG_CONTROL_VERSION 1700
/* Nonce key length, see below */
#define MOCK_AUTH_NONCE_LEN 32
@@ -40,6 +40,7 @@ typedef struct CheckPoint
TimeLineID PrevTimeLineID; /* previous TLI, if this record begins a new
* timeline (equals ThisTimeLineID otherwise) */
bool fullPageWrites; /* current full_page_writes */
+ int wal_level; /* current wal_level */
FullTransactionId nextXid; /* next free transaction ID */
Oid nextOid; /* next free OID */
MultiXactId nextMulti; /* next free MultiXactId */
--
2.39.3 (Apple Git-145)
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 16:23 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-16 16:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 17:16 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-17 13:44 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-18 13:47 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-18 16:27 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 44+ messages in thread
From: Fujii Masao @ 2024-07-18 13:47 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 16:23 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-16 16:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 17:16 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-17 13:44 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-18 13:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-18 16:27 ` Robert Haas <robertmhaas@gmail.com>
2024-07-18 17:52 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 1 reply; 44+ messages in thread
From: Robert Haas @ 2024-07-18 16:27 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-04 14:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-10 16:35 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 18:00 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 16:23 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-16 16:30 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 17:16 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-17 13:44 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
2024-07-18 13:47 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-18 16:27 ` Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Robert Haas <robertmhaas@gmail.com>
@ 2024-07-18 17:52 ` Fujii Masao <masao.fujii@oss.nttdata.com>
0 siblings, 0 replies; 44+ messages in thread
From: Fujii Masao @ 2024-07-18 17:52 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
* Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
@ 2024-07-03 19:13 ` Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 44+ messages in thread
From: Robert Haas @ 2024-07-03 19:13 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@oss.nttdata.com>; +Cc: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 44+ messages in thread
end of thread, other threads:[~2024-07-18 17:52 UTC | newest]
Thread overview: 44+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-07-03 14:08 Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 14:29 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-03 14:41 ` Daniel Gustafsson <daniel@yesql.se>
2024-07-03 14:45 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-03 14:46 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-03 15:01 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-03 15:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-04 14:35 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-08 19:37 ` Robert Haas <robertmhaas@gmail.com>
2024-07-09 17:57 ` Robert Haas <robertmhaas@gmail.com>
2024-07-10 05:56 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 14:10 ` Robert Haas <robertmhaas@gmail.com>
2024-07-10 14:18 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 14:29 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 14:46 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 15:08 ` Jelte Fennema-Nio <postgres@jeltef.nl>
2024-07-10 15:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 15:44 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 15:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 16:15 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 16:07 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 18:41 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-11 20:42 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-11 21:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-07-10 16:02 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-10 16:29 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-10 16:35 ` Robert Haas <robertmhaas@gmail.com>
2024-07-11 10:51 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-11 16:16 ` Robert Haas <robertmhaas@gmail.com>
2024-07-11 18:00 ` Robert Haas <robertmhaas@gmail.com>
2024-07-15 02:56 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-15 18:30 ` Robert Haas <robertmhaas@gmail.com>
2024-07-15 18:47 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 19:14 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-15 20:03 ` Robert Haas <robertmhaas@gmail.com>
2024-07-15 20:10 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 16:23 ` Robert Haas <robertmhaas@gmail.com>
2024-07-16 16:30 ` Nathan Bossart <nathandbossart@gmail.com>
2024-07-16 17:16 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-17 13:44 ` Robert Haas <robertmhaas@gmail.com>
2024-07-18 13:47 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-18 16:27 ` Robert Haas <robertmhaas@gmail.com>
2024-07-18 17:52 ` Fujii Masao <masao.fujii@oss.nttdata.com>
2024-07-03 19:13 ` Robert Haas <robertmhaas@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox