[PATCH] More docs on what to do and not do in extension code

On Mon, Jan 18, 2021 at 1:27 PM Craig Ringer
<craig.ringer@enterprisedb.com> wrote:

Hi folks

The attached patch expands the xfunc docs and bgworker docs a little, providing a starting point for developers to learn how to do some common tasks the Postgres Way.

It mentions in brief these topics:

* longjmp() based exception handling with elog(ERROR), PG_CATCH() and PG_RE_THROW() etc
* Latches, spinlocks, LWLocks, heavyweight locks, condition variables
* shm, DSM, DSA, shm_mq
* syscache, relcache, relation_open(), invalidations
* deferred signal handling, CHECK_FOR_INTERRUPTS()
* Resource cleanup hooks and callbacks like on_exit, before_shmem_exit, the resowner callbacks, etc
* signal handling in bgworkers

All very superficial, but all things I really wish I'd known a little about, or even that I needed to learn about, when I started working on postgres.

I'm not sure it's in quite the right place. I wonder if there should be a separate part of xfunc.sgml that covers the slightly more advanced bits of postgres backend and function coding like this, lists relevant README files in the source tree, etc.

I avoided going into details like how resource owners work. I don't want the docs to have to cover all that in detail; what I hope to do is start providing people with clear references to the right place in the code, READMEs, etc to look when they need to understand specific topics.

Thanks for the patch.

Here are some comments:

[1]
   background worker's main function, and must be unblocked by it; this is to
    allow the process to customize its signal handlers, if necessary.
-   Signals can be unblocked in the new process by calling
-   <function>BackgroundWorkerUnblockSignals</function> and blocked by calling
-   <function>BackgroundWorkerBlockSignals</function>.
+   It is important that all background workers set up and unblock signal
+   handling before they enter their main loops. Signal handling in background
+   workers is discussed separately in <xref linkend="bgworker-signals"/>.
   </para>

IMO, we can retain the statement about BackgroundWorkerUnblockSignals
and BackgroundWorkerBlockSignals, but mention the link to
"bgworker-signals" for more details and move the statement "it's
important to unblock signals before enter their main loop" to
"bgworker-signals" section and we can also reason there the
consequences if not done.

[2]
+   interupt-aware APIs</link> for the purpose. Do not
<function>usleep()</function>,
+   <function>system()</function>, make blocking system calls, etc.
+  </para>

Is it "Do not use <function>usleep()</function>,
<function>system()</function> or make blocking system calls etc." ?

[3] IMO, we can remove following from "bgworker-signals" if we retain
it where currently it is, as discussed in [1].
+    Signals can be unblocked in the new process by calling
+    <function>BackgroundWorkerUnblockSignals</function> and blocked by calling
+    <function>BackgroundWorkerBlockSignals</function>.

[4] Can we say
+    The default signal handlers set up for background workers <emphasis>do
+    default background worker signal handlers, it should call

instead of
+    The default signal handlers installed for background workers <emphasis>do
+    default background worker signal handling it should call

[5] IMO, we can have something like below
+    request, etc. Set up these handlers before unblocking signals as
+    shown below:

instead of
+    request, etc. To install these handlers, before unblocking interrupts
+    run:

[6] I think logs and errors either elog() or ereport can be used, so how about
+        Use <function>elog()</function> or <function>ereport()</function> for
+        logging output or raising errors instead of any direct stdio calls.

instead of
+        Use <function>elog()</function> and <function>ereport()</function> for
+        logging output and raising errors instead of any direct stdio calls.

[7] Can we use child processes instead of subprocess ? If okay in
other places in the patch as well.
+        and should only use the main thread. PostgreSQL generally
uses child processes
+        that coordinate over shared memory instead of threads - for
instance, see
+        <xref linkend="bgworker"/>.

instead of
+        and should only use the main thread. PostgreSQL generally
uses subprocesses
+        that coordinate over shared memory instead of threads - see
+        <xref linkend="bgworker"/>.

[8] Why should file descriptor manager API be used to execute
subprocesses/child processes?
+        To execute subprocesses and open files, use the routines provided by
+        the file descriptor manager like <function>BasicOpenFile</function>
+        and <function>OpenPipeStream</function> instead of a direct

[9]: "should always be"? even if it's a blocking extesion, does it work? If our intention is to recommend the developers, maybe we should avoid using the term "should" in the patch in other places as well. + Extension code should always be structured as a non-blocking
work? If our intention is to recommend the developers, maybe we should
avoid using the term "should" in the patch in other places as well.
+ Extension code should always be structured as a non-blocking

[10]: I think it is + you should avoid using <function>sleep()</function> or <function>usleep()</function>
+ you should avoid using <function>sleep()</function> or
<function>usleep()</function>

instead of
+ you should <function>sleep()</function> or
<function>usleep()</function>

[11] I think it is
+        block if this happens. So cleanup of resources is not
entirely managed by PostgreSQL, it
+       must be handled using appropriate callbacks provided by PostgreSQL

instead of
+        block if this happens. So all cleanup of resources not already
+        managed by the PostgreSQL runtime must be handled using appropriate

[12]: I think it is + found in corresponding PostgreSQL header and source files.
+ found in corresponding PostgreSQL header and source files.

instead of
+ found in the PostgreSQL headers and sources.

[13]: I think it is + Use PostgreSQL runtime concurrency and synchronisation primitives
+ Use PostgreSQL runtime concurrency and synchronisation primitives

+ between the PostgreSQL processes. These include signals and
ProcSignal multiplexed

instead of
+ Use the PostgreSQL runtime's concurrency and synchronisation primitives

+ between PostgreSQL processes. These include signals and
ProcSignal multiplexed

[14]: Is it "relation/table based state management"? + Sometimes relation-based state management for extensions is not
+ Sometimes relation-based state management for extensions is not

[15]: I think it is + use PostgreSQL shared-memory based inter-process communication
+ use PostgreSQL shared-memory based inter-process communication

instead of
+ use PostgreSQL's shared-memory based inter-process communication

[16] I think it is
+        or shared memory message queues (<acronym>shm_mq</acronym>). Examples
+        usage of some of these features can be found in the
+        <filename>src/test/modules/test_shm_mq/</filename> sample
extension. Others

instead of
+        or shared memory message queues (<acronym>shm_mq</acronym>). Examples
+        of the use of some these features can be found in the
+        <filename>src/test/modules/test_shm_mq/</filename> example
extension. Others

[17]: I think it is + syscache entries, as this can cause subtle bugs. See
+ syscache entries, as this can cause subtle bugs. See

instead of
+ syscache cache entries, as this can cause subtle bugs. See

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Hi

Thanks so much for reading over this!

Would you mind attaching a revised version of the patch with your edits?
Otherwise I'll go and merge them in once you've had your say on my comments
inline below.

Bruce, Robert, can I have an opinion from you on how best to locate and
structure these docs, or whether you think they're suitable for the main
docs at all? See patch upthread.

On Tue, 19 Jan 2021 at 22:33, Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

Here are some comments:

[1]
background worker's main function, and must be unblocked by it; this is
to
allow the process to customize its signal handlers, if necessary.
-   Signals can be unblocked in the new process by calling
-   <function>BackgroundWorkerUnblockSignals</function> and blocked by
calling
-   <function>BackgroundWorkerBlockSignals</function>.
+   It is important that all background workers set up and unblock signal
+   handling before they enter their main loops. Signal handling in
background
+   workers is discussed separately in <xref linkend="bgworker-signals"/>.
</para>

I wasn't sure either way on that, see your [3]IMO, we can remove following from "bgworker-signals" if we retain below.

[2]:

+   interupt-aware APIs</link> for the purpose. Do not
<function>usleep()</function>,
+   <function>system()</function>, make blocking system calls, etc.
+  </para>

Is it "Do not use <function>usleep()</function>,
<function>system()</function> or make blocking system calls etc." ?

Right. Good catch.

[3]: IMO, we can remove following from "bgworker-signals" if we retain

it where currently it is, as discussed in [1].
+    Signals can be unblocked in the new process by calling
+    <function>BackgroundWorkerUnblockSignals</function> and blocked by
calling
+    <function>BackgroundWorkerBlockSignals</function>.

If so, need to mention that they start blocked and link to the main text
where that's mentioned.

That's part of why I moved this chunk into the signal section.

[4]: Can we say

+    The default signal handlers set up for background workers <emphasis>do
+    default background worker signal handlers, it should call

instead of
+    The default signal handlers installed for background workers
<emphasis>do
+    default background worker signal handling it should call

Huh?

I don't understand this proposal.

s/install/set up/g?

[5]: IMO, we can have something like below

+    request, etc. Set up these handlers before unblocking signals as
+    shown below:

instead of
+    request, etc. To install these handlers, before unblocking interrupts
+    run:

Ditto

[6]: I think logs and errors either elog() or ereport can be used, so how

about
+        Use <function>elog()</function> or <function>ereport()</function>
for
+        logging output or raising errors instead of any direct stdio
calls.

instead of
+        Use <function>elog()</function> and
<function>ereport()</function> for
+        logging output and raising errors instead of any direct stdio
calls.

OK.

[7]: Can we use child processes instead of subprocess ? If okay in

other places in the patch as well.

Fine with me. The point is really that they're non-postgres processes being
spawned by a backend, and that doing so must be done carefully.

[8]: Why should file descriptor manager API be used to execute

subprocesses/child processes?
+        To execute subprocesses and open files, use the routines provided
by
+        the file descriptor manager like
<function>BasicOpenFile</function>
+        and <function>OpenPipeStream</function> instead of a direct

Yeah, that wording is confusing, agreed. The point was that you shouldn't
use system() or popen(), you should OpenPipeStream(). And similarly, you
should avoid fopen() etc and instead use the Pg wrapper BasicOpenFile().

"
PostgreSQL backends are required to limit the number of file descriptors
they
open. To open files, use postgres file descriptor manager routines like
BasicOpenFile()
instead of directly using open() or fopen(). To open pipes to or from
external processes,
use OpenPipeStream() instead of popen().
"

?

[9] "should always be"? even if it's a blocking extesion, does it
work? If our intention is to recommend the developers, maybe we should
avoid using the term "should" in the patch in other places as well.

The trouble is that it's a bit ... fuzzy.

You can get away with blocking for short periods without responding to
signals, but it's a "how long is a piece of string" issue.

"should be" is fine.

A hard "must" or "must not" would be misleading. But this isn't the place
to go into all the details of how time sensitive (or not) interrupt
handling of different kinds is in different places for different worker
types.

[11] I think it is
+        block if this happens. So cleanup of resources is not
entirely managed by PostgreSQL, it
+       must be handled using appropriate callbacks provided by PostgreSQL

instead of
+        block if this happens. So all cleanup of resources not already
+        managed by the PostgreSQL runtime must be handled using
appropriate

I don't agree with the proposed new wording here.

Delete the "So all" from my original, or

... Cleanup of any resources that are not managed

by the PostgreSQL runtime must be handled using appropriate ...

?

[12] I think it is
+ found in corresponding PostgreSQL header and source files.

instead of
+ found in the PostgreSQL headers and sources.

Sure.

[13] I think it is
+ Use PostgreSQL runtime concurrency and synchronisation primitives

+ between the PostgreSQL processes. These include signals and
ProcSignal multiplexed

instead of
+ Use the PostgreSQL runtime's concurrency and synchronisation
primitives

+ between PostgreSQL processes. These include signals and
ProcSignal multiplexed

OK.

[14]: Is it "relation/table based state management"?

+ Sometimes relation-based state management for extensions is not

Hopefully someone who's writing an extension knows that relation mostly ==
table. A relation could be a generic xlog relation etc too. So I think this
is correct as-is.

[15] I think it is
+ use PostgreSQL shared-memory based inter-process communication

instead of
+ use PostgreSQL's shared-memory based inter-process communication

Probably a linguistic preference, I don't mind.

[16]: I think it is

+        or shared memory message queues (<acronym>shm_mq</acronym>).
Examples
+        usage of some of these features can be found in the
+        <filename>src/test/modules/test_shm_mq/</filename> sample
extension. Others

instead of
+        or shared memory message queues (<acronym>shm_mq</acronym>).
Examples
+        of the use of some these features can be found in the
+        <filename>src/test/modules/test_shm_mq/</filename> example
extension. Others

It'd have to be "Example usage" but sure. I don't mind either version after
that correction.

[17] I think it is
+ syscache entries, as this can cause subtle bugs. See

instead of
+ syscache cache entries, as this can cause subtle bugs. See

PIN Number :)

Sure, agreed.

I really appreciate the proof read and comments.

Do you think I missed anything crucial? I've written some material that
summarises pg's concurrency and IPC primitives at a high level but it's
still too much to go into this docs section IMO.

On 1/22/21 1:36 AM, Craig Ringer wrote:

Would you mind attaching a revised version of the patch with your edits?
Otherwise I'll go and merge them in once you've had your say on my
comments inline below.

Bharath, do the revisions in [1]/messages/by-id/CAGRY4nyjHh-Tm89A8eS1x+JtZ-qHU7wY+R0DEEtWfv5TQ3HcGA@mail.gmail.com look OK to you?

Bruce, Robert, can I have an opinion from you on how best to locate and
structure these docs, or whether you think they're suitable for the main
docs at all? See patch upthread.

Bruce, Robert, any thoughts here?

Regards,
--
-David
david@pgmasters.net

[1]: /messages/by-id/CAGRY4nyjHh-Tm89A8eS1x+JtZ-qHU7wY+R0DEEtWfv5TQ3HcGA@mail.gmail.com
/messages/by-id/CAGRY4nyjHh-Tm89A8eS1x+JtZ-qHU7wY+R0DEEtWfv5TQ3HcGA@mail.gmail.com

On Thu, Mar 25, 2021 at 08:49:44AM -0400, David Steele wrote:

On 1/22/21 1:36 AM, Craig Ringer wrote:

Would you mind attaching a revised version of the patch with your edits?
Otherwise I'll go and merge them in once you've had your say on my
comments inline below.

Bharath, do the revisions in [1] look OK to you?

Bruce, Robert, can I have an opinion from you on how best to locate and
structure these docs, or whether you think they're suitable for the main
docs at all? See patch upthread.

Bruce, Robert, any thoughts here?

I know I sent an email earlier this month saying we shouldn't
over-document the backend hooks because the code could drift away from
the README content:

/messages/by-id/20210309172049.GD26575@momjian.us

Agreed. If you document the hooks too much, it allows them to drift
away from matching the code, which makes the hook documentation actually
worse than having no hook documentation at all.

However, for this doc patch, the content seem to be more strategic, so
less likely to change, and hard to figure out from the code directly.
Therefore, I think this would be a useful addition to the docs.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Fri, 26 Mar 2021 at 06:15, Bruce Momjian <bruce@momjian.us> wrote:

On Thu, Mar 25, 2021 at 08:49:44AM -0400, David Steele wrote:

On 1/22/21 1:36 AM, Craig Ringer wrote:

Would you mind attaching a revised version of the patch with your

edits?

Otherwise I'll go and merge them in once you've had your say on my
comments inline below.

Bharath, do the revisions in [1] look OK to you?

Bruce, Robert, can I have an opinion from you on how best to locate and
structure these docs, or whether you think they're suitable for the

main

docs at all? See patch upthread.

Bruce, Robert, any thoughts here?

I know I sent an email earlier this month saying we shouldn't
over-document the backend hooks because the code could drift away from
the README content:

/messages/by-id/20210309172049.GD26575@momjian.us

Agreed. If you document the hooks too much, it allows them to
drift
away from matching the code, which makes the hook documentation
actually
worse than having no hook documentation at all.

However, for this doc patch, the content seem to be more strategic, so
less likely to change, and hard to figure out from the code directly.
Therefore, I think this would be a useful addition to the docs.

Thanks for the kind words. It's good to hear that it may be useful. Let me
know if anything further is needed.

On Mon, 2021-01-18 at 15:56 +0800, Craig Ringer wrote:

The attached patch expands the xfunc docs and bgworker docs a little, providing a starting point for developers
to learn how to do some common tasks the Postgres Way.

I like these changes!

Here is a review:

+      <para>
+       See <xref linkend="xfunc-shared-addin"/> for information on how to
+       request extension shared memory allocations, <literal>LWLock</literal>s,
+       etc.
+      </para>

This doesn't sound very English to me, and I don't see the point in
repeating parts of the enumeration. How about

See ... for detailed information how to allocate these resources.

+  <para>
+   If a background worker needs to sleep or wait for activity it should

Missing comma after "activity".

+   always use <link linkend="xfunc-sleeping-interrupts-blocking">PostgreSQL's
+   interupt-aware APIs</link> for the purpose. Do not <function>usleep()</function>,
+   <function>system()</function>, make blocking system calls, etc.
+  </para>

"system" is not a verb. Suggestion:

Do not use <function>usleep()</function>, <function>system()</function>
or other blocking system calls.

+  <para>
+   The <filename>src/test/modules/worker_spi</filename> and
+   <filename>src/test/modules/test_shm_mq</filename> contain working examples
+   that demonstrates some useful techniques.
   </para>

That is missing a noun in my opinion, I would prefer:

The modules ... contain working examples ...

+  <sect1 id="bgworker-signals" xreflabel="Signal Handling in Background Workers">
+   <title>Signal Handling in Background Workers</title>

It is not a good idea to start a section in the middle of a documentation page.
That will lead to a weird TOC entry at the top of the page.

The better way to do this is to write a short introductory header and convert
most of the first half of the page into another section, so that we end up
with a page the has the introductory material and two TOC entries for the details.

+    The default signal handlers installed for background workers <emphasis>do
+    not interrupt queries or blocking calls into other postgres code</emphasis>

<productname>PostgreSQL</productname>, not "postgres".
Also, there is a comma missing at the end of the line.

+    so they are only suitable for simple background workers that frequently and
+    predictably return control to their main loop. If your worker uses the
+    default background worker signal handling it should call

Another missing comma after "handling".

+    <function>HandleMainLoopInterrupts()</function> on each pass through its
+    main loop.
+   </para>
+
+   <para>
+    Background workers that may make blocking calls into core PostgreSQL code
+    and/or run user-supplied queries should generally replace the default bgworker

Please stick with "background worker", "bgworker" is too sloppy IMO.

+    signal handlers with the handlers used for normal user backends. This will
+    ensure that the worker will respond in a timely manner to a termination
+    request, query cancel request, recovery conflict interrupt, deadlock detector
+    request, etc. To install these handlers, before unblocking interrupts
+    run:

The following would be more grammatical:

To install these handlers, run the following before unblocking interrupts:

+    Then ensure that your main loop and any other code that could run for some
+    time contains <function>CHECK_FOR_INTERRUPTS();</function> calls. A
+    background worker using these signal handlers must use <link
+    linkend="xfunc-resource-management">PostgreSQL's resource management APIs
+    and callbacks</link> to handle cleanup rather than relying on control
+    returning to the main loop because the signal handlers may call

There should be a comma before "because".

+    <function>proc_exit()</function> directly. This is recommended practice
+    for all types of extension code anyway.
+   </para>
+
+   <para>
+    See the comments in <filename>src/include/miscadmin.h</filename> in the
+    postgres headers for more details on signal handling.
+   </para>

"in the postgres headers" is redundant - at any rate, it should be "PostgreSQL".

+        Do not attempt to use C++ exceptions or Windows Structured Exception
+        Handling, and never call <function>exit()</function> directly.

I am alright with this addition, but I think it would be good to link to
<xref linkend="extend-cpp"/> from it.

+      <listitem id="xfunc-threading">
+       <para>
+        Individual PostgreSQL backends are single-threaded.
+        Never call any PostgreSQL function or access any PostgreSQL-managed data
+        structure from a thread other than the main

"PostgreSQL" should always have the <productname> tag.
This applies to a lot of places in this patch.

+ thread. If at all possible your extension should not start any threads

Comma after "possible".

+ and should only use the main thread. PostgreSQL generally uses subprocesses

Hm. If the extension does not start threads, it automatically uses the main thread.
I think that should be removed for clarity.

+        that coordinate over shared memory instead of threads - see
+        <xref linkend="bgworker"/>.

It also uses signals and light-weight locks - but I think that you don't need to
describe the coordination mechanisms here, which are explained in the link you added.

+        primitives like <function>WaitEventSetWait</function> where necessary. Any
+        potentially long-running loop should periodically call <function>
+        CHECK_FOR_INTERRUPTS()</function> to give PostgreSQL a chance to interrupt
+        the function in case of a shutdown request, query cancel, etc. This means

Are there other causes than shutdown or query cancellation?
At any rate, I am not fond of enumerations ending with "etc".

+ you should <function>sleep()</function> or <function>usleep()</function>

You mean: "you should *not* use sleep()"

+ for any nontrivial amount of time - use <function>WaitLatch()</function>

"&mdash;" would be better than "-".

+ or its variants instead. For details on interrupt handling see

Comma after "handling".

[...]
+ based cleanup. Your extension function could be terminated mid-execution

... could be terminated *in* mid-execution ...

+        by PostgreSQL if any function that it calls makes a
+        <function>CHECK_FOR_INTERRUPTS()</function> check. It may not

"makes" sound kind of clumsy in my ears.

+        Spinlocks, latches, condition variables, and more. Details on all of these
+        is far outside the scope of this document, and the best reference is
+        the relevant source code.

I don't think we have to add that last sentence. That holds for pretty much
everything in this documentation.

+       <para>
+        Sometimes relation-based state management for extensions is not
+        sufficient to meet their needs. In that case the extension may need to

Better:
Sometimes, relation-based state management is not sufficient to meet the
needs of an extension.

+        use PostgreSQL's shared-memory based inter-process communication
+        features, and should certainly do so instead of inventing its own or
+        trying to use platform level features. An extension may use
+        <link linkend="xfunc-shared-addin">"raw" shared memory requested from
+        the postmaster at startup</link> or higher level features like dynamic
+        shared memory segments (<acronym>DSM</acronym>),
+        dynamic shared areas (<acronym>DSA</acronym>),
+        or shared memory message queues (<acronym>shm_mq</acronym>). Examples
+        of the use of some these features can be found in the
+        <filename>src/test/modules/test_shm_mq/</filename> example extension. Others
+        can be found in various main PostgreSQL backend code.
+       </para>

Instead of the last sentence, I'd prefer
... or other parts of the source code.

+      <listitem id="xfunc-relcache-syscache">
+       <para>
+        Look up system catalogs and table information using the relcache and syscache

How about "table metadata" rather than "table information"?

+        APIs (<function>SearchSysCache...</function>,
+        <function>relation_open()</function>, etc) rather than attempting to run
+        SQL queries to fetch the information. Ensure that your function holds
+        any necessary locks on the target objects. Take care not to make any calls

... holds *the* necessary locks ...

+        that could trigger cache invalidations while still accessing any
+        syscache cache entries, as this can cause subtle bugs. See

Subtle? Perhaps "bugs that are hard to find".

+        <filename>src/backend/utils/cache/syscache.c</filename>,
+        <filename>src/backend/utils/cache/relcache.c</filename>,
+        <filename>src/backend/access/common/relation.c</filename> and their
+        headers for details.
+       </para>
+      </listitem>

Attached is a new version that has my suggested changes, plus a few from
Bharath Rupireddy (I do not agree with many of his suggestions).

Yours,
Laurenz Albe

From 9c3f7b19ab8364cf151ca8b053150d29d37c390c Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
Date: Sun, 30 May 2021 13:15:55 +0200
Subject: [PATCH] Expand the docs on PostgreSQL extension coding and background
 worker development a little so that key topics like allocation, interrupt
 handling, exit callbacks, transaction callbacks, PG_TRY()/PG_CATCH(),
 resource owners, transaction and snapshot state, etc are at least briefly
 mentioned with a few pointers to where to learn more.

Add some detail on background worker signal handling.

Author: Craig Ringer
Reviewed-By: Laurenz Albe, Bharath Rupireddy
Discussion: https://postgr.es/m/CAGRY4nxanz6MQrOv_babTjg02VwcXkq+eBsQhqdFX6-PgTapDQ@mail.gmail.com
---
 doc/src/sgml/bgworker.sgml |  94 +++++++++++++++++++--
 doc/src/sgml/xfunc.sgml    | 163 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 248 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 7fd673ab54..a99d048ed1 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -1,6 +1,6 @@
 <!-- doc/src/sgml/bgworker.sgml -->
 
-<chapter id="bgworker">
+<chapter id="bgworker" xreflabel="Background Worker Processes">
  <title>Background Worker Processes</title>
 
  <indexterm zone="bgworker">
@@ -29,6 +29,15 @@
   </para>
  </warning>
 
+ <para>
+  All code that will be executed in PostgreSQL background workers is expected
+  to follow the basic rules set out for all PostgreSQL extension code in <xref
+  linkend="xfunc-writing-code"/>.
+ </para>
+
+ <sect1>
+  <title>Background Worker initialization and resource allocation</title>
+
  <para>
   Background workers can be initialized at the time that
   <productname>PostgreSQL</productname> is started by including the module name in
@@ -95,6 +104,10 @@ typedef struct BackgroundWorker
        buffers, or any custom data structures which the worker itself may
        wish to create and use.
       </para>
+      <para>
+       See <xref linkend="xfunc-shared-addin"/> for information on how to
+       allocate these resources.
+      </para>
      </listitem>
     </varlistentry>
 
@@ -212,9 +225,9 @@ typedef struct BackgroundWorker
    Signals are initially blocked when control reaches the
    background worker's main function, and must be unblocked by it; this is to
    allow the process to customize its signal handlers, if necessary.
-   Signals can be unblocked in the new process by calling
-   <function>BackgroundWorkerUnblockSignals</function> and blocked by calling
-   <function>BackgroundWorkerBlockSignals</function>.
+   It is important that all background workers set up and unblock signal
+   handling before they enter their main loops.  Signal handling in background
+   workers is discussed separately in <xref linkend="bgworker-signals"/>.
   </para>
 
   <para>
@@ -296,13 +309,80 @@ typedef struct BackgroundWorker
   </para>
 
   <para>
-   The <filename>src/test/modules/worker_spi</filename> module
-   contains a working example,
-   which demonstrates some useful techniques.
+   Background workers that require inter-process communication and/or
+   synchronisation should use <productname>PostgreSQL</productname>'s built-in
+   IPC and concurrency features as discussed in
+   <xref linkend="xfunc-concurrency-synchronization"/> and
+   <xref linkend="xfunc-ipc"/>.
+  </para>
+
+  <para>
+   If a background worker needs to sleep or wait for activity, it should
+   always use
+   <link linkend="xfunc-sleeping-interrupts-blocking"><productname>PostgreSQL</productname>'s
+   interupt-aware APIs</link> for the purpose.  Do not use
+   <function>usleep()</function>, <function>system()</function> or other
+   blocking system calls.
+  </para>
+
+  <para>
+   The modules <filename>src/test/modules/worker_spi</filename> and
+   <filename>src/test/modules/test_shm_mq</filename> contain working examples
+   that demonstrates some useful techniques.
   </para>
 
   <para>
    The maximum number of registered background workers is limited by
    <xref linkend="guc-max-worker-processes"/>.
   </para>
+
+ </sect1>
+
+ <sect1 id="bgworker-signals" xreflabel="Signal Handling in Background Workers">
+  <title>Signal Handling in Background Workers</title>
+
+  <para>
+   Signals can be unblocked in the new process by calling
+   <function>BackgroundWorkerUnblockSignals</function> and blocked by calling
+   <function>BackgroundWorkerBlockSignals</function>.
+   The default signal handlers set up for background workers
+   <emphasis>do not interrupt queries or blocking calls into other
+   <productname>PostgreSQL</productname> code</emphasis>
+   so they are only suitable for simple background workers that frequently and
+   predictably return control to their main loop.  If your worker uses the
+   default background worker signal handling, it should call
+   <function>HandleMainLoopInterrupts()</function> on each pass through its
+   main loop.
+  </para>
+
+  <para>
+   Background workers that may make blocking calls into core
+   <productname>PostgreSQL</productname> code and/or run user-supplied queries
+   should generally replace the default background worker signal handlers with the
+   handlers used for normal user backends.  This will ensure that the worker will
+   respond in a timely manner to a termination request, query cancel request,
+   recovery conflict interrupt, deadlock detector request, etc.
+   To set up these handlers, run the following before unblocking interrupts:
+<programlisting>
+    pqsignal(SIGTERM, die);
+    pqsignal(SIGUSR1, procsignal_sigusr1_handler);
+    pqsignal(SIGHUP, SignalHandlerForConfigReload);
+</programlisting>
+   Then ensure that your main loop and any other code that could run for some
+   time contains <function>CHECK_FOR_INTERRUPTS()</function> calls.
+   A background worker using these signal handlers must use <link
+   linkend="xfunc-resource-management"><productname>PostgreSQL</productname>'s
+   resource management APIs and callbacks</link> to handle cleanup rather
+   than relying on control returning to the main loop, because the signal
+   handlers may call <function>proc_exit()</function> directly.
+   This is recommended practice for all types of extension code anyway.
+  </para>
+
+  <para>
+   See the comments in <filename>src/include/miscadmin.h</filename>
+   for more details on signal handling.
+  </para>
+
+ </sect1>
+
 </chapter>
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 41bcc5b79d..cc79871f04 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -2593,7 +2593,7 @@ CREATE FUNCTION concat_text(text, text) RETURNS text
     </para>
    </sect2>
 
-   <sect2>
+   <sect2 id="xfunc-writing-code" xreflabel="Writing PostgreSQL Extension Code">
     <title>Writing Code</title>
 
     <para>
@@ -2650,7 +2650,8 @@ CREATE FUNCTION concat_text(text, text) RETURNS text
         <function>malloc</function> and <function>free</function>.
         The memory allocated by <function>palloc</function> will be
         freed automatically at the end of each transaction, preventing
-        memory leaks.
+        memory leaks. See <filename>src/backend/utils/mmgr/README</filename>
+        for more details on PostgreSQL's memory management.
        </para>
       </listitem>
 
@@ -2694,6 +2695,164 @@ CREATE FUNCTION concat_text(text, text) RETURNS text
         error messages to this effect.
        </para>
       </listitem>
+
+      <listitem>
+       <para>
+        Use <function>elog()</function> and <function>ereport()</function> for
+        logging output and raising errors instead of any direct stdio calls.
+        Do not attempt to use C++ exceptions or Windows Structured Exception
+        Handling, and never call <function>exit()</function> directly
+        (for a discussion of extensions in C++, see <xref linkend="extend-cpp"/>).
+        PostgreSQL uses its own <function>longjmp()</function> based
+        exception system for <literal>ERROR</literal>-level <function>elog()</function>
+        and <function>ereport()</function> calls. These can be handled and forwarded with
+        <literal>PG_TRY()</literal> and <literal>PG_RE_THROW()</literal>:
+<programlisting>
+          PG_TRY();
+          {
+            ...
+          }
+          PG_CATCH();
+          {
+            ....
+            /* Do not attempt to swallow the exception, always rethrow */
+            PG_RE_THROW();
+          }
+          PG_END_TRY();
+</programlisting>
+        See <filename>src/include/utils/elog.h</filename> and
+        <filename>src/backend/utils/error/elog.c</filename> for details.
+       </para>
+      </listitem>
+
+      <listitem id="xfunc-threading">
+       <para>
+        Individual <productname>PostgreSQL</productname> backends are single-threaded.
+        Never call any <productname>PostgreSQL</productname> function or access any
+        <productname>PostgreSQL</productname>-managed data structure from a thread
+        other than the main thread.  If at all possible, your extension should not
+        start any threads.  <productname>PostgreSQL</productname> generally uses
+        subprocesses (see <xref linkend="bgworker"/>).
+       </para>
+      </listitem>
+
+      <listitem id="xfunc-file-and-pipe-io">
+       <para>
+        To execute subprocesses and open files, use the routines provided by
+        the file descriptor manager like <function>BasicOpenFile</function>
+        and <function>OpenPipeStream</function> instead of a direct
+        <function>open()</function>, <function>fopen()</function>,
+        <function>popen()</function> or <function>system()</function>.
+        See <filename>src/include/storage/fd.h</filename>.
+       </para>
+      </listitem>
+
+      <listitem id="xfunc-sleeping-interrupts-blocking"
+                xreflabel="Sleeping, Blocking and Interrupt Handling in Extensions">
+       <para>
+        Extension code should always be structured as a non-blocking
+        loop over any ongoing external activities. Avoid making blocking calls
+        into 3rd party libraries or uninterruptible
+        system calls. Use <productname>PostgreSQL</productname>'s provided wait
+        primitives like <function>WaitEventSetWait</function> where necessary. Any
+        potentially long-running loop should periodically call <function>
+        CHECK_FOR_INTERRUPTS()</function> to give <productname>PostgreSQL</productname>
+        a chance to interrupt the function in the case of events like a shutdown
+        request or query cancellation.
+        This means that you should nor use <function>sleep()</function> or
+        <function>usleep()</function> for any nontrivial amount of time &mdash;
+        use <function>WaitLatch()</function> or its variants instead.
+        For details on interrupt handling, see comments near the definition of the
+        <literal>CHECK_FOR_INTERRUPTS()</literal> macro in
+        <filename>src/include/util/miscadmin.h</filename>.
+       </para>
+      </listitem>
+
+      <listitem id="xfunc-resource-management"
+                xreflabel="Resource Management in Extensions">
+       <para>
+        Use <productname>PostgreSQL</productname>'s resource tracking and
+        registration mechanisms and the associated callbacks instead of relying
+        on flow control based cleanup. Your extension function could be terminated
+        in mid-execution by <productname>PostgreSQL</productname> if any function
+        that it calls performs a <function>CHECK_FOR_INTERRUPTS()</function> check.
+        If this happens, it may not get the opportunity to take cleanup actions
+        via a <literal>PG_TRY()</literal> ... <literal>PG_CATCH()</literal>
+        block.  So all cleanup of resources not already managed by the
+        <productname>PostgreSQL</productname> runtime must be handled using
+        appropriate callbacks provided by <productname>PostgreSQL</productname>.
+        These include, but are not limited to, these callback registration APIs:
+        <itemizedlist>
+         <listitem><para><function>before_shmem_exit()</function></para></listitem>
+         <listitem><para><function>on_shmem_exit()</function></para></listitem>
+         <listitem><para><function>on_proc_exit()</function></para></listitem>
+         <listitem><para><literal>PG_ENSURE_ERROR_CLEANUP()</literal></para></listitem>
+         <listitem><para><function>RegisterXactCallback()</function></para></listitem>
+         <listitem><para><function>RegisterResourceReleaseCallback()</function></para></listitem>
+        </itemizedlist>
+        More information on resource management, cleanup and lifecycle can be
+        found in the <productname>PostgreSQL</productname> headers and sources.
+       </para>
+      </listitem>
+
+      <listitem id="xfunc-concurrency-synchronization"
+                xreflabel="Concurrency and Synchronization in Extensions">
+       <para>
+        Use the <productname>PostgreSQL</productname> runtime's concurrency and
+        synchronisation primitives rather than platform-native primitives when
+        coordinating activity between <productname>PostgreSQL</productname> processes.
+        These include signals and ProcSignal multiplexed signals,
+        heavyweight locks ("normal" database object locks),
+        <link linkend="xfunc-shared-addin">LWLock</link>s, Spinlocks, latches,
+        condition variables, and more.
+       </para>
+      </listitem>
+
+      <listitem id="xfunc-ipc"
+                xreflabel="Inter-Process Communication in Extensions">
+       <para>
+        Sometimes, relation-based state management is not sufficient to meet the
+        needs of an extension.  In that case, the extension may need to use
+        <productname>PostgreSQL</productname>'s shared-memory based inter-process
+        communication features, and should certainly do so instead of inventing
+        its own or trying to use platform level features. An extension may use
+        <link linkend="xfunc-shared-addin">"raw" shared memory requested from
+        the postmaster at startup</link> or higher level features like dynamic
+        shared memory segments (<acronym>DSM</acronym>),
+        dynamic shared areas (<acronym>DSA</acronym>),
+        or shared memory message queues (<acronym>shm_mq</acronym>).
+        Examples of the use of some these features can be found in the
+        <filename>src/test/modules/test_shm_mq/</filename> example extension
+        or other parts of the source code.
+       </para>
+      </listitem>
+
+      <listitem id="xfunc-relcache-syscache">
+       <para>
+        Look up system catalogs and table metadata using the relcache and syscache
+        APIs (<function>SearchSysCache...</function>,
+        <function>relation_open()</function>, etc) rather than attempting to run
+        SQL queries to fetch the information.  Ensure that your function holds
+        the necessary locks on the target objects.  Take care not to make any calls
+        that could trigger cache invalidations while still accessing any
+        syscache cache entries, as this can cause bugs that are hard to find.
+        See <filename>src/backend/utils/cache/syscache.c</filename>,
+        <filename>src/backend/utils/cache/relcache.c</filename>,
+        <filename>src/backend/access/common/relation.c</filename> and their
+        headers for details.
+       </para>
+      </listitem>
+
+      <listitem>
+       <para>
+        There are many README files in the <productname>PostgreSQL</productname>
+        source tree that discuss specific development topics in more detail than
+        be covered here.  Many subsystems' source files also have detailed comments
+        that explain their correct use.
+        The existing sources are your best reference for more complex tasks.
+       </para>
+      </listitem>
+
      </itemizedlist>
     </para>
    </sect2>
-- 
2.26.3

Laurenz,

Thanks for your comments. Sorry it's taken me so long to get back to you.
Commenting inline below on anything I think needs comment; other proposed
changes look good.

On Sun, 30 May 2021 at 19:20, Laurenz Albe <laurenz.albe@cybertec.at> wrote:

+   always use <link
linkend="xfunc-sleeping-interrupts-blocking">PostgreSQL's
+   interupt-aware APIs</link> for the purpose. Do not
<function>usleep()</function>,
+   <function>system()</function>, make blocking system calls, etc.
+  </para>

"system" is not a verb.

When it's a function call it is, but I'm fine with your revision:

Do not use <function>usleep()</function>, <function>system()</function>

or other blocking system calls.

+ and should only use the main thread. PostgreSQL generally uses
subprocesses

Hm. If the extension does not start threads, it automatically uses the
main thread.
I think that should be removed for clarity.

IIRC I intended that to apply to the section that tries to say how to
survive running your own threads in the backend if you really must do so.

+ primitives like <function>WaitEventSetWait</function> where

necessary. Any
+        potentially long-running loop should periodically call <function>
+        CHECK_FOR_INTERRUPTS()</function> to give PostgreSQL a chance to
interrupt
+        the function in case of a shutdown request, query cancel, etc.
This means

Are there other causes than shutdown or query cancellation?
At any rate, I am not fond of enumerations ending with "etc".

I guess. I wanted to emphasise that if you mess this up postgres might fail
to shut down or your backend might fail to respond to SIGTERM /
pg_terminate_backend, as those are the most commonly reported symptoms when
such issues are encountered.

+ by PostgreSQL if any function that it calls makes a

+ <function>CHECK_FOR_INTERRUPTS()</function> check. It may not

"makes" sound kind of clumsy in my ears.

Yeah. I didn't come up with anything better right away but will look when I
get the chance to return to this patch.

Attached is a new version that has my suggested changes, plus a few from
Bharath Rupireddy (I do not agree with many of his suggestions).

Thanks very much. I will try to return to this soon and review the diff
then rebase and update the patch.

I have a large backlog to get through, and I've recently had the pleasure
of having to work on windows/powershell build system stuff, so it may still
take me a while.

On Tue, 29 Jun 2021 at 13:30, Craig Ringer <craig.ringer@enterprisedb.com>
wrote:

Laurenz,

Thanks for your comments. Sorry it's taken me so long to get back to you.
Commenting inline below on anything I think needs comment; other proposed
changes look good.

I'm not going to get back to this anytime soon.

If anybody wants to pick it up that's great. Otherwise at least it's there
in the mailing lists to search.

On 30 Aug 2021, at 04:20, Craig Ringer <craig.ringer@enterprisedb.com> wrote:

On Tue, 29 Jun 2021 at 13:30, Craig Ringer <craig.ringer@enterprisedb.com <mailto:craig.ringer@enterprisedb.com>> wrote:
Laurenz,

Thanks for your comments. Sorry it's taken me so long to get back to you. Commenting inline below on anything I think needs comment; other proposed changes look good.

I'm not going to get back to this anytime soon.

If anybody wants to pick it up that's great. Otherwise at least it's there in the mailing lists to search.

I'm marking this returned with feedback for now, please open a new entry when
there is an updated patch.

--
Daniel Gustafsson https://vmware.com/