From: | Craig Ringer <craig(at)2ndquadrant(dot)com> |
---|---|
To: | Jim Nasby <jim(dot)nasby(at)openscg(dot)com> |
Cc: | Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Joe Conway <mail(at)joeconway(dot)com> |
Subject: | Re: Faster methods for getting SPI results (460% improvement) |
Date: | 2017-04-05 02:44:14 |
Message-ID: | CAMsr+YEtPVjWAbC2aPNxXBcofVmj1bTBs-Fdi=sUqQ4=HK1+Fw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 5 April 2017 at 08:23, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 5 April 2017 at 08:00, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>
>> Taking a look at this now.
>
> Rebased to current master with conflicts and whitespace errors fixed.
> Review pending.
This patch fails to update the documentation at all.
https://www.postgresql.org/docs/devel/static/spi.html
The patch crashes in initdb with --enable-cassert builds:
performing post-bootstrap initialization ... TRAP:
FailedAssertion("!(myState->pub.mydest == DestSQLFunction)", File:
"functions.c", Line: 800)
sh: line 1: 30777 Aborted (core dumped)
"/home/craig/projects/2Q/postgres/tmp_install/home/craig/pg/10/bin/postgres"
--single -F -O -j -c search_path=pg_catalog -c exit_on_error=true
template1 > /dev/null
child process exited with exit code 134
Backtrace attached.
Details on patch 1:
missing newline
+}
+int
+SPI_execute_callback(const char *src, bool read_only, long tcount,
+/* Execute a previously prepared plan with a callback Destination */
caps "Destination"
+ // XXX throw error if callback is set
^^
+static DestReceiver spi_callbackDR = {
+ donothingReceive, donothingStartup, donothingCleanup, donothingCleanup,
+ DestSPICallback
+};
Is the callback destreceiver supposed to be a blackhole? Why? Its
name, spi_callbackDR and DestSPICallback, doesn't seem to indicate
that it drops its input.
Presumably that's a default destination you're then supposed to modify
with your own callbacks? There aren't any comments to indicate what's
going on here.
Comments on patch 2:
If this is the "bare minimum" patch, what is pending? Does it impose
any downsides or limits?
+/* Get switch execution contexts */
+extern PLyExecutionContext
*PLy_switch_execution_context(PLyExecutionContext *new);
Comment makes no sense to me. This seems something like memory context
switch, where you supply the new and return the old. But the comment
doesn't explain it at all.
+void PLy_CSStartup(DestReceiver *self, int operation, TupleDesc typeinfo);
+void PLy_CSDestroy(DestReceiver *self);
These are declared in the plpy_spi.c. Why aren't these static?
+ volatile MemoryContext oldcontext;
+ volatile ResourceOwner oldowner;
int rv;
- volatile MemoryContext oldcontext;
- volatile ResourceOwner oldowner;
Unnecessary code movement.
In PLy_Callback_New, I think your use of a sub-context is sensible. Is
it necessary to palloc0 though?
+static CallbackState *
+PLy_Callback_Free(CallbackState *callback)
The code here looks like it could be part of spi.c's callback support,
rather than plpy_spi specific, since you provide a destroy callback in
the SPI callback struct.
+ /* We need to store this because the TupleDesc the receive
function gets has no names. */
+ myState->desc = typeinfo;
Is it safe to just store the pointer to the TupleDesc here? What's the lifetime?
+ * will clean it up when the time is right. XXX This might result in a leak
+ * if an error happens and the result doesn't get dereferenced.
+ */
+ MemoryContextSwitchTo(TopMemoryContext);
+ result->tupdesc = CreateTupleDescCopy(typeinfo);
especially given this XXX comment...
Patch needs bug fix, docs updates, fixes for issues marked in
comments. But overall approach looks sensible enough.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachment | Content-Type | Size |
---|---|---|
bt.txt | text/plain | 11.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2017-04-05 02:47:42 | Re: increasing the default WAL segment size |
Previous Message | Tsunakawa, Takayuki | 2017-04-05 02:37:28 | Re: Supporting huge pages on Windows |