Re: Proposal : For Auto-Prewarm.

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

> On Tue, Feb 7, 2017 at 11:53 AM, Beena Emerson <memissemerson(at)gmail(dot)com>
> wrote:
> > launched by other applications. Also with max_worker_processes = 2 and
> > restart, the system crashes when the 2nd worker is not launched:
> > 2017-02-07 11:36:39.132 IST [20573] LOG: auto pg_prewarm load : number
> of
> > buffers actually tried to load 64
> > 2017-02-07 11:36:39.143 IST [18014] LOG: worker process: auto pg_prewarm
> > load (PID 20573) was terminated by signal 11: Segmentation fault
>
> SEGFAULT was the coding mistake I have called the C-language function
> directly without initializing the functioncallinfo. Thanks for
> raising. Below patch fixes same.
>
> --
> Thanks and Regards
> Mithun C Y
> EnterpriseDB: http://www.enterprisedb.com
>

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.

- "auto pg_prewarm load: number of buffers to load x”, other messages show
space before and after “:”, we should keep it consistent through out.

= 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.

= launch_pg_prewarm_dump()
With dump_interval=-1, Though function returns a pid, this worker is not
running in the 04 patch. 03 version it was launching. Dumping is not done
now.

=# 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

= Function names
- load_now could be better named as load_file, load_dumpfile or similar.
- dump_now -> dump_buffer or better?

= 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

= 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.

--
Thank you,

Beena Emerson

Have a Great Day!