On Thu, Jun 15, 2017 at 12:35 AM, Mithun Cy <mithun...@enterprisedb.com> wrote: > [ new patch ]
I think this is looking better. I have some suggestions: * 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. * I think the GUC could be pg_prewarm.autoprewarm rather than pg_prewarm.enable_autoprewarm. It's shorter and, I think, no less clear. * 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. * 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? * 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(). * 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(). * 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. * 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. * prewarm_buffer_pool() ends with a useless return statement. I suggest removing it. * 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. * 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. * 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. * 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 * 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. That's all I have time for today - hope it helps. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
