public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v8 4/8] Propagate changes to indisclustered to child/parents
4+ messages / 4 participants
[nested] [flat]
* [PATCH v8 4/8] Propagate changes to indisclustered to child/parents
@ 2020-10-07 03:11 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Justin Pryzby @ 2020-10-07 03:11 UTC (permalink / raw)
---
src/backend/commands/cluster.c | 109 ++++++++++++++++----------
src/backend/commands/indexcmds.c | 2 +
src/test/regress/expected/cluster.out | 46 +++++++++++
src/test/regress/sql/cluster.sql | 11 +++
4 files changed, 125 insertions(+), 43 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index cb4fc350c6..5c08f0642e 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -74,6 +74,7 @@ static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
+static void set_indisclustered(Oid indexOid, bool isclustered, Relation pg_index);
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
@@ -516,66 +517,88 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
index_close(OldIndex, NoLock);
}
+/*
+ * Helper for mark_index_clustered
+ * Mark a single index as clustered or not.
+ * pg_index is passed by caller to avoid repeatedly re-opening it.
+ */
+static void
+set_indisclustered(Oid indexOid, bool isclustered, Relation pg_index)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ indexTuple = SearchSysCacheCopy1(INDEXRELID,
+ ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ /* this was checked earlier, but let's be real sure */
+ if (isclustered && !indexForm->indisvalid)
+ elog(ERROR, "cannot cluster on invalid index %u", indexOid);
+
+ indexForm->indisclustered = isclustered;
+ CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
+ heap_freetuple(indexTuple);
+}
+
/*
* mark_index_clustered: mark the specified index as the one clustered on
*
- * With indexOid == InvalidOid, will mark all indexes of rel not-clustered.
+ * With indexOid == InvalidOid, mark all indexes of rel not-clustered.
+ * Otherwise, mark children of the clustered index as clustered, and parents of
+ * other indexes as unclustered.
+ * We wish to maintain the following properties:
+ * 1) Only one index on a relation can be marked clustered at once
+ * 2) If a partitioned index is clustered, then all its children must be
+ * clustered.
*/
void
mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
{
- HeapTuple indexTuple;
- Form_pg_index indexForm;
- Relation pg_index;
- ListCell *index;
-
- /*
- * If the index is already marked clustered, no need to do anything.
- */
- if (OidIsValid(indexOid))
- {
- if (get_index_isclustered(indexOid))
- return;
- }
+ ListCell *lc, *lc2;
+ List *indexes;
+ Relation pg_index = table_open(IndexRelationId, RowExclusiveLock);
+ List *inh = find_all_inheritors(RelationGetRelid(rel), ShareRowExclusiveLock, NULL);
/*
* Check each index of the relation and set/clear the bit as needed.
+ * Iterate over the relation's children rather than the index's children
+ * since we need to unset cluster for indexes on intermediate children,
+ * too.
*/
- pg_index = table_open(IndexRelationId, RowExclusiveLock);
-
- foreach(index, RelationGetIndexList(rel))
+ foreach(lc, inh)
{
- Oid thisIndexOid = lfirst_oid(index);
-
- indexTuple = SearchSysCacheCopy1(INDEXRELID,
- ObjectIdGetDatum(thisIndexOid));
- if (!HeapTupleIsValid(indexTuple))
- elog(ERROR, "cache lookup failed for index %u", thisIndexOid);
- indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+ Oid inhrelid = lfirst_oid(lc);
+ Relation thisrel = table_open(inhrelid, ShareRowExclusiveLock);
- /*
- * Unset the bit if set. We know it's wrong because we checked this
- * earlier.
- */
- if (indexForm->indisclustered)
+ indexes = RelationGetIndexList(thisrel);
+ foreach (lc2, indexes)
{
- indexForm->indisclustered = false;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
- }
- else if (thisIndexOid == indexOid)
- {
- /* this was checked earlier, but let's be real sure */
- if (!indexForm->indisvalid)
- elog(ERROR, "cannot cluster on invalid index %u", indexOid);
- indexForm->indisclustered = true;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
- }
+ bool isclustered;
+ Oid thisIndexOid = lfirst_oid(lc2);
+ List *parentoids = get_rel_relispartition(thisIndexOid) ?
+ get_partition_ancestors(thisIndexOid) : NIL;
- InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
- InvalidOid, is_internal);
+ /*
+ * A child of the clustered index must be set clustered;
+ * indexes which are not children of the clustered index are
+ * set unclustered
+ */
+ isclustered = (thisIndexOid == indexOid) ||
+ list_member_oid(parentoids, indexOid);
+ Assert(OidIsValid(indexOid) || !isclustered);
+ set_indisclustered(thisIndexOid, isclustered, pg_index);
+
+ InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
+ InvalidOid, is_internal);
+ }
- heap_freetuple(indexTuple);
+ list_free(indexes);
+ table_close(thisrel, ShareRowExclusiveLock);
}
+ list_free(inh);
table_close(pg_index, RowExclusiveLock);
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 127ba7835d..4ca1ffbfa4 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -25,6 +25,7 @@
#include "catalog/catalog.h"
#include "catalog/index.h"
#include "catalog/indexing.h"
+#include "catalog/partition.h"
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
@@ -32,6 +33,7 @@
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
#include "catalog/pg_type.h"
+#include "commands/cluster.h"
#include "commands/comment.h"
#include "commands/dbcommands.h"
#include "commands/defrem.h"
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index c74cfa88cc..1d436dfaae 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -495,6 +495,52 @@ Indexes:
"clstrpart_idx" btree (a) CLUSTER
Number of partitions: 3 (Use \d+ to list them.)
+-- Test that it recurses to grandchildren:
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a) CLUSTER
+
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a)
+
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+\d clstrpart33
+ Table "public.clstrpart33"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart3 DEFAULT
+Indexes:
+ "clstrpart33_a_idx" btree (a) CLUSTER
+
+-- Check that only one child is marked clustered after marking clustered on a different parent
+CREATE INDEX clstrpart1_idx_2 ON clstrpart1(a);
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+ALTER TABLE clstrpart1 CLUSTER ON clstrpart1_idx_2;
+\d clstrpart1
+ Partitioned table "public.clstrpart1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition of: clstrpart FOR VALUES FROM (1) TO (10)
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart1_a_idx" btree (a)
+ "clstrpart1_idx_2" btree (a) CLUSTER
+Number of partitions: 2 (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 9bcc77695c..0ded2be1ca 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -220,6 +220,17 @@ CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitio
CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
\d clstrpart
+-- Test that it recurses to grandchildren:
+\d clstrpart33
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+\d clstrpart33
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+\d clstrpart33
+-- Check that only one child is marked clustered after marking clustered on a different parent
+CREATE INDEX clstrpart1_idx_2 ON clstrpart1(a);
+ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+ALTER TABLE clstrpart1 CLUSTER ON clstrpart1_idx_2;
+\d clstrpart1
-- Test CLUSTER with external tuplesorting
--
2.17.0
--O5XBE6gyVG5Rl6Rj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0005-Invalidate-parent-indexes.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Add 64-bit XIDs into PostgreSQL 15
@ 2022-01-06 13:15 Finnerty, Jim <[email protected]>
2022-01-06 14:09 ` Re: Add 64-bit XIDs into PostgreSQL 15 Simon Riggs <[email protected]>
2022-01-07 03:35 ` Re: Add 64-bit XIDs into PostgreSQL 15 Alexander Korotkov <[email protected]>
0 siblings, 2 replies; 4+ messages in thread
From: Finnerty, Jim @ 2022-01-06 13:15 UTC (permalink / raw)
To: Maxim Orlov <[email protected]>; +Cc: pgsql-hackers
(Maxim) Re: -- If after upgrade page has no free space for special data, tuples are
converted to "double xmax" format: xmin became virtual
FrozenTransactionId, xmax occupies the whole 64bit.
Page converted to new format when vacuum frees enough space.
A better way would be to prepare the database for conversion to the 64-bit XID format before the upgrade so that it ensures that every page has enough room for the two new epochs (E bits).
1. Enforce the rule that no INSERT or UPDATE to an existing page will leave less than E bits of free space on a heap page
2. Run an online and restartable task, analogous to pg_repack, that rewrites and splits any page that has less than E bits of free space. This needs to be run on all non-temp tables in all schemas in all databases. DDL operations are not allowed on a target table while this operation runs, which is enforced by taking an ACCESS SHARE lock on each table while the process is running. To mitigate the effects of this restriction, the restartable task can be restricted to run only in certain hours. This could be implemented as a background maintenance task that runs for X hours as of a certain time of day and then kicks itself off again in 24-X hours, logging its progress.
When this task completes, the database is ready for upgrade to 64-bit XIDs, and there is no possibility that any page has insufficient free space for the special data.
Would you agree that this approach would completely eliminate the need for a "double xmax" representation?
/Jim
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Add 64-bit XIDs into PostgreSQL 15
2022-01-06 13:15 Re: Add 64-bit XIDs into PostgreSQL 15 Finnerty, Jim <[email protected]>
@ 2022-01-06 14:09 ` Simon Riggs <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: Simon Riggs @ 2022-01-06 14:09 UTC (permalink / raw)
To: Finnerty, Jim <[email protected]>; +Cc: Maxim Orlov <[email protected]>; pgsql-hackers
On Thu, 6 Jan 2022 at 13:15, Finnerty, Jim <[email protected]> wrote:
>
> (Maxim) Re: -- If after upgrade page has no free space for special data, tuples are
> converted to "double xmax" format: xmin became virtual
> FrozenTransactionId, xmax occupies the whole 64bit.
> Page converted to new format when vacuum frees enough space.
>
> A better way would be to prepare the database for conversion to the 64-bit XID format before the upgrade so that it ensures that every page has enough room for the two new epochs (E bits).
Most software has a one-stage upgrade model. What you propose would
have us install 2 things, with a step in-between, which makes it
harder to manage.
> 1. Enforce the rule that no INSERT or UPDATE to an existing page will leave less than E bits of free space on a heap page
>
> 2. Run an online and restartable task, analogous to pg_repack, that rewrites and splits any page that has less than E bits of free space. This needs to be run on all non-temp tables in all schemas in all databases. DDL operations are not allowed on a target table while this operation runs, which is enforced by taking an ACCESS SHARE lock on each table while the process is running. To mitigate the effects of this restriction, the restartable task can be restricted to run only in certain hours. This could be implemented as a background maintenance task that runs for X hours as of a certain time of day and then kicks itself off again in 24-X hours, logging its progress.
>
> When this task completes, the database is ready for upgrade to 64-bit XIDs, and there is no possibility that any page has insufficient free space for the special data.
>
> Would you agree that this approach would completely eliminate the need for a "double xmax" representation?
I agree about the idea for scanning existing data blocks, but why not
do this AFTER upgrade?
1. Upgrade, with important aspect not-enabled-yet, but everything else
working - all required software is delivered in one shot, with fast
upgrade
2. As each table is VACUUMed, we confirm/clean/groom data blocks so
each table is individually confirmed as being ready. The pace that
this happens at is under user control.
3. When all tables have been prepared, then restart to allow xid64 format usage
--
Simon Riggs http://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Add 64-bit XIDs into PostgreSQL 15
2022-01-06 13:15 Re: Add 64-bit XIDs into PostgreSQL 15 Finnerty, Jim <[email protected]>
@ 2022-01-07 03:35 ` Alexander Korotkov <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: Alexander Korotkov @ 2022-01-07 03:35 UTC (permalink / raw)
To: Finnerty, Jim <[email protected]>; +Cc: Maxim Orlov <[email protected]>; pgsql-hackers
,On Thu, Jan 6, 2022 at 4:15 PM Finnerty, Jim <[email protected]> wrote:
> (Maxim) Re: -- If after upgrade page has no free space for special data, tuples are
> converted to "double xmax" format: xmin became virtual
> FrozenTransactionId, xmax occupies the whole 64bit.
> Page converted to new format when vacuum frees enough space.
>
> A better way would be to prepare the database for conversion to the 64-bit XID format before the upgrade so that it ensures that every page has enough room for the two new epochs (E bits).
>
> 1. Enforce the rule that no INSERT or UPDATE to an existing page will leave less than E bits of free space on a heap page
>
> 2. Run an online and restartable task, analogous to pg_repack, that rewrites and splits any page that has less than E bits of free space. This needs to be run on all non-temp tables in all schemas in all databases. DDL operations are not allowed on a target table while this operation runs, which is enforced by taking an ACCESS SHARE lock on each table while the process is running. To mitigate the effects of this restriction, the restartable task can be restricted to run only in certain hours. This could be implemented as a background maintenance task that runs for X hours as of a certain time of day and then kicks itself off again in 24-X hours, logging its progress.
>
> When this task completes, the database is ready for upgrade to 64-bit XIDs, and there is no possibility that any page has insufficient free space for the special data.
>
> Would you agree that this approach would completely eliminate the need for a "double xmax" representation?
The "prepare" approach was the first tried.
https://github.com/postgrespro/pg_pageprep
But it appears to be very difficult and unreliable. After investing
many months into pg_pageprep, "double xmax" approach appears to be
very fast to implement and reliable.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-01-07 03:35 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 03:11 [PATCH v8 4/8] Propagate changes to indisclustered to child/parents Justin Pryzby <[email protected]>
2022-01-06 13:15 Re: Add 64-bit XIDs into PostgreSQL 15 Finnerty, Jim <[email protected]>
2022-01-06 14:09 ` Re: Add 64-bit XIDs into PostgreSQL 15 Simon Riggs <[email protected]>
2022-01-07 03:35 ` Re: Add 64-bit XIDs into PostgreSQL 15 Alexander Korotkov <[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