public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Fix handling of ? option
23+ messages / 7 participants
[nested] [flat]
* [PATCH] Fix handling of ? option
@ 2019-08-25 18:45 Quentin Rameau <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Quentin Rameau @ 2019-08-25 18:45 UTC (permalink / raw)
Using optind to check back the original given option this way is bogus
and could lead to dereferencing argv out of bounds with a missing
argument to an option.
The proper way to verify if an error has occured is to check if optopt
has been set.
---
src/bin/psql/startup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 4730c73396..333b49985a 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -507,6 +507,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
int c;
memset(options, 0, sizeof *options);
+ optopt = 0;
while ((c = getopt_long(argc, argv, "aAbc:d:eEf:F:h:HlL:no:p:P:qR:sStT:U:v:VwWxXz?01",
long_options, &optindex)) != -1)
@@ -667,7 +668,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
break;
case '?':
/* Actual help option given */
- if (strcmp(argv[optind - 1], "-?") == 0)
+ if (!optopt)
{
usage(NOPAGER);
exit(EXIT_SUCCESS);
--
2.23.0
--MP_/oEcYY8373P4jignUOgMh8T.--
^ permalink raw reply [nested|flat] 23+ messages in thread
* [PATCH] Fix handling of ? option
@ 2019-08-25 18:45 Quentin Rameau <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Quentin Rameau @ 2019-08-25 18:45 UTC (permalink / raw)
Using optind to check back the original given option this way is bogus
and could lead to dereferencing argv out of bounds with a missing
argument to an option.
The proper way to verify if an error has occured is to check if optopt
has been set.
---
src/bin/psql/startup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 4730c73396..e5f6894177 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -667,7 +667,7 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
break;
case '?':
/* Actual help option given */
- if (strcmp(argv[optind - 1], "-?") == 0)
+ if (!optopt)
{
usage(NOPAGER);
exit(EXIT_SUCCESS);
--
2.23.0
--MP_/_eGHSN7B9ucS1dXH.MiKkUY--
^ permalink raw reply [nested|flat] 23+ messages in thread
* Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2021-12-23 15:05 Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Bharath Rupireddy @ 2021-12-23 15:05 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Hi,
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN and REDO LSN [1]. It gives more context while
analyzing checkpoint-related issues. The pg_controldata gives the last
checkpoint LSN and REDO LSN, but having this info alongside the log
message helps analyze issues that happened previously, connect the
dots and identify the root cause.
Attaching a small patch herewith. Thoughts?
[1]
2021-12-23 14:58:54.694 UTC [1965649] LOG: checkpoint starting:
shutdown immediate
2021-12-23 14:58:54.714 UTC [1965649] LOG: checkpoint complete: wrote
0 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=0.001 s, sync=0.001 s, total=0.025 s; sync files=0,
longest=0.000 s, average=0.000 s; distance=0 kB, estimate=0 kB;
LSN=0/14D0AD0, REDO LSN=0/14D0AD0
Regards,
Bharath Rupireddy.
Attachments:
[application/octet-stream] v1-0001-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-l.patch (2.8K, ../../CALj2ACWt6kqriAHrO+AJj+OmP=suwbktHT5JoYAn-nqZe2gd2g@mail.gmail.com/2-v1-0001-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-l.patch)
download | inline diff:
From a641d82e8eb4fa9582a0f83a6f8da5ff468d1710 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Thu, 23 Dec 2021 15:03:30 +0000
Subject: [PATCH v1] Add checkpoint and redo LSN to LogCheckpointEnd log
message
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN and REDO LSN. It gives more context while
analyzing checkpoint-related issues. The pg_controldata gives the
last checkpoint LSN and REDO LSN, but having this info alongside
the log message helps analyze issues that happened previously,
connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1e1fbe957f..9816f7d328 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8924,7 +8924,8 @@ LogCheckpointEnd(bool restartpoint)
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "LSN=%X/%X, REDO LSN=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8937,14 +8938,17 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
(errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "LSN=%X/%X, REDO LSN=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8957,7 +8961,9 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
}
/*
--
2.25.1
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2021-12-24 05:51 Kyotaro Horiguchi <[email protected]>
parent: Bharath Rupireddy <[email protected]>
0 siblings, 2 replies; 23+ messages in thread
From: Kyotaro Horiguchi @ 2021-12-24 05:51 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
At Thu, 23 Dec 2021 20:35:54 +0530, Bharath Rupireddy <[email protected]> wrote in
> Hi,
>
> It is useful (for debugging purposes) if the checkpoint end message
> has the checkpoint LSN and REDO LSN [1]. It gives more context while
> analyzing checkpoint-related issues. The pg_controldata gives the last
> checkpoint LSN and REDO LSN, but having this info alongside the log
> message helps analyze issues that happened previously, connect the
> dots and identify the root cause.
>
> Attaching a small patch herewith. Thoughts?
A big +1 from me. I thought about proposing the same in the past.
> [1]
> 2021-12-23 14:58:54.694 UTC [1965649] LOG: checkpoint starting:
> shutdown immediate
> 2021-12-23 14:58:54.714 UTC [1965649] LOG: checkpoint complete: wrote
> 0 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
> write=0.001 s, sync=0.001 s, total=0.025 s; sync files=0,
> longest=0.000 s, average=0.000 s; distance=0 kB, estimate=0 kB;
> LSN=0/14D0AD0, REDO LSN=0/14D0AD0
I thougt about something like the following, but your proposal may be
clearer.
> WAL range=[0/14D0340, 0/14D0AD0]
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2021-12-24 12:07 Bharath Rupireddy <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 23+ messages in thread
From: Bharath Rupireddy @ 2021-12-24 12:07 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Fri, Dec 24, 2021 at 11:21 AM Kyotaro Horiguchi
<[email protected]> wrote:
>
> At Thu, 23 Dec 2021 20:35:54 +0530, Bharath Rupireddy <[email protected]> wrote in
> > Hi,
> >
> > It is useful (for debugging purposes) if the checkpoint end message
> > has the checkpoint LSN and REDO LSN [1]. It gives more context while
> > analyzing checkpoint-related issues. The pg_controldata gives the last
> > checkpoint LSN and REDO LSN, but having this info alongside the log
> > message helps analyze issues that happened previously, connect the
> > dots and identify the root cause.
> >
> > Attaching a small patch herewith. Thoughts?
>
> A big +1 from me. I thought about proposing the same in the past.
Thanks for the review. I've added a CF entry to not lose track -
https://commitfest.postgresql.org/36/3474/.
> > [1]
> > 2021-12-23 14:58:54.694 UTC [1965649] LOG: checkpoint starting:
> > shutdown immediate
> > 2021-12-23 14:58:54.714 UTC [1965649] LOG: checkpoint complete: wrote
> > 0 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
> > write=0.001 s, sync=0.001 s, total=0.025 s; sync files=0,
> > longest=0.000 s, average=0.000 s; distance=0 kB, estimate=0 kB;
> > LSN=0/14D0AD0, REDO LSN=0/14D0AD0
>
> I thougt about something like the following, but your proposal may be
> clearer.
>
> > WAL range=[0/14D0340, 0/14D0AD0]
Yeah the proposed in the v1 is clear saying checkpoint/restartpoint
LSN and REDO LSN.
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2021-12-24 12:11 Michael Paquier <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 23+ messages in thread
From: Michael Paquier @ 2021-12-24 12:11 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]
On Fri, Dec 24, 2021 at 02:51:34PM +0900, Kyotaro Horiguchi wrote:
> I thougt about something like the following, but your proposal may be
> clearer.
+ "LSN=%X/%X, REDO LSN=%X/%X",
This could be rather confusing for the average user, even if I agree
that this is some information that only an advanced user could
understand. Could it be possible to define those fields in a more
deterministic way? For one, it is hard to understand the relationship
between both fields without looking at the code, particulary if both
share the same value. It is at least rather.. Well, mostly, easy to
guess what each other field means in this context, which is not the
case of what you are proposing here. One idea could be use also
"start point" for REDO, for example.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2021-12-24 12:24 Bharath Rupireddy <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Bharath Rupireddy @ 2021-12-24 12:24 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Dec 24, 2021 at 5:42 PM Michael Paquier <[email protected]> wrote:
>
> On Fri, Dec 24, 2021 at 02:51:34PM +0900, Kyotaro Horiguchi wrote:
> > I thougt about something like the following, but your proposal may be
> > clearer.
>
> + "LSN=%X/%X, REDO LSN=%X/%X",
> This could be rather confusing for the average user, even if I agree
> that this is some information that only an advanced user could
> understand. Could it be possible to define those fields in a more
> deterministic way? For one, it is hard to understand the relationship
> between both fields without looking at the code, particulary if both
> share the same value. It is at least rather.. Well, mostly, easy to
> guess what each other field means in this context, which is not the
> case of what you are proposing here. One idea could be use also
> "start point" for REDO, for example.
How about "location=%X/%X, REDO start location=%X/%X"? The entire log
message can look like below:
2021-12-24 12:20:19.140 UTC [1977834] LOG: checkpoint
complete:location=%X/%X, REDO start location=%X/%X; wrote 7 buffers
(0.0%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.005 s,
sync=0.007 s, total=0.192 s; sync files=5, longest=0.005 s,
average=0.002 s; distance=293 kB, estimate=56584 kB
Another variant:
2021-12-24 12:20:19.140 UTC [1977834] LOG: checkpoint completed at
location=%X/%X with REDO start location=%X/%X: wrote 7 buffers (0.0%);
0 WAL file(s) added, 0 removed, 0 recycled; write=0.005 s, sync=0.007
s, total=0.192 s; sync files=5, longest=0.005 s, average=0.002 s;
distance=293 kB, estimate=56584 kB
2021-12-24 12:20:19.140 UTC [1977834] LOG: restartpoint completed at
location=%X/%X with REDO start location=%X/%X: wrote 7 buffers (0.0%);
0 WAL file(s) added, 0 removed, 0 recycled; write=0.005 s, sync=0.007
s, total=0.192 s; sync files=5, longest=0.005 s, average=0.002 s;
distance=293 kB, estimate=56584 kB
Thoughts?
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2021-12-28 02:56 Bharath Rupireddy <[email protected]>
parent: Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Bharath Rupireddy @ 2021-12-28 02:56 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Dec 24, 2021 at 5:54 PM Bharath Rupireddy
<[email protected]> wrote:
>
> On Fri, Dec 24, 2021 at 5:42 PM Michael Paquier <[email protected]> wrote:
> >
> > On Fri, Dec 24, 2021 at 02:51:34PM +0900, Kyotaro Horiguchi wrote:
> > > I thougt about something like the following, but your proposal may be
> > > clearer.
> >
> > + "LSN=%X/%X, REDO LSN=%X/%X",
> > This could be rather confusing for the average user, even if I agree
> > that this is some information that only an advanced user could
> > understand. Could it be possible to define those fields in a more
> > deterministic way? For one, it is hard to understand the relationship
> > between both fields without looking at the code, particulary if both
> > share the same value. It is at least rather.. Well, mostly, easy to
> > guess what each other field means in this context, which is not the
> > case of what you are proposing here. One idea could be use also
> > "start point" for REDO, for example.
>
> How about "location=%X/%X, REDO start location=%X/%X"? The entire log
> message can look like below:
>
> 2021-12-24 12:20:19.140 UTC [1977834] LOG: checkpoint
> complete:location=%X/%X, REDO start location=%X/%X; wrote 7 buffers
> (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.005 s,
> sync=0.007 s, total=0.192 s; sync files=5, longest=0.005 s,
> average=0.002 s; distance=293 kB, estimate=56584 kB
>
> Another variant:
> 2021-12-24 12:20:19.140 UTC [1977834] LOG: checkpoint completed at
> location=%X/%X with REDO start location=%X/%X: wrote 7 buffers (0.0%);
> 0 WAL file(s) added, 0 removed, 0 recycled; write=0.005 s, sync=0.007
> s, total=0.192 s; sync files=5, longest=0.005 s, average=0.002 s;
> distance=293 kB, estimate=56584 kB
> 2021-12-24 12:20:19.140 UTC [1977834] LOG: restartpoint completed at
> location=%X/%X with REDO start location=%X/%X: wrote 7 buffers (0.0%);
> 0 WAL file(s) added, 0 removed, 0 recycled; write=0.005 s, sync=0.007
> s, total=0.192 s; sync files=5, longest=0.005 s, average=0.002 s;
> distance=293 kB, estimate=56584 kB
Here are the 2 patches.
one(v2-1-XXX.patch) adding the info as:
2021-12-28 02:44:34.870 UTC [2384386] LOG: checkpoint complete:
location=0/1B03040, REDO start location=0/1B03008; wrote 466 buffers
(2.8%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.014 s,
sync=0.038 s, total=0.072 s; sync files=21, longest=0.022 s,
average=0.002 s; distance=6346 kB, estimate=6346 kB
another(v2-2-XXX.patch) adding the info as:
2021-12-28 02:52:24.464 UTC [2394396] LOG: checkpoint completed at
location=0/212FFC8 with REDO start location=0/212FF90: wrote 451
buffers (2.8%); 0 WAL file(s) added, 0 removed, 1 recycled;
write=0.012 s, sync=0.032 s, total=0.071 s; sync files=6,
longest=0.022 s, average=0.006 s; distance=6272 kB, estimate=6272 kB
attaching v1-0001-XXX from the initial mail again just for the sake of
completion:
2021-12-23 14:58:54.714 UTC [1965649] LOG: checkpoint complete: wrote
0 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled;
write=0.001 s, sync=0.001 s, total=0.025 s; sync files=0,
longest=0.000 s, average=0.000 s; distance=0 kB, estimate=0 kB;
LSN=0/14D0AD0, REDO LSN=0/14D0AD0
Thoughts?
Regards,
Bharath Rupireddy.
Attachments:
[application/octet-stream] v2-1-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-l.patch (2.5K, ../../CALj2ACUP84UYSimewLSs8--rT9dUTA7CUYTgXTnEb-2iHkc9wA@mail.gmail.com/2-v2-1-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-l.patch)
download | inline diff:
From 999cfd53ce4e16ccfff94c0022cd80fe8ff84be5 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 28 Dec 2021 02:45:51 +0000
Subject: [PATCH v2] Add checkpoint and redo LSN to LogCheckpointEnd log
message
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN and REDO LSN. It gives more context while
analyzing checkpoint-related issues. The pg_controldata gives the
last checkpoint LSN and REDO LSN, but having this info alongside
the log message helps analyze issues that happened previously,
connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1e1fbe957f..26eb2a22f3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8920,11 +8920,14 @@ LogCheckpointEnd(bool restartpoint)
if (restartpoint)
ereport(LOG,
- (errmsg("restartpoint complete: wrote %d buffers (%.1f%%); "
+ (errmsg("restartpoint complete: location=%X/%X, REDO start location=%X/%X; "
+ "wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
"distance=%d kB, estimate=%d kB",
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo),
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8940,11 +8943,14 @@ LogCheckpointEnd(bool restartpoint)
(int) (CheckPointDistanceEstimate / 1024.0))));
else
ereport(LOG,
- (errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
+ (errmsg("checkpoint complete: location=%X/%X, REDO start location=%X/%X; "
+ "wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
"distance=%d kB, estimate=%d kB",
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo),
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
--
2.25.1
[application/octet-stream] v2-2-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-l.patch (2.5K, ../../CALj2ACUP84UYSimewLSs8--rT9dUTA7CUYTgXTnEb-2iHkc9wA@mail.gmail.com/3-v2-2-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-l.patch)
download | inline diff:
From c23010c67a66dd21e318ae4d475ee0d85c5c1d08 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Tue, 28 Dec 2021 02:52:49 +0000
Subject: [PATCH v2] Add checkpoint and redo LSN to LogCheckpointEnd log
message
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN and REDO LSN. It gives more context while
analyzing checkpoint-related issues. The pg_controldata gives the
last checkpoint LSN and REDO LSN, but having this info alongside
the log message helps analyze issues that happened previously,
connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1e1fbe957f..cd3fce6a2c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8920,11 +8920,14 @@ LogCheckpointEnd(bool restartpoint)
if (restartpoint)
ereport(LOG,
- (errmsg("restartpoint complete: wrote %d buffers (%.1f%%); "
+ (errmsg("restartpoint completed at location=%X/%X with REDO start location=%X/%X: "
+ "wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
"distance=%d kB, estimate=%d kB",
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo),
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8940,11 +8943,14 @@ LogCheckpointEnd(bool restartpoint)
(int) (CheckPointDistanceEstimate / 1024.0))));
else
ereport(LOG,
- (errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
+ (errmsg("checkpoint completed at location=%X/%X with REDO start location=%X/%X: "
+ "wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
"distance=%d kB, estimate=%d kB",
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo),
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
--
2.25.1
[application/octet-stream] v1-0001-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-l.patch (2.8K, ../../CALj2ACUP84UYSimewLSs8--rT9dUTA7CUYTgXTnEb-2iHkc9wA@mail.gmail.com/4-v1-0001-Add-checkpoint-and-redo-LSN-to-LogCheckpointEnd-l.patch)
download | inline diff:
From a641d82e8eb4fa9582a0f83a6f8da5ff468d1710 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Thu, 23 Dec 2021 15:03:30 +0000
Subject: [PATCH v1] Add checkpoint and redo LSN to LogCheckpointEnd log
message
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN and REDO LSN. It gives more context while
analyzing checkpoint-related issues. The pg_controldata gives the
last checkpoint LSN and REDO LSN, but having this info alongside
the log message helps analyze issues that happened previously,
connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1e1fbe957f..9816f7d328 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8924,7 +8924,8 @@ LogCheckpointEnd(bool restartpoint)
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "LSN=%X/%X, REDO LSN=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8937,14 +8938,17 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
(errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "LSN=%X/%X, REDO LSN=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8957,7 +8961,9 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
}
/*
--
2.25.1
^ permalink raw reply [nested|flat] 23+ messages in thread
* [PATCH v9 2/4] Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-01 04:34 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Bharath Rupireddy @ 2022-02-01 04:34 UTC (permalink / raw)
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN(end) and REDO LSN(start). It gives more
context while analyzing checkpoint-related issues. The pg_controldata
gives the last checkpoint LSN and REDO LSN, but having this info
alongside the log message helps analyze issues that happened
previously, connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a6c6e67f80..60da34433b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8923,7 +8923,8 @@ LogCheckpointEnd(bool restartpoint)
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8936,14 +8937,18 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /* we are the only updator of these variables */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
(errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8956,7 +8961,10 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /* we are the only updator of these variables */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
}
/*
--
2.27.0
----Next_Part(Wed_Feb__9_11_52_04_2022_007)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v9-0003-Change-location-to-lsn-in-pg_controldata.patch"
^ permalink raw reply [nested|flat] 23+ messages in thread
* [PATCH v10 2/5] Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-01 04:34 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Bharath Rupireddy @ 2022-02-01 04:34 UTC (permalink / raw)
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN(end) and REDO LSN(start). It gives more
context while analyzing checkpoint-related issues. The pg_controldata
gives the last checkpoint LSN and REDO LSN, but having this info
alongside the log message helps analyze issues that happened
previously, connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 8c2882b49f..ba1b96bd1d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8923,7 +8923,8 @@ LogCheckpointEnd(bool restartpoint)
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8936,14 +8937,18 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /* we are the only updator of these variables */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
(errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -8956,7 +8961,10 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /* we are the only updator of these variables */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
}
/*
--
2.27.0
----Next_Part(Mon_Feb_14_14_52_15_2022_460)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v10-0003-Change-location-to-lsn-in-pg_controldata.patch"
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-07 03:02 Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-07 03:02 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 07 Feb 2022 10:16:34 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> At Fri, 4 Feb 2022 10:59:04 +0900, Fujii Masao <[email protected]> wrote in
> > On 2022/02/03 15:50, Kyotaro Horiguchi wrote:
> > > By the way, restart point should start only while recoverying, and at
> > > the timeof the start both checkpoint.redo and checkpoint LSN are
> > > already past. We shouldn't update minRecovery point after promotion,
> > > but is there any reason for not updating the checkPoint and
> > > checkPointCopy? If we update them after promotion, the
> > > which-LSN-to-show problem would be gone.
> >
> > I tried to find the reason by reading the past discussion, but have
> > not found that yet.
> >
> > If we update checkpoint and REDO LSN at pg_control in that case, we
> > also need to update min recovery point at pg_control? Otherwise the
> > min recovery point at pg_control still indicates the old LSN that
> > previous restart point set.
>
> I had an assuption that the reason I think it shouldn't update
> minRecoveryPoint is that it has been or is going to be reset to
> invalid LSN by promotion and the checkpoint should refrain from
> touching it.
Hmm.. It doesn't seem to be the case. If a server crashes just after
promotion and before requesting post-promtion checkpoint,
minRecoveryPoint stays at a valid LSN.
(Promoted at 0/7000028)
Database cluster state: in production
Latest checkpoint location: 0/6000060
Latest checkpoint's REDO location: 0/6000028
Latest checkpoint's REDO WAL file: 000000010000000000000006
Minimum recovery ending location: 0/7000090
Min recovery ending loc's timeline: 2
minRecoveryPoint/TLI are ignored in any case where a server in
in-production state is started. In other words, the values are
useless. There's no clear or written reason for unrecording the last
ongoing restartpoint after promotion.
Before fast-promotion was introduced, we shouldn't get there after
end-of-recovery checkpoint (but somehow reached sometimes?) but it is
quite normal nowadays. Or to the contrary, we're expecting it to
happen and it is regarded as a normal checkponit. So we should do
there nowadays are as the follows.
- If any later checkpoint/restartpoint has been established, just skip
remaining task then return false. (!chkpt_was_latest)
(I'm not sure this can happen, though.)
- we update control file only when archive recovery is still ongoing.
- Otherwise reset minRecoveryPoint then continue.
Do you have any thoughts or opinions?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 958220c495..ab8a4d9a1b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9658,6 +9658,7 @@ CreateRestartPoint(int flags)
XLogRecPtr endptr;
XLogSegNo _logSegNo;
TimestampTz xtime;
+ bool chkpt_was_latest = false;
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
@@ -9752,44 +9753,69 @@ CreateRestartPoint(int flags)
PriorRedoPtr = ControlFile->checkPointCopy.redo;
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Update pg_control, using current time if no later checkpoints have been
+ * performed.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
{
+ chkpt_was_latest = true;
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
/*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened
+ * already while writing out dirty buffers, but not necessarily -
+ * e.g. because no buffers were dirtied. We do this because a
+ * non-exclusive base backup uses minRecoveryPoint to determine which
+ * WAL files must be included in the backup, and the file (or files)
+ * containing the checkpoint record must be included, at a
+ * minimum. Note that for an ordinary restart of recovery there's no
+ * value in having the minimum recovery point any earlier than this
* anyway, because redo will begin just after the checkpoint record.
*/
- if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
- ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ {
+ ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
+ ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
- /* update local copy */
- minRecoveryPoint = ControlFile->minRecoveryPoint;
- minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ /* update local copy */
+ minRecoveryPoint = ControlFile->minRecoveryPoint;
+ minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ }
+ if (flags & CHECKPOINT_IS_SHUTDOWN)
+ ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
+ }
+ else
+ {
+ /*
+ * We have exited from archive-recovery mode after starting. Crash
+ * recovery ever after should always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
}
- if (flags & CHECKPOINT_IS_SHUTDOWN)
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
UpdateControlFile();
}
LWLockRelease(ControlFileLock);
+ /*
+ * Skip all post-checkpoint work if others have done that with later
+ * checkpoints.
+ */
+ if (!chkpt_was_latest)
+ {
+ ereport((log_checkpoints ? LOG : DEBUG2),
+ (errmsg("post-restartpoint cleanup is skpped at %X/%X, because later restartpoints have been already performed",
+ LSN_FORMAT_ARGS(lastCheckPoint.redo))));
+
+ /* this checkpoint has not bee established */
+ return false;
+ }
+
/*
* Update the average distance between checkpoints/restartpoints if the
* prior checkpoint exists.
Attachments:
[text/plain] PoC_tidy_up_the_behavior_ofrestart_point_at_end_of_recovery.txt (4.3K, ../../[email protected]/2-PoC_tidy_up_the_behavior_ofrestart_point_at_end_of_recovery.txt)
download | inline diff:
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 958220c495..ab8a4d9a1b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9658,6 +9658,7 @@ CreateRestartPoint(int flags)
XLogRecPtr endptr;
XLogSegNo _logSegNo;
TimestampTz xtime;
+ bool chkpt_was_latest = false;
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(&XLogCtl->info_lck);
@@ -9752,44 +9753,69 @@ CreateRestartPoint(int flags)
PriorRedoPtr = ControlFile->checkPointCopy.redo;
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Update pg_control, using current time if no later checkpoints have been
+ * performed.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
{
+ chkpt_was_latest = true;
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
/*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened
+ * already while writing out dirty buffers, but not necessarily -
+ * e.g. because no buffers were dirtied. We do this because a
+ * non-exclusive base backup uses minRecoveryPoint to determine which
+ * WAL files must be included in the backup, and the file (or files)
+ * containing the checkpoint record must be included, at a
+ * minimum. Note that for an ordinary restart of recovery there's no
+ * value in having the minimum recovery point any earlier than this
* anyway, because redo will begin just after the checkpoint record.
*/
- if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
- ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
+ if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
+ {
+ ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
+ ControlFile->minRecoveryPointTLI = lastCheckPoint.ThisTimeLineID;
- /* update local copy */
- minRecoveryPoint = ControlFile->minRecoveryPoint;
- minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ /* update local copy */
+ minRecoveryPoint = ControlFile->minRecoveryPoint;
+ minRecoveryPointTLI = ControlFile->minRecoveryPointTLI;
+ }
+ if (flags & CHECKPOINT_IS_SHUTDOWN)
+ ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
+ }
+ else
+ {
+ /*
+ * We have exited from archive-recovery mode after starting. Crash
+ * recovery ever after should always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
}
- if (flags & CHECKPOINT_IS_SHUTDOWN)
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
UpdateControlFile();
}
LWLockRelease(ControlFileLock);
+ /*
+ * Skip all post-checkpoint work if others have done that with later
+ * checkpoints.
+ */
+ if (!chkpt_was_latest)
+ {
+ ereport((log_checkpoints ? LOG : DEBUG2),
+ (errmsg("post-restartpoint cleanup is skpped at %X/%X, because later restartpoints have been already performed",
+ LSN_FORMAT_ARGS(lastCheckPoint.redo))));
+
+ /* this checkpoint has not bee established */
+ return false;
+ }
+
/*
* Update the average distance between checkpoints/restartpoints if the
* prior checkpoint exists.
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-07 04:51 Fujii Masao <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Fujii Masao @ 2022-02-07 04:51 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On 2022/02/07 12:02, Kyotaro Horiguchi wrote:
> - If any later checkpoint/restartpoint has been established, just skip
> remaining task then return false. (!chkpt_was_latest)
> (I'm not sure this can happen, though.)
>
> - we update control file only when archive recovery is still ongoing.
This comment seems to conflict with what your PoC patch does. Because with the patch, ControlFile->checkPoint and ControlFile->checkPointCopy seem to be updated even when ControlFile->state != DB_IN_ARCHIVE_RECOVERY.
I agree with what your PoC patch does for now. When we're not in archive recovery state, checkpoint and REDO locations in pg_control should be updated but min recovery point should be reset to invalid one (which instruments that subsequent crash recovery should replay all available WAL files).
> - Otherwise reset minRecoveryPoint then continue.
>
> Do you have any thoughts or opinions?
Regarding chkpt_was_latest, whether the state is DB_IN_ARCHIVE_RECOVERY or not, if checkpoint and redo locations in pg_control are updated, IMO we don't need to skip the "remaining tasks". Since those locations are updated and subsequent crash recovery will start from that redo location, for example, ISTM that we can safely delete old WAL files prior to the redo location as the "remaining tasks". Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-08 07:58 Kyotaro Horiguchi <[email protected]>
parent: Fujii Masao <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-08 07:58 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 7 Feb 2022 13:51:31 +0900, Fujii Masao <[email protected]> wrote in
>
>
> On 2022/02/07 12:02, Kyotaro Horiguchi wrote:
> > - If any later checkpoint/restartpoint has been established, just skip
> > remaining task then return false. (!chkpt_was_latest)
> > (I'm not sure this can happen, though.)
> > - we update control file only when archive recovery is still ongoing.
>
> This comment seems to conflict with what your PoC patch does. Because
> with the patch, ControlFile->checkPoint and
> ControlFile->checkPointCopy seem to be updated even when
> ControlFile->state != DB_IN_ARCHIVE_RECOVERY.
Ah, yeah, by "update" I meant that "move forward". Sorry for confusing
wording.
> I agree with what your PoC patch does for now. When we're not in
> archive recovery state, checkpoint and REDO locations in pg_control
> should be updated but min recovery point should be reset to invalid
> one (which instruments that subsequent crash recovery should replay
> all available WAL files).
Yes. All buffers before the last recovery point's end have been
flushed out so the recovery point is valid as a checkpoint. On the
other hand minRecoveryPoint is no longer needed and actually is
ignored at the next crash recovery. We can leave it alone but it is
consistent that it is cleared.
> > - Otherwise reset minRecoveryPoint then continue.
> > Do you have any thoughts or opinions?
>
> Regarding chkpt_was_latest, whether the state is
> DB_IN_ARCHIVE_RECOVERY or not, if checkpoint and redo locations in
> pg_control are updated, IMO we don't need to skip the "remaining
> tasks". Since those locations are updated and subsequent crash
> recovery will start from that redo location, for example, ISTM that we
> can safely delete old WAL files prior to the redo location as the
> "remaining tasks". Thought?
If I read you correctly, the PoC works that way. It updates pg_control
if the restart point is latest then performs the remaining cleanup
tasks in that case. Recovery state doesn't affect this process.
I reexamined about the possibility of concurrent checkpoints.
Both CreateCheckPoint and CreateRestartPoint are called from
checkpointer loop, shutdown handler of checkpointer and standalone
process. So I can't see a possibility of concurrent checkpoints.
In the past we had a time when startup process called CreateCheckPoint
directly in the crash recovery case where checkpoint is not running
but since 7ff23c6d27 checkpoint is started before startup process
starts. So I conclude that that cannot happen.
So the attached takes away the path for the case where the restart
point is overtaken by a concurrent checkpoint.
Thus.. the attached removes the ambiguity of of the proposed patch
about the LSNs in the restartpoint-ending log message.
Thoughts?
--
Kyotaro Horiguchi
NTT Open Source Software Center
From 43888869846b6de00fbddb215300a8ff774bbc04 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
Date: Tue, 8 Feb 2022 16:42:53 +0900
Subject: [PATCH v1] Get rid of unused path to handle concurrent checkpoints
CreateRestartPoint considered the case a concurrent checkpoint is
running. But after 7ff23c6d27 we no longer launch multiple checkpoints
simultaneously. That code path, if it is passed, may leave
unrecoverable database by removing WAL segments that are required by
the last established restartpoint.
---
src/backend/access/transam/xlog.c | 53 +++++++++++++++++++------------
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 958220c495..01a345e8bd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9752,29 +9752,30 @@ CreateRestartPoint(int flags)
PriorRedoPtr = ControlFile->checkPointCopy.redo;
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Update pg_control, using current time.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+
+ /* We mustn't have a concurrent checkpoint that advances checkpoint LSN */
+ Assert(lastCheckPoint.redo > ControlFile->checkPointCopy.redo);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /*
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record.
+ */
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9786,8 +9787,18 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /*
+ * We have exited from archive-recovery mode after this restartpoint
+ * started. Crash recovery ever after should always recover to the end
+ * of WAL.
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+ }
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
--
2.27.0
Attachments:
[text/plain] v1-0001-Get-rid-of-unused-path-to-handle-concurrent-check.patch.txt (3.8K, ../../[email protected]/2-v1-0001-Get-rid-of-unused-path-to-handle-concurrent-check.patch.txt)
download | inline diff:
From 43888869846b6de00fbddb215300a8ff774bbc04 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
Date: Tue, 8 Feb 2022 16:42:53 +0900
Subject: [PATCH v1] Get rid of unused path to handle concurrent checkpoints
CreateRestartPoint considered the case a concurrent checkpoint is
running. But after 7ff23c6d27 we no longer launch multiple checkpoints
simultaneously. That code path, if it is passed, may leave
unrecoverable database by removing WAL segments that are required by
the last established restartpoint.
---
src/backend/access/transam/xlog.c | 53 +++++++++++++++++++------------
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 958220c495..01a345e8bd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9752,29 +9752,30 @@ CreateRestartPoint(int flags)
PriorRedoPtr = ControlFile->checkPointCopy.redo;
/*
- * Update pg_control, using current time. Check that it still shows
- * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing;
- * this is a quick hack to make sure nothing really bad happens if somehow
- * we get here after the end-of-recovery checkpoint.
+ * Update pg_control, using current time.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+
+ /* We mustn't have a concurrent checkpoint that advances checkpoint LSN */
+ Assert(lastCheckPoint.redo > ControlFile->checkPointCopy.redo);
+
+ ControlFile->checkPoint = lastCheckPointRecPtr;
+ ControlFile->checkPointCopy = lastCheckPoint;
+
+ /*
+ * Ensure minRecoveryPoint is past the checkpoint record while archive
+ * recovery is still ongoing. Normally, this will have happened already
+ * while writing out dirty buffers, but not necessarily - e.g. because no
+ * buffers were dirtied. We do this because a non-exclusive base backup
+ * uses minRecoveryPoint to determine which WAL files must be included in
+ * the backup, and the file (or files) containing the checkpoint record
+ * must be included, at a minimum. Note that for an ordinary restart of
+ * recovery there's no value in having the minimum recovery point any
+ * earlier than this anyway, because redo will begin just after the
+ * checkpoint record.
+ */
+ if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY)
{
- ControlFile->checkPoint = lastCheckPointRecPtr;
- ControlFile->checkPointCopy = lastCheckPoint;
-
- /*
- * Ensure minRecoveryPoint is past the checkpoint record. Normally,
- * this will have happened already while writing out dirty buffers,
- * but not necessarily - e.g. because no buffers were dirtied. We do
- * this because a non-exclusive base backup uses minRecoveryPoint to
- * determine which WAL files must be included in the backup, and the
- * file (or files) containing the checkpoint record must be included,
- * at a minimum. Note that for an ordinary restart of recovery there's
- * no value in having the minimum recovery point any earlier than this
- * anyway, because redo will begin just after the checkpoint record.
- */
if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr)
{
ControlFile->minRecoveryPoint = lastCheckPointEndPtr;
@@ -9786,8 +9787,18 @@ CreateRestartPoint(int flags)
}
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
- UpdateControlFile();
}
+ else
+ {
+ /*
+ * We have exited from archive-recovery mode after this restartpoint
+ * started. Crash recovery ever after should always recover to the end
+ * of WAL.
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+ }
+ UpdateControlFile();
LWLockRelease(ControlFileLock);
/*
--
2.27.0
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-08 08:40 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-08 08:40 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Mmm.. checkpoint and checkpointer are quite confusing in this context..
At Tue, 08 Feb 2022 16:58:22 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> At Mon, 7 Feb 2022 13:51:31 +0900, Fujii Masao <[email protected]> wrote in
> >
> >
> > On 2022/02/07 12:02, Kyotaro Horiguchi wrote:
> > > - If any later checkpoint/restartpoint has been established, just skip
> > > remaining task then return false. (!chkpt_was_latest)
> > > (I'm not sure this can happen, though.)
> > > - we update control file only when archive recovery is still ongoing.
> >
> > This comment seems to conflict with what your PoC patch does. Because
> > with the patch, ControlFile->checkPoint and
> > ControlFile->checkPointCopy seem to be updated even when
> > ControlFile->state != DB_IN_ARCHIVE_RECOVERY.
>
> Ah, yeah, by "update" I meant that "move forward". Sorry for confusing
> wording.
Please ignore the "that".
> > I agree with what your PoC patch does for now. When we're not in
> > archive recovery state, checkpoint and REDO locations in pg_control
> > should be updated but min recovery point should be reset to invalid
> > one (which instruments that subsequent crash recovery should replay
> > all available WAL files).
>
> Yes. All buffers before the last recovery point's end have been
> flushed out so the recovery point is valid as a checkpoint. On the
> other hand minRecoveryPoint is no longer needed and actually is
> ignored at the next crash recovery. We can leave it alone but it is
> consistent that it is cleared.
>
> > > - Otherwise reset minRecoveryPoint then continue.
> > > Do you have any thoughts or opinions?
> >
> > Regarding chkpt_was_latest, whether the state is
> > DB_IN_ARCHIVE_RECOVERY or not, if checkpoint and redo locations in
> > pg_control are updated, IMO we don't need to skip the "remaining
> > tasks". Since those locations are updated and subsequent crash
> > recovery will start from that redo location, for example, ISTM that we
> > can safely delete old WAL files prior to the redo location as the
> > "remaining tasks". Thought?
>
> If I read you correctly, the PoC works that way. It updates pg_control
> if the restart point is latest then performs the remaining cleanup
> tasks in that case. Recovery state doesn't affect this process.
>
> I reexamined about the possibility of concurrent checkpoints.
>
> Both CreateCheckPoint and CreateRestartPoint are called from
> checkpointer loop, shutdown handler of checkpointer and standalone
> process. So I can't see a possibility of concurrent checkpoints.
>
> In the past we had a time when startup process called CreateCheckPoint
- directly in the crash recovery case where checkpoint is not running
- but since 7ff23c6d27 checkpoint is started before startup process
+ directly in the crash recovery case where checkpointer is not running
+ but since 7ff23c6d27 checkpointer is launched before startup process
> starts. So I conclude that that cannot happen.
>
> So the attached takes away the path for the case where the restart
> point is overtaken by a concurrent checkpoint.
>
> Thus.. the attached removes the ambiguity of of the proposed patch
> about the LSNs in the restartpoint-ending log message.
Thoughts?
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-08 09:03 Bharath Rupireddy <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Bharath Rupireddy @ 2022-02-08 09:03 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: Fujii Masao <[email protected]>; Nathan Bossart <[email protected]>; Stephen Frost <[email protected]>; Bossart, Nathan <[email protected]>; Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Feb 8, 2022 at 2:10 PM Kyotaro Horiguchi
<[email protected]> wrote:
> > Thus.. the attached removes the ambiguity of of the proposed patch
> > about the LSNs in the restartpoint-ending log message.
>
> Thoughts?
Thanks for the patch. I have few comments on the
v1-0001-Get-rid-of-unused-path-to-handle-concurrent-check.patch
1) Can we have this Assert right after "skipping restartpoint, already
performed at %X/%X" error message block? Does it make any difference?
My point is that if at all, we were to assert this, why can't we do it
before CheckPointGuts?
+ /* We mustn't have a concurrent checkpoint that advances checkpoint LSN */
+ Assert(lastCheckPoint.redo > ControlFile->checkPointCopy.redo);
+
2) Related to the above Assert, do we really need an assertion or a FATAL error?
3) Let's be consistent with "crash recovery" - replace
"archive-recovery" with "archive recovery"?
+ * We have exited from archive-recovery mode after this restartpoint
+ * started. Crash recovery ever after should always recover to the end
4) Isn't it enough to say "Crash recovery should always recover to the
end of WAL."?
+ * started. Crash recovery ever after should always recover to the end
5) Is there a reliable test case covering this code? Please point me
if the test case is shared upthread somewhere.
6) So, with this patch, the v8 patch-set posted at [1] doesn't need
any changes IIUC. If that's the case, please feel free to post all the
patches together such that they get tested in cfbot.
[1] - https://www.postgresql.org/message-id/CALj2ACUtZhTb%3D2ENkF3BQ3wi137uaGi__qzvXC-qFYC0XwjALw%40mail.g...
Regards,
Bharath Rupireddy.
^ permalink raw reply [nested|flat] 23+ messages in thread
* [PATCH v13] Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-03-04 04:22 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Bharath Rupireddy @ 2022-03-04 04:22 UTC (permalink / raw)
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN(end) and REDO LSN(start). It gives more
context while analyzing checkpoint-related issues. The pg_controldata
gives the last checkpoint LSN and REDO LSN, but having this info
alongside the log message helps analyze issues that happened
previously, connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ed16f279b1..b85c76d8f8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6121,7 +6121,8 @@ LogCheckpointEnd(bool restartpoint)
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -6134,14 +6135,21 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /*
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
+ */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
(errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -6154,7 +6162,13 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /*
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
+ */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
}
/*
--
2.27.0
----Next_Part(Wed_Mar_16_10_29_47_2022_770)----
^ permalink raw reply [nested|flat] 23+ messages in thread
* [PATCH v12 2/5] Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-03-04 04:22 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Bharath Rupireddy @ 2022-03-04 04:22 UTC (permalink / raw)
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN(end) and REDO LSN(start). It gives more
context while analyzing checkpoint-related issues. The pg_controldata
gives the last checkpoint LSN and REDO LSN, but having this info
alongside the log message helps analyze issues that happened
previously, connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index bd962763cc..ce3815ea99 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6120,7 +6120,8 @@ LogCheckpointEnd(bool restartpoint)
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -6133,14 +6134,21 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /*
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
+ */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
(errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -6153,7 +6161,13 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /*
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
+ */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
}
/*
--
2.27.0
----Next_Part(Tue_Mar_15_17_23_40_2022_661)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v12-0003-Change-location-to-lsn-in-pg_controldata.patch"
^ permalink raw reply [nested|flat] 23+ messages in thread
* [PATCH v11 2/5] Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-03-04 04:22 Bharath Rupireddy <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Bharath Rupireddy @ 2022-03-04 04:22 UTC (permalink / raw)
It is useful (for debugging purposes) if the checkpoint end message
has the checkpoint LSN(end) and REDO LSN(start). It gives more
context while analyzing checkpoint-related issues. The pg_controldata
gives the last checkpoint LSN and REDO LSN, but having this info
alongside the log message helps analyze issues that happened
previously, connect the dots and identify the root cause.
---
src/backend/access/transam/xlog.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3987aa81de..2e1123fd42 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6120,7 +6120,8 @@ LogCheckpointEnd(bool restartpoint)
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -6133,14 +6134,18 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /* we are the only updator of these variables */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
(errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -6153,7 +6158,10 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /* we are the only updator of these variables */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
}
/*
--
2.27.0
----Next_Part(Fri_Mar__4_14_10_38_2022_481)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v11-0003-Change-location-to-lsn-in-pg_controldata.patch"
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-03-28 19:31 Greg Stark <[email protected]>
1 sibling, 0 replies; 23+ messages in thread
From: Greg Stark @ 2022-03-28 19:31 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: Michael Paquier <[email protected]>; Bharath Rupireddy <[email protected]>; [email protected]; [email protected]; Stephen Frost <[email protected]>; Bossart, Nathan <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
This patch is currently showing up with a test failure in the CFBot
however I do *not* believe this is a bug in the patch. I think it's a
bug in that test which is being discussed elsewhere.
It's also a very short and straightforward patch that a committer
could probably make a decision about whether it's a good idea or not
and then apply it quickly if so.
Just to give people a leg up and an idea how short the patch is...
Here's the entire patch:
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index ed16f279b1..b85c76d8f8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6121,7 +6121,8 @@ LogCheckpointEnd(bool restartpoint)
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -6134,14 +6135,21 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /*
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
+ */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
(errmsg("checkpoint complete: wrote %d buffers (%.1f%%); "
"%d WAL file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s; "
"sync files=%d, longest=%ld.%03d s, average=%ld.%03d s; "
- "distance=%d kB, estimate=%d kB",
+ "distance=%d kB, estimate=%d kB; "
+ "lsn=%X/%X, redo lsn=%X/%X",
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,
@@ -6154,7 +6162,13 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs / 1000, (int) (longest_msecs % 1000),
average_msecs / 1000, (int) (average_msecs % 1000),
(int) (PrevCheckPointDistance / 1024.0),
- (int) (CheckPointDistanceEstimate / 1024.0))));
+ (int) (CheckPointDistanceEstimate / 1024.0),
+ /*
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
+ */
+ LSN_FORMAT_ARGS(ControlFile->checkPoint),
+ LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
}
/*
--
2.27.0
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-07-06 16:11 Fujii Masao <[email protected]>
1 sibling, 1 reply; 23+ messages in thread
From: Fujii Masao @ 2022-07-06 16:11 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On 2022/03/16 10:29, Kyotaro Horiguchi wrote:
> At Wed, 16 Mar 2022 09:19:13 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
>> In short, I split out the two topics other than checkpoint log to
>> other threads.
>
> So, this is about the main topic of this thread, adding LSNs to
> checkpint log. Other topics have moved to other treads [1], [2] ,
> [3].
>
> I think this is no longer controversial alone. So this patch is now
> really Read-for-Commiter and is waiting to be picked up.
+1
+ * ControlFileLock is not required as we are the only
+ * updator of these variables.
Isn't it better to add "at this time" or something at the end of the comment because only we're not always updator of them? No?
Barring any objection, I'm thinking to apply the above small change and commit the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-07-07 07:26 Kyotaro Horiguchi <[email protected]>
parent: Fujii Masao <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Kyotaro Horiguchi @ 2022-07-07 07:26 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Thu, 7 Jul 2022 01:11:33 +0900, Fujii Masao <[email protected]> wrote in
>
>
> On 2022/03/16 10:29, Kyotaro Horiguchi wrote:
> > At Wed, 16 Mar 2022 09:19:13 +0900 (JST), Kyotaro Horiguchi
> > <[email protected]> wrote in
> >> In short, I split out the two topics other than checkpoint log to
> >> other threads.
> > So, this is about the main topic of this thread, adding LSNs to
> > checkpint log. Other topics have moved to other treads [1], [2] ,
> > [3].
> > I think this is no longer controversial alone. So this patch is now
> > really Read-for-Commiter and is waiting to be picked up.
>
> +1
>
> + * ControlFileLock is not required as we are the only
> + * updator of these variables.
>
> Isn't it better to add "at this time" or something at the end of the
> comment because only we're not always updator of them? No?
Excluding initialization, (I believe) checkpointer is really the only
updator of the variables/members. But I'm fine with the addition.
> Barring any objection, I'm thinking to apply the above small change
> and commit the patch.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-07-07 13:45 Fujii Masao <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Fujii Masao @ 2022-07-07 13:45 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On 2022/07/07 16:26, Kyotaro Horiguchi wrote:
>> + * ControlFileLock is not required as we are the only
>> + * updator of these variables.
>>
>> Isn't it better to add "at this time" or something at the end of the
>> comment because only we're not always updator of them? No?
>
> Excluding initialization, (I believe) checkpointer is really the only
> updator of the variables/members. But I'm fine with the addition.
Ok, so I modified the patch slightly and pushed it. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 23+ messages in thread
* [PATCH v29 08/11] Add aggregates support in IVM
@ 2023-05-31 11:46 Yugo Nagata <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Yugo Nagata @ 2023-05-31 11:46 UTC (permalink / raw)
count, sum, adn avg are supported.
As a restriction, expressions specified in GROUP BY must appear in
the target list because tuples to be updated in IMMV are identified
by using this group key. However, in the case of aggregates without
GROUP BY, there is only one tuple in the view, so keys are not uses
to identify tuples.
When creating a IMMV, in addition to __ivm_count column, some hidden
columns for each aggregate are added to the target list. For example,
names of these hidden columns are ivm_count_avg and ivm_sum_avg for
the average function, and so on.
When a base table is modified, the aggregated values and related
hidden columns are also updated as well as __ivm_count__. The
way of update depends the kind of aggregate function.=E3=80=80Specifically,
sum and count are updated by simply adding or subtracting delta value
calculated from delta tables. avg is updated by using values of sum
and count stored in views as hidden columns and deltas calculated
from delta tables.
About aggregate functions except "count()" (sum and avg), NULLs in input
values are ignored, and the result of aggegate should be NULL when no
rows are selected. To support this specification, the numbers of non-NULL
input values are counted and stored in hidden columns. In the case of
count(), count(x) returns zero when no rows are selected, but count(*)
doesn't ignore NULL input.
---
src/backend/commands/createas.c | 264 +++++++++++++++++--
src/backend/commands/matview.c | 433 ++++++++++++++++++++++++++++++--
src/include/commands/createas.h | 1 +
3 files changed, 661 insertions(+), 37 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createa=
s.c
index 076f35ee6b..c8aa558f2e 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -54,14 +54,19 @@
#include "parser/parsetree.h"
#include "parser/parse_clause.h"
#include "parser/parse_func.h"
+#include "parser/parse_type.h"
#include "rewrite/rewriteHandler.h"
+#include "rewrite/rewriteManip.h"
#include "storage/smgr.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
+#include "utils/regproc.h"
+#include "utils/fmgroids.h"
#include "utils/rel.h"
#include "utils/rls.h"
#include "utils/snapmgr.h"
+#include "utils/syscache.h"
=20
typedef struct
{
@@ -75,6 +80,11 @@ typedef struct
BulkInsertState bistate; /* bulk insert state */
} DR_intorel;
=20
+typedef struct
+{
+ bool has_agg;
+} check_ivm_restriction_context;
+
/* utility functions for CTAS definition creation */
static ObjectAddress create_ctas_internal(List *attrList, IntoClause *into=
);
static ObjectAddress create_ctas_nodata(List *tlist, IntoClause *into);
@@ -89,8 +99,9 @@ static void CreateIvmTriggersOnBaseTablesRecurse(Query *q=
ry, Node *node, Oid mat
Relids *relids, bool ex_lock);
static void CreateIvmTrigger(Oid relOid, Oid viewOid, int16 type, int16 ti=
ming, bool ex_lock);
static void check_ivm_restriction(Node *node);
-static bool check_ivm_restriction_walker(Node *node, void *context);
+static bool check_ivm_restriction_walker(Node *node, check_ivm_restriction=
_context *context);
static Bitmapset *get_primary_key_attnos_from_query(Query *query, List **c=
onstraintList);
+static bool check_aggregate_supports_ivm(Oid aggfnoid);
=20
/*
* create_ctas_internal
@@ -421,6 +432,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt=
*stmt,
* rewriteQueryForIMMV -- rewrite view definition query for IMMV
*
* count(*) is added for counting distinct tuples in views.
+ * Also, additional hidden columns are added for aggregate values.
*/
Query *
rewriteQueryForIMMV(Query *query, List *colNames)
@@ -434,16 +446,49 @@ rewriteQueryForIMMV(Query *query, List *colNames)
rewritten =3D copyObject(query);
pstate->p_expr_kind =3D EXPR_KIND_SELECT_TARGET;
=20
- /*
- * Convert DISTINCT to GROUP BY and add count(*) for counting distinct
- * tuples in views.
- */
- if (rewritten->distinctClause)
+ /* group keys must be in targetlist */
+ if (rewritten->groupClause)
{
- TargetEntry *tle;
+ ListCell *lc;
+ foreach(lc, rewritten->groupClause)
+ {
+ SortGroupClause *scl =3D (SortGroupClause *) lfirst(lc);
+ TargetEntry *tle =3D get_sortgroupclause_tle(scl, rewritten->targetList=
);
=20
+ if (tle->resjunk)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("GROUP BY expression not appearing in select list is not sup=
ported on incrementally maintainable materialized view")));
+ }
+ }
+ /* Convert DISTINCT to GROUP BY. count(*) will be added afterward. */
+ else if (!rewritten->hasAggs && rewritten->distinctClause)
rewritten->groupClause =3D transformDistinctClause(NULL, &rewritten->tar=
getList, rewritten->sortClause, false);
=20
+ /* Add additional columns for aggregate values */
+ if (rewritten->hasAggs)
+ {
+ ListCell *lc;
+ List *aggs =3D NIL;
+ AttrNumber next_resno =3D list_length(rewritten->targetList) + 1;
+
+ foreach(lc, rewritten->targetList)
+ {
+ TargetEntry *tle =3D (TargetEntry *) lfirst(lc);
+ char *resname =3D (colNames =3D=3D NIL || foreach_current_index(lc) >=
=3D list_length(colNames) ?
+ tle->resname : strVal(list_nth(colNames, tle->resno - 1)));
+
+ if (IsA(tle->expr, Aggref))
+ makeIvmAggColumn(pstate, (Aggref *) tle->expr, resname, &next_resno, &=
aggs);
+ }
+ rewritten->targetList =3D list_concat(rewritten->targetList, aggs);
+ }
+
+ /* Add count(*) for counting distinct tuples in views */
+ if (rewritten->distinctClause || rewritten->hasAggs)
+ {
+ TargetEntry *tle;
+
fn =3D makeFuncCall(SystemFuncName("count"), NIL, COERCE_EXPLICIT_CALL, =
-1);
fn->agg_star =3D true;
=20
@@ -460,6 +505,91 @@ rewriteQueryForIMMV(Query *query, List *colNames)
return rewritten;
}
=20
+/*
+ * makeIvmAggColumn -- make additional aggregate columns for IVM
+ *
+ * For an aggregate column specified by aggref, additional aggregate colum=
ns
+ * are added, which are used to calculate the new aggregate value in IMMV.
+ * An additional aggregate columns has a name based on resname
+ * (ex. ivm_count_resname), and resno specified by next_resno. The created
+ * columns are returned to aggs, and the resno for the next column is also
+ * returned to next_resno.
+ *
+ * Currently, an additional count() is created for aggref other than count.
+ * In addition, sum() is created for avg aggregate column.
+ */
+void
+makeIvmAggColumn(ParseState *pstate, Aggref *aggref, char *resname, AttrNu=
mber *next_resno, List **aggs)
+{
+ TargetEntry *tle_count;
+ Node *node;
+ FuncCall *fn;
+ Const *dmy_arg =3D makeConst(INT4OID,
+ -1,
+ InvalidOid,
+ sizeof(int32),
+ Int32GetDatum(1),
+ false,
+ true); /* pass by value */
+ const char *aggname =3D get_func_name(aggref->aggfnoid);
+
+ /*
+ * For aggregate functions except count, add count() func with the same a=
rg parameters.
+ * This count result is used for determining if the aggregate value shoul=
d be NULL or not.
+ * Also, add sum() func for avg because we need to calculate an average v=
alue as sum/count.
+ *
+ * XXX: If there are same expressions explicitly in the target list, we c=
an use this instead
+ * of adding new duplicated one.
+ */
+ if (strcmp(aggname, "count") !=3D 0)
+ {
+ fn =3D makeFuncCall(SystemFuncName("count"), NIL, COERCE_EXPLICIT_CALL, =
-1);
+
+ /* Make a Func with a dummy arg, and then override this by the original =
agg's args. */
+ node =3D ParseFuncOrColumn(pstate, fn->funcname, list_make1(dmy_arg), NU=
LL, fn, false, -1);
+ ((Aggref *)node)->args =3D aggref->args;
+
+ tle_count =3D makeTargetEntry((Expr *) node,
+ *next_resno,
+ pstrdup(makeObjectName("__ivm_count",resname, "_")),
+ false);
+ *aggs =3D lappend(*aggs, tle_count);
+ (*next_resno)++;
+ }
+ if (strcmp(aggname, "avg") =3D=3D 0)
+ {
+ List *dmy_args =3D NIL;
+ ListCell *lc;
+ foreach(lc, aggref->aggargtypes)
+ {
+ Oid typeid =3D lfirst_oid(lc);
+ Type type =3D typeidType(typeid);
+
+ Const *con =3D makeConst(typeid,
+ -1,
+ typeTypeCollation(type),
+ typeLen(type),
+ (Datum) 0,
+ true,
+ typeByVal(type));
+ dmy_args =3D lappend(dmy_args, con);
+ ReleaseSysCache(type);
+ }
+ fn =3D makeFuncCall(SystemFuncName("sum"), NIL, COERCE_EXPLICIT_CALL, -1=
);
+
+ /* Make a Func with dummy args, and then override this by the original a=
gg's args. */
+ node =3D ParseFuncOrColumn(pstate, fn->funcname, dmy_args, NULL, fn, fal=
se, -1);
+ ((Aggref *)node)->args =3D aggref->args;
+
+ tle_count =3D makeTargetEntry((Expr *) node,
+ *next_resno,
+ pstrdup(makeObjectName("__ivm_sum",resname, "_")),
+ false);
+ *aggs =3D lappend(*aggs, tle_count);
+ (*next_resno)++;
+ }
+}
+
/*
* GetIntoRelEFlags --- compute executor flags needed for CREATE TABLE AS
*
@@ -943,11 +1073,13 @@ CreateIvmTrigger(Oid relOid, Oid viewOid, int16 type=
, int16 timing, bool ex_lock
static void
check_ivm_restriction(Node *node)
{
- check_ivm_restriction_walker(node, NULL);
+ check_ivm_restriction_context context =3D {false};
+
+ check_ivm_restriction_walker(node, &context);
}
=20
static bool
-check_ivm_restriction_walker(Node *node, void *context)
+check_ivm_restriction_walker(Node *node, check_ivm_restriction_context *co=
ntext)
{
if (node =3D=3D NULL)
return false;
@@ -976,6 +1108,10 @@ check_ivm_restriction_walker(Node *node, void *contex=
t)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("CTE is not supported on incrementally maintainable materia=
lized view")));
+ if (qry->groupClause !=3D NIL && !qry->hasAggs)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("GROUP BY clause without aggregate is not supported on incr=
ementally maintainable materialized view")));
if (qry->havingQual !=3D NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1028,6 +1164,8 @@ check_ivm_restriction_walker(Node *node, void *contex=
t)
}
}
=20
+ context->has_agg |=3D qry->hasAggs;
+
/* restrictions for rtable */
foreach(lc, qry->rtable)
{
@@ -1076,7 +1214,7 @@ check_ivm_restriction_walker(Node *node, void *contex=
t)
=20
}
=20
- query_tree_walker(qry, check_ivm_restriction_walker, NULL, QTW_IGNORE_=
RANGE_TABLE);
+ query_tree_walker(qry, check_ivm_restriction_walker, (void *) context,=
QTW_IGNORE_RANGE_TABLE);
=20
break;
}
@@ -1087,8 +1225,12 @@ check_ivm_restriction_walker(Node *node, void *conte=
xt)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("column name %s is not supported on incrementally maintain=
able materialized view", tle->resname)));
+ if (context->has_agg && !IsA(tle->expr, Aggref) && contain_aggs_of_lev=
el((Node *) tle->expr, 0))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("expression containing an aggregate in it is not supported =
on incrementally maintainable materialized view")));
=20
- expression_tree_walker(node, check_ivm_restriction_walker, NULL);
+ expression_tree_walker(node, check_ivm_restriction_walker, (void *) co=
ntext);
break;
}
case T_JoinExpr:
@@ -1100,14 +1242,36 @@ check_ivm_restriction_walker(Node *node, void *cont=
ext)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("OUTER JOIN is not supported on incrementally maintainable=
materialized view")));
=20
- expression_tree_walker(node, check_ivm_restriction_walker, NULL);
+ expression_tree_walker(node, check_ivm_restriction_walker, (void *) co=
ntext);
+ break;
}
- break;
case T_Aggref:
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("aggregate function is not supported on incrementally maintai=
nable materialized view")));
- break;
+ {
+ /* Check if this supports IVM */
+ Aggref *aggref =3D (Aggref *) node;
+ const char *aggname =3D format_procedure(aggref->aggfnoid);
+
+ if (aggref->aggfilter !=3D NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("aggregate function with FILTER clause is not supported on =
incrementally maintainable materialized view")));
+
+ if (aggref->aggdistinct !=3D NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("aggregate function with DISTINCT arguments is not supporte=
d on incrementally maintainable materialized view")));
+
+ if (aggref->aggorder !=3D NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("aggregate function with ORDER clause is not supported on i=
ncrementally maintainable materialized view")));
+
+ if (!check_aggregate_supports_ivm(aggref->aggfnoid))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("aggregate function %s is not supported on incrementally ma=
intainable materialized view", aggname)));
+ break;
+ }
default:
expression_tree_walker(node, check_ivm_restriction_walker, (void *) con=
text);
break;
@@ -1115,6 +1279,46 @@ check_ivm_restriction_walker(Node *node, void *conte=
xt)
return false;
}
=20
+/*
+ * check_aggregate_supports_ivm
+ *
+ * Check if the given aggregate function is supporting IVM
+ */
+static bool
+check_aggregate_supports_ivm(Oid aggfnoid)
+{
+ switch (aggfnoid)
+ {
+ /* count */
+ case F_COUNT_ANY:
+ case F_COUNT_:
+
+ /* sum */
+ case F_SUM_INT8:
+ case F_SUM_INT4:
+ case F_SUM_INT2:
+ case F_SUM_FLOAT4:
+ case F_SUM_FLOAT8:
+ case F_SUM_MONEY:
+ case F_SUM_INTERVAL:
+ case F_SUM_NUMERIC:
+
+ /* avg */
+ case F_AVG_INT8:
+ case F_AVG_INT4:
+ case F_AVG_INT2:
+ case F_AVG_NUMERIC:
+ case F_AVG_FLOAT4:
+ case F_AVG_FLOAT8:
+ case F_AVG_INTERVAL:
+
+ return true;
+
+ default:
+ return false;
+ }
+}
+
/*
* CreateIndexOnIMMV
*
@@ -1172,7 +1376,29 @@ CreateIndexOnIMMV(Query *query, Relation matviewRel)
index->concurrent =3D false;
index->if_not_exists =3D false;
=20
- if (query->distinctClause)
+ if (query->groupClause)
+ {
+ /* create unique constraint on GROUP BY expression columns */
+ foreach(lc, query->groupClause)
+ {
+ SortGroupClause *scl =3D (SortGroupClause *) lfirst(lc);
+ TargetEntry *tle =3D get_sortgroupclause_tle(scl, query->targetList);
+ Form_pg_attribute attr =3D TupleDescAttr(matviewRel->rd_att, tle->resno=
- 1);
+ IndexElem *iparam;
+
+ iparam =3D makeNode(IndexElem);
+ iparam->name =3D pstrdup(NameStr(attr->attname));
+ iparam->expr =3D NULL;
+ iparam->indexcolname =3D NULL;
+ iparam->collation =3D NIL;
+ iparam->opclass =3D NIL;
+ iparam->opclassopts =3D NIL;
+ iparam->ordering =3D SORTBY_DEFAULT;
+ iparam->nulls_ordering =3D SORTBY_NULLS_DEFAULT;
+ index->indexParams =3D lappend(index->indexParams, iparam);
+ }
+ }
+ else if (query->distinctClause)
{
/* create unique constraint on all columns */
foreach(lc, query->targetList)
@@ -1230,7 +1456,7 @@ CreateIndexOnIMMV(Query *query, Relation matviewRel)
(errmsg("could not create an index on materialized view \"%s\" automa=
tically",
RelationGetRelationName(matviewRel)),
errdetail("This target list does not have all the primary key column=
s, "
- "or this view does not contain DISTINCT clause."),
+ "or this view does not contain GROUP BY or DISTINCT clause."),
errhint("Create an index on the materialized view for efficient incr=
emental maintenance.")));
return;
}
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index aa518f20ef..ee41f0007d 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -30,6 +30,7 @@
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "commands/cluster.h"
+#include "commands/defrem.h"
#include "commands/matview.h"
#include "commands/tablecmds.h"
#include "commands/tablespace.h"
@@ -39,6 +40,7 @@
#include "executor/tstoreReceiver.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
+#include "optimizer/optimizer.h"
#include "parser/analyze.h"
#include "parser/parse_clause.h"
#include "parser/parse_func.h"
@@ -111,6 +113,13 @@ static HTAB *mv_trigger_info =3D NULL;
=20
static bool in_delta_calculation =3D false;
=20
+/* kind of IVM operation for the view */
+typedef enum
+{
+ IVM_ADD,
+ IVM_SUB
+} IvmOp;
+
/* ENR name for materialized view delta */
#define NEW_DELTA_ENRNAME "new_delta"
#define OLD_DELTA_ENRNAME "old_delta"
@@ -142,7 +151,7 @@ static RangeTblEntry *get_prestate_rte(RangeTblEntry *r=
te, MV_TriggerTable *tabl
QueryEnvironment *queryEnv, Oid matviewid);
static RangeTblEntry *replace_rte_with_delta(RangeTblEntry *rte, MV_Trigge=
rTable *table, bool is_new,
QueryEnvironment *queryEnv);
-static Query *rewrite_query_for_counting(Query *query, ParseState *pstate);
+static Query *rewrite_query_for_counting_and_aggregates(Query *query, Pars=
eState *pstate);
=20
static void calc_delta(MV_TriggerTable *table, int rte_index, Query *query,
DestReceiver *dest_old, DestReceiver *dest_new,
@@ -153,14 +162,27 @@ static Query *rewrite_query_for_postupdate_state(Quer=
y *query, MV_TriggerTable *
static void apply_delta(Oid matviewOid, Tuplestorestate *old_tuplestores, =
Tuplestorestate *new_tuplestores,
TupleDesc tupdesc_old, TupleDesc tupdesc_new,
Query *query, bool use_count, char *count_colname);
+static void append_set_clause_for_count(const char *resname, StringInfo bu=
f_old,
+ StringInfo buf_new,StringInfo aggs_list);
+static void append_set_clause_for_sum(const char *resname, StringInfo buf_=
old,
+ StringInfo buf_new, StringInfo aggs_list);
+static void append_set_clause_for_avg(const char *resname, StringInfo buf_=
old,
+ StringInfo buf_new, StringInfo aggs_list,
+ const char *aggtype);
+static char *get_operation_string(IvmOp op, const char *col, const char *a=
rg1, const char *arg2,
+ const char* count_col, const char *castType);
+static char *get_null_condition_string(IvmOp op, const char *arg1, const c=
har *arg2,
+ const char* count_col);
static void apply_old_delta(const char *matviewname, const char *deltaname=
_old,
List *keys);
static void apply_old_delta_with_count(const char *matviewname, const char=
*deltaname_old,
- List *keys, const char *count_colname);
+ List *keys, StringInfo aggs_list, StringInfo aggs_set,
+ const char *count_colname);
static void apply_new_delta(const char *matviewname, const char *deltaname=
_new,
StringInfo target_list);
static void apply_new_delta_with_count(const char *matviewname, const char=
* deltaname_new,
- List *keys, StringInfo target_list, const char* count_colname);
+ List *keys, StringInfo target_list, StringInfo aggs_set,
+ const char* count_colname);
static char *get_matching_condition_string(List *keys);
static void generate_equal(StringInfo querybuf, Oid opttype,
const char *leftop, const char *rightop);
@@ -1431,11 +1453,44 @@ IVM_immediate_maintenance(PG_FUNCTION_ARGS)
* When a base table is truncated, the view content will be empty if the
* view definition query does not contain an aggregate without a GROUP cl=
ause.
* Therefore, such views can be truncated.
+ *
+ * Aggregate views without a GROUP clause always have one row. Therefore,
+ * if a base table is truncated, the view will not be empty and will cont=
ain
+ * a row with NULL value (or 0 for count()). So, in this case, we refresh=
the
+ * view instead of truncating it.
*/
if (TRIGGER_FIRED_BY_TRUNCATE(trigdata->tg_event))
{
- ExecuteTruncateGuts(list_make1(matviewRel), list_make1_oid(matviewOid),
- NIL, DROP_RESTRICT, false, false);
+ if (!(query->hasAggs && query->groupClause =3D=3D NIL))
+ ExecuteTruncateGuts(list_make1(matviewRel), list_make1_oid(matviewOid),
+ NIL, DROP_RESTRICT, false, false);
+ else
+ {
+ Oid OIDNewHeap;
+ DestReceiver *dest;
+ uint64 processed =3D 0;
+ Query *dataQuery =3D rewriteQueryForIMMV(query, NIL);
+ char relpersistence =3D matviewRel->rd_rel->relpersistence;
+
+ /*
+ * Create the transient table that will receive the regenerated data. L=
ock
+ * it against access by any other process until commit (by which time it
+ * will be gone).
+ */
+ OIDNewHeap =3D make_new_heap(matviewOid, matviewRel->rd_rel->reltablesp=
ace,
+ matviewRel->rd_rel->relam,
+ relpersistence, ExclusiveLock);
+ LockRelationOid(OIDNewHeap, AccessExclusiveLock);
+ dest =3D CreateTransientRelDestReceiver(OIDNewHeap);
+
+ /* Generate the data */
+ processed =3D refresh_matview_datafill(dest, dataQuery, NULL, NULL, "");
+ refresh_by_heap_swap(matviewOid, OIDNewHeap, relpersistence);
+
+ /* Inform cumulative stats system about our activity */
+ pgstat_count_truncate(matviewRel);
+ pgstat_count_heap_insert(matviewRel, processed);
+ }
=20
/* Clean up hash entry and delete tuplestores */
clean_up_IVM_hash_entry(entry, false);
@@ -1475,8 +1530,8 @@ IVM_immediate_maintenance(PG_FUNCTION_ARGS)
/* Set all tables in the query to pre-update state */
rewritten =3D rewrite_query_for_preupdate_state(rewritten, entry->tables,
pstate, matviewOid);
- /* Rewrite for counting duplicated tuples */
- rewritten =3D rewrite_query_for_counting(rewritten, pstate);
+ /* Rewrite for counting duplicated tuples and aggregates functions*/
+ rewritten =3D rewrite_query_for_counting_and_aggregates(rewritten, pstate=
);
=20
/* Create tuplestores to store view deltas */
if (entry->has_old)
@@ -1527,7 +1582,7 @@ IVM_immediate_maintenance(PG_FUNCTION_ARGS)
=20
count_colname =3D pstrdup("__ivm_count__");
=20
- if (query->distinctClause)
+ if (query->hasAggs || query->distinctClause)
use_count =3D true;
=20
/* calculate delta tables */
@@ -1923,17 +1978,34 @@ replace_rte_with_delta(RangeTblEntry *rte, MV_Trigg=
erTable *table, bool is_new,
}
=20
/*
- * rewrite_query_for_counting
+ * rewrite_query_for_counting_and_aggregates
*
- * Rewrite query for counting duplicated tuples.
+ * Rewrite query for counting duplicated tuples and aggregate functions.
*/
static Query *
-rewrite_query_for_counting(Query *query, ParseState *pstate)
+rewrite_query_for_counting_and_aggregates(Query *query, ParseState *pstate)
{
TargetEntry *tle_count;
FuncCall *fn;
Node *node;
=20
+ /* For aggregate views */
+ if (query->hasAggs)
+ {
+ ListCell *lc;
+ List *aggs =3D NIL;
+ AttrNumber next_resno =3D list_length(query->targetList) + 1;
+
+ foreach(lc, query->targetList)
+ {
+ TargetEntry *tle =3D (TargetEntry *) lfirst(lc);
+
+ if (IsA(tle->expr, Aggref))
+ makeIvmAggColumn(pstate, (Aggref *)tle->expr, tle->resname, &next_resn=
o, &aggs);
+ }
+ query->targetList =3D list_concat(query->targetList, aggs);
+ }
+
/* Add count(*) for counting distinct tuples in views */
fn =3D makeFuncCall(SystemFuncName("count"), NIL, COERCE_EXPLICIT_CALL, -=
1);
fn->agg_star =3D true;
@@ -2006,6 +2078,8 @@ rewrite_query_for_postupdate_state(Query *query, MV_T=
riggerTable *table, int rte
return query;
}
=20
+#define IVM_colname(type, col) makeObjectName("__ivm_" type, col, "_")
+
/*
* apply_delta
*
@@ -2019,6 +2093,9 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tupl=
estores, Tuplestorestate *n
{
StringInfoData querybuf;
StringInfoData target_list_buf;
+ StringInfo aggs_list_buf =3D NULL;
+ StringInfo aggs_set_old =3D NULL;
+ StringInfo aggs_set_new =3D NULL;
Relation matviewRel;
char *matviewname;
ListCell *lc;
@@ -2041,6 +2118,15 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tup=
lestores, Tuplestorestate *n
initStringInfo(&querybuf);
initStringInfo(&target_list_buf);
=20
+ if (query->hasAggs)
+ {
+ if (old_tuplestores && tuplestore_tuple_count(old_tuplestores) > 0)
+ aggs_set_old =3D makeStringInfo();
+ if (new_tuplestores && tuplestore_tuple_count(new_tuplestores) > 0)
+ aggs_set_new =3D makeStringInfo();
+ aggs_list_buf =3D makeStringInfo();
+ }
+
/* build string of target list */
for (i =3D 0; i < matviewRel->rd_att->natts; i++)
{
@@ -2057,13 +2143,61 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tu=
plestores, Tuplestorestate *n
{
TargetEntry *tle =3D (TargetEntry *) lfirst(lc);
Form_pg_attribute attr =3D TupleDescAttr(matviewRel->rd_att, i);
+ char *resname =3D NameStr(attr->attname);
=20
i++;
=20
if (tle->resjunk)
continue;
=20
- keys =3D lappend(keys, attr);
+ /*
+ * For views without aggregates, all attributes are used as keys to iden=
tify a
+ * tuple in a view.
+ */
+ if (!query->hasAggs)
+ keys =3D lappend(keys, attr);
+
+ /* For views with aggregates, we need to build SET clause for updating a=
ggregate
+ * values. */
+ if (query->hasAggs && IsA(tle->expr, Aggref))
+ {
+ Aggref *aggref =3D (Aggref *) tle->expr;
+ const char *aggname =3D get_func_name(aggref->aggfnoid);
+
+ /*
+ * We can use function names here because it is already checked if these
+ * can be used in IMMV by its OID at the definition time.
+ */
+
+ /* count */
+ if (!strcmp(aggname, "count"))
+ append_set_clause_for_count(resname, aggs_set_old, aggs_set_new, aggs_=
list_buf);
+
+ /* sum */
+ else if (!strcmp(aggname, "sum"))
+ append_set_clause_for_sum(resname, aggs_set_old, aggs_set_new, aggs_li=
st_buf);
+
+ /* avg */
+ else if (!strcmp(aggname, "avg"))
+ append_set_clause_for_avg(resname, aggs_set_old, aggs_set_new, aggs_li=
st_buf,
+ format_type_be(aggref->aggtype));
+
+ else
+ elog(ERROR, "unsupported aggregate function: %s", aggname);
+ }
+ }
+
+ /* If we have GROUP BY clause, we use its entries as keys. */
+ if (query->hasAggs && query->groupClause)
+ {
+ foreach (lc, query->groupClause)
+ {
+ SortGroupClause *sgcl =3D (SortGroupClause *) lfirst(lc);
+ TargetEntry *tle =3D get_sortgroupclause_tle(sgcl, query->targetList);
+ Form_pg_attribute attr =3D TupleDescAttr(matviewRel->rd_att, tle->resno=
- 1);
+
+ keys =3D lappend(keys, attr);
+ }
}
=20
/* Start maintaining the materialized view. */
@@ -2094,7 +2228,8 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tupl=
estores, Tuplestorestate *n
if (use_count)
/* apply old delta and get rows to be recalculated */
apply_old_delta_with_count(matviewname, OLD_DELTA_ENRNAME,
- keys, count_colname);
+ keys, aggs_list_buf, aggs_set_old,
+ count_colname);
else
apply_old_delta(matviewname, OLD_DELTA_ENRNAME, keys);
=20
@@ -2120,7 +2255,7 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tupl=
estores, Tuplestorestate *n
/* apply new delta */
if (use_count)
apply_new_delta_with_count(matviewname, NEW_DELTA_ENRNAME,
- keys, &target_list_buf, count_colname);
+ keys, aggs_set_new, &target_list_buf, count_colname);
else
apply_new_delta(matviewname, NEW_DELTA_ENRNAME, &target_list_buf);
}
@@ -2135,6 +2270,250 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tu=
plestores, Tuplestorestate *n
elog(ERROR, "SPI_finish failed");
}
=20
+/*
+ * append_set_clause_for_count
+ *
+ * Append SET clause string for count aggregation to given buffers.
+ * Also, append resnames required for calculating the aggregate value.
+ */
+static void
+append_set_clause_for_count(const char *resname, StringInfo buf_old,
+ StringInfo buf_new,StringInfo aggs_list)
+{
+ /* For tuple deletion */
+ if (buf_old)
+ {
+ /* resname =3D mv.resname - t.resname */
+ appendStringInfo(buf_old,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, resname),
+ get_operation_string(IVM_SUB, resname, "mv", "t", NULL, NULL));
+ }
+ /* For tuple insertion */
+ if (buf_new)
+ {
+ /* resname =3D mv.resname + diff.resname */
+ appendStringInfo(buf_new,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, resname),
+ get_operation_string(IVM_ADD, resname, "mv", "diff", NULL, NULL));
+ }
+
+ appendStringInfo(aggs_list, ", %s",
+ quote_qualified_identifier("diff", resname)
+ );
+}
+
+/*
+ * append_set_clause_for_sum
+ *
+ * Append SET clause string for sum aggregation to given buffers.
+ * Also, append resnames required for calculating the aggregate value.
+ */
+static void
+append_set_clause_for_sum(const char *resname, StringInfo buf_old,
+ StringInfo buf_new, StringInfo aggs_list)
+{
+ char *count_col =3D IVM_colname("count", resname);
+
+ /* For tuple deletion */
+ if (buf_old)
+ {
+ /* sum =3D mv.sum - t.sum */
+ appendStringInfo(buf_old,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, resname),
+ get_operation_string(IVM_SUB, resname, "mv", "t", count_col, NULL)
+ );
+ /* count =3D mv.count - t.count */
+ appendStringInfo(buf_old,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, count_col),
+ get_operation_string(IVM_SUB, count_col, "mv", "t", NULL, NULL)
+ );
+ }
+ /* For tuple insertion */
+ if (buf_new)
+ {
+ /* sum =3D mv.sum + diff.sum */
+ appendStringInfo(buf_new,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, resname),
+ get_operation_string(IVM_ADD, resname, "mv", "diff", count_col, NULL)
+ );
+ /* count =3D mv.count + diff.count */
+ appendStringInfo(buf_new,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, count_col),
+ get_operation_string(IVM_ADD, count_col, "mv", "diff", NULL, NULL)
+ );
+ }
+
+ appendStringInfo(aggs_list, ", %s, %s",
+ quote_qualified_identifier("diff", resname),
+ quote_qualified_identifier("diff", IVM_colname("count", resname))
+ );
+}
+
+/*
+ * append_set_clause_for_avg
+ *
+ * Append SET clause string for avg aggregation to given buffers.
+ * Also, append resnames required for calculating the aggregate value.
+ */
+static void
+append_set_clause_for_avg(const char *resname, StringInfo buf_old,
+ StringInfo buf_new, StringInfo aggs_list,
+ const char *aggtype)
+{
+ char *sum_col =3D IVM_colname("sum", resname);
+ char *count_col =3D IVM_colname("count", resname);
+
+ /* For tuple deletion */
+ if (buf_old)
+ {
+ /* avg =3D (mv.sum - t.sum)::aggtype / (mv.count - t.count) */
+ appendStringInfo(buf_old,
+ ", %s =3D %s OPERATOR(pg_catalog./) %s",
+ quote_qualified_identifier(NULL, resname),
+ get_operation_string(IVM_SUB, sum_col, "mv", "t", count_col, aggtype),
+ get_operation_string(IVM_SUB, count_col, "mv", "t", NULL, NULL)
+ );
+ /* sum =3D mv.sum - t.sum */
+ appendStringInfo(buf_old,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, sum_col),
+ get_operation_string(IVM_SUB, sum_col, "mv", "t", count_col, NULL)
+ );
+ /* count =3D mv.count - t.count */
+ appendStringInfo(buf_old,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, count_col),
+ get_operation_string(IVM_SUB, count_col, "mv", "t", NULL, NULL)
+ );
+
+ }
+ /* For tuple insertion */
+ if (buf_new)
+ {
+ /* avg =3D (mv.sum + diff.sum)::aggtype / (mv.count + diff.count) */
+ appendStringInfo(buf_new,
+ ", %s =3D %s OPERATOR(pg_catalog./) %s",
+ quote_qualified_identifier(NULL, resname),
+ get_operation_string(IVM_ADD, sum_col, "mv", "diff", count_col, aggtype=
),
+ get_operation_string(IVM_ADD, count_col, "mv", "diff", NULL, NULL)
+ );
+ /* sum =3D mv.sum + diff.sum */
+ appendStringInfo(buf_new,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, sum_col),
+ get_operation_string(IVM_ADD, sum_col, "mv", "diff", count_col, NULL)
+ );
+ /* count =3D mv.count + diff.count */
+ appendStringInfo(buf_new,
+ ", %s =3D %s",
+ quote_qualified_identifier(NULL, count_col),
+ get_operation_string(IVM_ADD, count_col, "mv", "diff", NULL, NULL)
+ );
+ }
+
+ appendStringInfo(aggs_list, ", %s, %s, %s",
+ quote_qualified_identifier("diff", resname),
+ quote_qualified_identifier("diff", IVM_colname("sum", resname)),
+ quote_qualified_identifier("diff", IVM_colname("count", resname))
+ );
+}
+
+/*
+ * get_operation_string
+ *
+ * Build a string to calculate the new aggregate values.
+ */
+static char *
+get_operation_string(IvmOp op, const char *col, const char *arg1, const ch=
ar *arg2,
+ const char* count_col, const char *castType)
+{
+ StringInfoData buf;
+ StringInfoData castString;
+ char *col1 =3D quote_qualified_identifier(arg1, col);
+ char *col2 =3D quote_qualified_identifier(arg2, col);
+ char op_char =3D (op =3D=3D IVM_SUB ? '-' : '+');
+
+ initStringInfo(&buf);
+ initStringInfo(&castString);
+
+ if (castType)
+ appendStringInfo(&castString, "::%s", castType);
+
+ if (!count_col)
+ {
+ /*
+ * If the attributes don't have count columns then calc the result
+ * by using the operator simply.
+ */
+ appendStringInfo(&buf, "(%s OPERATOR(pg_catalog.%c) %s)%s",
+ col1, op_char, col2, castString.data);
+ }
+ else
+ {
+ /*
+ * If the attributes have count columns then consider the condition
+ * where the result becomes NULL.
+ */
+ char *null_cond =3D get_null_condition_string(op, arg1, arg2, count_col);
+
+ appendStringInfo(&buf,
+ "(CASE WHEN %s THEN NULL "
+ "WHEN %s IS NULL THEN %s "
+ "WHEN %s IS NULL THEN %s "
+ "ELSE (%s OPERATOR(pg_catalog.%c) %s)%s END)",
+ null_cond,
+ col1, col2,
+ col2, col1,
+ col1, op_char, col2, castString.data
+ );
+ }
+
+ return buf.data;
+}
+
+/*
+ * get_null_condition_string
+ *
+ * Build a predicate string for CASE clause to check if an aggregate value
+ * will became NULL after the given operation is applied.
+ */
+static char *
+get_null_condition_string(IvmOp op, const char *arg1, const char *arg2,
+ const char* count_col)
+{
+ StringInfoData null_cond;
+ initStringInfo(&null_cond);
+
+ switch (op)
+ {
+ case IVM_ADD:
+ appendStringInfo(&null_cond,
+ "%s OPERATOR(pg_catalog.=3D) 0 AND %s OPERATOR(pg_catalog.=3D) 0",
+ quote_qualified_identifier(arg1, count_col),
+ quote_qualified_identifier(arg2, count_col)
+ );
+ break;
+ case IVM_SUB:
+ appendStringInfo(&null_cond,
+ "%s OPERATOR(pg_catalog.=3D) %s",
+ quote_qualified_identifier(arg1, count_col),
+ quote_qualified_identifier(arg2, count_col)
+ );
+ break;
+ default:
+ elog(ERROR,"unknown operation");
+ }
+
+ return null_cond.data;
+}
+
+
/*
* apply_old_delta_with_count
*
@@ -2142,13 +2521,20 @@ apply_delta(Oid matviewOid, Tuplestorestate *old_tu=
plestores, Tuplestorestate *n
* which contains tuples to be deleted from to a materialized view given by
* matviewname. This is used when counting is required, that is, the view
* has aggregate or distinct.
+ *
+ * If the view desn't have aggregates or has GROUP BY, this requires a keys
+ * list to identify a tuple in the view. If the view has aggregates, this
+ * requires strings representing resnames of aggregates and SET clause for
+ * updating aggregate values.
*/
static void
apply_old_delta_with_count(const char *matviewname, const char *deltaname_=
old,
- List *keys, const char *count_colname)
+ List *keys, StringInfo aggs_list, StringInfo aggs_set,
+ const char *count_colname)
{
StringInfoData querybuf;
char *match_cond;
+ bool agg_without_groupby =3D (list_length(keys) =3D=3D 0);
=20
/* build WHERE condition for searching tuples to be deleted */
match_cond =3D get_matching_condition_string(keys);
@@ -2158,22 +2544,26 @@ apply_old_delta_with_count(const char *matviewname,=
const char *deltaname_old,
appendStringInfo(&querybuf,
"WITH t AS (" /* collecting tid of target tuples in the view */
"SELECT diff.%s, " /* count column */
- "(diff.%s OPERATOR(pg_catalog.=3D) mv.%s) AS for_dlt, "
+ "(diff.%s OPERATOR(pg_catalog.=3D) mv.%s AND %s) AS for_dlt, "
"mv.ctid "
+ "%s " /* aggregate columns */
"FROM %s AS mv, %s AS diff "
"WHERE %s" /* tuple matching condition */
"), updt AS (" /* update a tuple if this is not to be deleted */
"UPDATE %s AS mv SET %s =3D mv.%s OPERATOR(pg_catalog.-) t.%s "
+ "%s" /* SET clauses for aggregates */
"FROM t WHERE mv.ctid OPERATOR(pg_catalog.=3D) t.ctid AND NOT for_dl=
t "
")"
/* delete a tuple if this is to be deleted */
"DELETE FROM %s AS mv USING t "
"WHERE mv.ctid OPERATOR(pg_catalog.=3D) t.ctid AND for_dlt",
count_colname,
- count_colname, count_colname,
+ count_colname, count_colname, (agg_without_groupby ? "false" : "true"=
),
+ (aggs_list !=3D NULL ? aggs_list->data : ""),
matviewname, deltaname_old,
match_cond,
matviewname, count_colname, count_colname, count_colname,
+ (aggs_set !=3D NULL ? aggs_set->data : ""),
matviewname);
=20
if (SPI_exec(querybuf.data, 0) !=3D SPI_OK_DELETE)
@@ -2237,10 +2627,15 @@ apply_old_delta(const char *matviewname, const char=
*deltaname_old,
* matviewname. This is used when counting is required, that is, the view
* has aggregate or distinct. Also, when a table in EXISTS sub queries
* is modified.
+ *
+ * If the view desn't have aggregates or has GROUP BY, this requires a keys
+ * list to identify a tuple in the view. If the view has aggregates, this
+ * requires strings representing SET clause for updating aggregate values.
*/
static void
apply_new_delta_with_count(const char *matviewname, const char* deltaname_=
new,
- List *keys, StringInfo target_list, const char* count_colname)
+ List *keys, StringInfo aggs_set, StringInfo target_list,
+ const char* count_colname)
{
StringInfoData querybuf;
StringInfoData returning_keys;
@@ -2271,6 +2666,7 @@ apply_new_delta_with_count(const char *matviewname, c=
onst char* deltaname_new,
appendStringInfo(&querybuf,
"WITH updt AS (" /* update a tuple if this exists in the view */
"UPDATE %s AS mv SET %s =3D mv.%s OPERATOR(pg_catalog.+) diff.%s "
+ "%s " /* SET clauses for aggregates */
"FROM %s AS diff "
"WHERE %s " /* tuple matching condition */
"RETURNING %s" /* returning keys of updated tuples */
@@ -2278,6 +2674,7 @@ apply_new_delta_with_count(const char *matviewname, c=
onst char* deltaname_new,
"SELECT %s FROM %s AS diff "
"WHERE NOT EXISTS (SELECT 1 FROM updt AS mv WHERE %s);",
matviewname, count_colname, count_colname, count_colname,
+ (aggs_set !=3D NULL ? aggs_set->data : ""),
deltaname_new,
match_cond,
returning_keys.data,
diff --git a/src/include/commands/createas.h b/src/include/commands/createa=
s.h
index 76a7873ebf..599bae3b5a 100644
--- a/src/include/commands/createas.h
+++ b/src/include/commands/createas.h
@@ -30,6 +30,7 @@ extern void CreateIvmTriggersOnBaseTables(Query *qry, Oid=
matviewOid);
extern void CreateIndexOnIMMV(Query *query, Relation matviewRel);
=20
extern Query *rewriteQueryForIMMV(Query *query, List *colNames);
+extern void makeIvmAggColumn(ParseState *pstate, Aggref *aggref, char *res=
name, AttrNumber *next_resno, List **aggs);
=20
extern int GetIntoRelEFlags(IntoClause *intoClause);
=20
--=20
2.25.1
--Multipart=_Mon__28_Aug_2023_16_05_30_+0900_b1OvQD_3A3ZMTGvj
Content-Type: text/x-diff;
name="v29-0009-Add-support-for-min-max-aggregates-for-IVM.patch"
Content-Disposition: attachment;
filename="v29-0009-Add-support-for-min-max-aggregates-for-IVM.patch"
Content-Transfer-Encoding: 7bit
^ permalink raw reply [nested|flat] 23+ messages in thread
end of thread, other threads:[~2023-05-31 11:46 UTC | newest]
Thread overview: 23+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-08-25 18:45 [PATCH] Fix handling of ? option Quentin Rameau <[email protected]>
2019-08-25 18:45 [PATCH] Fix handling of ? option Quentin Rameau <[email protected]>
2021-12-23 15:05 Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2021-12-24 05:51 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Kyotaro Horiguchi <[email protected]>
2021-12-24 12:07 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2021-12-24 12:11 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Michael Paquier <[email protected]>
2021-12-24 12:24 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2021-12-28 02:56 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2022-02-01 04:34 [PATCH v10 2/5] Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2022-02-01 04:34 [PATCH v9 2/4] Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2022-02-07 03:02 Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Kyotaro Horiguchi <[email protected]>
2022-02-07 04:51 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Fujii Masao <[email protected]>
2022-02-08 07:58 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Kyotaro Horiguchi <[email protected]>
2022-02-08 08:40 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Kyotaro Horiguchi <[email protected]>
2022-02-08 09:03 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2022-03-04 04:22 [PATCH v11 2/5] Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2022-03-04 04:22 [PATCH v12 2/5] Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2022-03-04 04:22 [PATCH v13] Add checkpoint and redo LSN to LogCheckpointEnd log message Bharath Rupireddy <[email protected]>
2022-03-28 19:31 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Greg Stark <[email protected]>
2022-07-06 16:11 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Fujii Masao <[email protected]>
2022-07-07 07:26 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Kyotaro Horiguchi <[email protected]>
2022-07-07 13:45 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Fujii Masao <[email protected]>
2023-05-31 11:46 [PATCH v29 08/11] Add aggregates support in IVM Yugo Nagata <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox