Re: Faster methods for getting SPI results (460% improvement) - Mailing list pgsql-hackers

On 5 April 2017 at 08:23, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 5 April 2017 at 08:00, Craig Ringer <craig@2ndquadrant.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