Re: Add index scan progress to pg_stat_progress_vacuum

On 1/10/22, 5:01 PM, "Imseih (AWS), Sami" <simseih@amazon.com> wrote:
> I have attached the 3rd revision of the patch which also includes the documentation changes. Also attached is a
renderedhtml of the docs for review.
 
>
> "max_index_vacuum_cycle_time" has been removed.
> "index_rows_vacuumed" renamed to "index_tuples_removed". "tuples" is a more consistent with the terminology used.
> "vacuum_cycle_ordinal_position" renamed to "index_ordinal_position".

Thanks for the new version of the patch!

nitpick: I get one whitespace error when applying the patch.

        Applying: Expose progress for the "vacuuming indexes" phase of a VACUUM operation.
        .git/rebase-apply/patch:44: tab in indent.
           Whenever <xref linkend="guc-vacuum-failsafe-age"/> is triggered, index
        warning: 1 line adds whitespace errors.

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>num_indexes_to_vacuum</structfield> <type>bigint</type>
+      </para>
+      <para>
+       The number of indexes that will be vacuumed. Only indexes with
+       <literal>pg_index.indisready</literal> set to "true" will be vacuumed.
+       Whenever <xref linkend="guc-vacuum-failsafe-age"/> is triggered, index
+       vacuuming will be bypassed.
+      </para></entry>
+     </row>
+    </tbody>
+   </tgroup>
+  </table>

We may want to avoid exhaustively listing the cases when this value
will be zero.  I would suggest saying, "When index cleanup is skipped,
this value will be zero" instead.

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>relid</structfield> <type>oid</type>
+      </para>
+      <para>
+       OID of the table being vacuumed.
+      </para></entry>
+     </row>

Do we need to include this field?  I would expect indexrelid to go
here.

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>leader_pid</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Process ID of the parallel group leader. This field is <literal>NULL</literal>
+       if this process is a parallel group leader or the
+       <literal>vacuuming indexes</literal> phase is not performed in parallel.
+      </para></entry>
+     </row>

Are there cases where the parallel group leader will have an entry in
this view when parallelism is enabled?

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>index_ordinal_position</structfield> <type>bigint</type>
+      </para>
+      <para>
+       The order in which the index is being vacuumed. Indexes are vacuumed by OID in ascending order.
+      </para></entry>
+     </row>

Should we include the bit about the OID ordering?  I suppose that is
unlikely to change in the near future, but I don't know if it is
relevant information.  Also, do we need to include the "index_"
prefix?  This view is specific for indexes.  (I have the same question
for index_tuples_removed.)

Should this new table go after the "VACUUM phases" table?  It might
make sense to keep the phases table closer to where it is referenced.

+    /* Advertise the number of indexes to vacuum if we are not in failsafe mode */
+    if (!lazy_check_wraparound_failsafe(vacrel))
+        pgstat_progress_update_param(PROGRESS_VACUUM_TOTAL_INDEX_VACUUM, vacrel->nindexes);

Shouldn't this be 0 when INDEX_CLEANUP is off, too?

+#define PROGRESS_VACUUM_CURRENT_INDRELID         7
+#define PROGRESS_VACUUM_LEADER_PID               8
+#define PROGRESS_VACUUM_INDEX_ORDINAL            9
+#define PROGRESS_VACUUM_TOTAL_INDEX_VACUUM       10
+#define PROGRESS_VACUUM_DEAD_TUPLES_VACUUMED     11

nitpick: I would suggest the following names to match the existing
style:

        PROGRESS_VACUUM_NUM_INDEXES_TO_VACUUM
        PROGRESS_VACUUM_INDEX_LEADER_PID
        PROGRESS_VACUUM_INDEX_INDEXRELID
        PROGRESS_VACUUM_INDEX_ORDINAL_POSITION
        PROGRESS_VACUUM_INDEX_TUPLES_REMOVED

Nathan