From: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp |
Cc: | Rahila(dot)Syed(at)nttdata(dot)com, alvherre(at)2ndquadrant(dot)com, masao(dot)fujii(at)gmail(dot)com, sawada(dot)mshk(at)gmail(dot)com, thom(at)linux(dot)com, pgsql-hackers(at)postgresql(dot)org, robertmhaas(at)gmail(dot)com |
Subject: | Re: [PROPOSAL] VACUUM Progress Checker. |
Date: | 2015-11-10 07:52:30 |
Message-ID: | 20151110.165230.216607939.horiguchi.kyotaro@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello, I have some random comments on this patch addition to
Amit's comments.
- Type of the flag of vacuum activity.
ACTIVITY_IS_VACUUM is the alone entry in the enum, and the
variable to store it is named as *flag. If you don't have any
plan to extend this information, the name of this variable would
seems better to be something like pgstat_report_vacuum_running
and in the type of boolean.
- Type of st_progress_param and so.
The variable st_progress_param has very generic name but as
looking the pg_stat_get_vacuum_progress, every elements of it is
in a definite role. If so, the variable should be a struct.
st_progress_param_float is currently totally useless.
- Definition of progress_message.
The definition of progress_message in lazy_scan_heap is "char
[PROGRESS_MESSAGE_LENGTH][N_PROGRESS_PARAM]" which looks to be
inversed. The following snprintf,
| snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s", schemaname);
certainly destroys the data already stored in it if any.
- snprintf()
You are so carefully to use snprintf,
+ snprintf(progress_message[0], PROGRESS_MESSAGE_LENGTH, "%s", schemaname);
+ strcat(progress_message[0],".");
+ strcat(progress_message[0],relname);
but the strcats following ruin it.
- Calculation of total_heap_pages in lazy_scan_heap.
The current code subtracts the number of blocks when
skipping_all_visible_blocks is set in two places. But I think
it is enough to decrement when skipping.
I'll be happy if this can be of any help.
regards,
At Tue, 10 Nov 2015 14:44:23 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <56418437(dot)5080003(at)lab(dot)ntt(dot)co(dot)jp>
> Thanks for the v6. A few quick comments:
>
> - duplicate_oids error in HEAD.
>
> - a compiler warning:
>
> pgstat.c:2898: warning: no previous prototype for ‘pgstat_reset_activityflag’
>
> To fix that use void for empty parameter list -
>
> -extern void pgstat_reset_activityflag();
> +extern void pgstat_reset_activityflag(void);
>
> One more change you could do is 's/activityflag/activity_flag/g', which I
> guess is a naming related guideline in place.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2015-11-10 08:02:12 | Re: [PROPOSAL] VACUUM Progress Checker. |
Previous Message | Amit Kapila | 2015-11-10 07:34:25 | Re: eXtensible Transaction Manager API |