Re: [v9.3] Extra Daemons (Re: elegant and effective way for running jobs inside a database) - Mailing list pgsql-hackers

2012/11/21 Alvaro Herrera <alvherre@2ndquadrant.com>:
> Alvaro Herrera escribió:
>> FWIW I have pushed this to github; see
>> https://github.com/alvherre/postgres/compare/bgworker
>>
>> It's also attached.
>>
>> The UnBlockSig stuff is the main stumbling block as I see it because it
>> precludes compilation on Windows.  Maybe we should fix that by providing
>> another function that the module is to call after initialization is done
>> and before it gets ready to work ... but having a function that only
>> calls PG_SETMASK() feels somewhat useless to me; and I don't see what
>> else we could do in there.
>
> I cleaned up some more stuff and here's another version.  In particular
> I added wrapper functions to block and unblock signals, so that this
> doesn't need exported UnBlockSig.
>
> I also changed ServerLoop so that it sleeps until a crashed bgworker
> needs to be restarted -- if a worker terminates, and it has requested
> (say) 2s restart time, don't have it wait until the full 60s postmaster
> sleep time has elapsed.
>
> The sample code has been propped up somewhat, too.
>
I checked the v7 patch.

I didn't find out some major problems being remained in this version.
Even though the timing to unblock signals is still under discussion,
it seems to me BackgroundWorkerUnblockSignals() is a reasonable
solution. At least, no tangible requirement to override signal handlers
except for SIGTERM & SIGINT supported by framework.

Some minor comments here:

If we provide background worker a support routine of sigsetjmp()
block with transaction rollback, does it make sense to mimic the
block in PostgresMain()? It reports the raised error and calls
AbortCurrentTransaction() to release overall resources.

How about to drop auth_counter module? I don't think we need
to have two different example modules in contrib.
My preference is worker_spi rather than auth_counter, because
it also provides example to handle transactions.

At SubPostmasterMain(), it checks whether argv[1] has
"--forkbgworker" using strncmp() on the first 14 byte. It should include
the "=" character to be placed on 15th bytes, to avoid unexpected
match.

Also, "cookie" is picked up using atoi(). In case when "--forkbgworker="
takes non-numerical characters, atoi() returns 0. Even though it might
be paranoia, the initial value of BackgroundWorkerCookie should be
1, in spite of 0.

At BackgroundWorkerInitializeConnection(),
+   /* XXX is this the right errcode? */
+   if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
+       ereport(FATAL,
+               (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+                errmsg("database connection requirement not indicated
during registration")));

It only happen when extension calls this function under incorrect
performing mode. So, it is a situation of Assert, not ereport.

So, I'd like to hand over committers this patch near future.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>