Re: [PROPOSAL] VACUUM Progress Checker.

You're so quick.

At Tue, 8 Mar 2016 17:02:24 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <56DE8710(dot)4070202(at)lab(dot)ntt(dot)co(dot)jp>
> On 2016/03/07 23:48, Robert Haas wrote:
> >> I don't like to treat the target object id differently from other
> >> parameters. It could not be needed at all, or could be needed two
> >> or more in contrast. Although oids are not guaranteed to fit
> >> uint32, we have already stored BlockNumber there.
> >
> > Well...
> >
> > There's not much point in deciding that the parameters are uint32,
> > because we don't have that type at the SQL level.
> > pgstat_progress_update_param() really ought to take either int32 or
> > int64 as an argument, because that's what we can actually handle from
> > SQL, and it seems pretty clear that int64 is better since otherwise we
> > can't fit, among other things, a block number.
> >
> > Given that, I tend to think that treating the command target specially
> > and passing that as an OID is reasonable. We're not going to be able
> > to pass variable-sized arrays through this mechanism, ever, because
> > our shared memory segment doesn't work like that. And it seems to me
> > that nearly every command somebody might want to report progress on
> > will touch, basically, one relation a a time. So I don't see the harm
> > in hardcoding that idea into the facility.

We'd concatenate two int32s into int64s but widening each
parameters to int64 would be preferable. Additional 4 bytes by
the defalut number of maxbackends 100 by 10 parameters = 4kb, 4MB
for 1000 backends is not so big for modern machines?

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

+ /*
+ * 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.

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};

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

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

> * 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:
>
> /* Typedef for callback function to determine if a tuple is bulk-deletable */
> -typedef bool (*IndexBulkDeleteCallback) (ItemPointer itemptr, void *state);
> +typedef bool (*IndexBulkDeleteCallback) (ItemPointer itemptr,
> + BlockNumber index_blkno,
> + void *state);
>
> Then added 2 more fields to LVRelStats:
>
> @@ -143,6 +143,8 @@ typedef struct LVRelStats
> int num_index_scans;
> TransactionId latestRemovedXid;
> bool lock_waiter_detected;
> + BlockNumber last_index_blkno;
> + BlockNumber index_blks_vacuumed;
>
> Then in lazy_tid_reaped(), if the index block number received in the
> index_blkno argument has changed from the previous call, increment the
> count of index blocks processed and
> pgstat_report_update_param(index_blks_done). I wonder if we should reset
> the the saved block number and the count for every index vacuumed by
> lazy_vacuum_index(). Right now, total_index_blks counts all indexes and
> counting blocks using the rough method mentioned above is sensible only
> for one index at time. Actually, the method has different kinds of
> problems to deal with anyway. For example, with a btree index, one can
> expect that the final count does not match total_index_blks obtained using
> RelationGetNumberOfBlocks(). Moreover, each AM has its own idiosyncratic
> 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.

> * 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?

reagards,

--
Kyotaro Horiguchi
NTT Open Source Software Center