Re: [HACKERS] CLUSTER command progress monitor

On 2019/02/23 6:02, Robert Haas wrote:
> On Fri, Dec 28, 2018 at 3:20 AM Tatsuro Yamada
> <yamada.tatsuro@lab.ntt.co.jp> wrote:
>> This patch is rebased on HEAD.
>> I'll tackle revising the patch based on feedbacks next month.
> 
> +   Running <command>VACUUM FULL</command> is listed in
> <structname>pg_stat_progress_cluster</structname>
> +   view because it uses <command>CLUSTER</command> command
> internally.  See <xref linkend='cluster-progress-reporting'>.
> 
> It's not really true to say that VACUUM FULL uses the CLUSTER command
> internally.  It's not really true.  It uses a good chunk of the same
> infrastructure, but it certainly doesn't use the actual command, and
> it's not really the exact same thing either, because internally it's
> doing a sequential scan but no sort, which never happens with CLUSTER.
> I'm not sure exactly how to rephrase this, but I think we need to make
> it more precise.
> 
> One idea is that maybe we should try to think of a design that could
> also handle the rewriting variants of ALTER TABLE, and call it
> pg_stat_progress_rewrite.  Maybe that's moving the goalposts too far,
> but I'm not saying we'd necessarily have to do all the work now, just
> have a view that we think could also handle that.  Then again, maybe
> the needs are too different.
> 

Hmm..., I see.
If possible, I'd like to stop thinking of VACUUM FULL to avoid complication of
the implementation.
For now, I haven't enough time to design pg_stat_progress_rewrite. I suppose that
it's tough work.


> +   Whenever <command>CLUSTER</command> is running, the
> +   <structname>pg_stat_progress_cluster</structname> view will contain
> +   one row for each backend that is currently clustering or vacuuming
> (VACUUM FULL).
> 
> That sentence contradicts itself.  Just say that it contains a row for
> each backend that is currently running CLUSTER or VACUUM FULL.
> 

Fixed.


> @@ -105,6 +107,7 @@ static void reform_and_rewrite_tuple(HeapTuple tuple,
>   void
>   cluster(ClusterStmt *stmt, bool isTopLevel)
>   {
> +
>    if (stmt->relation != NULL)
>    {
>    /* This is the single-relation case. */
> 
> Useless hunk.
> 

Fixed.


> @@ -186,7 +189,9 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
>    heap_close(rel, NoLock);
> 
>    /* Do the job. */
> + pgstat_progress_start_command(PROGRESS_COMMAND_CLUSTER, tableOid);
>    cluster_rel(tableOid, indexOid, stmt->options);
> + pgstat_progress_end_command();
>    }
>    else
>    {
> 
> It seems like that stuff should be inside cluster_rel().
>

Fixed.


> + /* Set reltuples to total_tuples */
> + pgstat_progress_update_param(PROGRESS_CLUSTER_TOTAL_HEAP_TUPLES,
> OldHeap->rd_rel->reltuples);
> 
> I object.  If the user wants that, they can get it from pg_class
> themselves via an SQL query.  It's also an estimate, not something we
> know to be accurate; I want us to only report facts here, not theories

I understand that progress monitor should only report facts, so I
removed that code.



> + pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
> PROGRESS_CLUSTER_PHASE_SCAN_HEAP_AND_WRITE_NEW_HEAP);
> + pgstat_progress_update_param(PROGRESS_CLUSTER_SCAN_METHOD,
> PROGRESS_CLUSTER_METHOD_INDEX_SCAN);
> 
> I think you should use pgstat_progress_update_multi_param() if
> updating multiple parameters at the same time.
> 
> Also, some lines in this patch, such as this one, are very long.
> Consider techniques to reduce the line length to 80 characters or
> less, such as inserting a line break between the two arguments.

Fixed.


> + /* Set scan_method to NULL */
> + pgstat_progress_update_param(PROGRESS_CLUSTER_SCAN_METHOD, -1);
> 
> NULL and -1 are not the same thing.

Oops, fixed.


> I think that we shouldn't have both
> PROGRESS_CLUSTER_PHASE_SCAN_HEAP_AND_WRITE_NEW_HEAP and
> PROGRESS_CLUSTER_PHASE_SCAN_HEAP.  They're the same thing.  Let's just
> use PROGRESS_CLUSTER_PHASE_SCAN_HEAP for both.  Actually, better yet,
> let's get rid of PROGRESS_CLUSTER_SCAN_METHOD and have
> PROGRESS_CLUSTER_PHASE_SEQ_SCAN_HEAP and
> PROGRESS_CLUSTER_PHASE_INDEX_SCAN_HEAP.  That seems noticeably
> simpler.

Fixed.


> I agree that it's acceptable to report
> PROGRESS_CLUSTER_HEAP_TUPLES_VACUUMED and
> PROGRESS_CLUSTER_HEAP_TUPLES_RECENTLY_DEAD, but I'm not sure I
> understand why it's valuable to do so in the context of a progress
> indicator.

Actually, I'm not sure why I added it since so much time has passed. :(
So, I'll remove PROGRESS_CLUSTER_HEAP_TUPLES_RECENTLY_DEAD at least.


Attached patch is wip patch.


Thanks!
Tatsuro Yamada