Date: Mon, 5 Oct 2020 22:07:33 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: =?utf-8?B?5p2O5p2wKOaFjui/vSk=?= <adger.lj@alibaba-inc.com>,
	pgsql-hackers <pgsql-hackers@lists.postgresql.org>,
	=?utf-8?B?5pu+5paH5peMKOS5ieS7jik=?= <wenjing.zwj@alibaba-inc.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: =?utf-8?B?5Zue5aSN77yaaG8=?= =?utf-8?Q?w?= to create index
 concurrently on partitioned table
Message-ID: <20201006030733.GL17626@telsasoft.com>
References: <20200908023915.GD7158@telsasoft.com>
 <20200908043105.GE19261@paquier.xyz>
 <20200912001301.GQ18552@telsasoft.com>
 <20200912013239.GA8636@paquier.xyz>
 <20200914143102.GX18552@telsasoft.com>
 <20200924081103.GS28585@paquier.xyz>
 <20200926195655.GA13816@telsasoft.com>
 <20201005084627.GI1464@paquier.xyz>
 <20201005200832.GF17626@telsasoft.com>
 <20201006024255.GJ1464@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201006024255.GJ1464@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Oct 06, 2020 at 11:42:55AM +0900, Michael Paquier wrote:
> On Mon, Oct 05, 2020 at 03:08:32PM -0500, Justin Pryzby wrote:
> > It means that we might do N catalog updates for a partition heirarchy that's N
> > levels deep.  Normally, N=2, and we'd clear indisclustered for the index as
> > well as its parent.  This is not essential, though.
> 
> Hmm.  I got to think more about this one, and being able to ensure a
> consistent state of indisclustered for partitioned tables and all
> their partitions across multiple transactions at all times would be a
> nice property, as we could easily track down if an operation has
> failed in-flight.  The problem here is that we are limited by
> indisclustered being a boolean, so we may set indisclustered one way
> or another on some partitions, or on some parent partitioned tables,
> but we are not sure if what's down is actually clustered for the
> leaves of a partitioned index, or not.  Or maybe we even have an
> inconsistent state, so this does not provide a good user experience.
> The consistency of a partition tree is a key thing, and we have such 
> guarantees with REINDEX (with or without concurrently), so what I'd
> like to think that what makes sense for indisclustered on a
> partitioned index is to have the following set of guarantees, and
> relying on indisvalid as being true iff an index partition tree is
> complete on a given table partition tree is really important:
> - If indisclustered is set to true for a partitioned index, then we
> have the guarantee that all its partition and partitioned indexes have
> indisclustered set to true, across all the layers down to the leaves.
> - If indisclustered is set to false for a partitioned index, then we
> have the guarantee that all its partition and partitioned indexes have
> indisclustered set to false.
> 
> If we happen to attach a new partition to a partitioned table of such
> a tree, I guess that it is then logic to have indisclustered set to
> the same value as the partitioned index when the partition inherits an
> index.  So, it seems to me that with the current catalogs we are
> limited by indisclustered as being a boolean to maintain some kind of
> consistency across transactions of CLUSTER, as we would use one
> transaction per leaf for the work.  In order to make things better
> here, we could switch indisclustered to a char, able to use three
> states:
> - 'e' or enabled, equivalent to indisclustered == true.  There should
> be only one index on a table with 'e' set at a given time.
> - 'd' or disabled, equivalent to indisclustered == false.
> - a new third state, for an equivalent of work-in-progress, let's call
> it 'w', or whatever.
> 
> Then, when we begin a CLUSTER on a partitioned table, I would imagine
> the following:
> - Switch all the indexes selected to 'w' in a first transaction, and
> do not reset the state of other indexes if there is one 'e'.  For
> CLUSTER without USING, we switch the existing 'e' to 'w' if there is
> such an index.  If there are no indexes select-able, issue an error.
> If we find an index with 'w', we have a candidate from a previous
> failed command, so this gets used.  I don't think that this design
> requires a new DDL either as we could reset all 'w' and 'e' to 'd' if
> using ALTER TABLE SET WITHOUT CLUSTER on a partitioned table.  For
> CLUSTER with USING, the partitioned index selected and its leaves are
> switched to 'w', similarly.
> - Then do the work for all the partitions, one partition per
> transaction, keeping 'w'.
> - In a last transaction, switch all the partitions from 'w' to 'e',
> resetting any existing 'e'.

Honestly, I think you're over-thinking and over-engineering indisclustered.

If "clusteredness" was something we offered to maintain across DML, I think
that might be important to provide stronger guarantees.  As it is now, I don't
think this patch is worth changing the catalog definition.

> ALTER TABLE CLUSTER ON should also be a supported operation, where 'e'
> gets switched for all the partitions in a single transaction.  Of
> course, this should not work for an invalid index.  Even with such a
> design, I got to wonder if there could be cases where a user does
> *not* want to cluster the leaves of a partition tree with the same
> index.  If that were to happen, the partition tree may need a redesign
> so making things work so as we force partitions to inherit what's
> wanted for the partitioned table makes the most sense to me.

I wondered the same thing: should clustering a parent index cause the the child
indexes to be marked as clustered ?  Or should it be possible for an
intermediate child to say (marked as) clustered on some other index.  I don't
have strong feelings, but the patch currently sets indisclustered as a natural
consequence of the implementation, so I tentatively think that's what's right.
After all, CLUSTER sets indisclustered without having to also say
"ALTER..CLUSTER ON".

This is relevant to the question I raised about unsetting indisclustered for
each indexes *parent* when clustering on a different index.

I think it would be strange if we refused "ALTER..CLUSTER ON" for a partition
just because a different partitioned index was set clustered.  We'd clear that,
like always, and then (in my proposal) also clear its parents "indisclustered".
I still don't think that's essential, though.

> By the way, I mentioned that previously, but this thread got used for
> REINDEX that is finished, and now we have a discussion going on with
> CLUSTER.  There is also stuff related to CIC and DIC.  There is also
> only one CF entry for all that.  I really think that this work had
> better be split into separate threads, with separate CF entries. Do
> you mind if I change the contents of the CF app so as the existing 
> item is renamed for REINDEX, that got committed?  Then we could create
> a new entry for CIC/DIC (it may make sense to split these two as
> well, but I am not if there are any overlaps between the APIs of the
> two), and a new entry for CLUSTER, depending on the state of the work.
> The original subject of the thread is also unrelated, this makes the
> review process unnecessarily complicated, and that's really
> confusing.  Each discussion should go into its own, independent,
> thread.

I didn't think it's worth the overhead of closing and opening more CFs.
But I don't mind.

-- 
Justin