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

+typedef enum BackendCommandType
+} 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},

> 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.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to