Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\))
Subject: Re: Progress report of CREATE INDEX for nested partitioned tables
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
In-Reply-To: <ZBJdxTagTuOZkgov@telsasoft.com>
Date: Thu, 16 Mar 2023 19:04:16 +0400
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
 Matthias van de Meent <boekewurm+postgres@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Tomas Vondra <tomas.vondra@enterprisedb.com>,
 pgsql-hackers@lists.postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <266AAEDA-786B-4DDB-B115-AE8EF0E5F283@gmail.com>
References: <1000008.1678480570@sss.pgh.pa.us>
 <ZA4xlW9VB+1fYgyo@telsasoft.com> <1605789.1678652046@sss.pgh.pa.us>
 <ZA5My7SUI0/VnmCS@telsasoft.com> <1621054.1678659913@sss.pgh.pa.us>
 <1634535.1678666528@sss.pgh.pa.us> <ZA8qEBqN1Yvd7sCc@telsasoft.com>
 <1760574.1678718579@sss.pgh.pa.us> <ZBCF6CLwLDGNVLm7@telsasoft.com>
 <34FE3024-5025-46B7-B435-8DDD59072705@gmail.com>
 <ZBJdxTagTuOZkgov@telsasoft.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Archived-At: 
 <https://www.postgresql.org/message-id/266AAEDA-786B-4DDB-B115-AE8EF0E5F283%40gmail.com>
Precedence: bulk


> 16 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2023 =D0=B3., =D0=B2 04:07, Justin =
Pryzby <pryzby@telsasoft.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=
=B0):
>=20
> On Tue, Mar 14, 2023 at 06:58:14PM +0400, Ilya Gladyshev wrote:
>>> The only change from the current patch is (3).  (1) still calls
>>> count_leaf_partitions(), but only once.  I'd prefer that to =
rearranging
>>> the progress reporting to set the TOTAL in ProcessUtilitySlow().
>>=20
>> As for reusing TOTAL calculated outside of DefineIndex, as I can see, =
ProcessUtilitySlow is not the only call site for DefineIndex (although, =
I don=E2=80=99t know whether all of them need progress tracking), for =
instance, there is ALTER TABLE that calls DefineIndex to create index =
for constraints. So I feel like rearranging progress reporting will =
result in unnecessary code duplication in those call sites, so passing =
in an optional parameter seems to be easier here, if we are going to =
optimize it, after all. Especially if back-patching is a non-issue.
>=20
> Yeah.  See attached.  I don't like duplicating the loop.  Is this =
really
> the right direction to go ?
>=20
> I haven't verified if the child tables are locked in all the paths =
which
> would call count_leaf_partitions().  But why is it important to lock
> them for this?  If they weren't locked before, that'd be a =
pre-existing
> problem...
> <0001-fix-CREATE-INDEX-progress-report-with-nested-partiti.patch>

I=E2=80=99m not sure what the general policy on locking is, but I have =
checked ALTER TABLE ADD INDEX, and the all the partitions seem to be =
locked on the first entry to DefineIndex there. All other call sites =
pass in the parentIndexId, which means the progress tracking machinery =
will not be initialized, so I think, we don=E2=80=99t need to do locking =
in count_leaf_partitions().=20

The approach in the patch looks good to me. Some nitpicks on the patch:=20=

1. There=E2=80=99s an unnecessary second call to get_rel_relkind in =
ProcessUtilitySlow, we can just use what=E2=80=99s in the variable =
relkind.
2. We can also combine else and if to have one less nested level like =
that:
+				else if =
(!RELKIND_HAS_PARTITIONS(child_relkind))

3. There was a part of the comment saying "If the index was built by =
calling DefineIndex() recursively, the called function is responsible =
for updating the progress report for built indexes.", I think it is =
still useful to have it there.