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

On 2015/10/29 23:22, Syed, Rahila wrote:
> Please find attached an updated patch.

A few more comments on v6:

>      relname = RelationGetRelationName(onerel);
> +    schemaname = get_namespace_name(RelationGetNamespace(onerel));
>      ereport(elevel,
>              (errmsg("vacuuming \"%s.%s\"",
>                      get_namespace_name(RelationGetNamespace(onerel)),
>                      relname)));
> +    snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s", schemaname);
> +    strcat(progress_message[0],".");
> +    strcat(progress_message[0],relname);

How about the following instead -

+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s",
+                     generate_relation_name(onerel));

>      if (next_not_all_visible_block >= SKIP_PAGES_THRESHOLD)
> +    {
>          skipping_all_visible_blocks = true;
> +        if(!scan_all)
> +            total_heap_pages = total_heap_pages - next_not_all_visible_block;
> +    }
>      else
>          skipping_all_visible_blocks = false;

...

>               */
>              if (next_not_all_visible_block - blkno > SKIP_PAGES_THRESHOLD)
> +            {
>                  skipping_all_visible_blocks = true;
> +                if(!scan_all)
> +                    total_heap_pages = total_heap_pages - (next_not_all_visible_block - blkno);
> +            }

Fujii-san's review comment about these code blocks does not seem to be
addressed. He suggested to keep total_heap_pages fixed while adding number
of skipped pages to that of scanned pages. For that, why not add a
scanned_heap_pages variable instead of using vacrelstats->scanned_pages.

> +        if (has_privs_of_role(GetUserId(), beentry->st_userid))
> +        {
> +            values[2] = UInt32GetDatum(beentry->st_progress_param[0]);
> +            values[3] = UInt32GetDatum(beentry->st_progress_param[1]);
> +            values[4] = UInt32GetDatum(beentry->st_progress_param[2]);
> +            values[5] = UInt32GetDatum(beentry->st_progress_param[3]);
> +            values[6] = UInt32GetDatum(total_pages);
> +            values[7] = UInt32GetDatum(scanned_pages);
> +
> +            if (total_pages != 0)
> +                values[8] = Float8GetDatum(scanned_pages * 100 / total_pages);
> +            else
> +                nulls[8] = true;
> +        }
> +        else
> +        {
> +            values[2] = CStringGetTextDatum("<insufficient privilege>");
> +            nulls[3] = true;
> +            nulls[4] = true;
> +            nulls[5] = true;
> +            nulls[6] = true;
> +            nulls[7] = true;
> +            nulls[8] = true;
> +        }

This is most likely not correct, that is, putting a text datum into
supposedly int4 column. I see this when I switch to a unprivileged user:

pgbench=# \x
pgbench=# \c - other
pgbench=> SELECT * FROM pg_stat_vacuum_progress;
-[ RECORD 1 ]-------+------------------------
pid                 | 20395
table_name          | public.pgbench_accounts
total_heap_pages    | 44895488
scanned_heap_pages  |
total_index_pages   |
scanned_index_pages |
total_pages         |
scanned_pages       |
percent_complete    |

I'm not sure if applying the privilege check for columns of
pg_stat_vacuum_progress is necessary, but I may be wrong.

Thanks,
Amit