relhassubclass and partitioned indexes
Hi,
$subject came up in [1]/messages/by-id/3acdcbf4-6a62-fb83-3bfd-5f275602ca7d@lab.ntt.co.jp.
Should relhassubclass be set/reset for partitioned indexes?
The only use case being sought here is to use find_inheritance_children()
for getting an index's partitions, but it uses relhassublcass test to
short-circuit scanning pg_inherits. That means it cannot be used to get
an index's children, because relhassublcass is never set for indexes.
Michael suggested on the linked thread to get rid of relhassubclass
altogether, like we did for relhaspkey recently, but I'm not sure whether
it would be a good idea right yet.
Thoughts?
Thanks,
Amit
[1]: /messages/by-id/3acdcbf4-6a62-fb83-3bfd-5f275602ca7d@lab.ntt.co.jp
/messages/by-id/3acdcbf4-6a62-fb83-3bfd-5f275602ca7d@lab.ntt.co.jp
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
Should relhassubclass be set/reset for partitioned indexes?
Seems like a reasonable idea to me, at least the "set" end of it.
We don't ever clear relhassubclass for tables, so maybe that's
not necessary for indexes either.
Michael suggested on the linked thread to get rid of relhassubclass
altogether, like we did for relhaspkey recently, but I'm not sure whether
it would be a good idea right yet.
We got rid of relhaspkey mostly because it was of no use to the backend.
That's far from true for relhassubclass.
regards, tom lane
On Fri, Oct 19, 2018 at 01:45:03AM -0400, Tom Lane wrote:
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
Should relhassubclass be set/reset for partitioned indexes?
Seems like a reasonable idea to me, at least the "set" end of it.
We don't ever clear relhassubclass for tables, so maybe that's
not necessary for indexes either.
No objections to the proposal. Allowing find_inheritance_children to
find index trees for partitioned indexes could be actually useful for
extensions like pg_partman.
Michael suggested on the linked thread to get rid of relhassubclass
altogether, like we did for relhaspkey recently, but I'm not sure whether
it would be a good idea right yet.We got rid of relhaspkey mostly because it was of no use to the backend.
That's far from true for relhassubclass.
Partitioned tables are expected to have partitions, so the optimizations
related to relhassubclass don't seem much worth worrying. However
relations not having inherited tables may take a performance hit. If
this flag removal would be done, we'd need to be careful about the
performance impact and the cost of extra lookups at pg_inherit.
--
Michael
Thanks for commenting.
On 2018/10/19 15:17, Michael Paquier wrote:
On Fri, Oct 19, 2018 at 01:45:03AM -0400, Tom Lane wrote:
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
Should relhassubclass be set/reset for partitioned indexes?
Seems like a reasonable idea to me, at least the "set" end of it.
We don't ever clear relhassubclass for tables, so maybe that's
not necessary for indexes either.
We *do* reset opportunistically during analyze of inheritance parent; the
following code in acquire_inherited_sample_rows() can reset it:
/*
* Check that there's at least one descendant, else fail. This could
* happen despite analyze_rel's relhassubclass check, if table once had a
* child but no longer does. In that case, we can clear the
* relhassubclass field so as not to make the same mistake again later.
* (This is safe because we hold ShareUpdateExclusiveLock.)
*/
if (list_length(tableOIDs) < 2)
{
/* CCI because we already updated the pg_class row in this command */
CommandCounterIncrement();
SetRelationHasSubclass(RelationGetRelid(onerel), false);
ereport(elevel,
(errmsg("skipping analyze of \"%s.%s\" inheritance tree ---
this inheritance tree contains no child tables",
get_namespace_name(RelationGetNamespace(onerel)),
RelationGetRelationName(onerel))));
return 0;
}
Similarly, perhaps we can make REINDEX on a partitioned index reset the
flag if it doesn't find any children. We won't be able to do that today
though, because:
static void
ReindexPartitionedIndex(Relation parentIdx)
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("REINDEX is not yet implemented for partitioned
indexes")));
}
No objections to the proposal. Allowing find_inheritance_children to
find index trees for partitioned indexes could be actually useful for
extensions like pg_partman.
Thanks. Attached a patch to set relhassubclass when an index partition is
added to a partitioned index.
Regards,
Amit
Attachments:
0001-Set-relhassubclass-on-index-parents.patchtext/plain; charset=UTF-8; name=0001-Set-relhassubclass-on-index-parents.patchDownload
From f0c01ab41b35a5f21a90b0294d8216da78eb8882 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Fri, 19 Oct 2018 17:05:00 +0900
Subject: [PATCH 1/2] Set relhassubclass on index parents
---
src/backend/catalog/index.c | 5 ++++
src/backend/commands/indexcmds.c | 4 +++
src/test/regress/expected/indexing.out | 46 +++++++++++++++++++++-------------
src/test/regress/sql/indexing.sql | 11 ++++++--
4 files changed, 46 insertions(+), 20 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f1ef4c319a..62cc6a5bb9 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -996,8 +996,13 @@ index_create(Relation heapRelation,
/* update pg_inherits, if needed */
if (OidIsValid(parentIndexRelid))
+ {
StoreSingleInheritance(indexRelationId, parentIndexRelid, 1);
+ /* Also, set the parent's relhassubclass. */
+ SetRelationHasSubclass(parentIndexRelid, true);
+ }
+
/*
* Register constraint and dependencies for the index.
*
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3975f62c00..c392352871 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2608,6 +2608,10 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
systable_endscan(scan);
relation_close(pg_inherits, RowExclusiveLock);
+ /* If we added an index partition to parent, set its relhassubclass. */
+ if (OidIsValid(parentOid))
+ SetRelationHasSubclass(parentOid, true);
+
if (fix_dependencies)
{
ObjectAddress partIdx;
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index 225f4e9527..710b32192f 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -1,25 +1,35 @@
-- Creating an index on a partitioned table makes the partitions
-- automatically get the index
create table idxpart (a int, b int, c text) partition by range (a);
+-- relhassubclass of a partitioned index is false before creating its partition
+-- it will be set below once partitions get created
+create index check_relhassubclass_of_this on idxpart (a);
+select relhassubclass from pg_class where relname = 'check_relhassubclass_of_this';
+ relhassubclass
+----------------
+ f
+(1 row)
+
+drop index check_relhassubclass_of_this;
create table idxpart1 partition of idxpart for values from (0) to (10);
create table idxpart2 partition of idxpart for values from (10) to (100)
partition by range (b);
create table idxpart21 partition of idxpart2 for values from (0) to (100);
create index on idxpart (a);
-select relname, relkind, inhparent::regclass
+select relname, relkind, relhassubclass, inhparent::regclass
from pg_class left join pg_index ix on (indexrelid = oid)
left join pg_inherits on (ix.indexrelid = inhrelid)
where relname like 'idxpart%' order by relname;
- relname | relkind | inhparent
------------------+---------+----------------
- idxpart | p |
- idxpart1 | r |
- idxpart1_a_idx | i | idxpart_a_idx
- idxpart2 | p |
- idxpart21 | r |
- idxpart21_a_idx | i | idxpart2_a_idx
- idxpart2_a_idx | I | idxpart_a_idx
- idxpart_a_idx | I |
+ relname | relkind | relhassubclass | inhparent
+-----------------+---------+----------------+----------------
+ idxpart | p | t |
+ idxpart1 | r | f |
+ idxpart1_a_idx | i | f | idxpart_a_idx
+ idxpart2 | p | t |
+ idxpart21 | r | f |
+ idxpart21_a_idx | i | f | idxpart2_a_idx
+ idxpart2_a_idx | I | t | idxpart_a_idx
+ idxpart_a_idx | I | t |
(8 rows)
drop table idxpart;
@@ -110,16 +120,16 @@ Partition of: idxpart FOR VALUES FROM (0, 0) TO (10, 10)
Indexes:
"idxpart1_a_b_idx" btree (a, b)
-select relname, relkind, inhparent::regclass
+select relname, relkind, relhassubclass, inhparent::regclass
from pg_class left join pg_index ix on (indexrelid = oid)
left join pg_inherits on (ix.indexrelid = inhrelid)
where relname like 'idxpart%' order by relname;
- relname | relkind | inhparent
-------------------+---------+-----------------
- idxpart | p |
- idxpart1 | r |
- idxpart1_a_b_idx | i | idxpart_a_b_idx
- idxpart_a_b_idx | I |
+ relname | relkind | relhassubclass | inhparent
+------------------+---------+----------------+-----------------
+ idxpart | p | t |
+ idxpart1 | r | f |
+ idxpart1_a_b_idx | i | f | idxpart_a_b_idx
+ idxpart_a_b_idx | I | t |
(4 rows)
drop table idxpart;
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index f145384fbc..b0feb1911d 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -1,12 +1,19 @@
-- Creating an index on a partitioned table makes the partitions
-- automatically get the index
create table idxpart (a int, b int, c text) partition by range (a);
+
+-- relhassubclass of a partitioned index is false before creating its partition
+-- it will be set below once partitions get created
+create index check_relhassubclass_of_this on idxpart (a);
+select relhassubclass from pg_class where relname = 'check_relhassubclass_of_this';
+drop index check_relhassubclass_of_this;
+
create table idxpart1 partition of idxpart for values from (0) to (10);
create table idxpart2 partition of idxpart for values from (10) to (100)
partition by range (b);
create table idxpart21 partition of idxpart2 for values from (0) to (100);
create index on idxpart (a);
-select relname, relkind, inhparent::regclass
+select relname, relkind, relhassubclass, inhparent::regclass
from pg_class left join pg_index ix on (indexrelid = oid)
left join pg_inherits on (ix.indexrelid = inhrelid)
where relname like 'idxpart%' order by relname;
@@ -54,7 +61,7 @@ create table idxpart1 partition of idxpart for values from (0, 0) to (10, 10);
create index on idxpart1 (a, b);
create index on idxpart (a, b);
\d idxpart1
-select relname, relkind, inhparent::regclass
+select relname, relkind, relhassubclass, inhparent::regclass
from pg_class left join pg_index ix on (indexrelid = oid)
left join pg_inherits on (ix.indexrelid = inhrelid)
where relname like 'idxpart%' order by relname;
--
2.11.0
On Fri, Oct 19, 2018 at 06:46:15PM +0900, Amit Langote wrote:
Thanks. Attached a patch to set relhassubclass when an index partition is
added to a partitioned index.
Thanks, committed after adding a test with ALTER TABLE ONLY, and
checking upgrades as well as ATTACH partition for ALTER INDEX and ALTER
TABLE. In all cases the same code paths are taken.
/* update pg_inherits, if needed */
if (OidIsValid(parentIndexRelid))
+ {
StoreSingleInheritance(indexRelationId, parentIndexRelid, 1);+ /* Also, set the parent's relhassubclass. */ + SetRelationHasSubclass(parentIndexRelid, true); + }
Calling SetRelationHasSubclass() updates pg_class for this parent
relation. We would need CommandCounterIncrement() if the tuple gets
updated, but that's not the case as far as I checked for all code paths
where this gets called. This would be seen immediately by the way..
--
Michael
Hi,
On 2018/10/22 11:09, Michael Paquier wrote:
On Fri, Oct 19, 2018 at 06:46:15PM +0900, Amit Langote wrote:
Thanks. Attached a patch to set relhassubclass when an index partition is
added to a partitioned index.Thanks, committed after adding a test with ALTER TABLE ONLY, and
checking upgrades as well as ATTACH partition for ALTER INDEX and ALTER
TABLE. In all cases the same code paths are taken.
Thank you for committing with those changes. I didn't know about create
index on "only".
/* update pg_inherits, if needed */
if (OidIsValid(parentIndexRelid))
+ {
StoreSingleInheritance(indexRelationId, parentIndexRelid, 1);+ /* Also, set the parent's relhassubclass. */ + SetRelationHasSubclass(parentIndexRelid, true); + }Calling SetRelationHasSubclass() updates pg_class for this parent
relation. We would need CommandCounterIncrement() if the tuple gets
updated, but that's not the case as far as I checked for all code paths
where this gets called. This would be seen immediately by the way..
I assume you're talking about avoiding getting into a situation that
results in "ERROR: tuple concurrently updated".
Afaics, acquire_inherited_sample_rows() "does" perform CCI, because as the
comment says the parent's pg_class row may already have been updated in
that case:
/* CCI because we already updated the pg_class row in this command */
CommandCounterIncrement();
SetRelationHasSubclass(RelationGetRelid(onerel), false);
In all the other case, SetRelationHasSubclass() seems to be the first time
that the parent's pg_class row is updated.
Thanks,
Amit
On 2018-Oct-22, Amit Langote wrote:
Hi,
On 2018/10/22 11:09, Michael Paquier wrote:
On Fri, Oct 19, 2018 at 06:46:15PM +0900, Amit Langote wrote:
Thanks. Attached a patch to set relhassubclass when an index partition is
added to a partitioned index.Thanks, committed after adding a test with ALTER TABLE ONLY, and
checking upgrades as well as ATTACH partition for ALTER INDEX and ALTER
TABLE. In all cases the same code paths are taken.Thank you for committing with those changes. I didn't know about create
index on "only".
pg_dump uses it :-)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/10/23 0:45, Alvaro Herrera wrote:
On 2018-Oct-22, Amit Langote wrote:
Hi,
On 2018/10/22 11:09, Michael Paquier wrote:
On Fri, Oct 19, 2018 at 06:46:15PM +0900, Amit Langote wrote:
Thanks. Attached a patch to set relhassubclass when an index partition is
added to a partitioned index.Thanks, committed after adding a test with ALTER TABLE ONLY, and
checking upgrades as well as ATTACH partition for ALTER INDEX and ALTER
TABLE. In all cases the same code paths are taken.Thank you for committing with those changes. I didn't know about create
index on "only".pg_dump uses it :-)
Aha, I see.
Thanks,
Amit