Re: [HACKERS] CLUSTER command progress monitor

On Thu, Aug 15, 2019 at 12:48 PM Tatsuro Yamada
<tatsuro.yamada.tf@nttcom.co.jp> wrote:
>
> Hi Michael, Alvaro and Robert!
>
> On 2019/08/14 11:52, Michael Paquier wrote:
> > On Wed, Aug 14, 2019 at 11:38:01AM +0900, Tatsuro Yamada wrote:
> >> On 2019/08/13 14:40, Tatsuro Yamada wrote:
> >>> On 2019/08/02 3:43, Alvaro Herrera wrote:
> >>>> Hmm, I'm trying this out now and I don't see the index_rebuild_count
> >>>> ever go up.  I think it's because the indexes are built using parallel
> >>>> index build ... or maybe it was the table AM changes that moved things
> >>>> around, not sure.  There's a period at the end when the CLUSTER command
> >>>> keeps working, but it's gone from pg_stat_progress_cluster.
> >>>
> >>> Thanks for your report.
> >>> I'll investigate it. :)
> >>
> >> I did "git bisect" and found the commit:
> >>
> >>   03f9e5cba0ee1633af4abe734504df50af46fbd8
> >>   Report progress of REINDEX operations
> >
> > I am adding an open item for this one.
> > --
> > Michael
>
>
> Okay, I checked it on the wiki.
>
>    https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items
>    - index_rebuild_count in CLUSTER reporting never increments
>
> To be clear, 03f9e5cb broke CLUSTER progress reporting, but
> I investigated little more and share my ideas to fix the problem.
>
> * Call stack
> ========================================
> cluster_rel
>    pgstat_progress_start_command(CLUSTER) *A1
>    rebuild_relation
>      finish_heap_swap
>        reindex_relation
>          reindex_index
>            pgstat_progress_start_command(CREATE_INDEX) *B1
>            pgstat_progress_end_command() *B2
>          pgstat_progress_update_param(INDEX_REBUILD_COUNT, i) <- failed :(
>    pgstat_progress_end_command() *A2
>
>    Note
>      These are sets:
>        A1 and A2,
>        B1 and B2
> ========================================
>
>
> * Ideas to fix
>    There are Three options, I guess.
> ========================================
>    1. Call pgstat_progress_start_command(CLUSTER) again
>       before pgstat_progress_update_param(INDEX_REBUILD_COUNT, i).
>
>    2. Add "save and restore" functions for the following two
>       variables of MyBeentry in pgstat.c.
>          - st_progress_command
>          - st_progress_command_target
>
>    3. Use Hash or List to store multiple values for the two
>       variables in pgstat.c.
> ========================================
>
>
> I tried 1. and it shown index_rebuild_count, but it also shown
> "initializing" phase again and other columns were empty. So, it is
> not suitable to fix the problem. :(
> I'm going to try 2. and 3., but, unfortunately, I can't get enough
> time to do that after PGConf.Asia 2019.
> If we selected 3., it affects following these progress reporting
> features: VACUUM, CLUSTER, CREATE_INDEX and ANALYZE. But it's okay,
> I suppose. Any comments welcome! :)

I looked at this open item. I prefer #3 but I think it would be enough
to have a small stack using a fixed length array to track nested
progress information and the current executing command (maybe 4 or 8
would be enough for maximum nested level for now?). That way, we don't
need any change the interfaces. AFAICS there is no case where we call
only either pgstat_progress_start_command or
pgstat_progress_end_command without each other (although
pgstat_progress_update_param is called without start). So I think that
having a small stack for tracking multiple reports would work.

Attached the draft patch that fixes this issue. Please review it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center