On Fri, Feb 6, 2015 at 4:39 AM, Naoya Anzai wrote:
> I've been working as a PostgreSQL Support Engineer for 6 years.
Thanks for your input Anzai-san.
> I am a newcomer of reviewing, and My programming skill is not so high.
> But the following members also participate in this review. (We say
> "Two heads are better than one." :))
FWIW, any review for any kind of patches are always welcome, either
knowing or not the code you are looking at for a given patch. It is
even encouraged to look at new areas when possible.
> Feature test
> ====================
> [blah]
> CREATE TABLE buzz ( ... ) WITH ( log_autovacuum_min_duration = -1 );
> I expect autovacuum log is NOT printed out even if elapsed time is
> more than 1000ms in this case. My thought is wrong, isn't it? In my
> opinion, there is an use case that autovacuum log is always printed
> out in all tables excepting specified tables. I think there is a
> person who wants to use it like this case, but I (or he) can NOT use
> in this situation.
>
> How about your opinion?
The patch is working as expected. Similarly to the other parameters
like vacuum_cost_delay, a value of -1 is the default behavior, meaning
that the underlying GUC parameter is used. I thought about using a
special value like -2 to define the behavior you are mentioning here,
aka with "-2" disable custom value and GUC parameter but I don't think
it is worth adding as that's an ugly 3 line of code of this type:
if (log_min_duration == -2)
enforce_log_min = -1;
And you can actually get what you are looking for by setting
min_duration through ALTER TABLE to a very high value, like say 2e9 to
suppress the autovacuum output of a set of tables out of the rest.
> Coding review
> ====================
> I think description of log_autovacuum_min_duration in reloptions.c
> (line:215) should be like other parameters (match with guc.c). So
> it should be "Sets the minimum execution time above which autovacuum
> actions will be logged." but not "Log autovacuum execution for
> given threshold".
What about that then?
"Minimum execution time above which autovacuum actions will be logged"
> Architecture review
> ====================
> About the modification of gram.y.
>
> I think it is not good that log_min_duration is initialized to
> zeros in gram.y when "FREEZE" option is set. Because any VACUUM
> queries never use this parameter. I think log_min_duration always
> should be initialized to -1.
Looking at this patch this morning, actually I think that my last
patch as well as your suggestion are both wrong. To put it in short
words, log_min_duration should be set only if VACOPT_VERBOSE is
defined in query declaration. So I changed this portion of the patch
this way.
--
Michael