Thanks, Robert, I have tried to fix all of you comments and merged to fixes suggested by Thom in patch 15.
On Fri, Jun 23, 2017 at 3:22 AM, Robert Haas <robertmh...@gmail.com> wrote: > > * I suggest renaming launch_autoprewarm_dump() to > autoprewarm_start_worker(). I think that will be clearer. Remember > that user-visible names, internal names, and the documentation should > all match. -- Fixed as suggested. > > * I think the GUC could be pg_prewarm.autoprewarm rather than > pg_prewarm.enable_autoprewarm. It's shorter and, I think, no less > clear. -- I have made GUC name as autoprewarm. > > * In the documentation, don't say "This is a SQL callable function > to....". This is a list of SQL-callable functions, so each thing in > the list is one. Just delete this from the beginning of each > sentence. -- Fixed, Thom has provided the fix and I have merged same to my patch. > * The reason for the AT_PWARM_* naming is not very obvious. Does AT > mean "at" or "auto" or something else? How about > AUTOPREWARM_INTERVAL_DISABLED, AUTOPREWARM_INTERVAL_SHUTDOWN_ONLY, > AUTOPREWARM_INTERVAL_DEFAULT? -- Fixed as suggested. The AUTOPREWARM_INTERVAL_DISABLED is removed now as suggested by below comments. > > * Instead of defining apw_sigusr1_handler, I think you could just use > procsignal_sigusr1_handler. Instead of defining apw_sigterm_handler, > perhaps you could just use die(). got_sigterm would go away, and > you'd just CHECK_FOR_INTERRUPTS(). -- Hi have registered procsignal_sigusr1_handler instead of apw_sigusr1_handler. But I have some doubts about using die instead of apw_sigterm_handler in main autoprewarm worker. On shutdown(sigterm) we should dump and then exit, so doing a CHECK_FOR_INTERRUPTS() we might miss dumping the buffer contents. I think I need to modify some server code in ProcessInterrupts to handle this, please let me know if I am wrong about this. For per-database prewarm worker, this seems right so I am registering die for SIGTERM and calling CHECK_FOR_INTERRUPTS(). Also for autoprewarm_dump_now(). > > * The PG_TRY()/PG_CATCH() block in autoprewarm_dump_now() could reuse > reset_apw_state(), which might be better named detach_apw_shmem(). > Similarly, init_apw_state() could be init_apw_shmem(). -- Fixed. > * Instead of load_one_database(), I suggest > autoprewarm_database_main(). That is more parallel to > autoprewarm_main(), which you have elsewhere, and makes it more > obvious that it's the main entrypoint for some background worker. -- Fixed. > * Instead of launch_and_wait_for_per_database_worker(), I suggest > autoprewarm_one_database(), and instead of prewarm_buffer_pool(), I > suggest autoprewarm_buffers(). The motivation for changing prewarm > to autoprewarm is that we want the names here to be clearly distinct > from the other parts of pg_prewarm that are not related to > autoprewarm. The motivation for changing buffer_pool to buffers is > just that it's a little shorter. Personally I also like the sound it > of it better, but YMMV. -- Fixed as suggested. I have renamed as suggested. > * prewarm_buffer_pool() ends with a useless return statement. I > suggest removing it. -- Sorry Fixed. > > * Instead of creating our own buffering system via buffer_file_write() > and buffer_file_flush(), why not just use the facilities provided by > the operating system? fopen() et. al. provide buffering, and we have > AllocateFile() to provide a FILE *; it's just like > OpenTransientFile(), which you are using, but you'll get the buffering > stuff for free. Maybe there's some reason why this won't work out > nicely, but off-hand it seems like it might. It looks like you are > already using AllocateFile() to read the dump, so using it to write > the dump as well seems like it would be logical. -- Now using AllocateFile(). > * I think that it would be cool if, when autoprewarm completed, it > printed a message at LOG rather than DEBUG1, and with a few more > details, like "autoprewarm successfully prewarmed %d of %d > previously-loaded blocks". This would require some additional > tracking that you haven't got right now; you'd have to keep track not > only of the number of blocks read from the file but how many of those > some worker actually loaded. You could do that with an extra counter > in the shared memory area that gets incremented by the per-database > workers. > > * dump_block_info_periodically() calls ResetLatch() immediately before > WaitLatch; that's backwards. See the commit message for commit > 887feefe87b9099eeeec2967ec31ce20df4dfa9b and the comments it added to > the top of latch.h for details on how to do this correctly. -- Sorry Fixed. > * dump_block_info_periodically()'s main loop is a bit confusing. I > think that after calling dump_now(true) it should just "continue", > which will automatically retest got_sigterm. You could rightly object > to that plan on the grounds that we then wouldn't recheck got_sighup > promptly, but you can fix that by moving the got_sighup test to the > top of the loop, which is a good idea anyway for at least two other > reasons. First, you probably want to check for a pending SIGHUP on > initially entering this function, because something might have changed > during the prewarm phase, and second, see the previous comment about > using the "another valid coding pattern" from latch.h, which puts the > ResetLatch() at the bottom of the loop. -- Agree, my idea was while we were dumping or just immediately after dumping if we receive sigterm we need not dump again for shutdown. I think I am wrong so fixed as you have suggested. > > * I think that launch_autoprewarm_dump() should ereport(ERROR, ...) > rather than just return NULL if the feature is disabled. Maybe > something like ... ERROR: pg_prewarm.dump_interval must be > non-negative in order to launch worker -- I have removed pg_prewarm.dump_interval = -1 case as you have suggested below. So no need for error now. > * Not sure about this one, but maybe we should consider just getting > rid of pg_prewarm.dump_interval = -1 altogether and make the minimum > value 0. If pg_prewarm.autoprewarm = on, then we start the worker and > dump according to the dump interval; if pg_prewarm.autoprewarm = off > then we don't start the worker automatically, but we still let you > start it manually. If you do, it respects the configured > dump_interval. With this design, we don't need the error suggested in > the previous item at all, and the code can be simplified in various > places --- all the checks for AT_PWARM_OFF go away. And I don't see > that we're really losing anything. There's not much sense in dumping > but not prewarming or prewarming but not dumping, so having > pg_prewarm.autoprewarm configure whether the worker is started > automatically rather than whether it prewarms (with a separate control > for whether it dumps) seems to make sense. The one time when you want > to do one without the other is when you first install the extension -- > during the first server lifetime, you'll want to dump, so that after > the next restart you have something to preload. But this design would > allow that. -- Agree. Removed the case pg_prewarm.dump_interval = -1. I had similar doubt which I have tried to raise previously On Tue, May 30, 2017 at 10:16 AM, Mithun Cy <mithun...@enterprisedb.com> wrote: >>There is another GUC setting pg_prewarm.dump_interval if = -1 we stop >>the running autoprewarm worker. I have a doubt should we combine these >>2 entities into one such that it controls the state of autoprewarm >>worker? Now I have one doubt, do we need a mechanism to stop running autoprewarm worker while keeping the server alive? Can I use the pg_prewarm.autoprewarm for the same purpose? -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com
autoprewarm_16.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers