pg.ddx.io pgsql-hackers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
[PATCH][HOTFIX] vacuum_cost_delay type change from int to real have not been done everywhere 2+ messages / 2 participants [nested] [flat]
* [PATCH][HOTFIX] vacuum_cost_delay type change from int to real have not been done everywhere @ 2019-03-26 16:19 Nikolay Shaplov <dhyan@nataraj.su> 2019-03-26 17:38 ` Re: [PATCH][HOTFIX] vacuum_cost_delay type change from int to real have not been done everywhere Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 1 reply; 2+ messages in thread From: Nikolay Shaplov @ 2019-03-26 16:19 UTC (permalink / raw) To: pgsql-hackers; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Julien Rouhaud <rjuju123@gmail.com>; Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; David Rowley <david.rowley@2ndquadrant.com>; Jeff Janes <jeff.janes@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; Joe Conway <mail@joeconway.com>; Peter Geoghegan <pg@bowt.ie> Hi! In caf626b2 type of vacuum_cost_delay have been switched from int to real, everywhere, but not in *RelOpts[] arrays. For some reason it continued to build and work. But I think it is better to move vacuum_cost_delay from int to real there too... Patch attached. PS. As you can see current reloption code is error-prone. To properly change reloptions you should simultaneously change code in several different places, and as you can see, it may not report if you missed something. I am working on reloptions code refactoring now, please join reviewing my patches. This work is important as you can see from this example... Attachments: [text/x-patch] vacuum_cost_delay_hotfix_1.diff (1.1K, ../../4861742.12LTaSB3sv@x200m/2-vacuum_cost_delay_hotfix_1.diff) download | inline diff: diff --git a/.gitignore b/.gitignore index 794e35b..37331c2 100644 diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index 3b0b138..ba90235 100644 --- a/src/backend/access/common/reloptions.c +++ b/src/backend/access/common/reloptions.c @@ -214,15 +214,6 @@ static relopt_int intRelOpts[] = }, { { - "autovacuum_vacuum_cost_delay", - "Vacuum cost delay in milliseconds, for autovacuum", - RELOPT_KIND_HEAP | RELOPT_KIND_TOAST, - ShareUpdateExclusiveLock - }, - -1, 0, 100 - }, - { - { "autovacuum_vacuum_cost_limit", "Vacuum cost amount available before napping, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST, @@ -366,6 +357,16 @@ static relopt_real realRelOpts[] = }, { { + "autovacuum_vacuum_cost_delay", + "Vacuum cost delay in milliseconds, for autovacuum", + RELOPT_KIND_HEAP | RELOPT_KIND_TOAST, + ShareUpdateExclusiveLock + }, + -1, 0, 100 + }, + + { + { "seq_page_cost", "Sets the planner's estimate of the cost of a sequentially fetched disk page.", RELOPT_KIND_TABLESPACE, ^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [PATCH][HOTFIX] vacuum_cost_delay type change from int to real have not been done everywhere 2019-03-26 16:19 [PATCH][HOTFIX] vacuum_cost_delay type change from int to real have not been done everywhere Nikolay Shaplov <dhyan@nataraj.su> @ 2019-03-26 17:38 ` Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 2+ messages in thread From: Tom Lane @ 2019-03-26 17:38 UTC (permalink / raw) To: Nikolay Shaplov <dhyan@nataraj.su>; +Cc: pgsql-hackers; Julien Rouhaud <rjuju123@gmail.com>; Andrew Dunstan <andrew.dunstan@2ndquadrant.com>; David Rowley <david.rowley@2ndquadrant.com>; Jeff Janes <jeff.janes@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; Joe Conway <mail@joeconway.com>; Peter Geoghegan <pg@bowt.ie> Nikolay Shaplov <dhyan@nataraj.su> writes: > In caf626b2 type of vacuum_cost_delay have been switched from int to real, > everywhere, but not in *RelOpts[] arrays. Ugh. > For some reason it continued to build and work. I'm not quite sure why it worked either; apparently, the type of that array entry doesn't have anything to do with the variable's storage format. The bounds-check code must think it's dealing with an integer, but that doesn't matter either for the values we need. > PS. As you can see current reloption code is error-prone. Yeah, that was pretty obvious already :-(. Having more than one place defining the type of an option is clearly bogus. I missed that this entry was type-specific because you'd really have to go up to the top of the array to notice that; and since the type information *is* contained in another entry, my bogometer failed to trigger. Fix pushed, thanks for the report! regards, tom lane ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2019-03-26 17:38 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-03-26 16:19 [PATCH][HOTFIX] vacuum_cost_delay type change from int to real have not been done everywhere Nikolay Shaplov <dhyan@nataraj.su> 2019-03-26 17:38 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox