Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
От | Melanie Plageman |
---|---|
Тема | Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode |
Дата | |
Msg-id | CAAKRu_byrV-W3NcROimLrz_vX-ZvKTv9yxvoWgfjOeQUsJ5kpA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode (David Rowley <dgrowleyml@gmail.com>) |
Ответы |
Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
(David Rowley <dgrowleyml@gmail.com>)
Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode (Andres Freund <andres@anarazel.de>) |
Список | pgsql-hackers |
On Mon, Apr 3, 2023 at 8:37 PM David Rowley <dgrowleyml@gmail.com> wrote: > > On Tue, 4 Apr 2023 at 02:49, Melanie Plageman <melanieplageman@gmail.com> wrote: > > v9 attached. > > I've made a pass on the v9-0001 patch only. Here's what I noted down: Thanks for the review! Attached v10 addresses the review feedback below. Remaining TODOs: - tests - do something about config reload changing GUC > v9-0001: > > 1. In the documentation and comments, generally we always double-space > after a period. I see quite often you're not following this. I've gone through and done this. I noticed after building the docs that it doesn't seem to affect how many spaces are after a period in the rendered docs, but I suppose it affects readability when editing the sgml files. > 2. Doc: We could generally seem to break tags within paragraphs into > multiple lines. You're doing that quite a bit, e.g: > > linkend="glossary-buffer-access-strategy">Buffer Access > Strategy</glossterm>. <literal>0</literal> will disable use of a I've updated all of the ones I could find that I did this with. > 2. This is not a command > > <command>BUFFER_USAGE_LIMIT</command> parameter. > > <option> is probably what you want. I have gone through and attempted to correct all option/command/application tag usages. > 3. I'm not sure I agree that it's a good idea to refer to the strategy > with multiple different names. Here you've called it a "ring buffer", > but in the next sentence, you're calling it a Buffer Access Strategy. > > Specifies the ring buffer size for <command>VACUUM</command>. This size > is used to calculate the number of shared buffers which will be reused as > part of a <glossterm linkend="glossary-buffer-access-strategy">Buffer > Access Strategy</glossterm>. <literal>0</literal> disables use of a I've updated this to always prefix any use of ring with "Buffer Access Strategy". I don't know how you'll feel about it. It felt awkward in some places to use Buffer Access Strategy as a complete stand-in for ring buffer. > 4. Can you explain your choice in not just making < 128 a hard error > rather than clamping? > > I guess it means checks like this are made more simple, but that does > not seem like a good enough reason: > > /* check for out-of-bounds */ > if (result < -1 || result > MAX_BAS_RING_SIZE_KB) > > postgres=# vacuum (parallel -1) pg_class; > ERROR: parallel workers for vacuum must be between 0 and 1024 > > Maybe the above is a good guide to follow. > > To allow you to get rid of the clamping code, you'd likely need an > assign hook function for vacuum_buffer_usage_limit. I've added a check hook and replicated the same restrictions in ExecVacuum() where it parses the limit. I have included enforcement of the conditional limit that the ring cannot occupy more than 1/8 of shared buffers. The immediate consequence of this was that my tests were no longer stable (except for the integer overflow one). I have removed them for now until I can come up with a better testing strategy. On the topic of testing, I also thought we should remove the VACUUM(BUFFER_USAGE_LIMIT X, PARALLEL X) test. Though the parallel workers do make their own strategy rings and such a test would be covering some code, I am hesitant to write a test that would never really fail. The observable behavior of not using a strategy will be either 1) basically nothing or 2) the same for parallel and non-parallel. What do you think? > 5. I see vacuum.sgml is full of inconsistencies around the use of > <literal> vs <option>. I was going to complain about your: > > <literal>ONLY_DATABASE_STATS</literal> option. If > <literal>ANALYZE</literal> is also specified, the > <literal>BUFFER_USAGE_LIMIT</literal> value is used for both the vacuum > > but I see you've likely just copied what's nearby. > > There are also plenty of usages of <option> in that file. I'd rather > see you use <option>. Maybe there can be some other patch that sweeps > the entire docs to look for <literal>OPTION_NAME</literal> and > replaces them to use <option>. I haven't done the separate sweep patch, but I have updated my own usages in this set. > 6. I was surprised to see you've added both > GetAccessStrategyWithSize() and GetAccessStrategyWithNBuffers(). I > think the former is suitable for both. GetAccessStrategyWithNBuffers() > seems to be just used once outside of freelist.c This has been updated and reorganized. > 7. I don't think bas_nbuffers() is a good name for an external > function. StrategyGetBufferCount() seems better. I've used this name. > 8. I don't quite follow this comment: > > /* > * TODO: should this be 0 so that we are sure that vacuum() never > * allocates a new bstrategy for us, even if we pass in NULL for that > * parameter? maybe could change how failsafe NULLs out bstrategy if > * so? > */ > > Can you explain under what circumstances would vacuum() allocate a > bstrategy when do_autovacuum() would not? Is this a case of a config > reload where someone changes vacuum_buffer_usage_limit from 0 to > something non-zero? If so, perhaps do_autovacuum() needs to detect > this and allocate a strategy rather than having vacuum() do it once > per table (wastefully). Hmm. Yes, I started hacking on this, but I think it might be a bit tricky to get right. I think it would make sense to check if vacuum_buffer_usage_limit goes from 0 to not 0 or from not 0 to 0 and allow disabling and enabling the buffer access strategy, however, I'm not sure we want to allow changing the size during an autovacuum worker's run. I started writing code to just allow enabling and disabling, but I'm a little concerned that the distinction will be difficult to understand for the user with no obvious indication of what is happening. That is, you change the size and it silently does nothing, but you set it to/from 0 and it silently does something? One alternative for now is to save the ring size before looping through the relations in do_autovacuum() and always restore that value in tab->at_params.ring_size in table_recheck_autovac(). I'm not sure what to do. > 9. buffer/README. I think it might be overkill to document details > about how the new vacuum option works in a section talking about > Buffer Ring Replacement Strategy. Perhaps it just worth something > like: > > "In v16, the 256KB ring was made configurable by way of the > vacuum_buffer_usage_limit GUC and the BUFFER_USAGE_LIMIT VACUUM > option." I've made the change you suggested. > 10. I think if you do #4 then you can get rid of all the range checks > and DEBUG1 elogs in GetAccessStrategyWithSize(). Done. > 11. This seems a bit badly done: > > int vacuum_buffer_usage_limit = -1; > > int VacuumCostPageHit = 1; /* GUC parameters for vacuum */ > int VacuumCostPageMiss = 2; > int VacuumCostPageDirty = 20; > > I'd class vacuum_buffer_usage_limit as a "GUC parameters for vacuum" > too. Probably the CamelCase naming should be followed too. I've made this change. > 12. ANALYZE too? > > {"vacuum_buffer_usage_limit", PGC_USERSET, RESOURCES_MEM, > gettext_noop("Sets the buffer pool size for VACUUM and autovacuum."), I've mentioned this here and also added the option for ANALYZE. > 13. VacuumParams.ring_size has no comments explaining what it is. I've added one. > 14. vacuum_buffer_usage_limit seems to be lumped in with unrelated GUCs > > extern PGDLLIMPORT int maintenance_work_mem; > extern PGDLLIMPORT int max_parallel_maintenance_workers; > +extern PGDLLIMPORT int vacuum_buffer_usage_limit; > > extern PGDLLIMPORT int VacuumCostPageHit; > extern PGDLLIMPORT int VacuumCostPageMiss; I've moved it down a line. > 15. No comment explaining what these are: > > #define MAX_BAS_RING_SIZE_KB (16 * 1024 * 1024) > #define MIN_BAS_RING_SIZE_KB 128 I've added one. > 16. Parameter names in function declaration and definition don't match in: > > extern BufferAccessStrategy > GetAccessStrategyWithNBuffers(BufferAccessStrategyType btype, int > nbuffers); > extern BufferAccessStrategy > GetAccessStrategyWithSize(BufferAccessStrategyType btype, int > nbuffers); I've fixed this. > Also, line wraps at 79 chars. (80 including line feed) I've fixed that function prototype instance of it. In general line wrap limit + pgindent can be quite challenging. I often break something onto multiple lines to appease the line limit and then pgindent will add an absurd number of tabs to align the second line in a way that looks truly awful. I try to make local variables when this is a problem, but it is often quite annoying to do that. I wish there was some way to make pgindent do something different in these cases. > 17. If you want to test the 16GB upper limit, maybe going 1KB (or > 8KB?) rather than 1GB over 16GB is better? 2097153kB, I think. > > VACUUM (BUFFER_USAGE_LIMIT '17 GB') vac_option_tab; I've removed this test for now until I figure out a way to actually hit this reliably with different-sized shared buffers. - Melanie
Вложения
В списке pgsql-hackers по дате отправления: