Re: Proposal : For Auto-Prewarm.

Hello,

On Tue, Feb 7, 2017 at 5:52 PM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com>
wrote:

> Thanks Beena,
>
> On Tue, Feb 7, 2017 at 4:46 PM, Beena Emerson <memissemerson(at)gmail(dot)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!