Обсуждение: pgsql: Set pg_class.relhassubclass for partitioned indexes

Set pg_class.relhassubclass for partitioned indexes

Like for relations, switching this parameter is optimistic by turning it
on each time a partitioned index gains a partition.  So seeing this
parameter set to true means that the partitioned index has or has had
partitions.  The flag cannot be reset yet for partitioned indexes, which
is something not obvious anyway as partitioned relations exist to have
partitions.

This allows to track more conveniently partition trees for indexes,
which will come in use with an upcoming patch helping in listing
partition trees with an SQL-callable function.

Author: Amit Langote
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/80306490-b5fc-ea34-4427-f29c52156052@lab.ntt.co.jp

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/17f206fbc824d2b4b14480199ca9ff7dea417eda

Modified Files
--------------
src/backend/catalog/index.c            |  5 ++-
src/backend/commands/indexcmds.c       |  4 +++
src/test/regress/expected/indexing.out | 56 +++++++++++++++++++++++-----------
src/test/regress/sql/indexing.sql      | 18 +++++++++--
4 files changed, 62 insertions(+), 21 deletions(-)

Michael Paquier <michael@paquier.xyz> writes:
> Set pg_class.relhassubclass for partitioned indexes

Seems like this commit should have touched the catalogs.sgml description
for that column, as well as the pg_class.h comment for it.  Neither of
those are worded in a way that suggests it could be set for non-table
relations.

            regards, tom lane

On 2018/10/22 11:59, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Set pg_class.relhassubclass for partitioned indexes
> 
> Seems like this commit should have touched the catalogs.sgml description
> for that column, as well as the pg_class.h comment for it.  Neither of
> those are worded in a way that suggests it could be set for non-table
> relations.

Oops, sorry.

Attached fixes catalog.sgml to say table or index instead of just table.

As for pg_class.h, changing just the following line might seem odd, as
along with the surrounding lines, maybe this really says, "relation has..."

    bool        relhassubclass; /* has (or has had) derived classes */

Thanks,
Amit

On Sun, Oct 21, 2018 at 10:59:27PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Set pg_class.relhassubclass for partitioned indexes
>
> Seems like this commit should have touched the catalogs.sgml description
> for that column, as well as the pg_class.h comment for it.  Neither of
> those are worded in a way that suggests it could be set for non-table
> relations.

The pg_class.h comment looked fine for me first.  How would you reword
it?

relispartition tells "True if table is a partition", which is not
actually true as it can apply to indexes.  So this should be changed in
v11 as well, no?

For relhassubclass, perhaps we could do a bit more than just referring
to children...  What about the following:
"True if relation has (or once had) any inheritance children or
partitions"
Here is the current formulation:
"True if table has (or once had) any inheritance children"

And this should be changed in v10~ additionally?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Oct 21, 2018 at 10:59:27PM -0400, Tom Lane wrote:
>> Seems like this commit should have touched the catalogs.sgml description
>> for that column, as well as the pg_class.h comment for it.  Neither of
>> those are worded in a way that suggests it could be set for non-table
>> relations.

> The pg_class.h comment looked fine for me first.  How would you reword
> it?

Well, the question is what "derived class" means, but I'd tend to think
it means something that has an associated composite type; which indexes
do not.  So maybe instead of "has (or has had) derived classes", we could
write "has (or has had) child tables or indexes"?  I'm not wedded to
particular wording for this, but I think what's there now is a bit
misleading.

> relispartition tells "True if table is a partition", which is not
> actually true as it can apply to indexes.  So this should be changed in
> v11 as well, no?

Good point, that column's description is obsolete as well.

            regards, tom lane

On Sun, Oct 21, 2018 at 11:20:04PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Sun, Oct 21, 2018 at 10:59:27PM -0400, Tom Lane wrote:
>>> Seems like this commit should have touched the catalogs.sgml description
>>> for that column, as well as the pg_class.h comment for it.  Neither of
>>> those are worded in a way that suggests it could be set for non-table
>>> relations.
>
>> The pg_class.h comment looked fine for me first.  How would you reword
>> it?
>
> Well, the question is what "derived class" means, but I'd tend to think
> it means something that has an associated composite type; which indexes
> do not.  So maybe instead of "has (or has had) derived classes", we could
> write "has (or has had) child tables or indexes"?  I'm not wedded to
> particular wording for this, but I think what's there now is a bit
> misleading.

Your suggestion looks good to me.

>> relispartition tells "True if table is a partition", which is not
>> actually true as it can apply to indexes.  So this should be changed in
>> v11 as well, no?
>
> Good point, that column's description is obsolete as well.

Amit, Tom, what do you think about the attached?  This merges all the
things proposed.
--
Michael

On 2018/10/22 13:35, Michael Paquier wrote:
> Amit, Tom, what do you think about the attached?  This merges all the
> things proposed.

Thanks, looks good to me.

Regards,
Amit

On Mon, Oct 22, 2018 at 01:46:49PM +0900, Amit Langote wrote:
> On 2018/10/22 13:35, Michael Paquier wrote:
>> Amit, Tom, what do you think about the attached?  This merges all the
>> things proposed.
>
> Thanks, looks good to me.

Committed, and back-patched the part about relispartition to v11.
--
Michael