Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sVU16-00A5g3-6a for pgsql-hackers@arkaria.postgresql.org; Sun, 21 Jul 2024 10:42:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1sVU12-008ZKy-Ru for pgsql-hackers@arkaria.postgresql.org; Sun, 21 Jul 2024 10:42:09 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sVU12-008ZKp-C6 for pgsql-hackers@lists.postgresql.org; Sun, 21 Jul 2024 10:42:08 +0000 Received: from mail-ej1-x630.google.com ([2a00:1450:4864:20::630]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1sVU0w-000hBR-2L for pgsql-hackers@lists.postgresql.org; Sun, 21 Jul 2024 10:42:07 +0000 Received: by mail-ej1-x630.google.com with SMTP id a640c23a62f3a-a77b60cafecso348752766b.1 for ; Sun, 21 Jul 2024 03:42:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1721558521; x=1722163321; darn=lists.postgresql.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=fHHgmOoXWI0cNuCrhNz27E5HwW17ZK8mSOEH+Bx+ay8=; b=lvsGu/2ZnLGbPXLWYKtdNrrpRcNARZGRXIvz/bHdSYEaQIHkh7bj06N+XZkASK4WM+ eDdEJfGWK7sV1cniveX3cfEOg8xAUjTmnxUR+xao9owrQvUZasDz43lvsnJTqbIHbpD6 OrfuCeoHrxuM03/6yQmM6XK42fwMgG8LP0HJstVj/Yx81kx9o1/CkbzTcusm6B/gORjB lfM3FND6af1kVU8/Cc9n9gMk5d9zkXeeEg2uq91EHIpY3mXNg8lRaSLRmendyUfiFc+8 TdRYOzdnJXf4/yYhreCJmbGAXNX7BuBqKoStzly/0E1lcEd6VQlnPvdZW0OfhbMYkkxT yiyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721558521; x=1722163321; 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=fHHgmOoXWI0cNuCrhNz27E5HwW17ZK8mSOEH+Bx+ay8=; b=lZUyKLOQAng7xtYF+VLVDBecOfEm5I0ms1zZsO67jtfoykeFwDiLldc3BxhLLCo8Cp gD84NsZO5mqQiYQBrdRpXFASvPbSt8N5eMLr0J8f51KFPi/K3EPxb5SSaY8qBdr1lMhc gMgjKtkzbsu6XqVzUhWeFLwDQ71kIOYXnWXWQMW1qFB6dyE4OPD3+yLV/tZlflB8PYui X+nA/dgiPlAR/wa7jcXLRhJknRUSv9AiwFOH0wLt7ouLUrD2n+ckR/NiSXzxIxN/n6GL 6+YIlWoDiWv9b8fQ5Xy5/XwWGO+cbYwLpHYSper9FyQJiwnRFiUT4tdSjfHCTWp85/G5 BJWg== X-Gm-Message-State: AOJu0YyX1X7hGAGNPr9Uo61e3snekkiZSQTuBIjtZ9e9Ue68wKz4hGwZ /l71keniOwKfBI0p2H1hvgM7v0PPEQlO4trGdW4GMrRfI19T1jVH X-Google-Smtp-Source: AGHT+IFu0/8nL7jb5KuUhIDGqv6B88B1axD6RwbA9MHVMCxJj+zXA8sGR4T9zXPb+K1ovBIvefiSjg== X-Received: by 2002:a17:907:7d86:b0:a77:bfca:da54 with SMTP id a640c23a62f3a-a7a4bfa7767mr247244566b.29.1721558514791; Sun, 21 Jul 2024 03:41:54 -0700 (PDT) Received: from smtpclient.apple ([2a01:4b00:c00c:a500:a87e:5a58:1908:8b36]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a7a3c7e2f5fsm277915166b.92.2024.07.21.03.41.54 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 21 Jul 2024 03:41:54 -0700 (PDT) From: Ilya Gladyshev Message-Id: <18D00CB1-D34B-4B71-AC65-96F27D69A9D8@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_22C65095-A578-4E34-8C94-14FE9BFA22BE" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.600.62\)) Subject: Re: REINDEX not updating partition progress Date: Sun, 21 Jul 2024 11:41:43 +0100 In-Reply-To: <779baf7f-6fc7-4431-a08e-eec63d84c6a3@gmail.com> Cc: pgsql-hackers@lists.postgresql.org To: Michael Paquier References: <779baf7f-6fc7-4431-a08e-eec63d84c6a3@gmail.com> X-Mailer: Apple Mail (2.3774.600.62) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --Apple-Mail=_22C65095-A578-4E34-8C94-14FE9BFA22BE Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Forgot to update partition_relid in reindex_index in the second patch. = Fixed in attachment. =EF=BF=BC=EF=BF=BC > 21 =D0=B8=D1=8E=D0=BB=D1=8F 2024=E2=80=AF=D0=B3., =D0=B2 01:49, Ilya = Gladyshev =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0): >=20 >=20 >=20 >> 19 =D0=B8=D1=8E=D0=BB=D1=8F 2024=E2=80=AF=D0=B3., =D0=B2 04:17, = Michael Paquier = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >>=20 >> On Fri, Jul 12, 2024 at 11:07:49PM +0100, Ilya Gladyshev wrote: >>> While working on CIC for partitioned tables [1], I noticed that = REINDEX for >>> partitioned tables is not tracking keeping progress of partitioned = tables, >>> so I'm creating a separate thread for this fix as suggested. >>=20 >> This limitation is not a bug, because we already document that >> partitions_total and partitions_done are 0 during a REINDEX. = Compared >> to CREATE INDEX, a REINDEX is a bit wilder as it would work on all = the >> indexes for all the partitions, providing this information makes >> sense. >>=20 >> Agreed that this could be better, but what's now on HEAD is not wrong >> either. >=20 > Thanks for pointing it out, I didn=E2=80=99t notice it. You=E2=80=99re = right, this not a bug, but an improvement. Will remove the bits from the = documentation as well. >=20 >>=20 >> Still it does not do it because we know that the fields are going to >> be overwritten pretty quickly anyway, no? For now, we could just do >> without progress_index_partition_done(), I guess, keeping it to the >> incrementation of PROGRESS_CREATEIDX_PARTITIONS_DONE. There's an >> argument that this makes the code slightly easier to follow, with = less >> wrappers around the progress reporting. >=20 > The use-case that I thought this would improve is REINDEX CONCURRENT, = where data from later stages of the previous partitions would linger for = quite a while before it gets to the same stage of the current partition. = I don=E2=80=99t think this is of big importance, so I=E2=80=99m ok with = making code simpler and leaving it out. >=20 >>=20 >> + int progress_params[3] =3D { >> + PROGRESS_CREATEIDX_COMMAND, >> + PROGRESS_CREATEIDX_PHASE, >> + PROGRESS_CREATEIDX_PARTITIONS_TOTAL >> + }; >> + int64 progress_values[3]; >> + Oid heapId =3D relid; >>=20 >> Rather than having new code blocks, let's use a style consistent with >> DefineIndex() where we have the pgstat_progress_update_multi_param(), >> with a single {} block. >=20 > Fixed. >=20 >> Adding the counter increment at the end of the loop in >> ReindexMultipleInternal() is a good idea. It considers both the >> concurrent and non-concurrent cases. >=20 > Another reason to do so is that reindex_relation calls itself = recursively, so it'd require some additional mechanism so that it = wouldn't increment multiple times per partition. >>=20 >> + progress_values[2] =3D list_length(partitions); >> + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, = heapId); >>=20 >> Hmm. Is setting the relid only once for = pg_stat_progress_create_index >> the best choice there is? Could it be better to report the partition >> OID instead?=20 >=20 > Technically, we could do this, but it looks like no other commands do = it and currently there=E2=80=99s no API to do it without erasing the = rest of progress information. >=20 >> Let's remember that indexes may have been attached with >> names inconsistent with the partitioned table's index. It is a bit >> confusing to stick to the relid all the partitioned table all the >> time, for all the indexes of all the partitions reindexed. >=20 > I agree that it=E2=80=99s a bit confusing especially because = documentation gives no hints about it. Judging by documentation, I would = expect relid and index_relid to correspond to the same table. However, = if we say that this field represents the oid of the relation on which = the command was invoked, then I think it does make sense. Edited = documentation to say that in the new patch. >=20 >> Could it >> be better to actually introduce an entirely new field to the progress >> table? What I would look for is more information: >> 1) the partitioned table OID on which the REINDEX runs >> 2) the partition table being processed >> 3) the index OID being processed (old and new for concurrent case). >>=20 >> The patch sets 1) to the OID of the partitioned table, lacks 2) and >> sets 3) each time an index is rebuilt. >> =E2=80=94 >> Michael >=20 >=20 > I like this approach more, but I=E2=80=99m not sure whether adding = another field for partition oid is worth it, since we already have = index_relid that we can use to join with pg_index to get that. On the = other hand, index_relid is missing during regular CREATE INDEX, so this = new field could be useful to indicate which table is being indexed in = this case. I'm on the fence about this, attached this as a separate = patch, if you think it's a good idea. >=20 > Thank you for the review! >=20 > = --Apple-Mail=_22C65095-A578-4E34-8C94-14FE9BFA22BE Content-Type: multipart/mixed; boundary="Apple-Mail=_83E2974A-0797-4388-8AFC-6B42B3130560" --Apple-Mail=_83E2974A-0797-4388-8AFC-6B42B3130560 Content-Transfer-Encoding: 7bit Content-Type: text/html; charset=us-ascii Forgot to update partition_relid in reindex_index in the second patch. Fixed in attachment.

--Apple-Mail=_83E2974A-0797-4388-8AFC-6B42B3130560 Content-Disposition: attachment; filename=v2-0001-make-REINDEX-track-partition-progress.patch Content-Type: application/octet-stream; x-unix-mode=0644; name="v2-0001-make-REINDEX-track-partition-progress.patch" Content-Transfer-Encoding: quoted-printable =46rom=205f3d4c5b4877b31bb95caa6086d1939aef4f3d12=20Mon=20Sep=2017=20= 00:00:00=202001=0AFrom:=20Ilya=20Gladyshev=20= =0ADate:=20Sat,=2020=20Jul=202024=2014:08:33=20= +0100=0ASubject:=20[PATCH=20v2=201/2]=20make=20REINDEX=20track=20= partition=20progress=0A=0A---=0A=20doc/src/sgml/monitoring.sgml=20=20=20=20= =20|=20=208=20++----=0A=20src/backend/catalog/index.c=20=20=20=20=20=20|=20= 11=20++++++--=0A=20src/backend/commands/indexcmds.c=20|=2048=20= ++++++++++++++++++++++++++++----=0A=20src/include/catalog/index.h=20=20=20= =20=20=20|=20=201=20+=0A=204=20files=20changed,=2055=20insertions(+),=20= 13=20deletions(-)=0A=0Adiff=20--git=20a/doc/src/sgml/monitoring.sgml=20= b/doc/src/sgml/monitoring.sgml=0Aindex=2055417a6fa9..00bb423288=20100644=0A= ---=20a/doc/src/sgml/monitoring.sgml=0A+++=20= b/doc/src/sgml/monitoring.sgml=0A@@=20-5888,7=20+5888,7=20@@=20FROM=20= pg_stat_get_backend_idset()=20AS=20backendid;=0A=20=20=20=20=20=20=20=20= relid=20oid=0A=20=20=20=20=20=20=20= =0A=20=20=20=20=20=20=20=0A-=20=20=20=20=20=20=20OID=20of=20= the=20table=20on=20which=20the=20index=20is=20being=20created.=0A+=20=20=20= =20=20=20=20OID=20of=20the=20table=20on=20which=20the=20command=20was=20= run.=0A=20=20=20=20=20=20=20=0A=20=20=20=20=20=20=0A= =20=0A@@=20-5992,8=20+5992,7=20@@=20FROM=20pg_stat_get_backend_idset()=20= AS=20backendid;=0A=20=20=20=20=20=20=20=0A=20=20=20=20=20=20=20=20= Total=20number=20of=20partitions=20on=20which=20the=20index=20is=20to=20= be=20created=0A=20=20=20=20=20=20=20=20or=20attached,=20including=20both=20= direct=20and=20indirect=20partitions.=0A-=20=20=20=20=20=20=20= 0=20during=20a=20REINDEX,=20or=20= when=0A-=20=20=20=20=20=20=20the=20index=20is=20not=20partitioned.=0A+=20= =20=20=20=20=20=200=20when=20the=20index=20is=20not=20= partitioned.=0A=20=20=20=20=20=20=20=0A=20=20=20=20=20=20= =0A=20=0A@@=20-6004,8=20+6003,7=20@@=20FROM=20= pg_stat_get_backend_idset()=20AS=20backendid;=0A=20=20=20=20=20=20=20= =0A=20=20=20=20=20=20=20=20Number=20of=20partitions=20on=20which=20= the=20index=20has=20already=20been=20created=0A=20=20=20=20=20=20=20=20= or=20attached,=20including=20both=20direct=20and=20indirect=20= partitions.=0A-=20=20=20=20=20=20=200=20during=20a=20= REINDEX,=20or=20when=0A-=20=20=20=20=20=20=20the=20= index=20is=20not=20partitioned.=0A+=20=20=20=20=20=20=20= 0=20when=20the=20index=20is=20not=20partitioned.=0A=20= =20=20=20=20=20=20=0A=20=20=20=20=20=20=0A=20=20=20=20= =20=0Adiff=20--git=20a/src/backend/catalog/index.c=20= b/src/backend/catalog/index.c=0Aindex=20a819b4197c..90488f9140=20100644=0A= ---=20a/src/backend/catalog/index.c=0A+++=20= b/src/backend/catalog/index.c=0A@@=20-3560,6=20+3560,7=20@@=20= reindex_index(const=20ReindexStmt=20*stmt,=20Oid=20indexId,=0A=20=09= volatile=20bool=20skipped_constraint=20=3D=20false;=0A=20=09PGRUsage=09= ru0;=0A=20=09bool=09=09progress=20=3D=20((params->options=20&=20= REINDEXOPT_REPORT_PROGRESS)=20!=3D=200);=0A+=09bool=09=09partition=20=3D=20= ((params->options=20&=20REINDEXOPT_PARTITION)=20!=3D=200);=0A=20=09bool=09= =09set_tablespace=20=3D=20false;=0A=20=0A=20=09pg_rusage_init(&ru0);=0A= @@=20-3605,8=20+3606,9=20@@=20reindex_index(const=20ReindexStmt=20*stmt,=20= Oid=20indexId,=0A=20=09=09=09indexId=0A=20=09=09};=0A=20=0A-=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=0A-=09=09=09= =09=09=09=09=09=09=20=20heapId);=0A+=09=09if=20(!partition)=0A+=09=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=0A+=09=09=09= =09=09=09=09=09=09=09=20=20heapId);=0A=20=09=09= pgstat_progress_update_multi_param(2,=20progress_cols,=20progress_vals);=0A= =20=09}=0A=20=0A@@=20-3846,8=20+3848,11=20@@=20reindex_index(const=20= ReindexStmt=20*stmt,=20Oid=20indexId,=0A=20=09index_close(iRel,=20= NoLock);=0A=20=09table_close(heapRelation,=20NoLock);=0A=20=0A-=09if=20= (progress)=0A+=09if=20(progress=20&&=20!partition)=0A+=09{=0A+=09=09/*=20= progress=20for=20partitions=20is=20tracked=20in=20the=20caller=20*/=0A=20= =09=09pgstat_progress_end_command();=0A+=09}=0A=20}=0A=20=0A=20/*=0Adiff=20= --git=20a/src/backend/commands/indexcmds.c=20= b/src/backend/commands/indexcmds.c=0Aindex=20c5a56c75f6..d4c1811f50=20= 100644=0A---=20a/src/backend/commands/indexcmds.c=0A+++=20= b/src/backend/commands/indexcmds.c=0A@@=20-3217,9=20+3217,16=20@@=20= ReindexPartitions(const=20ReindexStmt=20*stmt,=20Oid=20relid,=20const=20= ReindexParams=20*param=0A=20=09ListCell=20=20=20*lc;=0A=20=09= ErrorContextCallback=20errcallback;=0A=20=09ReindexErrorInfo=20errinfo;=0A= +=09ReindexParams=20newparams;=0A+=09Oid=09=09=09heapId=20=3D=20relid;=0A= =20=0A=20=09Assert(RELKIND_HAS_PARTITIONS(relkind));=0A=20=0A+=09if=20= (relkind=20=3D=3D=20RELKIND_PARTITIONED_INDEX)=0A+=09{=0A+=09=09heapId=20= =3D=20IndexGetRelation(relid,=20true);=0A+=09}=0A+=0A=20=09/*=0A=20=09=20= *=20Check=20if=20this=20runs=20in=20a=20transaction=20block,=20with=20an=20= error=20callback=20to=0A=20=09=20*=20provide=20more=20context=20under=20= which=20a=20problem=20happens.=0A@@=20-3278,11=20+3285,33=20@@=20= ReindexPartitions(const=20ReindexStmt=20*stmt,=20Oid=20relid,=20const=20= ReindexParams=20*param=0A=20=09=09MemoryContextSwitchTo(old_context);=0A=20= =09}=0A=20=0A+=0A+=09{=0A+=09=09int=09=09=09progress_params[3]=20=3D=20{=0A= +=09=09=09PROGRESS_CREATEIDX_COMMAND,=0A+=09=09=09= PROGRESS_CREATEIDX_PHASE,=0A+=09=09=09= PROGRESS_CREATEIDX_PARTITIONS_TOTAL=0A+=09=09};=0A+=09=09int64=09=09= progress_values[3];=0A+=0A+=09=09progress_values[0]=20=3D=20= (params->options=20&=20REINDEXOPT_CONCURRENTLY)=20!=3D=200=20?=0A+=09=09=09= PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY=20:=0A+=09=09=09= PROGRESS_CREATEIDX_COMMAND_REINDEX;=0A+=09=09progress_values[1]=20=3D=20= 0;=0A+=09=09progress_values[2]=20=3D=20list_length(partitions);=0A+=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=20heapId);=0A= +=09=09pgstat_progress_update_multi_param(3,=20progress_params,=20= progress_values);=0A+=09}=0A+=0A=20=09/*=0A=20=09=20*=20Process=20each=20= partition=20listed=20in=20a=20separate=20transaction.=20=20Note=20that=0A= =20=09=20*=20this=20commits=20and=20then=20starts=20a=20new=20= transaction=20immediately.=0A=20=09=20*/=0A-=09= ReindexMultipleInternal(stmt,=20partitions,=20params);=0A+=09newparams=20= =3D=20*params;=0A+=09newparams.options=20|=3D=20REINDEXOPT_PARTITION;=0A= +=09ReindexMultipleInternal(stmt,=20partitions,=20&newparams);=0A+=0A+=09= pgstat_progress_end_command();=0A=20=0A=20=09/*=0A=20=09=20*=20Clean=20= up=20working=20storage=20---=20note=20we=20must=20do=20this=20after=0A@@=20= -3303,6=20+3332,7=20@@=20static=20void=0A=20= ReindexMultipleInternal(const=20ReindexStmt=20*stmt,=20const=20List=20= *relids,=20const=20ReindexParams=20*params)=0A=20{=0A=20=09ListCell=20=20= =20*l;=0A+=09bool=09=09partitions=20=3D=20((params->options=20&=20= REINDEXOPT_PARTITION)=20!=3D=200);=0A=20=0A=20=09PopActiveSnapshot();=0A=20= =09CommitTransactionCommand();=0A@@=20-3396,6=20+3426,9=20@@=20= ReindexMultipleInternal(const=20ReindexStmt=20*stmt,=20const=20List=20= *relids,=20const=20Reind=0A=20=09=09}=0A=20=0A=20=09=09= CommitTransactionCommand();=0A+=0A+=09=09if=20(partitions)=0A+=09=09=09= pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,=201);=0A=20= =09}=0A=20=0A=20=09StartTransactionCommand();=0A@@=20-3448,6=20+3481,7=20= @@=20ReindexRelationConcurrently(const=20ReindexStmt=20*stmt,=20Oid=20= relationOid,=20const=20Rein=0A=20=09char=09=20=20=20*relationName=20=3D=20= NULL;=0A=20=09char=09=20=20=20*relationNamespace=20=3D=20NULL;=0A=20=09= PGRUsage=09ru0;=0A+=09bool=09=09partition=20=3D=20((params->options=20&=20= REINDEXOPT_PARTITION)=20!=3D=200);=0A=20=09const=20int=09= progress_index[]=20=3D=20{=0A=20=09=09PROGRESS_CREATEIDX_COMMAND,=0A=20=09= =09PROGRESS_CREATEIDX_PHASE,=0A@@=20-3791,7=20+3825,8=20@@=20= ReindexRelationConcurrently(const=20ReindexStmt=20*stmt,=20Oid=20= relationOid,=20const=20Rein=0A=20=09=09if=20= (indexRel->rd_rel->relpersistence=20=3D=3D=20RELPERSISTENCE_TEMP)=0A=20=09= =09=09elog(ERROR,=20"cannot=20reindex=20a=20temporary=20table=20= concurrently");=0A=20=0A-=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=20= idx->tableId);=0A+=09=09if=20(!partition)=0A+=09=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=20= idx->tableId);=0A=20=0A=20=09=09progress_vals[0]=20=3D=20= PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;=0A=20=09=09= progress_vals[1]=20=3D=200;=09/*=20initializing=20*/=0A@@=20-3966,7=20= +4001,8=20@@=20ReindexRelationConcurrently(const=20ReindexStmt=20*stmt,=20= Oid=20relationOid,=20const=20Rein=0A=20=09=09=20*=20Update=20progress=20= for=20the=20index=20to=20build,=20with=20the=20correct=20parent=0A=20=09=09= =20*=20table=20involved.=0A=20=09=09=20*/=0A-=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=20= newidx->tableId);=0A+=09=09if=20(!partition)=0A+=09=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=20= newidx->tableId);=0A=20=09=09progress_vals[0]=20=3D=20= PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;=0A=20=09=09= progress_vals[1]=20=3D=20PROGRESS_CREATEIDX_PHASE_BUILD;=0A=20=09=09= progress_vals[2]=20=3D=20newidx->indexId;=0A@@=20-4030,7=20+4066,8=20@@=20= ReindexRelationConcurrently(const=20ReindexStmt=20*stmt,=20Oid=20= relationOid,=20const=20Rein=0A=20=09=09=20*=20Update=20progress=20for=20= the=20index=20to=20build,=20with=20the=20correct=20parent=0A=20=09=09=20= *=20table=20involved.=0A=20=09=09=20*/=0A-=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=20= newidx->tableId);=0A+=09=09if=20(!partition)=0A+=09=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=20= newidx->tableId);=0A=20=09=09progress_vals[0]=20=3D=20= PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;=0A=20=09=09= progress_vals[1]=20=3D=20PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN;=0A=20= =09=09progress_vals[2]=20=3D=20newidx->indexId;=0A@@=20-4265,7=20+4302,8=20= @@=20ReindexRelationConcurrently(const=20ReindexStmt=20*stmt,=20Oid=20= relationOid,=20const=20Rein=0A=20=0A=20=09= MemoryContextDelete(private_context);=0A=20=0A-=09= pgstat_progress_end_command();=0A+=09if=20(!partition)=0A+=09=09= pgstat_progress_end_command();=0A=20=0A=20=09return=20true;=0A=20}=0A= diff=20--git=20a/src/include/catalog/index.h=20= b/src/include/catalog/index.h=0Aindex=207d434f8e65..ccba65fbbf=20100644=0A= ---=20a/src/include/catalog/index.h=0A+++=20= b/src/include/catalog/index.h=0A@@=20-42,6=20+42,7=20@@=20typedef=20= struct=20ReindexParams=0A=20#define=20REINDEXOPT_REPORT_PROGRESS=200x02=20= /*=20report=20pgstat=20progress=20*/=0A=20#define=20= REINDEXOPT_MISSING_OK=20=090x04=09/*=20skip=20missing=20relations=20*/=0A= =20#define=20REINDEXOPT_CONCURRENTLY=090x08=09/*=20concurrent=20mode=20= */=0A+#define=20REINDEXOPT_PARTITION=200x10=20/*=20reindexing=20is=20= done=20as=20part=20of=20partitioned=20table/index=20reindex=20*/=0A=20=0A= =20/*=20state=20info=20for=20validate_index=20bulkdelete=20callback=20*/=0A= =20typedef=20struct=20ValidateIndexState=0A--=20=0A2.39.3=20(Apple=20= Git-146)=0A=0A= --Apple-Mail=_83E2974A-0797-4388-8AFC-6B42B3130560 Content-Transfer-Encoding: 7bit Content-Type: text/html; charset=us-ascii
--Apple-Mail=_83E2974A-0797-4388-8AFC-6B42B3130560 Content-Disposition: attachment; filename=v2-0002-partition_relid-column-for-create-index-progress.patch Content-Type: application/octet-stream; x-unix-mode=0644; name="v2-0002-partition_relid-column-for-create-index-progress.patch" Content-Transfer-Encoding: quoted-printable =46rom=20de67cad872046ae5bae6e3a0a6033594b9d1ee86=20Mon=20Sep=2017=20= 00:00:00=202001=0AFrom:=20Ilya=20Gladyshev=20= =0ADate:=20Sun,=2021=20Jul=202024=2001:40:15=20= +0100=0ASubject:=20[PATCH=20v2=202/2]=20partition_relid=20column=20for=20= create=20index=20progress=0A=0A---=0A=20doc/src/sgml/monitoring.sgml=20=20= =20=20=20=20=20=20=20|=20=209=20+++++++++=0A=20= src/backend/catalog/index.c=20=20=20=20=20=20=20=20=20=20|=20=208=20= +++++---=0A=20src/backend/catalog/system_views.sql=20|=20=203=20++-=0A=20= src/backend/commands/indexcmds.c=20=20=20=20=20|=2018=20= +++++++++++++-----=0A=20src/include/commands/progress.h=20=20=20=20=20=20= |=20=201=20+=0A=20src/test/regress/expected/rules.out=20=20|=20=203=20= ++-=0A=206=20files=20changed,=2032=20insertions(+),=2010=20deletions(-)=0A= =0Adiff=20--git=20a/doc/src/sgml/monitoring.sgml=20= b/doc/src/sgml/monitoring.sgml=0Aindex=2000bb423288..595f19a8f8=20100644=0A= ---=20a/doc/src/sgml/monitoring.sgml=0A+++=20= b/doc/src/sgml/monitoring.sgml=0A@@=20-6006,6=20+6006,15=20@@=20FROM=20= pg_stat_get_backend_idset()=20AS=20backendid;=0A=20=20=20=20=20=20=20=20= 0=20when=20the=20index=20is=20not=20partitioned.=0A=20= =20=20=20=20=20=20=0A=20=20=20=20=20=20=0A+=20=20=20= =20=20=0A+=20=20=20=20=20=20=0A+=20=20= =20=20=20=20=20partition_relid=20= bigint=0A+=20=20=20=20=20=20=0A+=20=20=20=20=20=20= =0A+=20=20=20=20=20=20=20OID=20of=20the=20partition=20on=20which=20= the=20index=20is=20currently=20being=20created.=0A+=20=20=20=20=20=20=20= 0=20when=20the=20index=20is=20not=20partitioned.=0A+=20= =20=20=20=20=20=0A+=20=20=20=20=20=0A=20=20=20=20=20= =0A=20=20=20=20=0A=20=20=20=0Adiff=20--git=20= a/src/backend/catalog/index.c=20b/src/backend/catalog/index.c=0Aindex=20= 90488f9140..6789e61f98=20100644=0A---=20a/src/backend/catalog/index.c=0A= +++=20b/src/backend/catalog/index.c=0A@@=20-3599,17=20+3599,19=20@@=20= reindex_index(const=20ReindexStmt=20*stmt,=20Oid=20indexId,=0A=20=09{=0A=20= =09=09const=20int=09progress_cols[]=20=3D=20{=0A=20=09=09=09= PROGRESS_CREATEIDX_COMMAND,=0A-=09=09=09PROGRESS_CREATEIDX_INDEX_OID=0A+=09= =09=09PROGRESS_CREATEIDX_INDEX_OID,=0A+=09=09=09= PROGRESS_CREATEIDX_PARTITION_RELID=0A=20=09=09};=0A=20=09=09const=20= int64=20progress_vals[]=20=3D=20{=0A=20=09=09=09= PROGRESS_CREATEIDX_COMMAND_REINDEX,=0A-=09=09=09indexId=0A+=09=09=09= indexId,=0A+=09=09=09partition=20?=20heapId=20:=200=0A=20=09=09};=0A=20=0A= =20=09=09if=20(!partition)=0A=20=09=09=09= pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,=0A=20=09=09=09= =09=09=09=09=09=09=09=20=20heapId);=0A-=09=09= pgstat_progress_update_multi_param(2,=20progress_cols,=20progress_vals);=0A= +=09=09pgstat_progress_update_multi_param(3,=20progress_cols,=20= progress_vals);=0A=20=09}=0A=20=0A=20=09/*=0Adiff=20--git=20= a/src/backend/catalog/system_views.sql=20= b/src/backend/catalog/system_views.sql=0Aindex=2019cabc9a47..00b7f3bda8=20= 100644=0A---=20a/src/backend/catalog/system_views.sql=0A+++=20= b/src/backend/catalog/system_views.sql=0A@@=20-1280,7=20+1280,8=20@@=20= CREATE=20VIEW=20pg_stat_progress_create_index=20AS=0A=20=20=20=20=20=20=20= =20=20S.param12=20AS=20tuples_total,=0A=20=20=20=20=20=20=20=20=20= S.param13=20AS=20tuples_done,=0A=20=20=20=20=20=20=20=20=20S.param14=20= AS=20partitions_total,=0A-=20=20=20=20=20=20=20=20S.param15=20AS=20= partitions_done=0A+=20=20=20=20=20=20=20=20S.param15=20AS=20= partitions_done,=0A+=20=20=20=20=20=20=20=20S.param18=20AS=20= partition_relid=0A=20=20=20=20=20FROM=20= pg_stat_get_progress_info('CREATE=20INDEX')=20AS=20S=0A=20=20=20=20=20=20= =20=20=20LEFT=20JOIN=20pg_database=20D=20ON=20S.datid=20=3D=20D.oid;=0A=20= =0Adiff=20--git=20a/src/backend/commands/indexcmds.c=20= b/src/backend/commands/indexcmds.c=0Aindex=20d4c1811f50..284f7a919a=20= 100644=0A---=20a/src/backend/commands/indexcmds.c=0A+++=20= b/src/backend/commands/indexcmds.c=0A@@=20-617,6=20+617,10=20@@=20= DefineIndex(Oid=20tableId,=0A=20=09=09=09=09=09=09=09=09=09=20= PROGRESS_CREATEIDX_COMMAND_CREATE_CONCURRENTLY=20:=0A=20=09=09=09=09=09=09= =09=09=09=20PROGRESS_CREATEIDX_COMMAND_CREATE);=0A=20=09}=0A+=09else=0A+=09= {=0A+=09=09= pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITION_RELID,=20= tableId);=0A+=09}=0A=20=0A=20=09/*=0A=20=09=20*=20No=20index=20OID=20to=20= report=20yet=0A@@=20-3486,9=20+3490,10=20@@=20= ReindexRelationConcurrently(const=20ReindexStmt=20*stmt,=20Oid=20= relationOid,=20const=20Rein=0A=20=09=09PROGRESS_CREATEIDX_COMMAND,=0A=20=09= =09PROGRESS_CREATEIDX_PHASE,=0A=20=09=09PROGRESS_CREATEIDX_INDEX_OID,=0A= -=09=09PROGRESS_CREATEIDX_ACCESS_METHOD_OID=0A+=09=09= PROGRESS_CREATEIDX_ACCESS_METHOD_OID,=0A+=09=09= PROGRESS_CREATEIDX_PARTITION_RELID=0A=20=09};=0A-=09int64=09=09= progress_vals[4];=0A+=09int64=09=09progress_vals[5];=0A=20=0A=20=09/*=0A=20= =09=20*=20Create=20a=20memory=20context=20that=20will=20survive=20forced=20= transaction=20commits=20we=0A@@=20-3832,7=20+3837,8=20@@=20= ReindexRelationConcurrently(const=20ReindexStmt=20*stmt,=20Oid=20= relationOid,=20const=20Rein=0A=20=09=09progress_vals[1]=20=3D=200;=09/*=20= initializing=20*/=0A=20=09=09progress_vals[2]=20=3D=20idx->indexId;=0A=20= =09=09progress_vals[3]=20=3D=20idx->amId;=0A-=09=09= pgstat_progress_update_multi_param(4,=20progress_index,=20= progress_vals);=0A+=09=09progress_vals[4]=20=3D=20partition=20?=20= idx->tableId=20:=200;=0A+=09=09pgstat_progress_update_multi_param(5,=20= progress_index,=20progress_vals);=0A=20=0A=20=09=09/*=20Choose=20a=20= temporary=20relation=20name=20for=20the=20new=20index=20*/=0A=20=09=09= concurrentName=20=3D=20ChooseRelationName(get_rel_name(idx->indexId),=0A= @@=20-4007,7=20+4013,8=20@@=20ReindexRelationConcurrently(const=20= ReindexStmt=20*stmt,=20Oid=20relationOid,=20const=20Rein=0A=20=09=09= progress_vals[1]=20=3D=20PROGRESS_CREATEIDX_PHASE_BUILD;=0A=20=09=09= progress_vals[2]=20=3D=20newidx->indexId;=0A=20=09=09progress_vals[3]=20= =3D=20newidx->amId;=0A-=09=09pgstat_progress_update_multi_param(4,=20= progress_index,=20progress_vals);=0A+=09=09progress_vals[4]=20=3D=20= partition=20?=20newidx->tableId=20:=200;=0A+=09=09= pgstat_progress_update_multi_param(5,=20progress_index,=20= progress_vals);=0A=20=0A=20=09=09/*=20Perform=20concurrent=20build=20of=20= new=20index=20*/=0A=20=09=09index_concurrently_build(newidx->tableId,=20= newidx->indexId);=0A@@=20-4072,7=20+4079,8=20@@=20= ReindexRelationConcurrently(const=20ReindexStmt=20*stmt,=20Oid=20= relationOid,=20const=20Rein=0A=20=09=09progress_vals[1]=20=3D=20= PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN;=0A=20=09=09progress_vals[2]=20= =3D=20newidx->indexId;=0A=20=09=09progress_vals[3]=20=3D=20newidx->amId;=0A= -=09=09pgstat_progress_update_multi_param(4,=20progress_index,=20= progress_vals);=0A+=09=09progress_vals[4]=20=3D=20partition=20?=20= newidx->tableId=20:=200;=0A+=09=09pgstat_progress_update_multi_param(5,=20= progress_index,=20progress_vals);=0A=20=0A=20=09=09= validate_index(newidx->tableId,=20newidx->indexId,=20snapshot);=0A=20=0A= diff=20--git=20a/src/include/commands/progress.h=20= b/src/include/commands/progress.h=0Aindex=205616d64523..6cd8b41a4c=20= 100644=0A---=20a/src/include/commands/progress.h=0A+++=20= b/src/include/commands/progress.h=0A@@=20-89,6=20+89,7=20@@=0A=20#define=20= PROGRESS_CREATEIDX_PARTITIONS_TOTAL=09=0913=0A=20#define=20= PROGRESS_CREATEIDX_PARTITIONS_DONE=09=0914=0A=20/*=2015=20and=2016=20= reserved=20for=20"block=20number"=20metrics=20*/=0A+#define=20= PROGRESS_CREATEIDX_PARTITION_RELID=09=0917=0A=20=0A=20/*=20Phases=20of=20= CREATE=20INDEX=20(as=20advertised=20via=20PROGRESS_CREATEIDX_PHASE)=20*/=0A= =20#define=20PROGRESS_CREATEIDX_PHASE_WAIT_1=09=09=091=0Adiff=20--git=20= a/src/test/regress/expected/rules.out=20= b/src/test/regress/expected/rules.out=0Aindex=204c789279e5..2bcb72c014=20= 100644=0A---=20a/src/test/regress/expected/rules.out=0A+++=20= b/src/test/regress/expected/rules.out=0A@@=20-2027,7=20+2027,8=20@@=20= pg_stat_progress_create_index|=20SELECT=20s.pid,=0A=20=20=20=20=20= s.param12=20AS=20tuples_total,=0A=20=20=20=20=20s.param13=20AS=20= tuples_done,=0A=20=20=20=20=20s.param14=20AS=20partitions_total,=0A-=20=20= =20=20s.param15=20AS=20partitions_done=0A+=20=20=20=20s.param15=20AS=20= partitions_done,=0A+=20=20=20=20s.param18=20AS=20partition_relid=0A=20=20= =20=20FROM=20(pg_stat_get_progress_info('CREATE=20INDEX'::text)=20s(pid,=20= datid,=20relid,=20param1,=20param2,=20param3,=20param4,=20param5,=20= param6,=20param7,=20param8,=20param9,=20param10,=20param11,=20param12,=20= param13,=20param14,=20param15,=20param16,=20param17,=20param18,=20= param19,=20param20)=0A=20=20=20=20=20=20LEFT=20JOIN=20pg_database=20d=20= ON=20((s.datid=20=3D=20d.oid)));=0A=20pg_stat_progress_vacuum|=20SELECT=20= s.pid,=0A--=20=0A2.39.3=20(Apple=20Git-146)=0A=0A= --Apple-Mail=_83E2974A-0797-4388-8AFC-6B42B3130560 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8


21 =D0=B8=D1=8E=D0=BB=D1=8F 2024=E2=80=AF=D0=B3., =D0=B2= 01:49, Ilya Gladyshev <ilya.v.gladyshev@gmail.com> = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):



19 = =D0=B8=D1=8E=D0=BB=D1=8F 2024=E2=80=AF=D0=B3., =D0=B2 04:17, Michael = Paquier <michael@paquier.xyz> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81= =D0=B0=D0=BB(=D0=B0):

On Fri, Jul 12, 2024 at = 11:07:49PM +0100, Ilya Gladyshev wrote:
While= working on CIC for partitioned tables [1], I noticed that REINDEX = for
partitioned tables is not tracking keeping progress of = partitioned tables,
so I'm creating a separate thread for this fix as = suggested.

This limitation is not a bug, because we = already document that
partitions_total and partitions_done are 0 = during a REINDEX.  Compared
to CREATE INDEX, a REINDEX is a bit = wilder as it would work on all the
indexes for all the partitions, = providing this information makes
sense.

Agreed that this could = be better, but what's now on HEAD is not = wrong
either.

Thanks for pointing it out, I didn=E2=80=99t notice it. = You=E2=80=99re right, this not a bug, but an improvement. Will remove = the bits from the documentation as well.


Still it does not do = it because we know that the fields are going to
be overwritten pretty = quickly anyway, no?  For now, we could just do
without = progress_index_partition_done(), I guess, keeping it to = the
incrementation of PROGRESS_CREATEIDX_PARTITIONS_DONE. =  There's an
argument that this makes the code slightly easier to = follow, with less
wrappers around the progress = reporting.

The use-case that I thought this would improve is = REINDEX CONCURRENT, where data from later stages of the previous = partitions would linger for quite a while before it gets to the same = stage of the current partition. I don=E2=80=99t think this is of big = importance, so I=E2=80=99m ok with making code simpler and leaving it = out.


+ int progress_params[3] =3D = {
+ = = PROGRESS_CREATEIDX_COMMAND,
+ = PROGRESS_CREATEIDX_PHASE,
+ = PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+ = };
+ int64 progress_values[3];
+ Oid heapId =3D relid;

Rather = than having new code blocks, let's use a style consistent = with
DefineIndex() where we have the = pgstat_progress_update_multi_param(),
with a single {} = block.

Fixed.

Adding the counter = increment at the end of the loop in
ReindexMultipleInternal() is a = good idea.  It considers both the
concurrent and non-concurrent = cases.

Another reason to do so is that reindex_relation calls = itself recursively, so it'd require some additional mechanism so that it = wouldn't increment multiple times per partition.

+   progress_values[2] =3D = list_length(partitions);
+ =   pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, = heapId);

Hmm.  Is setting the relid only once for = pg_stat_progress_create_index
the best choice there is?  Could = it be better to report the partition
OID instead? 

Technically, we could do this, but = it looks like no other commands do it and currently there=E2=80=99s no = API to do it without erasing the rest of progress = information.

 Let's remember that indexes may have been = attached with
names inconsistent with the partitioned table's index. =  It is a bit
confusing to stick to the relid all the partitioned = table all the
time, for all the indexes of all the partitions = reindexed.

I agree that it=E2=80=99s a bit confusing especially = because documentation gives no hints about it. Judging by documentation, = I would expect relid and index_relid to correspond to the same table. = However, if we say that this field represents the oid of the relation on = which the command was invoked, then I think it does make sense. Edited = documentation to say that in the new patch.

 Could it
be better to actually = introduce an entirely new field to the progress
table?  What I = would look for is more information:
1) the partitioned table OID on = which the REINDEX runs
2) the partition table being processed
3) = the index OID being processed (old and new for concurrent = case).

The patch sets 1) to the OID of the partitioned table, = lacks 2) and
sets 3) each time an index is = rebuilt.
=E2=80=94
Michael

I = like this approach more, but I=E2=80=99m not sure whether adding another = field for partition oid is worth it, since we already have index_relid = that we can use to join with pg_index to get that. On the other hand, = index_relid is missing during regular CREATE INDEX, so this new field = could be useful to indicate which table is being indexed in this case. = I'm on the fence about this, attached this as a separate patch, if you = think it's a good idea.

Thank you for the = review!

<v2-0002-partition_reli= d-column-for-create-index-progress.patch><v2-0001-make-REINDEX-t= rack-partition-progress.patch>

= --Apple-Mail=_83E2974A-0797-4388-8AFC-6B42B3130560-- --Apple-Mail=_22C65095-A578-4E34-8C94-14FE9BFA22BE--