Re: Table-level log_autovacuum_min_duration

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(at)mxc(dot)nes(dot)nec(dot)co(dot)jp>
Taiki Kondo <kondo-taiki(at)mxt(dot)nes(dot)nec(dot)co(dot)jp>
Huong Dangminh <dangminh-huong(at)mxm(dot)nes(dot)nec(dot)co(dot)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