On 2024/07/19 1:27, Robert Haas wrote:
On Thu, Jul 18, 2024 at 9:47 AM Fujii Masao 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 ch
On Thu, Jul 18, 2024 at 9:47 AM Fujii Masao 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 te
On 2024/07/17 22:44, Robert Haas wrote:
On Tue, Jul 16, 2024 at 1:16 PM Fujii Masao 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 summari
On Tue, Jul 16, 2024 at 1:16 PM Fujii Masao 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,
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 as
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 futu
On Mon, Jul 15, 2024 at 4:10 PM Nathan Bossart 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 co
On Mon, Jul 15, 2024 at 04:03:13PM -0400, Robert Haas wrote:
> On Mon, Jul 15, 2024 at 2:47 PM Nathan Bossart
> 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,
On Mon, Jul 15, 2024 at 2:47 PM Nathan Bossart 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
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
On Mon, Jul 15, 2024 at 02:30:42PM -0400, Robert Haas wrote:
> On Sun, Jul 14, 2024 at 10:56 PM Fujii Masao
> 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
On Sun, Jul 14, 2024 at 10:56 PM Fujii Masao
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
On 2024/07/12 1:16, Robert Haas wrote:
On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao 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
Nathan Bossart 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
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 implemen
On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao 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
v4-0001-Do-not-summarize-WAL-if-generated-with-
On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao 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 shoul
On 2024/07/11 1:35, Robert Haas wrote:
On Wed, Jul 10, 2024 at 10:10 AM Robert Haas wrote:
On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao 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 shutdow
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 co
On Wed, Jul 10, 2024 at 10:10 AM Robert Haas wrote:
> On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao
> 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 af
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
On Wed, Jul 10, 2024 at 11:54:38AM -0400, Tom Lane wrote:
> Nathan Bossart 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.
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_h
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 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
i
Nathan Bossart 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-
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 tr
Jelte Fennema-Nio writes:
> On Wed, 10 Jul 2024 at 16: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
On Wed, 10 Jul 2024 at 16:46, Nathan Bossart 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_
On Wed, Jul 10, 2024 at 04:29:14PM +0200, Jelte Fennema-Nio wrote:
> On Wed, 10 Jul 2024 at 16: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
On Wed, 10 Jul 2024 at 16: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'
>
On Wed, Jul 10, 2024 at 10:10:30AM -0400, Robert Haas wrote:
> On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao
> 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 summar
On Wed, Jul 10, 2024 at 1:56 AM Fujii Masao 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-checkpo
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
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 beha
On Thu, Jul 4, 2024 at 10:35 AM Fujii Masao 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
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)
+ {
+
On Wed, Jul 3, 2024 at 10:09 AM Fujii Masao 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 f
Nathan Bossart 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
On Wed, 3 Jul 2024 at 16:46, Jelte Fennema-Nio wrote:
>
> Ugh copy paste mistake, this is what I meant
>
> On Wed, 3 Jul 2024 at 16:45, Jelte Fennema-Nio wrote:
> > This hits the already existing check:
> > summarize_wal = 'true'
> > wal_level = 'minimal'
> >
> > This hits the new check:
> > wal_
Ugh copy paste mistake, this is what I meant
On Wed, 3 Jul 2024 at 16:45, Jelte Fennema-Nio 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 thr
On Wed, 3 Jul 2024 at 16:30, Nathan Bossart 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 deser
> On 3 Jul 2024, at 16:29, Nathan Bossart wrote:
> This sort of thing comes up enough that perhaps we should add a
> better-supported way to deal with GUCs that depend on each other...
+1
--
Daniel Gustafsson
On Wed, 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 c
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
44 matches
Mail list logo
