Feedback-ID: ie3de48e3:Fastmail
Date: Sun, 13 Sep 2026 14:52:54 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Fujii Masao <masao.fujii@gmail.com>
Cc: Adam Lee <adam8157@gmail.com>, pgsql-hackers@lists.postgresql.org,
	Antonin Houska <ah@cybertec.at>
Subject: Re: CLUSTER progress: wrong index_rebuild_count for tables with TOAST
Message-ID: <aqaXjrKBgrdGSN5z@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAHGQGwFUsrBvTurkSU8TEc=DZTunqteUXrasseqNMT7AMBZKRw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aqaXjrKBgrdGSN5z%40alvherre.pgsql>
Precedence: bulk

On 2026-Sep-09, Fujii Masao wrote:

> I think the approach in the patch, i.e., passing INDEX_CREATE_SUPPRESS_PROGRESS
> to suppress progress reporting when creating TOAST indexes, looks good.
> But, adding that test seems overkill to me. I'd prefer to simplify the patch
> as in the attached 0001 patch. Thoughts?

I agree that the test is overkill -- after all, we don't test any of
progress reporting, and I'm not sure it's really a great approach to do
that by adding bespoke injection points.

Your 0001 looks good to me.

Maybe in a future release we can discuss a framework for making progress
updates visible in debug builds, so that they can be observed from a
new test framework.

> While working on this patch, I also found a related but separate issue:
> during REPACK (CONCURRENTLY), index_rebuild_count in
> pg_stat_progress_repack and pg_stat_progress_cluster did not advance
> as indexes were rebuilt. The attached 0002 patch fixes this issue.

Hmm, yeah, this patch looks good also.

I admit that the flow is a bit confusingly different in the concurrent
vs. non-concurrent cases: in the former, finish_heap_swap() is called
with reindex=false, so reindex_relation() is not called from there, and
instead we get these counter updates (with your patch) from
build_new_indexes(); in the concurrent patch, the counter updates come
from inside finish_heap_swap() instead.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"There's no problem so awful that you can't add some
 guilt to it and make it even worse"                     (Calvin [& Hobbes])