Re: [PROPOSAL] VACUUM Progress Checker.

Hi,

On 2015/12/03 15:27, Kyotaro HORIGUCHI wrote:
> At Thu, 3 Dec 2015 14:18:50 +0900, Amit Langote wrote
>> On 2015/12/03 13:47, Kyotaro HORIGUCHI wrote:
>>>
>>> Apart from the representation of the relation, OID would be
>>> better as a field in beentry.
>>
>> I wonder if the field should be a standalone field or as yet another
>> st_progress_* array?
>>
>> IMHO, there are some values that a command would report that should not be
>> mixed with pgstat_report_progress()'s interface. That is, things like
>> command ID/name, command target (table name or OID) should not be mixed
>> with actual progress parameters like num_pages, num_indexes (integers),
>> processing "phase" (string) that are shared via st_progress_* fields. The
>> first of them  already has its own reporting interface in proposed patch
>> in the form of pgstat_report_activity_flag(). Although, we must be careful
>> to choose these interfaces carefully.
> 
> Sorry I misunderstood the patch.
> 
> Agreed. The patch already separates integer values and texts.
> And re-reviewing the patch, there's no fields necessary to be
> passed as string.
> 
> total_heap_pages, scanned_heap_pages, total_index_pages,
> scanned_index_pages, vacrelstats->num_index_scans are currently
> in int32.
> 
> Phase can be in integer, and schema_name and relname can be
> represented by one OID, uint32.

AIUI, st_progress_message (strings) are to be used to share certain
messages as progress information. I think the latest vacuum-progress patch
uses it to report which phase lazy_scan_heap() is in, for example,
"Scanning heap" for phase 1 of its processing and "Vacuuming index and
heap" for phase 2. Those values are shown to the user in a text column
named "phase" of the pg_stat_vacuum_progress view. That said, reporting
phase as an integer value may also be worth a consideration. Some other
command might choose to do that.

> Finally, *NO* text field is needed at least this usage. So
> progress_message is totally useless regardless of other usages
> unknown to us.

I think it may be okay at this point to add just those st_progress_*
fields which are required by lazy vacuum progress reporting. If someone
comes up with instrumentation ideas for some other command, they could
post patches to add more st_progress_* fields and to implement
instrumentation and a progress view for that command. This is essentially
what Robert said in [1] in relation to my suggestion of taking out
st_progress_param_float from this patch.

By the way, there are some non-st_progress_* fields that I was talking
about in my previous message. For example, st_activity_flag (which I have
suggested to rename to st_command instead). It needs to be set once at the
beginning of the command processing and need not be touched again. I think
it may be a better idea to do the same for table name or OID. It also
won't change over the duration of the command execution. So, we could set
it once at the beginning where that becomes known. Currently in the patch,
it's reported in st_progress_message[0] by every pgstat_report_progress()
invocation. So, the table name will be strcpy()'d to shared memory for
every scanned block that's reported.

Thanks,
Amit

[1]
http://www.postgresql.org/message-id/CA+TgmoYdZk9nPDtS+_kOt4S6fDRQLW+1jnJBmG0pkRT4ynxO=Q@mail.gmail.com