Date: Sun, 12 Mar 2023 15:09:57 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
	Ilya Gladyshev <ilya.v.gladyshev@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Progress report of CREATE INDEX for nested partitioned tables
Message-ID: <ZA4xlW9VB+1fYgyo@telsasoft.com>
References: <5073D187-4200-4A2D-BAC0-91C657E3C22E@gmail.com>
 <CAEze2Wizpeqsgu9AE3RV_ELzsJGFtz774oGGJhmimc+Mz=b=0Q@mail.gmail.com>
 <20230201155336.GU22427@telsasoft.com>
 <CAEze2WgvJOvHv0mp3LwDbF27g7e_HgiWvbWdGH8980S14VBPoQ@mail.gmail.com>
 <DF2FEE0B-8D65-4926-B2E1-79C84363DBAC@gmail.com>
 <CAEze2Wgj=Ktn_B4U1sRRnWwitQcB4gLkb1QR8627bGRfzW4QOA@mail.gmail.com>
 <20230202151807.GY22427@telsasoft.com>
 <20230208224049.GO1653@telsasoft.com>
 <20230217002651.GW1653@telsasoft.com>
 <1000008.1678480570@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1000008.1678480570@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/ZA4xlW9VB%2B1fYgyo%40telsasoft.com>
Precedence: bulk

On Fri, Mar 10, 2023 at 03:36:10PM -0500, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > Update to address a compiler warning in the supplementary patches adding
> > assertions.
> 
> I took a look through this.  It seems like basically a good solution,
> but the count_leaf_partitions() function is bothering me, for two
> reasons:
> 
> 1. It seems like a pretty expensive thing to do.  Don't we have the
> info at hand somewhere already?

I don't know where that would be.  We need the list of both direct *and*
indirect partitions.  See:
https://www.postgresql.org/message-id/5073D187-4200-4A2D-BAC0-91C657E3C22E%40gmail.com

If it would help to avoid the concern, then I might consider proposing
not to call get_rel_relkind() ...

> 2. Is it really safe to do find_all_inheritors with NoLock?  If so,
> a comment explaining why would be good.

In both cases (both for the parent and for case of a partitioned child
with pre-existing indexes being ATTACHed), the table itself is already
locked by DefineIndex():

    lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
    rel = table_open(relationId, lockmode);

and
                childrel = table_open(childRelid, lockmode);
                ...
                table_close(childrel, NoLock);

And, find_all_inheritors() will also have been called by
ProcessUtilitySlow().  Maybe it's sufficient to mention that ?

-- 
Justin