Re: Table-level log_autovacuum_min_duration - Mailing list pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, failed
Spec compliant:           tested, failed
Documentation:            tested, failed

Hi,

I'm Naoya Anzai.
I've been working as a PostgreSQL Support Engineer for 6 years.
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." :))

Akira Kurosawa <kurosawa-akira@mxc.nes.nec.co.jp>
Taiki Kondo <kondo-taiki@mxt.nes.nec.co.jp>
Huong Dangminh <dangminh-huong@mxm.nes.nec.co.jp>

So I believe reviewing patches is not difficult for us.

This is a review of Table-level log_autovacuum_min_duration patch:
http://www.postgresql.org/message-id/CAB7nPqTBQsbEgvb8cOH01K7ARGYs9KBuV8dr+aqGonFVb8koAQ@mail.gmail.com

Submission review
====================
The patch applies cleanly to HEAD, and it works fine on Fedora 
release 20.
There is no regression test,but I think it is no problem 
because other parameter also is not tested.


Usability review
====================
pg_dump/pg_restore support is OK.
I think this feature is a nice idea and I also want this feature.


Feature test
====================
I executed following commands after setting 
"log_autovacuum_min_duration" to 1000 in the GUC. ("bar" table is 
already created with no options.)
CREATE TABLE foo ( ... ) WITH ( log_autovacuum_min_duration = 0 );ALTER TABLE bar SET (log_autovacuum_min_duration = 0
);

Then, only in "foo" and "bar" table, autovacuum log was printed out 
even if elapsed time of autovacuum is lesser than 1000ms. This 
behavior was expected and there was no crash or failed assertion. 
So it looked good for me. But, I executed following command, in 
"buzz" table, autovacuum log was printed out if elapsed time is 
more than 1000ms.
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?


Performance review
====================
Not reviewed from this point of view.


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

There is no new function which is defined in this patch, and 
modified contents are not related to OS-dependent contents, so I 
think it will work fine on Windows/BSD etc.


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.


Regards,
Naoya Anzai (NEC Solution Innovators,Ltd.)


The new status of this patch is: Waiting on Author