public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v6 5/7] Invalidate parent index cluster on attach
5+ messages / 3 participants
[nested] [flat]
* [PATCH v6 5/7] Invalidate parent index cluster on attach
@ 2020-11-06 01:11 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Justin Pryzby @ 2020-11-06 01:11 UTC (permalink / raw)
---
src/backend/commands/indexcmds.c | 21 +++++++++++++++++++++
src/test/regress/expected/cluster.out | 14 ++++++++++++++
src/test/regress/sql/cluster.sql | 5 +++++
3 files changed, 40 insertions(+)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 0a65698c28..ffc809822b 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -4006,6 +4006,27 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
/* set relispartition correctly on the partition */
update_relispartition(partRelid, OidIsValid(parentOid));
+ /*
+ * If the attached index is not clustered, invalidate cluster mark on
+ * any parents
+ */
+ if ((OidIsValid(parentOid) && get_index_isclustered(parentOid)) ||
+ get_index_isclustered(partRelid))
+ {
+ Relation indrel;
+
+ /* Make relispartition visible */
+ CommandCounterIncrement();
+
+ indrel = table_open(IndexGetRelation(partRelid, false),
+ ShareUpdateExclusiveLock);
+ mark_index_clustered(indrel,
+ get_index_isclustered(partRelid) ? partRelid : InvalidOid,
+ true);
+ table_close(indrel, ShareUpdateExclusiveLock);
+
+ }
+
if (fix_dependencies)
{
/*
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 6d88978387..f0c962db75 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -567,6 +567,20 @@ Indexes:
"clstrpart_idx" btree (a)
Number of partitions: 3 (Use \d+ to list them.)
+-- Check that attaching an unclustered index marks the parent unclustered:
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+CREATE TABLE clstrpart5 (LIKE clstrpart INCLUDING INDEXES);
+ALTER TABLE clstrpart ATTACH PARTITION clstrpart5 FOR VALUES FROM (40)TO(50);
+\d clstrpart
+ Partitioned table "public.clstrpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart_idx" btree (a)
+Number of partitions: 4 (Use \d+ to list them.)
+
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
create index cluster_sort on clstr_4 (hundred, thousand, tenthous);
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index c9bb204a93..ff7ffed6e4 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -239,6 +239,11 @@ CLUSTER clstrpart1 USING clstrpart1_idx_2;
ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
ALTER TABLE clstrpart1 SET WITHOUT CLUSTER;
\d clstrpart
+-- Check that attaching an unclustered index marks the parent unclustered:
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+CREATE TABLE clstrpart5 (LIKE clstrpart INCLUDING INDEXES);
+ALTER TABLE clstrpart ATTACH PARTITION clstrpart5 FOR VALUES FROM (40)TO(50);
+\d clstrpart
-- Test CLUSTER with external tuplesorting
--
2.17.0
--FsscpQKzF/jJk6ya
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0006-Preserve-indisclustered-on-children-of-clustered-.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-10 16:00 Fujii Masao <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Fujii Masao @ 2022-02-10 16:00 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/02/09 11:52, Kyotaro Horiguchi wrote:
> 0001: The fix of CreateRestartPoint
This patch might be ok for the master branch. But since concurrent checkpoint and restartpoint can happen in v14 or before, we would need another patch based on that assumption, for the backport. How about fixing the bug all the branches at first, then apply this patch in the master to improve the code?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-14 05:40 Kyotaro Horiguchi <[email protected]>
parent: Fujii Masao <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-14 05:40 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Fri, 11 Feb 2022 01:00:03 +0900, Fujii Masao <[email protected]> wrote in
>
>
> On 2022/02/09 11:52, Kyotaro Horiguchi wrote:
> > 0001: The fix of CreateRestartPoint
>
> This patch might be ok for the master branch. But since concurrent
> checkpoint and restartpoint can happen in v14 or before, we would need
> another patch based on that assumption, for the backport. How about
> fixing the bug all the branches at first, then apply this patch in the
> master to improve the code?
For backbranches, the attached for pg14 does part of the full patch.
Of the following, I think we should do (a) and (b) to make future
backpatchings easier.
a) Use RedoRecPtr and PriorRedoPtr after they are assigned.
b) Move assignment to PriorRedoPtr into the ControlFileLock section.
c) Skip udpate of minRecoveryPoint only when the checkpoint gets old.
d) Skip call to UpdateCheckPointDistanceEstimate() when RedoRecPtr <=
PriorRedoPtr.
# Mmm. The v9-0001 contains a silly mistake here..
Still I'm not sure whether that case really happens and how checkpoint
behaves *after* that happenes, but at least it protects database from
the possible unrecoverable state due to the known issue here..
It doesn't apply even on pg13 (due to LSN_FORMAT_ARGS). I will make
the per-version patches if you are fine with this.
regards.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
[text/x-patch] v2-0001-Correctly-update-contfol-file-at-the-end-of-archi.patch (6.8K, ../../[email protected]/2-v2-0001-Correctly-update-contfol-file-at-the-end-of-archi.patch)
download | inline diff:
From df516b487617c570332ea076b91773b810629a11 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
Date: Mon, 14 Feb 2022 13:04:33 +0900
Subject: [PATCH v2] Correctly update contfol file at the end of archive
recovery
CreateRestartPoint runs WAL file cleanup basing on the checkpoint just
have finished in the function. If the database has exited
DB_IN_ARCHIVE_RECOVERY state when the function is going to update
control file, the function refrains from updating the file at all then
proceeds to WAL cleanup having the latest REDO LSN, which is now
inconsistent with the control file. As the result, the succeeding
cleanup procedure overly removes WAL files against the control file
and leaves unrecoverable database until the next checkpoint finishes.
Since all buffers have flushed out, we may safely regard that restart
point established regardless of the recovery state. Thus we may and
should update checkpoint LSNs of checkpoint file in that case. Still
we update minRecoveryPoint only during archive recovery but explicitly
clear if we have exited recovery.
Addition to that fix, this commit makes some cosmetic changes that
consist with the changes we are going to make on the master branch.
---
src/backend/access/transam/xlog.c | 81 ++++++++++++++++++++-----------
1 file changed, 53 insertions(+), 28 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6208e123e5..28c3c4b7cf 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9653,7 +9653,7 @@ CreateRestartPoint(int flags)
/* Also update the info_lck-protected copy */
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->RedoRecPtr = lastCheckPoint.redo;
+ XLogCtl->RedoRecPtr = RedoRecPtr;
SpinLockRelease(&XLogCtl->info_lck);
/*
@@ -9672,7 +9672,10 @@ CreateRestartPoint(int flags)
/* Update the process title */
update_checkpoint_display(flags, true, false);
- CheckPointGuts(lastCheckPoint.redo, flags);
+ CheckPointGuts(RedoRecPtr, flags);
+
+ /* Update pg_control */
+ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
/*
* Remember the prior checkpoint's redo ptr for
@@ -9681,52 +9684,74 @@ 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 (PriorRedoPtr < RedoRecPtr)
{
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
ControlFile->time = (pg_time_t) time(NULL);
/*
- * 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.
+ * This is a quick hack to make sure nothing really bad happens if
+ * somehow we get here after the end-of-recovery checkpoint.
*/
- 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;
}
- if (flags & CHECKPOINT_IS_SHUTDOWN)
- ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
+ else
+ {
+ /*
+ * Aarchive recovery has ended. Crash recovery ever after should
+ * always recover to the end of WAL
+ */
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
+ }
+
UpdateControlFile();
}
LWLockRelease(ControlFileLock);
/*
* Update the average distance between checkpoints/restartpoints if the
- * prior checkpoint exists.
+ * prior checkpoint exists and we have advanced REDO LSN.
*/
- if (PriorRedoPtr != InvalidXLogRecPtr)
+ if (PriorRedoPtr != InvalidXLogRecPtr && RedoRecPtr > PriorRedoPtr)
UpdateCheckPointDistanceEstimate(RedoRecPtr - PriorRedoPtr);
+ /*
+ * Now we can safely regard this restart point as established.
+ *
+ * Since all buffers have been flushed so we regard this restart point as
+ * established. We could omit some of the following steps in the case
+ * where we have omitted control file updates, but we don't bother avoid
+ * them from performing since that case rarely happens and they don't harm
+ * even if they take place.
+ */
+
/*
* Delete old log files, those no longer needed for last restartpoint to
* prevent the disk holding the xlog from growing full.
@@ -9804,7 +9829,7 @@ CreateRestartPoint(int flags)
xtime = GetLatestXTime();
ereport((log_checkpoints ? LOG : DEBUG2),
(errmsg("recovery restart point at %X/%X",
- LSN_FORMAT_ARGS(lastCheckPoint.redo)),
+ LSN_FORMAT_ARGS(RedoRecPtr)),
xtime ? errdetail("Last completed transaction was at log time %s.",
timestamptz_to_str(xtime)) : 0));
--
2.27.0
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-14 05:42 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Kyotaro Horiguchi @ 2022-02-14 05:42 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 14 Feb 2022 14:40:22 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> It doesn't apply even on pg13 (due to LSN_FORMAT_ARGS). I will make
> the per-version patches if you are fine with this.
Oops! I forgot to rename the patch to avoid confusion on CF-bots.
I'll resend new version soon to avoid the confusion..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
@ 2022-02-21 16:59 Fujii Masao <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Fujii Masao @ 2022-02-21 16:59 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/14 14:40, Kyotaro Horiguchi wrote:
> For backbranches, the attached for pg14 does part of the full patch.
Thanks for updating the patch!
> Of the following, I think we should do (a) and (b) to make future
> backpatchings easier.
>
> a) Use RedoRecPtr and PriorRedoPtr after they are assigned.
>
> b) Move assignment to PriorRedoPtr into the ControlFileLock section.
I failed to understand how (a) and (b) can make the backpatching easier. How easy to backpatch seems the same whether we apply (a) and (b) or not...
> c) Skip udpate of minRecoveryPoint only when the checkpoint gets old.
Yes.
> d) Skip call to UpdateCheckPointDistanceEstimate() when RedoRecPtr <=
> PriorRedoPtr.
But "RedoRecPtr <= PriorRedoPtr" will never happen, will it? Because a restartpoint is skipped at the beginning of CreateRestartPoint() in that case. If this understanding is right, the check of "RedoRecPtr <= PriorRedoPtr" is not necessary before calling UpdateCheckPointDistanceEstimate().
+ ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+ ControlFile->minRecoveryPointTLI = 0;
Don't we need to update LocalMinRecoveryPoint and LocalMinRecoveryPointTLI after this? Maybe it's not necessary, but ISTM that it's safer and better to always update them whether the state is DB_IN_ARCHIVE_RECOVERY or not.
if (flags & CHECKPOINT_IS_SHUTDOWN)
ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;
Same as above. IMO it's safer and better to always update the state (whether the state is DB_IN_ARCHIVE_RECOVERY or not) if CHECKPOINT_IS_SHUTDOWN flag is passed.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2022-02-21 16:59 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-06 01:11 [PATCH v6 5/7] Invalidate parent index cluster on attach Justin Pryzby <[email protected]>
2022-02-10 16:00 Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Fujii Masao <[email protected]>
2022-02-14 05:40 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Kyotaro Horiguchi <[email protected]>
2022-02-14 05:42 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Kyotaro Horiguchi <[email protected]>
2022-02-21 16:59 ` Re: Add checkpoint and redo LSN to LogCheckpointEnd log message Fujii Masao <[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