On 2015/11/10 17:02, Amit Langote wrote:
> On 2015/10/29 23:22, Syed, Rahila wrote:
>> Please find attached an updated patch.
>
> A few more comments on v6:
I backed up a little, studied the proposal and the patch in little some
more detail. Here are still more comments -
Going through the thread, it seems there are following problems being solved -
1) General purpose interface for (maintenance?) commands to report a set
of internal values of different internal types using shared memory as IPC;
values are exposed to users as is and/or some derived values like
percent_done using view/functions
2) For a start, instrumenting lazy vacuum to report such internal values
And maybe,
3) Estimating the amount of work to be done and time required based on
historical statistics like n_dead_tup, visibility map and run-time
resources available like maintenance_work_mem
Latest version of the patch (v6) implements 1 and 2. The code is starting
to look good though see some comments below.
* Regarding (2): Some random thoughts on the patch and in general -
For lazy vacuum, lazy_scan_heap() seems like the best place which can
provide granular progress report in terms of the heap block number (of
total number of heap blocks in the relation) currently undergoing
per-block pass 1 processing. About pass 2, ie, lazy_index_vacuum() and
lazy_vacuum_heap(), I don't see how we can do better than reporting its
progress only after finishing all of it without any finer-grained
instrumentation. They are essentially block-box as far as the proposed
instrumentation approach is concerned. Being able to report progress per
index seems good but as a whole, a user would have to wait arbitrarily
long before numbers move forward. We might as well just report a bool
saying we're about to enter a potentially time-consuming index vacuum
round with possibly multiple indexes followed by lazy_vacuum_heap()
processing. Additionally, we can report the incremented count of the
vacuuming round (pass 2) once we are through it. So, we'd report two
values viz. waiting_vacuum_pass (bool) and num_vacuum_pass (int). The
former is reported twice - 'true' as we are about to begin the round and
'false' once done. We can keep the total_index_pages (counting all
indexes) and index_pages_done as the patch currently reports. The latter
moves forward for every index we finish processing, and also should be
reset for every pass 2 round. Note that we can leave them out of
percent_done of overall vacuum progress. Until we have a good solution for
number (3) above, it seems to difficult to incorporate index pages into
overall progress.
As someone pointed out upthread, the final heap truncate phase can take
arbitrarily long and is outside the scope of lazy_scan_heap() to
instrument. Perhaps a bool, say, waiting_heap_trunc could be reported for
the same. Note that, it would have to be reported from lazy_vacuum_rel().
I spotted a potential oversight regarding report of scanned_pages. It
seems pages that are skipped because of not getting a pin, being new,
being empty could be left out of the progress equation.
* Regarding (1): These are mostly code comments -
IMHO, float progress parameters (st_progress_param_float[]) can be taken
out. They are currently unused and it's unlikely that some command would
want to report them. OTOH, as suggested in above paragraph, why not have
bool parameters? In addition to a few I mentioned in the context of lazy
vacuum instrumentation, it seems likely that they would be useful for
other commands, too.
Instead of st_activity_flag, how about st_command and calling
ACTIVITY_IS_VACUUM, say, COMMAND_LAZY_VACUUM?
pgstat_report_activity_flag() then would become pgstat_report_command().
Like floats, I would think we could take out st_progress_message[][]. I
see that it is currently used to report table name. For that, we might as
well add a single st_command_target[NAMEDATALEN] string which is set at
the beginning of command processing using, say,
pgstat_report_command_target(). It stores the name of relation/object that
the command is going to work on.
Maybe, we don't need each command to proactively pgstat_reset_command().
That would be similar to how st_activity is not proactively cleared but is
rather reset by the next query/command or when some other backend uses the
shared memory slot. Also, we could have a SQL function
pg_stat_reset_local_progress() which clears the st_command after which the
backend is no longer shown in the progress view.
I think it would be better to report only changed parameter arrays when
performing pgstat_report_progress(). So, if we have following shared
memory progress parameters and the reporting function signature:
typedef struct PgBackendStatus
{
...
uint16 st_command;
char st_command[NAMEDATALEN];
uint32 st_progress_uint32_param[N_PROGRESS_PARAM];
bool st_progress_bool_param[N_PROGRESS_PARAM];
} PgBackendStatus;
void pgstat_report_progress(uint32 *uint32_param, int num_uint32_param,
bool *bool_param, int num_bool_param);
and if we need to report a bool parameter change, say, waiting_vacuum_pass
in lazy_scan_heap(), we do -
pgstat_report_progress(NULL, 0, progress_bool_param, 2);
That is, no need for pgstat_report_progress() to overwrite the shared
st_progress_uint32_param if none of its members have changed since the
last report.
Currently, ACTIVITY_IS_VACUUM is reported even for VACOPT_ANALYZE and
VACOPT_FULL commands. They are not covered by lazy_scan_heap(), so such
commands are needlessly shown in the progress view with 0s in most of the
fields.
Regarding pg_stat_get_vacuum_progress(): I think a backend can simply be
skipped if (!has_privs_of_role(GetUserId(), beentry->st_userid)) (cannot
put that in plain English, :))
Please add documentation for the newly added view and SQL functions, if any.
I'm marking this as "Waiting on author" in the commitfest app. Also, let's
hear from more people.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
