Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pNHGc-0003UV-V7 for pgsql-hackers@arkaria.postgresql.org; Wed, 01 Feb 2023 17:51:31 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1pNHGb-0007qp-Nm for pgsql-hackers@arkaria.postgresql.org; Wed, 01 Feb 2023 17:51:29 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pNHGb-0007qf-9E for pgsql-hackers@lists.postgresql.org; Wed, 01 Feb 2023 17:51:29 +0000 Received: from mail-wm1-x331.google.com ([2a00:1450:4864:20::331]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1pNHGY-00026i-Uu for pgsql-hackers@lists.postgresql.org; Wed, 01 Feb 2023 17:51:28 +0000 Received: by mail-wm1-x331.google.com with SMTP id o36so7255309wms.1 for ; Wed, 01 Feb 2023 09:51:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=J/zVIIFdk/PqLoI7elw2zua9IncCVL+IP64fBoLgrPg=; b=WoDYwXzDzMWNuWXhi7TlssdKu+MKcki1Uhu73HJLWl8I67utUkLNTTuuzWECZ0zOMm ggCR6ljzZFNmiVArp7uDylG43qCKlx0UgYaPARw6TkDjG+exhvFKcsk13caxK7FUrKR1 J/JsTZLIq7dhN+SQgWCga5vbykmKP8TDIGNg9W/jL7VU6nQ7eiqFlVwFqM1i6Tz0MArg OIGH9/AvVVBFLVmTfkVbfpLyuagzYCL0ro4rHTaQ1Cyvs4E5VrVGFBWEIxnxpXWPm3IP P/VsFKTqUV51Xqh+glkle9nJU86c2iHnt1uITip3M3nQb+g/wqahn7T+fGRbSTKmb5bV 7/Kw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=J/zVIIFdk/PqLoI7elw2zua9IncCVL+IP64fBoLgrPg=; b=I0qigfTC6tWCrbjO44vnJoMq+v7lAiDqsih1KhVbRGjhn/EbyzWEFc90lTtI4LKHdq Q6SQEOe++twWTxhiPMjjb0H9tn8R1jl80cvueBwlUPN5wKONMn75j5UFtshjJPO+VQdV d5NMvpiImEnH49ZNrqjO1COo0QtSvU2Ds+S7m/HuICwQnJFZ7ZbR04JIZ0qu4Oqh6ZaD 49E/4Nuoj5jkfCOJUngtxBgJIFfE2tlSSf5f2jf1t7Yv19uoo/s0CEcW/Dq9V9w6sCVZ 1xE8Ki/zOA24c3RVtTDGQmcMf75xBuf3Ivh2Dczw4rhRxnF3Q4pgUgAlZWsFzPXavywq 8YCQ== X-Gm-Message-State: AO0yUKXC9VhnnazMsmeZgqB0BHqOcZlHHda9eSIxVuOV2BsObiDzm8UD 4WqXNwtn7urBtXlR2x7IqpGTakCcRWd/mcKZ X-Google-Smtp-Source: AK7set9L7bwzsECvx1AtSkRmg7sAcbkTrYtMkE7kbRBjDODl3YHHt7pt8mQ1JHR86E4mpvyXEEhs1A== X-Received: by 2002:a05:600c:601b:b0:3dd:1982:4ce0 with SMTP id az27-20020a05600c601b00b003dd19824ce0mr2954000wmb.16.1675273885784; Wed, 01 Feb 2023 09:51:25 -0800 (PST) Received: from smtpclient.apple ([46.71.221.11]) by smtp.gmail.com with ESMTPSA id t28-20020a05600c329c00b003dc59081603sm2213751wmp.48.2023.02.01.09.51.21 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 01 Feb 2023 09:51:25 -0800 (PST) From: Ilya Gladyshev Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_6BB4C024-27EF-4F24-8A24-A39006F5CDC8" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\)) Subject: Re: Progress report of CREATE INDEX for nested partitioned tables Date: Wed, 1 Feb 2023 21:51:06 +0400 In-Reply-To: Cc: Justin Pryzby , Alvaro Herrera , Tomas Vondra , pgsql-hackers@lists.postgresql.org To: Matthias van de Meent References: <20230201120126.wrkiohouhdohzac5@alvherre.pgsql> <5073D187-4200-4A2D-BAC0-91C657E3C22E@gmail.com> <20230201155336.GU22427@telsasoft.com> X-Mailer: Apple Mail (2.3731.300.101.1.3) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_6BB4C024-27EF-4F24-8A24-A39006F5CDC8 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 1 =D1=84=D0=B5=D0=B2=D1=80. 2023 =D0=B3., =D0=B2 20:27, Matthias van = de Meent =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0): >=20 > On Wed, 1 Feb 2023 at 16:53, Justin Pryzby > wrote: >>=20 >> On Wed, Feb 01, 2023 at 04:21:35PM +0100, Matthias van de Meent = wrote: >>> On Wed, 1 Feb 2023 at 15:21, Ilya Gladyshev = wrote: >>>>> 1 =D1=84=D0=B5=D0=B2=D1=80. 2023 =D0=B3., =D0=B2 16:01, Alvaro = Herrera =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= (=D0=B0): >>>>> Hmm, count_leaf_partitions has to scan pg_inherits and do a = syscache >>>>> lookup for every single element therein ... this sounds slow. >>>>>=20 >>>>> In one of the callsites, we already have the partition descriptor >>>>> available. We could just scan partdesc->is_leaf[] and add one for = each >>>>> 'true' value we see there. >>>>=20 >>>> The problem is that partdesc contains only direct children of the = table and we need all the children down the inheritance tree to count = the total number of leaf partitions in the first callsite. >>>>=20 >>>>> In the other callsite, we had the table open just a few lines = before the >>>>> place you call count_leaf_partitions. Maybe we can rejigger = things by >>>>> examining its state before closing it: if relkind is not = partitioned we >>>>> know leaf_partitions=3D0, and only if partitioned we count leaf = partitions. >>>>> I think that would save some work. I also wonder if it's worth = writing >>>>> a bespoke function for counting leaf partitions rather than = relying on >>>>> find_all_inheritors. >>>>=20 >>>> Sure, added this condition to avoid the extra work here. >>>>=20 >>>=20 >>>> When creating an index on a partitioned table, this column is = set to >>>> - the total number of partitions on which the index is to be = created. >>>> + the total number of leaf partitions on which the index is = to be created or attached. >>>=20 >>> I think we should also add a note about the (now) non-constant = nature >>> of the value, something along the lines of "This value is updated as >>> we're processing and discovering partitioned tables in the partition >>> hierarchy". >>=20 >> But the TOTAL is constant, right ? Updating the total when being = called >> recursively is the problem these patches fix. >=20 > If that's the case, then I'm not seeing the 'fix' part of the patch. I > thought this patch was fixing the provably incorrect TOTAL value where > DONE > TOTAL due to the recursive operation overwriting the DONE/TOTAL > values instead of updating them. >=20 > In HEAD we set TOTAL to whatever number partitioned table we're > currently processing has - regardless of whether we're the top level > statement. > With the patch we instead add the number of child relations to that > count, for which REL_HAS_STORAGE(child) -- or at least, in the v3 > posted by Ilya. Approximately immediately after updating that count we > recurse to the child relations, and that only returns once it is done > creating the indexes, so both TOTAL and DONE go up as we process more > partitions in the hierarchy. The TOTAL in the patch is set only when processing the top-level parent = and it is not updated when we recurse, so yes, it is constant. =46rom = v3: @@ -1219,8 +1243,14 @@ DefineIndex(Oid relationId, Relation parentIndex; TupleDesc parentDesc; =20 - = pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL, - = nparts); + if (!OidIsValid(parentIndexId)) + { + int total_parts; + + total_parts =3D = count_leaf_partitions(relationId); + = pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL, + = total_parts); + } It is set to the total number of children on all levels of the = hierarchy, not just the current one, so the total value doesn=E2=80=99t = need to be updated later, because it is set to the correct value from = the very beginning.=20 It is the DONE counter that is updated, and when we attach an index of a = partition that is itself a partitioned table (like a2 in your example, = if it already had an index created), it will be updated by the number of = children of the partition. @@ -1431,9 +1463,25 @@ DefineIndex(Oid relationId, = SetUserIdAndSecContext(child_save_userid, = child_save_sec_context); } + else + { + int attached_parts =3D 1; + + if = (RELKIND_HAS_PARTITIONS(child_relkind)) + attached_parts =3D = count_leaf_partitions(childRelid); + + /* + * If the index was attached, we = need to update progress + * here, in its parent. For a = partitioned index, we need + * to mark all of its children = that were included in + * = PROGRESS_CREATEIDX_PARTITIONS_TOTAL as done. If the + * index was built by calling = DefineIndex() recursively, + * the called function is = responsible for updating the + * progress report for built = indexes. + */ + = pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, = attached_parts); + } =20 - = pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, - = i + 1); free_attrmap(attmap); } =20 --Apple-Mail=_6BB4C024-27EF-4F24-8A24-A39006F5CDC8 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

1 = =D1=84=D0=B5=D0=B2=D1=80. 2023 =D0=B3., =D0=B2 20:27, Matthias van de = Meent <boekewurm+postgres@gmail.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0):

On Wed, = 1 Feb 2023 at 16:53, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Feb = 01, 2023 at 04:21:35PM +0100, Matthias van de Meent = wrote:
On Wed, 1 Feb 2023 at 15:21, Ilya = Gladyshev <ilya.v.gladyshev@gmail.com> wrote:
1 =D1=84=D0=B5=D0=B2=D1=80. 2023 = =D0=B3., =D0=B2 16:01, Alvaro Herrera <alvherre@alvh.no-ip.org> = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):
Hmm, = count_leaf_partitions has to scan pg_inherits and do a = syscache
lookup for every single element therein ... this sounds = slow.

In one of the callsites, we already have the partition = descriptor
available.  We could just scan partdesc->is_leaf[] = and add one for each
'true' value we see = there.

The problem is that partdesc contains only = direct children of the table and we need all the children down the = inheritance tree to count the total number of leaf partitions in the = first callsite.

In the other callsite, = we had the table open just a few lines before the
place you call = count_leaf_partitions.  Maybe we can rejigger things = by
examining its state before closing it: if relkind is not = partitioned we
know leaf_partitions=3D0, and only if partitioned we = count leaf partitions.
I think that would save some work.  I = also wonder if it's worth writing
a bespoke function for counting = leaf partitions rather than relying = on
find_all_inheritors.

Sure, added this = condition to avoid the extra work = here.


      When creating an index = on a partitioned table, this column is set to
- =       the total number of partitions on = which the index is to be created.
+ =       the total number of leaf partitions = on which the index is to be created or attached.

I = think we should also add a note about the (now) non-constant = nature
of the value, something along the lines of "This value is = updated as
we're processing and discovering partitioned tables in the = partition
hierarchy".

But the TOTAL is constant, = right ?  Updating the total when being called
recursively is the = problem these patches fix.

If = that's the case, then I'm not seeing the 'fix' part of the patch. = I
thought this patch was = fixing the provably incorrect TOTAL value where
DONE > TOTAL due to the recursive = operation overwriting the DONE/TOTAL
values = instead of updating them.

In HEAD = we set TOTAL to whatever number partitioned table we're
currently processing has - regardless of = whether we're the top level
statement.
With = the patch we instead add the number of child relations to that
count, for which REL_HAS_STORAGE(child) -- = or at least, in the v3
posted = by Ilya. Approximately immediately after updating that count = we
recurse to the child = relations, and that only returns once it is done
creating the indexes, so both TOTAL and = DONE go up as we process more
partitions in the = hierarchy.

The TOTAL in the = patch is set only when processing the top-level parent and it is not = updated when we recurse, so yes, it is constant. =46rom = v3:

@@ -1219,8 +1243,14 @@ DefineIndex(Oid = relationId,
  Relation = parentIndex;
  TupleDesc = parentDesc;
 
- = pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
- = = nparts);
+ if = (!OidIsValid(parentIndexId))
+ = {
+ int = total_parts;
+
+ = total_parts =3D = count_leaf_partitions(relationId);
+ = pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
+ = = total_parts);
+ = }


It is set to the = total number of children on all levels of the hierarchy, not just the = current one, so the total value doesn=E2=80=99t need to be updated = later, because it is set to the correct value from the very = beginning. 

It is the DONE counter that is = updated, and when we attach an index of a partition that is itself a = partitioned table (like a2 in your example, if it already had an index = created), it will be updated by the number of children of the = partition.

@@ -1431,9 +1463,25 @@ = DefineIndex(Oid relationId,
  = SetUserIdAndSecContext(child_save_userid,
  =   = child_save_sec_context);
  = }
+ = else
+ = {
+ = int attached_parts =3D 1;
+
+ = if = (RELKIND_HAS_PARTITIONS(child_relkind))
+ = attached_parts =3D = count_leaf_partitions(childRelid);
+
+ = /*
+ = * If the index was attached, we need to update progress
+ = * here, in its parent. For a partitioned index, we = need
+ = * to mark all of its children = that were included in
+ = * PROGRESS_CREATEIDX_PARTITIONS_TOTAL as done. If the
+ = * index was built by calling DefineIndex() = recursively,
+ = * the called function is responsible for updating the
+ = * progress report for built indexes.
+ = */
+ = pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, = attached_parts);
+ = }
 
- = pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- = = i + 1);
  = free_attrmap(attmap);
  = }
 

= --Apple-Mail=_6BB4C024-27EF-4F24-8A24-A39006F5CDC8--