Re: [PATCH] More docs on what to do and not do in extension code - Mailing list pgsql-hackers
| From | Craig Ringer |
|---|---|
| Subject | Re: [PATCH] More docs on what to do and not do in extension code |
| Date | |
| Msg-id | CAGRY4nyjHh-Tm89A8eS1x+JtZ-qHU7wY+R0DEEtWfv5TQ3HcGA@mail.gmail.com Whole thread Raw |
| In response to | Re: [PATCH] More docs on what to do and not do in extension code (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
| Responses |
Re: [PATCH] More docs on what to do and not do in extension code
(David Steele <david@pgmasters.net>)
|
| List | pgsql-hackers |
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] 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.
pgsql-hackers by date: