Обсуждение: Table-level log_autovacuum_min_duration
Hi all, Today I spent a bit of time looking at the activity of autovacuum for one table particularly bloated. log_autovacuum_min_duration was enabled and set to a high value but even with that I had to deal with some spam from the jobs of other tables. It would be cool to have the possibility to do some filtering IMO. So, what about having a relopt to control log_autovacuum_min_duration? RelationData->rd_options is largely accessible for a given relation in the code paths of vacuum and analyze. Thoughts? -- Michael
Michael Paquier wrote: > Hi all, > > Today I spent a bit of time looking at the activity of autovacuum for > one table particularly bloated. log_autovacuum_min_duration was > enabled and set to a high value but even with that I had to deal with > some spam from the jobs of other tables. It would be cool to have the > possibility to do some filtering IMO. So, what about having a relopt > to control log_autovacuum_min_duration? RelationData->rd_options is > largely accessible for a given relation in the code paths of vacuum > and analyze. +1 -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Dec 18, 2014 at 11:15 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Michael Paquier wrote: >> Hi all, >> >> Today I spent a bit of time looking at the activity of autovacuum for >> one table particularly bloated. log_autovacuum_min_duration was >> enabled and set to a high value but even with that I had to deal with >> some spam from the jobs of other tables. It would be cool to have the >> possibility to do some filtering IMO. So, what about having a relopt >> to control log_autovacuum_min_duration? RelationData->rd_options is >> largely accessible for a given relation in the code paths of vacuum >> and analyze. > > +1 As long as I got this idea in mind, I looked at the code to see how much it would be possible to tune log_autovacuum_min_duration in the code paths of analyze and vacuum. First, I think that it would be good to have parameters for both parent relations and their toast relation similarly to the other parameters... But now, here are some things to consider if we use directly the reloptions available with RelationData: - If the parameters toast.autovacuum_* are not set, toast relations inherit values from their parent relation. Looking at autovacuum.c which is the only place where autovacuum options are located, we keep a hash table to save the mapping toast -> parent relation. Using that it is easy to fetch for a given toast relation the relopts of its parent. Note this is strictly located in the autovacuum code path, so to let vacuum and analyze now the custom value of log_autovacuum_min_duration, with the inheritance property kept, we would need to pass some extra values from autovacuum to the calls of vacuum(). - It would not be possible to log autovacuum and analyze being skipped when lock is not available because in this case Relation cannot be safely fetched, so there are no reltoptions directly available. This is for example this kind of message: skipping analyze of "foo" --- lock not available Both those things could be solved by passing a value through VacuumStmt, like what we do when passing a value for multixact_freeze_min_age, or with an extra argument in vacuum() for example. Now I am not sure if it is worth it, and we could live as well with a parameter that do not support the inheritance parent relation -> toast, so log value set for a toast relation and its parent share no dependency. In short that's a trade between code simplicity and usability. Thoughts? -- Michael
On Sat, Dec 20, 2014 at 9:17 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > But now, here are some things to consider if we use directly the > reloptions available with RelationData: > - If the parameters toast.autovacuum_* are not set, toast relations > inherit values from their parent relation. Looking at autovacuum.c > which is the only place where autovacuum options are located, we keep > a hash table to save the mapping toast -> parent relation. Using that > it is easy to fetch for a given toast relation the relopts of its > parent. Note this is strictly located in the autovacuum code path, so > to let vacuum and analyze now the custom value of > log_autovacuum_min_duration, with the inheritance property kept, we > would need to pass some extra values from autovacuum to the calls of > vacuum(). > - It would not be possible to log autovacuum and analyze being skipped > when lock is not available because in this case Relation cannot be > safely fetched, so there are no reltoptions directly available. This > is for example this kind of message: > skipping analyze of "foo" --- lock not available > > Both those things could be solved by passing a value through > VacuumStmt, like what we do when passing a value for > multixact_freeze_min_age, or with an extra argument in vacuum() for > example. Now I am not sure if it is worth it, and we could live as > well with a parameter that do not support the inheritance parent > relation -> toast, so log value set for a toast relation and its > parent share no dependency. In short that's a trade between code > simplicity and usability. I'm not sure I follow all of the particulars of what you are asking here, but in general I would say that you shouldn't hesitate to pass more information down the call stack when that helps to obtain correct behavior. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Dec 20, 2014 at 11:17 PM, Michael Paquier <michael.paquier@gmail.com> wrote: >> Michael Paquier wrote: >>> Today I spent a bit of time looking at the activity of autovacuum for >>> one table particularly bloated. log_autovacuum_min_duration was >>> enabled and set to a high value but even with that I had to deal with >>> some spam from the jobs of other tables. It would be cool to have the >>> possibility to do some filtering IMO. So, what about having a relopt >>> to control log_autovacuum_min_duration? RelationData->rd_options is >>> largely accessible for a given relation in the code paths of vacuum >>> and analyze. OK, instead of only words, attached is a patch adding relopts called log_autovacuum_min_duration and toast.log_autovacuum_min_duration to control the logging of autovacuum at relation-level. The default value of those parameters is -1, meaning that in this case the global log_autovacuum_min_duration is used to control the logs. The patch has finished by being simpler than I though first by using VacuumStmt to pass the relopts from autovacuum to the code paths of analyze and vacuum. Note that this uses the same mechanisms as the other relopts, meaning that the toast relations use the values of their parent tables if it is defined. I am adding it to the next CF. -- Michael
Вложения
I wrote: > I am adding it to the next CF. Patch 2 attached. I forgot the update of copyfuncs.c and equalfuncs.c. -- Michael
Вложения
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
> 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 I'm sorry, I just sent it by mistake. All of them have passed. --- Naoya Anzai
On Fri, Feb 6, 2015 at 4:50 AM, Naoya Anzai <anzai-naoya@mxu.nes.nec.co.jp> wrote: >> 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 > I'm sorry, I just sent it by mistake. > All of them have passed. That's fine. I think you used the UI on the commit fest app, and it is not intuitive that you need to check those boxes at first sight when using it for the first time. -- Michael
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
Вложения
On Fri, Feb 6, 2015 at 11:14 AM, Michael Paquier wrote: > 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. And I forgot to change VacuumStmt for the ANALYZE portion in gram.y... Patch updated attached. -- Michael
Вложения
Thanks for your reply. >> Feature test >> ==================== (snip) > 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; I disscussed about this use case with my co-workers, who said "that code is not good", then we concluded that it was actually a rare case. If such a case sometimes happens in fact, I (or someone) will suggest a different way from this patch to handle this case. We know there is a practical workaround. :) >> 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" That's roughly match. For matching with guc.c, you might be better to add "Sets the" to that discription. >> 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. >And I forgot to change VacuumStmt for the ANALYZE portion in gram.y... >Patch updated attached. Sorry, I could not correctly express my opinion to you. I mean "log_autovacuum_min_duration" is used only by AutoVacuum, Any VACUUM queries (including VACUUM VERBOSE) never use this parameter. So, when someone executes Manual Vacuum, "log_min_duration" always should be initialized to -1. ANALYZE should also be the same. In other words, it is not necessary to initialize "log_min_duration" to zero when perform the VACUUM(or ANALYZE) VERBOSE. "VERBOSE" is provided only for changing the log level of Manual VACUUM from DEBUG2 to INFO. Regards, ----- Naoya.
On Mon, Feb 9, 2015 at 3:47 PM, Naoya Anzai <anzai-naoya@mxu.nes.nec.co.jp> wrote: >>> Feature test >>> ==================== > (snip) >> 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; > > I discussed about this use case with my co-workers, who said > "that code is not good", then we concluded that it was actually > a rare case. If such a case sometimes happens in fact, I (or someone) > will suggest a different way from this patch to handle this case. > > We know there is a practical workaround. :) OK, done. >>> 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" > > That's roughly match. For matching with guc.c, you might be better to > add "Sets the" to that discription. OK, done this way... >>And I forgot to change VacuumStmt for the ANALYZE portion in gram.y... >>Patch updated attached. > > Sorry, I could not correctly express my opinion to you. I mean > "log_autovacuum_min_duration" is used only by AutoVacuum, Any VACUUM > queries (including VACUUM VERBOSE) never use this parameter. So, > when someone executes Manual Vacuum, "log_min_duration" always should > be initialized to -1. > > ANALYZE should also be the same. > > In other words, it is not necessary to initialize "log_min_duration" > to zero when perform the VACUUM(or ANALYZE) VERBOSE. "VERBOSE" is > provided only for changing the log level of Manual VACUUM from > DEBUG2 to INFO. Well, I see your point but this is not completely true: we could as well rely entirely on this parameter instead of VACOPT_VERBOSE to determine if autovacuum, a vacuum or an analyze are in verbose mode, and remove VACOPT_VERBOSE, but I can imagine people complaining if VACOPT_VERBOSE is removed. So let's set it up unconditionally to -1 in gram.y for now. However if people think that it is fine to remove VACOPT_VERBOSE, we could use exclusively this parameter in VacuumStmt. Or even add sanity checks at the top of vacuum() to ensure that VACOPT_VERBOSE is set only when log_min_duration is positive. Additional opinions on this matter are welcome. -- Michael
Вложения
An updated patch is attached, I think that previous version broke value assignment of log_min_duration in table_recheck_autovac: the value in reltoptions should be used only if it is positive. At the same time I wrote some more documentation about the fact that a toast table will use the value of its parent table if nothing is set.
--
--
Michael
Вложения
Hi, Michael-san > An updated patch is attached, I'm sorry for confusing you. I think you don't have to implement this code to disable this feature with using value "-2".Because this use case is a rare case, and there is a practical workaround using huge value like "2e9". (You suggested "2e9" to me, didn't you? :) ) So, please remove this code. > Well, I see your point but this is not completely true: we could as > well rely entirely on this parameter instead of VACOPT_VERBOSE to > determine if autovacuum, a vacuum or an analyze are in verbose mode, > and remove VACOPT_VERBOSE, but I can imagine people complaining if > VACOPT_VERBOSE is removed. So let's set it up unconditionally to -1 in > gram.y for now. However if people think that it is fine to remove > VACOPT_VERBOSE, we could use exclusively this parameter in VacuumStmt. > Or even add sanity checks at the top of vacuum() to ensure that > VACOPT_VERBOSE is set only when log_min_duration is positive. > Additional opinions on this matter are welcome. I understand your point at last. :) You mean that ... Log_autovacuum_min_duration assumes a role of VACOPT_VERBOSE. Even if this parameter never use currently for manual vacuum, log_autovacuum_min_duration should be set zero(anytime output) when we executes "VACUUM(or ANALYZE) VERBOSE". Is my understanding correct? If so,it sounds logical. If we can abolish VERBOSE option, I think it's ideal that we will prepare a new parameter like a log_min_duration_vacuum(and log_min_duration_analyze) which integrating "VERBOSE feature" and "log_autovacuum_min_duration". Regards, --- Naoya Anzai
On Thu, Feb 12, 2015 at 5:44 PM, Naoya Anzai <anzai-naoya@mxu.nes.nec.co.jp> wrote:
Hi, Michael-san
> An updated patch is attached,
I'm sorry for confusing you.
I think you don't have to implement this code to disable this
feature with using value "-2".Because this use case is a rare case,
and there is a practical workaround using huge value like "2e9".
(You suggested "2e9" to me, didn't you? :) ) So, please remove this code.
I will clean up the code.
> Well, I see your point but this is not completely true: we could as
> well rely entirely on this parameter instead of VACOPT_VERBOSE to
> determine if autovacuum, a vacuum or an analyze are in verbose mode,
> and remove VACOPT_VERBOSE, but I can imagine people complaining if
> VACOPT_VERBOSE is removed. So let's set it up unconditionally to -1 in
> gram.y for now. However if people think that it is fine to remove
> VACOPT_VERBOSE, we could use exclusively this parameter in VacuumStmt.
> Or even add sanity checks at the top of vacuum() to ensure that
> VACOPT_VERBOSE is set only when log_min_duration is positive.
> Additional opinions on this matter are welcome.
I understand your point at last. :)
You mean that ...
Log_autovacuum_min_duration assumes a role of VACOPT_VERBOSE.
Even if this parameter never use currently for manual vacuum,
log_autovacuum_min_duration should be set zero(anytime output)
when we executes "VACUUM(or ANALYZE) VERBOSE".
Is my understanding correct? If so,it sounds logical.
Yup, that's my opinion. Now I don't know if people would mind to remove VACOPT_VERBOSE and replace the control it does by log_min_duration in VacuumStmt. At least both things are overlapping, and log_min_duration offers more options than the plain VACOPT_VERBOSE.
If we can abolish VERBOSE option,
I think it's ideal that we will prepare a new parameter like
a log_min_duration_vacuum(and log_min_duration_analyze) which
integrating "VERBOSE feature" and "log_autovacuum_min_duration".
What I think you are proposing here is a VERBOSE option that hypothetically gets activated if a manual VACUUM takes more than a certain amount specified by those parameters. I doubt this would be useful. In any case this is unrelated to this patch.
--
Michael
>> You mean that ... >> Log_autovacuum_min_duration assumes a role of VACOPT_VERBOSE. >> Even if this parameter never use currently for manual vacuum, >> log_autovacuum_min_duration should be set zero(anytime output) >> when we executes "VACUUM(or ANALYZE) VERBOSE". >> Is my understanding correct? If so,it sounds logical. >> > >Yup, that's my opinion. Now I don't know if people would mind to remove >VACOPT_VERBOSE and replace the control it does by log_min_duration in >VacuumStmt. At least both things are overlapping, and log_min_duration >offers more options than the plain VACOPT_VERBOSE. OK. I agree with you. Please re-implement as you are thinking. >> If we can abolish VERBOSE option, >> I think it's ideal that we will prepare a new parameter like >> a log_min_duration_vacuum(and log_min_duration_analyze) which >> integrating "VERBOSE feature" and "log_autovacuum_min_duration". >> > >What I think you are proposing here is a VERBOSE option that hypothetically >gets activated if a manual VACUUM takes more than a certain amount >specified by those parameters. I doubt this would be useful. In any case >this is unrelated to this patch. I suspect manual vacuum often executes as "semi-auto vacuum" by job-scheduler, cron, etc in actual maintenance cases. Whether auto or manual, I think that's important to output their logs in the same mechanism. Sorry, I seem to have wandered from the subject. Naoya
On Fri, Feb 13, 2015 at 10:16 AM, Naoya Anzai <anzai-naoya@mxu.nes.nec.co.jp> wrote: >>> You mean that ... >>> Log_autovacuum_min_duration assumes a role of VACOPT_VERBOSE. >>> Even if this parameter never use currently for manual vacuum, >>> log_autovacuum_min_duration should be set zero(anytime output) >>> when we executes "VACUUM(or ANALYZE) VERBOSE". >>> Is my understanding correct? If so,it sounds logical. >>> >> >>Yup, that's my opinion. Now I don't know if people would mind to remove >>VACOPT_VERBOSE and replace the control it does by log_min_duration in >>VacuumStmt. At least both things are overlapping, and log_min_duration >>offers more options than the plain VACOPT_VERBOSE. > > OK. I agree with you. > Please re-implement as you are thinking. OK will do that today. >>> If we can abolish VERBOSE option, >>> I think it's ideal that we will prepare a new parameter like >>> a log_min_duration_vacuum(and log_min_duration_analyze) which >>> integrating "VERBOSE feature" and "log_autovacuum_min_duration". >>> >> >>What I think you are proposing here is a VERBOSE option that hypothetically >>gets activated if a manual VACUUM takes more than a certain amount >>specified by those parameters. I doubt this would be useful. In any case >>this is unrelated to this patch. > > I suspect manual vacuum often executes as "semi-auto vacuum" > by job-scheduler, cron, etc in actual maintenance cases. > Whether auto or manual, I think that's important to output > their logs in the same mechanism. > > Sorry, I seem to have wandered from the subject. No problem. That's a constructive discussion :) -- Michael
On Fri, Feb 13, 2015 at 10:16 AM, Naoya Anzai <anzai-naoya@mxu.nes.nec.co.jp> wrote: >>> You mean that ... >>> Log_autovacuum_min_duration assumes a role of VACOPT_VERBOSE. >>> Even if this parameter never use currently for manual vacuum, >>> log_autovacuum_min_duration should be set zero(anytime output) >>> when we executes "VACUUM(or ANALYZE) VERBOSE". >>> Is my understanding correct? If so,it sounds logical. >>> >> >>Yup, that's my opinion. Now I don't know if people would mind to remove >>VACOPT_VERBOSE and replace the control it does by log_min_duration in >>VacuumStmt. At least both things are overlapping, and log_min_duration >>offers more options than the plain VACOPT_VERBOSE. > > OK. I agree with you. > Please re-implement as you are thinking. Thanks. Attached is an updated patch will all those things implemented. >>> If we can abolish VERBOSE option, >>> I think it's ideal that we will prepare a new parameter like >>> a log_min_duration_vacuum(and log_min_duration_analyze) which >>> integrating "VERBOSE feature" and "log_autovacuum_min_duration". >>> >> >>What I think you are proposing here is a VERBOSE option that hypothetically >>gets activated if a manual VACUUM takes more than a certain amount >>specified by those parameters. I doubt this would be useful. In any case >>this is unrelated to this patch. > > I suspect manual vacuum often executes as "semi-auto vacuum" > by job-scheduler, cron, etc in actual maintenance cases. > Whether auto or manual, I think that's important to output > their logs in the same mechanism. > > Sorry, I seem to have wandered from the subject. This patch is a step in this direction as it enables any backend-side code to set up VacuumStmt with a custom timestamp value to output logs after a given timing, for example in background workers. For the client-side of things, I am unsure if we'd actually want it, we should always VERBOSE when this option is invoked through a query, and not add any hypothetical condition on it... Btw, after hacking on this it happens that we cannot completely remove VACOPT_VERBOSE as gram.y needs to take into account options with parenthesis :) But we can limit its use to the query parser only, similarly for VACOPT_FREEZE. -- Michael
Вложения
Hi, Michael I found that definition of VERBOSE and log_autovacuum is not pretty match. For example, "VERBOSE" can output logs of scanning indices and scanning detail of analyze, but log_autovacuum can't output them. Please see following sequences. 1. execute these queries. DROP TABLE t1;CREATE TABLE t1(id integer primary key,name text);ALTER TABLE t1 SET (log_autovacuum_min_duration=0);ALTERTABLE t1 ALTER COLUMN name SET STORAGE EXTERNAL;INSERT INTO t1 SELECT GENERATE_SERIES(1,100),repeat('a',3000);UPDATEt1 SET name='update'; 2. after a while, output the following logs by autovacuum movement. (For your convenience, I put the "### TYPE ###" prefix on each logs.) ### VERBOSE ###INFO: vacuuming "public.t1"### VERBOSE ###INFO: scanned index "t1_pkey" to remove 33 row versions### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.### VERBOSE ###INFO: "t1": removed 33 row versions in 1 pages###VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.### VERBOSE ###INFO: index "t1_pkey" now contains 100row versions in 2 pages### VERBOSE ###DETAIL: 33 index row versions were removed.### VERBOSE ### 0 index pageshave been deleted, 0 are currently reusable.### VERBOSE ### CPU 0.00s/0.00u sec elapsed 0.00 sec.### VERBOSE ###INFO: "t1": found 100 removable, 100 nonremovable row versions in 2 out of 2 pages### VERBOSE ###DETAIL: 0dead row versions cannot be removed yet.### VERBOSE ### There were 0 unused item pointers.### VERBOSE ### Skipped 0 pages due to buffer pins.### VERBOSE ### 0 pages are entirely empty.### VERBOSE ### CPU 0.00s/0.00usec elapsed 0.00 sec.### LOG_AVAC ###LOG: automatic vacuum of table "naoya.public.t1": index scans: 1### LOG_AVAC### pages: 0 removed, 2 remain, 0 skipped due to pins### LOG_AVAC ### tuples: 100 removed, 100 remain,0 are dead but not yet removable### LOG_AVAC ### buffer usage: 47 hits, 4 misses, 4 dirtied### LOG_AVAC ### avg read rate: 14.192 MB/s, avg write rate: 14.192 MB/s### LOG_AVAC ### system usage: CPU 0.00s/0.00u secelapsed 0.00 sec### VERBOSE ###INFO: analyzing "public.t1"### VERBOSE ###INFO: "t1": scanned 2 of 2 pages, containing100 live rows and 0 dead rows; 100 rows in sample, 100 estimated total rows### LOG_AVAC ###LOG: automatic analyzeof table "naoya.public.t1" system usage: CPU 0.00s/0.00u sec elapsed 0.04 sec### VERBOSE ###INFO: vacuuming "pg_toast.pg_toast_72882"###VERBOSE ###INFO: scanned index "pg_toast_72882_index" to remove 200 row versions### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.### VERBOSE ###INFO: "pg_toast_72882": removed 200 row versionsin 50 pages### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.### VERBOSE ###INFO: index "pg_toast_72882_index"now contains 0 row versions in 2 pages### VERBOSE ###DETAIL: 200 index row versions were removed.###VERBOSE ### 0 index pages have been deleted, 0 are currently reusable.### VERBOSE ### CPU 0.00s/0.00usec elapsed 0.00 sec.### VERBOSE ###INFO: "pg_toast_72882": found 200 removable, 0 nonremovable row versionsin 50 out of 50 pages### VERBOSE ###DETAIL: 0 dead row versions cannot be removed yet.### VERBOSE ### Therewere 0 unused item pointers.### VERBOSE ### Skipped 0 pages due to buffer pins.### VERBOSE ### 0 pagesare entirely empty.### VERBOSE ### CPU 0.00s/0.00u sec elapsed 0.00 sec.### VERBOSE ###INFO: "pg_toast_72882":truncated 50 to 0 pages### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.03 sec.### LOG_AVAC ###LOG: automatic vacuum of table "naoya.pg_toast.pg_toast_72882": index scans: 1### LOG_AVAC ### pages: 50 removed,0 remain, 0 skipped due to pins### LOG_AVAC ### tuples: 200 removed, 0 remain, 0 are dead but not yet removable###LOG_AVAC ### buffer usage: 223 hits, 2 misses, 1 dirtied### LOG_AVAC ### avg read rate: 0.457 MB/s,avg write rate: 0.228 MB/s### LOG_AVAC ### system usage: CPU 0.00s/0.00u sec elapsed 0.03 sec I think VACOPT_VERBOSE should not be easily replaced to log_min_duration>=0. And, in this v7 patch looks that VERBOSE log is always output in INFO-Level when log_autovacuum_min_duration is set to 0. Is this your point? Regards, --- Naoya
On Tue, Feb 17, 2015 at 5:16 PM, Naoya Anzai wrote: > I found that definition of VERBOSE and log_autovacuum is not > pretty match. For example, "VERBOSE" can output logs of > scanning indices and scanning detail of analyze, but > log_autovacuum can't output them. > [...] Doh. I forgot to add !IsAutoVacuumWorkerProcess() in the checks with VACOPT_VERBOSE that I previously removed... > And, in this v7 patch looks that VERBOSE log is always output > in INFO-Level when log_autovacuum_min_duration is set to 0. > Is this your point? That's my bug, fixed in the attached. -- Michael
Вложения
Hi, Michael I thought about VACOPT_VERBOSE again. As a result, I think you should not delete VACOPT_VERBOSE. According to the last mail I have posted, the difference of manual-vacuum log and auto-vacuum log exists clearly. So, at least you should not touch the mechanism of VACOPT_VERBOSE until both vacuum log formats are unified to a same format. If you agree my think, please undo your removing VACOPT_VERBOSE work. Regards, --- Naoya
On Thu, Feb 19, 2015 at 2:13 PM, Naoya Anzai <anzai-naoya@mxu.nes.nec.co.jp> wrote: > As a result, I think you should not delete VACOPT_VERBOSE. In v8 it is not deleted. It is still declared, and its use is isolated in gram.y, similarly to VACOPT_FREEZE. > According to the last mail I have posted, the difference of > manual-vacuum log and auto-vacuum log exists clearly. Did you test the latest patch v8? I have added checks in it to see if a process is an autovacuum worker to control elevel and the extra logs of v7 do not show up. > So, at least you should not touch the mechanism of VACOPT_VERBOSE > until both vacuum log formats are unified to a same format. If you mean that we should have the same kind of log outputs for autovacuum and manual vacuum, I think that this is not going to happen. Autovacuum entries are kept less verbose on purpose, contract that v7 clealy broke. > If you agree my think, please undo your removing VACOPT_VERBOSE work. Well, I don't agree :) And I am guessing that you did not look at v8 as well. Centralizing the control of logs using log_min_duration is more extensible than simply having VACOPT_VERBOSE. -- Michael
On Thu, Feb 19, 2015 at 3:32 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Feb 19, 2015 at 2:13 PM, Naoya Anzai > <anzai-naoya@mxu.nes.nec.co.jp> wrote: >> As a result, I think you should not delete VACOPT_VERBOSE. > > In v8 it is not deleted. It is still declared, and its use is isolated > in gram.y, similarly to VACOPT_FREEZE. > >> According to the last mail I have posted, the difference of >> manual-vacuum log and auto-vacuum log exists clearly. > > Did you test the latest patch v8? I have added checks in it to see if > a process is an autovacuum worker to control elevel and the extra logs > of v7 do not show up. > >> So, at least you should not touch the mechanism of VACOPT_VERBOSE >> until both vacuum log formats are unified to a same format. > > If you mean that we should have the same kind of log outputs for > autovacuum and manual vacuum, I think that this is not going to > happen. Autovacuum entries are kept less verbose on purpose, contract > that v7 clealy broke. > >> If you agree my think, please undo your removing VACOPT_VERBOSE work. > > Well, I don't agree :) And I am guessing that you did not look at v8 > as well. Centralizing the control of logs using log_min_duration is > more extensible than simply having VACOPT_VERBOSE. With the patch, VACUUM ANALYZE VERBOSE doesn't emit any verbose message. Why did you remove that functionality? Regards, -- Fujii Masao
On Thu, Mar 5, 2015 at 7:10 PM, Fujii Masao wrote: > With the patch, VACUUM ANALYZE VERBOSE doesn't emit any verbose message. > Why did you remove that functionality? Oops. Sorry about that. In gram.y, the combination of VacuumStmt with AnalyzeStmt overwrote the value of log_min_duration incorrectly. I also found another bug related to logging of ANALYZE not working correctly because of the use of IsAutoVacuumWorkerProcess() instead of VACOPT_VERBOSE (this is reducing the diffs of the patch btw). All those things are fixed in the attached. -- Michael
Вложения
On Thu, Mar 5, 2015 at 9:49 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Mar 5, 2015 at 7:10 PM, Fujii Masao wrote: >> With the patch, VACUUM ANALYZE VERBOSE doesn't emit any verbose message. >> Why did you remove that functionality? > > Oops. Sorry about that. In gram.y, the combination of VacuumStmt with > AnalyzeStmt overwrote the value of log_min_duration incorrectly. I > also found another bug related to logging of ANALYZE not working > correctly because of the use of IsAutoVacuumWorkerProcess() instead of > VACOPT_VERBOSE (this is reducing the diffs of the patch btw). All > those things are fixed in the attached. Thanks for updating the patch! Why does log_min_duration need to be set even when manual VACUUM command is executed? Per the latest version of the patch, log_min_duration is checked only when the process is autovacuum worker. So ISTM that log_min_duration doesn't need to be set in gram.y. It's even confusing to me. Or if you're going to implement something like "VACUUM VERBOSE DURATION n" (i.e., verbose message is output if n seconds have been elapsed), that might be necessary, though... Regards, -- Fujii Masao
On Fri, Mar 6, 2015 at 12:44 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Thu, Mar 5, 2015 at 9:49 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Mar 5, 2015 at 7:10 PM, Fujii Masao wrote: >>> With the patch, VACUUM ANALYZE VERBOSE doesn't emit any verbose message. >>> Why did you remove that functionality? >> >> Oops. Sorry about that. In gram.y, the combination of VacuumStmt with >> AnalyzeStmt overwrote the value of log_min_duration incorrectly. I >> also found another bug related to logging of ANALYZE not working >> correctly because of the use of IsAutoVacuumWorkerProcess() instead of >> VACOPT_VERBOSE (this is reducing the diffs of the patch btw). All >> those things are fixed in the attached. > > Thanks for updating the patch! > > Why does log_min_duration need to be set even when manual VACUUM command is > executed? Per the latest version of the patch, log_min_duration is checked only > when the process is autovacuum worker. So ISTM that log_min_duration doesn't > need to be set in gram.y. It's even confusing to me. Or if you're going to > implement something like "VACUUM VERBOSE DURATION n" (i.e., verbose message > is output if n seconds have been elapsed), that might be necessary, though... Thanks for reminding. The DURATION-like clause was exactly a point mentioned by Anzai-san upthread, and it made sense to me to be in-line with the other parameters controlling the freeze (discussion somewhat related to that => http://www.postgresql.org/message-id/CAB7nPqRZX7Pv2B-R7xHmAh52tfjAQGfy9btqwFstgQgXks=iSw@mail.gmail.com) but we can live without it for this patch as VACOPT_VERBOSE is used only by manual VACUUM and not by autovacuum to choose the log elevel. -- Michael
Вложения
On Fri, Mar 6, 2015 at 1:07 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Mar 6, 2015 at 12:44 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >> On Thu, Mar 5, 2015 at 9:49 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Thu, Mar 5, 2015 at 7:10 PM, Fujii Masao wrote: >>>> With the patch, VACUUM ANALYZE VERBOSE doesn't emit any verbose message. >>>> Why did you remove that functionality? >>> >>> Oops. Sorry about that. In gram.y, the combination of VacuumStmt with >>> AnalyzeStmt overwrote the value of log_min_duration incorrectly. I >>> also found another bug related to logging of ANALYZE not working >>> correctly because of the use of IsAutoVacuumWorkerProcess() instead of >>> VACOPT_VERBOSE (this is reducing the diffs of the patch btw). All >>> those things are fixed in the attached. >> >> Thanks for updating the patch! >> >> Why does log_min_duration need to be set even when manual VACUUM command is >> executed? Per the latest version of the patch, log_min_duration is checked only >> when the process is autovacuum worker. So ISTM that log_min_duration doesn't >> need to be set in gram.y. It's even confusing to me. Or if you're going to >> implement something like "VACUUM VERBOSE DURATION n" (i.e., verbose message >> is output if n seconds have been elapsed), that might be necessary, though... > > Thanks for reminding. The DURATION-like clause was exactly a point > mentioned by Anzai-san upthread, and it made sense to me to be in-line > with the other parameters controlling the freeze (discussion somewhat > related to that => > http://www.postgresql.org/message-id/CAB7nPqRZX7Pv2B-R7xHmAh52tfjAQGfy9btqwFstgQgXks=iSw@mail.gmail.com) > but we can live without it for this patch as VACOPT_VERBOSE is used > only by manual VACUUM and not by autovacuum to choose the log elevel. Are you planning to update the patch so that it's based on the commit 0d83138? Regards, -- Fujii Masao
On Thu, Mar 19, 2015 at 12:23 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > Are you planning to update the patch so that it's based on the commit 0d83138? Yes... Very soon. -- Michael
On Thu, Mar 19, 2015 at 12:40 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Mar 19, 2015 at 12:23 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >> Are you planning to update the patch so that it's based on the commit 0d83138? > > Yes... Very soon. And here is the rebased patch. -- Michael
Вложения
On Thu, Mar 19, 2015 at 1:43 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Mar 19, 2015 at 12:40 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Mar 19, 2015 at 12:23 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >>> Are you planning to update the patch so that it's based on the commit 0d83138? >> >> Yes... Very soon. > > And here is the rebased patch. Thanks for rebasing the patch! Looks good to me. One concern about this patch is; currently log_autovacuum_min_duration can be changed even while autovacuum worker is running. So, for example, when the admin notices that autovacuum is taking very long time, he or she can enable logging of autovacuum activity on the fly. But this patch completely prevents us from doing that, because, with the patch, autovacuum worker always picks up the latest setting value at its starting time and then keeps using it to the end. Probably I can live with this. But does anyone has other thought? Regards, -- Fujii Masao
On Mon, Mar 23, 2015 at 1:54 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Thu, Mar 19, 2015 at 1:43 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Mar 19, 2015 at 12:40 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Thu, Mar 19, 2015 at 12:23 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >>>> Are you planning to update the patch so that it's based on the commit 0d83138? >>> >>> Yes... Very soon. >> >> And here is the rebased patch. > > Thanks for rebasing the patch! Looks good to me. > > One concern about this patch is; currently log_autovacuum_min_duration can be > changed even while autovacuum worker is running. So, for example, when > the admin notices that autovacuum is taking very long time, he or she can > enable logging of autovacuum activity on the fly. But this patch completely > prevents us from doing that, because, with the patch, autovacuum worker always > picks up the latest setting value at its starting time and then keeps using it > to the end. Probably I can live with this. But does anyone has other thought? In AutoVacWorkerMain, I am reading the following: * Currently, we don't pay attention to postgresql.conf changes that * happen during a single daemon iteration,so we can ignore SIGHUP. */ pqsignal(SIGHUP, SIG_IGN); So a worker does not see changes in postgresql.conf once it is run and processes a database, no? The launcher does run ProcessConfigFile() when SIGHUP shows up though. Regards, -- Michael
Michael Paquier wrote: > In AutoVacWorkerMain, I am reading the following: > > * Currently, we don't pay attention to postgresql.conf changes that > * happen during a single daemon iteration, so we can ignore SIGHUP. > */ > pqsignal(SIGHUP, SIG_IGN); > > So a worker does not see changes in postgresql.conf once it is run and > processes a database, no? The launcher does run ProcessConfigFile() > when SIGHUP shows up though. Maybe this is something that we should change. For example, I wonder if this can also affect the cost-delay balancing heuristics; if two backends run the rebalance with different GUC settings because postgresql.conf changed in between each of them starting, would the settings bounce back and forth. I think it's worth reconsidering this. (Don't really remember in detail how it works; maybe it's fine now.) In any case, for log_autovacuum_min_duration it also seems worth keeping reasonably close track of GUC changes. I think reading them just before starting vacuum of a new relation should be enough. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > Michael Paquier wrote: >> So a worker does not see changes in postgresql.conf once it is run and >> processes a database, no? The launcher does run ProcessConfigFile() >> when SIGHUP shows up though. > Maybe this is something that we should change. Yeah, checking for SIGHUP in the worker outer loop (ie once per table) seems like a reasonable thing. regards, tom lane
On Mon, Mar 23, 2015 at 7:07 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Michael Paquier wrote:
>> So a worker does not see changes in postgresql.conf once it is run and
>> processes a database, no? The launcher does run ProcessConfigFile()
>> when SIGHUP shows up though.
> Maybe this is something that we should change.
Yeah, checking for SIGHUP in the worker outer loop (ie once per table)
seems like a reasonable thing.
regards, tom lane
Could it be done more often? Maybe every time it is about to do a cost_delay sleep?
I've certainly regretted the inability to change autovacuum_vacuum_cost_delay mid-table on more than one occasion.
This was mostly during doing testing work, but still I'm sure other people have run into this problem, perhaps without knowing it.
Cheers,
Jeff
Jeff Janes <jeff.janes@gmail.com> writes: > On Mon, Mar 23, 2015 at 7:07 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Yeah, checking for SIGHUP in the worker outer loop (ie once per table) >> seems like a reasonable thing. > Could it be done more often? Maybe every time it is about to do a > cost_delay sleep? That sounds risky from here. Normal backends don't check it more often than once per command. regards, tom lane
On Mon, Mar 23, 2015 at 11:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> Michael Paquier wrote: >>> So a worker does not see changes in postgresql.conf once it is run and >>> processes a database, no? The launcher does run ProcessConfigFile() >>> when SIGHUP shows up though. > >> Maybe this is something that we should change. > > Yeah, checking for SIGHUP in the worker outer loop (ie once per table) > seems like a reasonable thing. That sounds fine to me as well. A patch would not be complicated, but is this portion really 9.5 material? Also, this is a discussion wider than only log_autovacuum_min_duration, as autovacuum cost parameters are also available as reloptions. -- Michael
Michael Paquier wrote: > On Mon, Mar 23, 2015 at 11:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > >> Michael Paquier wrote: > >>> So a worker does not see changes in postgresql.conf once it is run and > >>> processes a database, no? The launcher does run ProcessConfigFile() > >>> when SIGHUP shows up though. > > > >> Maybe this is something that we should change. > > > > Yeah, checking for SIGHUP in the worker outer loop (ie once per table) > > seems like a reasonable thing. > > That sounds fine to me as well. A patch would not be complicated, but > is this portion really 9.5 material? IMO yes. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Mar 23, 2015 at 7:46 PM, Alvaro Herrera wrote: > Michael Paquier wrote: >> On Mon, Mar 23, 2015 at 11:07 PM, Tom Lane wrote: >> > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> >> Michael Paquier wrote: >> >>> So a worker does not see changes in postgresql.conf once it is run and >> >>> processes a database, no? The launcher does run ProcessConfigFile() >> >>> when SIGHUP shows up though. >> > >> >> Maybe this is something that we should change. >> > >> > Yeah, checking for SIGHUP in the worker outer loop (ie once per table) >> > seems like a reasonable thing. >> >> That sounds fine to me as well. A patch would not be complicated, but >> is this portion really 9.5 material? > > IMO yes. So, attached are two patches: - 0001 enables SIGHUP tracking in do_autovacuum(), which is checked before processing one table. I reused avl_sighup_handler for the worker, renaming it av_sighup_handler.. - 0002 is the patch to add log_autovacuum_min_duration as a reloption. There is nothing really changed, the value of log_autovacuum_min_duration being picked up at startup with table_recheck_autovac() is used until the end for one table. Regards, -- Michael
Вложения
Michael Paquier wrote: > So, attached are two patches: > - 0001 enables SIGHUP tracking in do_autovacuum(), which is checked > before processing one table. I reused avl_sighup_handler for the > worker, renaming it av_sighup_handler.. > - 0002 is the patch to add log_autovacuum_min_duration as a reloption. > There is nothing really changed, the value of > log_autovacuum_min_duration being picked up at startup with > table_recheck_autovac() is used until the end for one table. Thanks. You added this in the worker loop processing each table: /* * Check for config changes before processing each collected table. */ if (got_SIGHUP) { got_SIGHUP= false; ProcessConfigFile(PGC_SIGHUP); /* shutdown requested in config file? */ if (!AutoVacuumingActive()) break; } I think bailing out if autovac is disabled is a good idea; for instance, if this is a for-wraparound vacuum, we should keep running. My first reaction is that this part of the test ought to be moved down a screenful or so, until we've ran the recheck step and we have the for-wraparound flag in hand; only bail out if that one is not set. But on the other hand, maybe the worker will process some tables that are not for-wraparound in between some other tables that are, so that might turn out to be a bad idea as well. (Though I'm not 100% that it works that way; consider commit f51ead09df for instance.) Maybe the test to use for this is something along the lines of "if autovacuum was enabled before and is no longer enabled, bail out, otherwise keep running". This implies that we need to keep track of whether autovac was enabled at the start of the worker run. Another thing, but not directly related to this patch: while looking at the doc changes, I noticed that we don't have index entries for the reloptions we have; for instance, the word "fillfactor" doesn't appear in the bookindex.html page at all. Having these things all crammed in the CREATE TABLE page seems somewhat poor. I think we could improve on this by having a listing of settable parameters in a separate section, and have CREATE TABLE, ALTER TABLE, and the Server Configuration chapter link to that; we could also have index entries for these items. Of course, the simpler fix is to just add a few <indexterm> tags. As a note, there is no point to "Assert(foo != NULL)" tests when foo is later dereferenced in the same routine: the crash is going to happen anyway at the dereference, so it's just a LOC uselessly wasted. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
--- a/doc/src/sgml/ref/create_table.sgml +++ b/doc/src/sgml/ref/create_table.sgml @@ -881,9 +881,11 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI <literal>toast.</literal>,which can be used to control the behavior of the table's secondary <acronym>TOAST</> table,if any (see <xref linkend="storage-toast"> for more information about TOAST). - Note that the TOAST table inherits the - <literal>autovacuum_*</literal> values from its parent table, if there are - no <literal>toast.autovacuum_*</literal> settings set. + Note that the TOAST table inherits values of <literal>autovacuum_*</literal> + and <literal>log_autovacuum_min_duration</literal> from its parent table, if + there are no values set for respectively + <literal>toast.autovacuum_*</literal> and + <literal>toast.log_autovacuum_min_duration</literal>. </para> I think this could use some wordsmithing; I didn't like listing the parameters explicitely, and Jaime Casanova suggested not using the terms "inherit" and "parent table" in this context. So I came up with this: Note that the TOAST table uses the parameter values defined for the main table, for each parameter applicable to TOAST tablesand for which no value is set in the TOAST table itself. Thoughts? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Apr 3, 2015 at 5:57 AM, Alvaro Herrera wrote: > You added this in the worker loop processing each table: > > /* > * Check for config changes before processing each collected table. > */ > if (got_SIGHUP) > { > got_SIGHUP = false; > ProcessConfigFile(PGC_SIGHUP); > > /* shutdown requested in config file? */ > if (!AutoVacuumingActive()) > break; > } > > I think bailing out if autovac is disabled is a good idea; for instance, > if this is a for-wraparound vacuum, we should keep running. My first > reaction is that this part of the test ought to be moved down a > screenful or so, until we've ran the recheck step and we have the > for-wraparound flag in hand; only bail out if that one is not set. But > on the other hand, maybe the worker will process some tables that are > not for-wraparound in between some other tables that are, so that might > turn out to be a bad idea as well. (Though I'm not 100% that it works > that way; consider commit f51ead09df for instance.) Maybe the test to > use for this is something along the lines of "if autovacuum was enabled > before and is no longer enabled, bail out, otherwise keep running". > This implies that we need to keep track of whether autovac was enabled > at the start of the worker run. I did consider the case of wraparound but came to the conclusion that bailing out as fast as possible was the idea. But well, I guess that we could play it safe and not add this check after all. The main use-case of this change is now to be able to do re-balancing of the cost parameters. We could still decide later if a worker could bail out earlier or not, depending on what. > Another thing, but not directly related to this patch: while looking at > the doc changes, I noticed that we don't have index entries for the > reloptions we have; for instance, the word "fillfactor" doesn't appear > in the bookindex.html page at all. Having these things all crammed in > the CREATE TABLE page seems somewhat poor. I think we could improve on > this by having a listing of settable parameters in a separate section, > and have CREATE TABLE, ALTER TABLE, and the Server Configuration chapter > link to that; we could also have index entries for these items. > Of course, the simpler fix is to just add a few <indexterm> tags. This sounds like a good idea, and this refactoring would meritate a different patch and a different thread. I guess that it should be a new section in Server Configuration then, named "Relation Options", with two different subsections for index options and table options. > As a note, there is no point to "Assert(foo != NULL)" tests when foo is > later dereferenced in the same routine: the crash is going to happen > anyway at the dereference, so it's just a LOC uselessly wasted. Check. I still think that it is useful in this case though... But removed. > I think this could use some wordsmithing; I didn't like listing the > parameters explicitely, and Jaime Casanova suggested not using the terms > "inherit" and "parent table" in this context. So I came up with this: > Note that the TOAST table uses the parameter values defined for the main > table, for each parameter applicable to TOAST tables and for which no > value is set in the TOAST table itself. Fine for me. -- Michael
Вложения
On Fri, Apr 3, 2015 at 3:26 PM, Michael Paquier wrote: > [...] > Fine for me. And here are the correct patches. Sorry for that. -- Michael
Вложения
Michael Paquier wrote: > On Fri, Apr 3, 2015 at 3:26 PM, Michael Paquier wrote: > > [...] > > Fine for me. > > And here are the correct patches. Sorry for that. Thanks, pushed. I added one extra comment to the SIGHUP patch in the place where you previously had the exit. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Apr 3, 2015 at 11:59 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Michael Paquier wrote: >> On Fri, Apr 3, 2015 at 3:26 PM, Michael Paquier wrote: >> > [...] >> > Fine for me. >> >> And here are the correct patches. Sorry for that. > > Thanks, pushed. I added one extra comment to the SIGHUP patch in the > place where you previously had the exit. Thanks! -- Michael