Re: [PROPOSAL] VACUUM Progress Checker. - Mailing list pgsql-hackers

Hi Amit,

Thank you for updating the patch.

> -----Original Message-----
> From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]
> Sent: Thursday, March 10, 2016 3:36 PM
> To: Robert Haas <robertmhaas@gmail.com>
> Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>; Amit Langote
> <amitlangote09@gmail.com>; SPS ポクレ ヴィナヤック(三技術)
> <pokurev@pm.nttdata.co.jp>; pgsql-hackers@postgresql.org; SPS 坂野 昌
> 平(三技術) <bannos@nttdata.co.jp>
> Subject: Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.
> 
> On 2016/03/10 14:29, Amit Langote wrote:
> > I rebased remainder patches (attached).
> >
> > 0001 is a small patch to fix issues reported by Tomas and Vinayak.
> > 0002 and 0003 are WIP patches to implement progress reporting for
> vacuum.
> 
> Oops, in 0002, I wrongly joined with pg_class in the definition of
> pg_stat_progress_vacuum to output the schema-qualified name of the table
> being vacuumed.  That means we need to connect to the correct database,
> which is undesirable. Updated version fixes that (shows database name and
> relid).  You may also have noticed that I said pg_stat_progress_vacuum, not
> pg_stat_vacuum_progress (IMHO, the former is a better name).
> 
> Updated patches attached.
In 0002-
+CREATE VIEW pg_stat_progress_vacuum AS
+    SELECT
+            S.pid AS pid,
+            D.datname AS database,
+            S.relid AS relid,
.
.
.
.
+    FROM pg_database D, pg_stat_get_progress_info('VACUUM') AS S
+    WHERE S.datid = D.oid;
I think we need to use datid instead of datname.
Robert added datid in pg_stat_get_progress_info() and we are using that function here.
+values[1] = ObjectIdGetDatum(beentry->st_databaseid);

+DATA(insert OID = 3318 (  pg_stat_get_progress_info           PGNSP PGUID 12 1 100 0 0 f f f f f t s r 1 0 2249 "25"
"{25,23,26,26,20,20,20,20,20,20,20,20,20,20}""{i,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{cmdtype,pid,datid,relid,param1,param2,param3,param4,param5,param6,param7,param8,param9,param10}"_null_ _null_
pg_stat_get_progress_info_null_ _null_ _null_ ));
 

So I think it's better to report datid not datname.
The definition of view is simply like:
+CREATE VIEW pg_stat_progress_vacuum AS
+    SELECT
+            S.pid AS pid,
+            S.datid AS datid,
+            S.relid AS relid,
+            CASE S.param1
+                WHEN 1 THEN 'scanning heap'
+                WHEN 2 THEN 'vacuuming indexes'
+                WHEN 3 THEN 'vacuuming heap'
+                WHEN 4 THEN 'cleanup'
+                ELSE 'unknown phase'
+            END AS processing_phase,
+            S.param2 AS total_heap_blocks,
+            S.param3 AS current_heap_block,
+            S.param4 AS total_index_blocks,
+            S.param5 AS index_blocks_done,
+            S.param6 AS index_vacuum_count,
+            CASE S.param2
+                WHEN 0 THEN round(100.0, 2)
+                ELSE round((S.param3 + 1) * 100.0 / S.param2, 2)
+            END AS percent_done
+    FROM pg_stat_get_progress_info('VACUUM') AS S;

In the pg_stat_activity view, datid and datname are the separate columns. So maybe we can add datname as separate
columnin pg_stat_progress_vacuum, but I think it's not required only datid is sufficient.
 
Any comment?

Regards,
Vinayak Pokale