public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v7 3/7] Propagate changes to indisclustered to child/parents
7+ messages / 4 participants
[nested] [flat]
* [PATCH v7 3/7] Propagate changes to indisclustered to child/parents
@ 2020-10-07 03:11 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 7+ 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 9b6673867c..78c2c2ba72 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);
@@ -510,66 +511,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
--AA9g+nFNFPYNJKiL
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0004-Invalidate-parent-indexes.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v1] Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version()
@ 2026-02-27 03:27 Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Bertrand Drouvot @ 2026-02-27 03:27 UTC (permalink / raw)
Standard practice in PostgreSQL is to use "foo(void)" instead of "foo()", as the
latter looks like an "old-style" function declaration. 9b05e2ec08a did fix
all the ones reported by -Wstrict-prototypes.
af2d4ca191a4 introduced a new one, this commit fixes it.
Author: Bertrand Drouvot <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/utils/adt/pg_locale.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
100.0% src/backend/utils/adt/
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index ac324ecaad2..6c5c1019e1e 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1650,7 +1650,7 @@ pg_towlower(pg_wchar wc, pg_locale_t locale)
/* version of Unicode used by ICU */
const char *
-pg_icu_unicode_version()
+pg_icu_unicode_version(void)
{
#ifdef USE_ICU
return U_UNICODE_VERSION;
--
2.34.1
--D6058a7DswGNtGTg--
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v1] Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version()
@ 2026-02-27 03:27 Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Bertrand Drouvot @ 2026-02-27 03:27 UTC (permalink / raw)
Standard practice in PostgreSQL is to use "foo(void)" instead of "foo()", as the
latter looks like an "old-style" function declaration. 9b05e2ec08a did fix
all the ones reported by -Wstrict-prototypes.
af2d4ca191a4 introduced a new one, this commit fixes it.
Author: Bertrand Drouvot <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/utils/adt/pg_locale.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
100.0% src/backend/utils/adt/
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index ac324ecaad2..6c5c1019e1e 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1650,7 +1650,7 @@ pg_towlower(pg_wchar wc, pg_locale_t locale)
/* version of Unicode used by ICU */
const char *
-pg_icu_unicode_version()
+pg_icu_unicode_version(void)
{
#ifdef USE_ICU
return U_UNICODE_VERSION;
--
2.34.1
--D6058a7DswGNtGTg--
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version()
@ 2026-02-27 14:58 Joe Conway <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Joe Conway @ 2026-02-27 14:58 UTC (permalink / raw)
To: Chao Li <[email protected]>; Bertrand Drouvot <[email protected]>; +Cc: [email protected]; Jeff Davis <[email protected]>
On 2/27/26 01:04, Chao Li wrote:
> What I'm interested in is the broader policy: when reviewing
> patches, if we encounter a foo() declaration, should we consistently
> request a change to foo(void)? If yes, the standard should be
> documented somewhere.
Perhaps here?
https://wiki.postgresql.org/wiki/Committing_checklist
--
Joe Conway
PostgreSQL Contributors Team
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version()
@ 2026-03-03 01:22 Andreas Karlsson <[email protected]>
parent: Joe Conway <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Andreas Karlsson @ 2026-03-03 01:22 UTC (permalink / raw)
To: Joe Conway <[email protected]>; Chao Li <[email protected]>; Bertrand Drouvot <[email protected]>; +Cc: [email protected]; Jeff Davis <[email protected]>
On 2/27/26 3:58 PM, Joe Conway wrote:
> On 2/27/26 01:04, Chao Li wrote:
>> What I'm interested in is the broader policy: when reviewing
>> patches, if we encounter a foo() declaration, should we consistently
>> request a change to foo(void)? If yes, the standard should be
>> documented somewhere.
>
> Perhaps here?
>
> https://wiki.postgresql.org/wiki/Committing_checklist
Nah, I think we should just enable the warnings unless we have a good
reason not to.
Andreas
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version()
@ 2026-03-09 07:57 Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Bertrand Drouvot @ 2026-03-09 07:57 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Chao Li <[email protected]>; [email protected]; Jeff Davis <[email protected]>
Hi,
On Mon, Mar 02, 2026 at 07:36:20PM +0100, Peter Eisentraut wrote:
> On 27.02.26 07:45, Bertrand Drouvot wrote:
> > Hi,
> >
> > On Fri, Feb 27, 2026 at 02:04:30PM +0800, Chao Li wrote:
> > >
> > >
> > > What I'm interested in is the broader policy: when reviewing patches,
> > > if we encounter a foo() declaration, should we consistently request a change to foo(void)?
> > > If yes, the standard should be documented somewhere.
> >
> > I think that they should be consistently fixed for the reasons mentioned in
> > [1], and that the best way to achieve this goal would be to enable -Wstrict-prototypes
> > by default ([2]).
>
> Yes, why not add -Wstrict-prototypes and perhaps -Wold-style-definition to
> the standard warnings. Then we don't have to keep chasing these manually.
Yeah, I'll look at adding those.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version()
@ 2026-03-09 16:41 Bertrand Drouvot <[email protected]>
parent: Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Bertrand Drouvot @ 2026-03-09 16:41 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Chao Li <[email protected]>; [email protected]; Jeff Davis <[email protected]>
Hi,
On Mon, Mar 09, 2026 at 09:44:45AM +0100, Peter Eisentraut wrote:
> On 09.03.26 08:57, Bertrand Drouvot wrote:
> > Hi,
> >
> > On Mon, Mar 02, 2026 at 07:36:20PM +0100, Peter Eisentraut wrote:
> > > On 27.02.26 07:45, Bertrand Drouvot wrote:
> > > > Hi,
> > > >
> > > > On Fri, Feb 27, 2026 at 02:04:30PM +0800, Chao Li wrote:
> > > > >
> > > > >
> > > > > What I'm interested in is the broader policy: when reviewing patches,
> > > > > if we encounter a foo() declaration, should we consistently request a change to foo(void)?
> > > > > If yes, the standard should be documented somewhere.
> > > >
> > > > I think that they should be consistently fixed for the reasons mentioned in
> > > > [1], and that the best way to achieve this goal would be to enable -Wstrict-prototypes
> > > > by default ([2]).
> > >
> > > Yes, why not add -Wstrict-prototypes and perhaps -Wold-style-definition to
> > > the standard warnings. Then we don't have to keep chasing these manually.
> >
> > Yeah, I'll look at adding those.
>
> I played with this a little bit. A problem I found is that the generated
> configure code itself generates its test programs with 'main()', and so with
> these warnings, many of these tests will fail. So you'd need to create some
> different arrangement where you test for the warnings but only add the flags
> at the end of configure.
>
> But also, my research indicates that -Wstrict-prototypes and
> -Wold-style-definition are available in all supported gcc and clang
> versions, so maybe you could avoid this problem by not testing for them and
> just unconditionally adding them at the end.
Yeah, I did observe the same. I moved the new flags late enough in configure.ac
to avoid any error (for example, PGAC_PRINTF_ARCHETYPE which uses -Werror and
would fail to detect gnu_printf if -Wstrict-prototypes is active) leading to the
mingw_cross_warning CI task failing.
I just shared the patch in a dedicated thread [1].
[1]: https://postgr.es/m/aa73q1aT0A3/vke/@ip-10-97-1-34.eu-west-3.compute.internal
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-03-09 16:41 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 03:11 [PATCH v7 3/7] Propagate changes to indisclustered to child/parents Justin Pryzby <[email protected]>
2026-02-27 03:27 [PATCH v1] Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version() Bertrand Drouvot <[email protected]>
2026-02-27 03:27 [PATCH v1] Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version() Bertrand Drouvot <[email protected]>
2026-02-27 14:58 ` Re: Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version() Joe Conway <[email protected]>
2026-03-03 01:22 ` Re: Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version() Andreas Karlsson <[email protected]>
2026-03-09 07:57 ` Re: Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version() Bertrand Drouvot <[email protected]>
2026-03-09 16:41 ` Re: Use pg_icu_unicode_version(void) instead of pg_icu_unicode_version() Bertrand Drouvot <[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