On Wed, Aug 25, 2010 at 9:39 PM, Itagaki Takahiro
<itagaki.takahiro@gmail.com> wrote:
> On Tue, Jul 13, 2010 at 11:31 PM, Markus Wanner <markus@bluegap.ch> wrote:
>> This patch turns the existing autovacuum launcher into an always running
>> process, partly called the coordinator. If autovacuum is disabled, the
>> coordinator process still gets started and keeps around, but it doesn't
>> dispatch vacuum jobs.
>
> I think this part is a reasonable proposal, but...
It's not clear to me whether it's better to have a single coordinator
process that handles both autovacuum and other things, or whether it's
better to have two separate processes.
>> The coordinator process now uses imessages to communicate with background
>> (autovacuum) workers and to trigger a vacuum job.
>> It also adds two new controlling GUCs: min_spare_background_workers and
>> max_spare_background_workers.
>
> Other changes in the patch doesn't seem be always needed for the purpose.
> In other words, the patch is not minimal.
> The original purpose could be done without IMessage.
> Also, min/max_spare_background_workers are not used in the patch at all.
> (BTW, min/max_spare_background_*helpers* in postgresql.conf.sample is
> maybe typo.)
>
> The most questionable point for me is why you didn't add any hook functions
> in the coordinator process. With the patch, you can extend the coordinator
> protocols with IMessage, but it requires patches to core at handle_imessage().
> If you want fully-extensible workers, we should provide a method to develop
> worker codes in an external plugin.
I agree with this criticism, but the other thing that strikes me as a
nonstarter is having the postmaster participate in the imessages
framework. Our general rule is that the postmaster must avoid
touching shared memory; else a backend that scribbles on shared memory
might take out the postmaster, leading to a failure of the
crash-and-restart logic.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company