Обсуждение: [PATCH] More docs on what to do and not do in extension code
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.
Вложения
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
learnhow 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,
whenI 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
slightlymore 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
tolook 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
[10] I think it is
+ 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.
instead of
+ found in the PostgreSQL headers and sources.
[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
[14] Is it "relation/table based state management"?
+ Sometimes relation-based state management for extensions is not
[15] I think it is
+ 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
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] 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] 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] https://www.postgresql.org/message-id/CAGRY4nyjHh-Tm89A8eS1x%2BJtZ-qHU7wY%2BR0DEEtWfv5TQ3HcGA%40mail.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:
https://www.postgresql.org/message-id/20210309172049.GD26575%40momjian.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:
https://www.postgresql.org/message-id/20210309172049.GD26575%40momjian.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> "—" 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
Вложения
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 needscomment; 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/