Hello, On Tue, Feb 7, 2017 at 5:52 PM, Mithun Cy <mithun...@enterprisedb.com> wrote:
> Thanks Beena, > > On Tue, Feb 7, 2017 at 4:46 PM, Beena Emerson <memissemer...@gmail.com> > wrote: > > Few more comments: > > > > = Background worker messages: > > > > - Workers when launched, show messages like: "logical replication > launcher > > started”, "autovacuum launcher started”. We should probably have a > similar > > message to show that the pg_prewarm load and dump bgworker has started. > > -- Thanks, I will add startup and shutdown message. > > > - "auto pg_prewarm load: number of buffers to load x”, other messages > show > > space before and after “:”, we should keep it consistent through out. > > > > -- I think you are testing patch 03. The latest patch_04 have > corrected same. Can you please re-test it. > I had initially written comments for 03 and then again tested for 04 and retained comments valid for it. I guess I missed to removed this. Sorry. > > > > > = Action of -1. > > I thought we decided that interval value of -1 would mean that the auto > > prewarm worker will not be run at all. With current code, -1 is > explained to > > mean it will not dump. I noticed that reloading with new option as -1 > stops > > both the workers but restarting loads the data and then quits. Why does > it > > allow loading with -1? Please explain this better in the documents. > > > > -- '-1' means we do not want to dump at all. So we decide not to > continue with launched bgworker and it exits. As per your first > comment, if I register the startup and shutdown messages for auto > pg_prewarm I think it will look better. Will try to explain it in a > better way in documentation. The "auto pg_prewarm load" will not be > affected with dump_interval value. It will always start, load(prewarm) > and then exit. > > > > > = launch_pg_prewarm_dump() > > > =# SELECT launch_pg_prewarm_dump(); > > launch_pg_prewarm_dump > > ------------------------ > > 53552 > > (1 row) > > > > $ ps -ef | grep 53552 > > b_emers+ 53555 4391 0 16:21 pts/1 00:00:00 grep --color=auto 53552 > > -- If dump_interval = -1 "auto pg_prewarm dump" will exit immediately. > > > = Function names > > - load_now could be better named as load_file, load_dumpfile or similar. > > - dump_now -> dump_buffer or better? > > I did choose load_now and dump_now to indicate we are doing it > immediately as invoking them was based on a timer/state. Probably we > can improve that but dump_buffer, load_file may not be the right > replacement. > > > > > = Corrupt file > > if the dump file is corrupted, the system crashes and the prewarm > bgworkers > > are not restarted. This needs to be handled better. > > > > WARNING: terminating connection because of crash of another server > process > > 2017-02-07 16:36:58.680 IST [54252] DETAIL: The postmaster has commanded > > this server process to roll back the current transaction and exit, > because > > another server process exited abnormally and possibly corrupted shared > > memory > > --- Can you please paste you autopgprewarm.save file, I edited the > file manually to some illegal entry but did not see any crash. Only > we failed to load as block number were invalid. Please share your > tests so that I can reproduce same. > I only changed the fork number from 0 to 10 in one of the entry. > > = Documentation > > > > I feel the documentation needs to be improved greatly. > > > > - The first para in pg_prewarm should mention the autoload feature too. > > > > - The new section should be named “The pg_prewarm autoload” or something > > better. "auto pg_prewarm bgworker” does not seem appropriate. The > > configuration parameter should also be listed out clearly like in > auth-delay > > page. The new function launch_pg_prewarm_dump() should be listed under > > Functions. > > -- Thanks I will try to improve the documentation. And, put more details > there. > > -- Thank you, Beena Emerson Have a Great Day!