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
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
