Обсуждение: relhassubclass and partitioned indexes

Hi,

$subject came up in [1].

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]
https://www.postgresql.org/message-id/3acdcbf4-6a62-fb83-3bfd-5f275602ca7d%40lab.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

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