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

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(dot)rupireddyforpostgres(at)gmail(dot)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] 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.