Re: [PROPOSAL] VACUUM Progress Checker.

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