Thread: Proposals for making it easier to write correct bgworkers

Hi all

As I've gained experience working on background workers, it's become increasingly clear that they're a bit too different to normal backends for many nontrivial uses.

I thought I'd take a moment to note some of it here, along with some proposals for things we could potentially do to make it much easier to use bgworkers correctly especially when using them to run queries.

This is NOT A PATCH SET. It's a set of discussion proposals and it's also intended as a bit of a helper for people just getting started on bgworkers. There are a lot of subtle differences in the runtime environment a basic bgworker provides vs the runtime environment extension authors will be used to when writing fmgr-callable C functions.

(It looks like pg12 and pg13 have some improvements, so some of the issues I was going to mention with error cleanup paths and locking aren't relevant anymore.)

DIFFERENCES WHEN CODING FOR BGWORKERS

----

Some of the most important differences vs normal backends that I've noticed are:

* There's no supported way to recover from an error and continue execution. Each bgworker has to roll its own logic based on PostgresMain()'s sigsetjmp() handler and maintain it.

* The bgworker infrastructure doesn't set up the the  application_name in PGPROC

* Example bgworkers roll their own signal handlers rather than using die, procsignal_sigusr1_handler, and PostgresSigHupHandler. This makes them seem to work OK, but fail to respect CHECK_FOR_INTERRUPTS() and other expected behaviour when they call into the executor etc.

* Each bgworker has to roll its own mainloop with ConfigReloadPending / ProcessConfigFile() etc, and this isn't illustrated at all in the examples

* Example workers don't interact with the pgstat subsystem

Managing bgworker lifecycle is also quite difficult. The postmaster doesn't offer any help and we don't expose much help from core. Any extension that spawns workers needs to do its own handling of worker startup registration in shmem, worker exit reaping, handling workers that exit before registering, etc. This can be seen in the logical replication code, which rolls its own management per src/backend/replication/logical/launcher.c .

PROPOSED CHANGES IN EXAMPLE BGWORKERS/CONTRIBS

-----

So I'd like to make these changes to example bgworkers and to contrib extensions:

* Change example bgworkers in contrib to set up normal default signal handlers, not define their own

* Add ConfigReloadPending and ProcessConfigFile() to all example bgworkers

* Show pgstat use in example bgworkers

PROPOSED BGW SETUP AND MAINLOOP HELPERS

----

and I'm wondering if anyone thinks it's a good idea to add some bgworker mainloop helper API, where one call is made at the start of the bgw, before BackgroundWorkerInitializeConnection(), to:

* set application_name

* set process title

* set up default signal handlers and unblock signals

* creates and assigns a BGWMemoryContext named after the bgw

* set state to idle in pgstat

* sets up MyProcPort (or teach InitPostgres to do so for a bgw)

* takes a function pointer for a before_shmem_exit cleanup handler

  to encourage ext authors to use this approach

then a mainloop helper that:

* Checks that CurrentMemoryContext == BGWMemoryContext

* Runs CHECK_FOR_INTERRUPTS()

* Checks for postmaster death and exits

* Checks for and performs config file reload

PROPOSED ERROR HANDLING HELPER

----

It'd also be beneficial to have a helper for bgw mainloops with error recovery, generalizing and extracting the pattern I see in all these routines:

src/backend/postmaster/autovacuum.c=AutoVacLauncherMain
src/backend/postmaster/autovacuum.c=AutoVacWorkerMain
src/backend/postmaster/bgworker.c=StartBackgroundWorker
src/backend/postmaster/bgwriter.c=BackgroundWriterMain
src/backend/postmaster/checkpointer.c=CheckpointerMain
src/backend/postmaster/walwriter.c=WalWriterMain
src/backend/tcop/postgres.c=PostgresMain

which all do their own error reset loops. There's way too much copy/paste going on there IMO, even before we consider bgworkers, and it's nearly impossible for anyone who isn't quite an experienced Pg developer to write a bgw that can do anything with errors except promote ERROR to FATAL and die.

This would be modeled on PosgresMain(). The worker would only need to add its own logic to the sigsetjmp() error jump path, not duplicate all the core cleanup work.

PROPOSED GENERALISED WORKER MANAGEMENT

----

Finally I'm wondering if there's any interest in generalizing the logical rep worker management for other bgworkers. I've done a ton of work with worker management and it's something I'm sure I could take on but I don't want to write it without knowing there's some chance of acceptance.

The general idea is to provide a way for bgworkers to start up managers for pools / sets of workers. They launch them and have a function they can call in their mainloop that watches their child worker states, invoking callbacks when they fail to launch, launch successfully, exit cleanly after finishing their work, or die with an error. Workers are tracked in a shmem seg where the start of the seg must be a key struct (akin to how the hash API works). We would provide calls to look up a worker shmem struct by key, signal a worker by key, wait for a worker to exit (up to timeout), etc. Like in the logical rep code, access to the worker registration shmem would be controlled by LWLock. The extension code can put whatever it wants in the worker shmem entries after the key, including various unions or whatever - the worker management API won't care.

This abstracts all the low level mess away from bgworker implementations and lets them focus on writing the code they want to run.

I'd probably suggest doing so by extracting the logical rep worker management, and making the logical rep code use the generalized worker management. So it'd be proven, and have in core users.

PROPOSED XACT API WRAPPER

----

Additionally there are some more general postgres API patterns that the team working on pglogical and BDR has landed up writing wrappers for or compensating for. One of the most important is transaction management. In many places in pglogical where we start lightweight transactions in pglogical we use a wrapper that starts the txn if there isn't already one open and remembers whether one was started. Then the end helper closes it only if the start helper opened it. Finally, the memory context is always reset to the memory context before txn start once the txn ends, instead of being left at TopMemoryContext, since we've learned that that's an extremely common source of bugs.

I propose a wrapper for lightweight txn management, like the one we use in many places in pglogical. This is not a copy of that code, it's just a scratched out example of roughly how we do it:

LocalTransactionState txn;

StartTransactionIfNotStarted(&txn, LXACT_WANT_SNAPSHOT);

...

CommitTransactionCommandIfStarted(&txn);

where

static inline void

StartTransactionIfNotStarted(LocalTransactionState *txn, int flags)

{

    txn->started = !TransactionInProgress();

    txn->save_mctx = CurrentMemoryContext;

    if (txn->started)

    {

        txn->pushed_snapshot = false;

        StartTransactionCommand();

        if (flags & LXACT_WANT_SNAPSHOT)

        {

            txn->pushed_snapshot = true;

            PushActiveSnapshot(GetTransactionSnapshot());

        }

    }

}

static inline void

CommitTransactionIfNotStarted(LocalTransactionState *txn)

{

    if (txn->started)

    {

        Assert(TransactionInProgress());

        Assert(CurrentMemoryContext == TopTransactionContext);

        if (txn->pushed_snapshot)

            PopActiveSnapshot();

        CommitTransactionCommand();

    }

    else

    {

        /* Caller didn't change context between start and end */

        Assert(CurrentMemoryContext == txn->save_mctx);

    }

    (void) MemoryContextSwitchTo(txn->save_mctx);

}

--

 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

Hi all

As I've gained experience working on background workers, it's become increasingly clear that they're a bit too different to normal backends for many nontrivial uses.

PROPOSED GENERALISED WORKER MANAGEMENT

----

Finally I'm wondering if there's any interest in generalizing the logical rep worker management for other bgworkers. I've done a ton of work with worker management and it's something I'm sure I could take on but I don't want to write it without knowing there's some chance of acceptance.

The general idea is to provide a way for bgworkers to start up managers for pools / sets of workers. They launch them and have a function they can call in their mainloop that watches their child worker states, invoking callbacks when they fail to launch, launch successfully, exit cleanly after finishing their work, or die with an error. Workers are tracked in a shmem seg where the start of the seg must be a key struct (akin to how the hash API works). We would provide calls to look up a worker shmem struct by key, signal a worker by key, wait for a worker to exit (up to timeout), etc. Like in the logical rep code, access to the worker registration shmem would be controlled by LWLock. The extension code can put whatever it wants in the worker shmem entries after the key, including various unions or whatever - the worker management API won't care.

This abstracts all the low level mess away from bgworker implementations and lets them focus on writing the code they want to run.

I'd probably suggest doing so by extracting the logical rep worker management, and making the logical rep code use the generalized worker management. So it'd be proven, and have in core users.