Re: [PROPOSAL] VACUUM Progress Checker.

Horiguchi-san,

Thanks for the review!

On 2016/03/08 18:19, Kyotaro HORIGUCHI wrote:
>> Updated versions attached.
>>
>> * changed st_progress_param to int64 and so did the argument of
>> pgstat_progress_update_param(). Likewise changed param1..param10 of
>> pg_stat_get_progress_info()'s output columns to bigint.
>>
>> * Added back the Oid field st_command_target and corresponding function
>> pgstat_progress_set_command_target(Oid).
>
> + beentry->st_command = COMMAND_INVALID;
> + MemSet(&beentry->st_progress_param, 0, sizeof(beentry->st_progress_param));
>
> The MemSet seems useless since it gets the same initialization on
> setting st_command.

Right, every backend start should not have to pay that price. Fixed in
the latest version.

>
> + /*
> + * Report values for only those backends which are running the given
> + * command. XXX - privilege check is maybe dubious.
> + */
> + if (!beentry ||
> + beentry->st_command != cmdtype ||
> + !has_privs_of_role(GetUserId(), beentry->st_userid))
> + continue;
>
> We can simplly ignore unpriviledged backends, or all zeroz or
> nulls to signal that the caller has no priviledge.

As suggested by Robert, used pg_stat_get_activity() style. In this case,
show 'pid' to all but the rest only to role members.

> 0002
>
> + FROM pg_stat_get_progress_info(1) AS S;
>
> Ah... This magick number seems quite bad.. The function should
> take the command type in maybe string type.
>
> + FROM pg_stat_get_progress_info('lazy vacuum') AS S;
>
> Using an array of the names would be acceptable, maybe.
>
> | char *progress_command_names[] = {'lazy vacuum', NULL};

Hm, I think the way it is *may* be OK the way it is, but...

As done in the patch, the way we identify commands is with the enum
BackendCommandType:

+typedef enum BackendCommandType
+{
+ COMMAND_INVALID = 0,
+ COMMAND_LAZY_VACUUM
+} BackendCommandType;

Perhaps we could create a struct:

typedef struct PgStatProgressCommand
{
char *cmd_name;
BackendCommandType cmd_type;
} PgStatProgressCommand;

static const struct PgStatProgressCommand commands[] = {
{"vacuum", COMMAND_LAZY_VACUUM},
{NULL, COMMAND_INVALID}
};

> However the numbers for the phases ('scanning heap' and so..) is
> acceptable for me for reasons uncertain to me, it also could be
> represented in names but is might be rahter bothersome..

In initial versions of the patch, it used to be char * that was passed for
identifying phases. But, then we got rid of char * progress parameters
altogether. So, there are no longer any text columns in
pg_stat_get_progress_info()'s result. It may not work out well in long
run to forever not have those (your recent example comes to mind).

>
> + WHEN 0 THEN 100::numeric(5, 2)
> + ELSE ((S.param3 + 1)::numeric / S.param2 * 100)::numeric(5, 2)
>
> This usage of numeric seems overkill to me.

Hmm, how could this rather be written?

>> * I attempted to implement a method to report index blocks done from
>> lazy_tid_reaped() albeit with some limitations. Patch 0003 is that
>> attempt. In summary, I modified the index bulk delete callback interface
>> to receive a BlockNumber argument index_blkno:

[ snip ]

>> way of traversing the index pages. I dared only touch the btree case to
>> make it pass current block number to the callback. It finishes with
>> index_blks_done << total_index_blks since I guess the callback is called
>> only on the leaf pages. Any ideas?
>
> To the contrary, I suppose it counts one index page more than
> once for the cases of uncorrelated heaps. index_blks_vacuumd can
> exceed RelationGetNumberOfBlocks() in extreme cases. If I'm not
> missing something, it stands on a quite fragile graound.

Yeah, the method is not entirely foolproof yet.

>> * I am also tempted to add num_dead_tuples and dead_tuples_vacuumed to add
>> granularity to 'vacuuming heap' phase but didn't in this version. Should we?
>
> How do you think they are to be used?

I just realized there are objections to some columns be counters for pages
and others counting tuples. So, I guess I withdraw. I am just worried
that 'vacuuming heap' phase may take arbitrarily long if dead tuples array
is big. Since we were thinking of adding more granularity to 'vacuuming
indexes' phase, I thought we should do for the former too.

Thanks,
Amit