Re: Faster methods for getting SPI results (460% improvement)

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