Odd behavior with PG_TRY
In the attached patch (snippet below), I'm seeing something strange with
args->in.r.atts[]. Prior to entering the PG_TRY block, I can inspect
things in lldb just fine:
(lldb) call args->in.r.atts[0].func
(PLyDatumToObFunc) $49 = 0x000000010fc4dc70
(plpython3.so`PLyString_FromDatum at plpy_typeio.c:621)
(lldb) call &args->in.r.atts[0]
(PLyDatumToOb *) $50 = 0x00007fd2b302f6d0
(lldb) call args->in.r.atts[0]
(PLyDatumToOb) $51 = {
func = 0x000000010fc4dc70 (plpython3.so`PLyString_FromDatum at
plpy_typeio.c:621)
typfunc = {
fn_addr = 0x000000010f478b50 (postgres`textout at varlena.c:521)
fn_oid = 47
...
But I'm getting a EXC_BAD_ACCESS (code=1, address=0xb302f6d0) on the
last if in the snippet below. Looking at the variables again, I see:
(lldb) call args->in.r.atts[i].func
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory
(lldb) call args->in.r.atts[i]
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory
I saw the comment on PG_TRY about marking things as volatile, but my
understanding from the comment is I shouldn't even need to do that,
since these variables aren't being modified.
static bool
PLy_CSreceive(TupleTableSlot *slot, DestReceiver *self)
{
volatile TupleDesc desc = slot->tts_tupleDescriptor;
volatile CallbackState *myState = (CallbackState *) self;
volatile PLyTypeInfo *args = myState->args;PLyExecutionContext *old_exec_ctx = PLy_switch_execution_context(myState->exec_ctx);
MemoryContext scratch_context = PLy_get_scratch_context(myState->exec_ctx);
MemoryContext oldcontext = CurrentMemoryContext;/* Verify saved state matches incoming slot */
Assert(myState->desc == desc);
Assert(args->in.r.natts == desc->natts);/* Make sure the tuple is fully deconstructed */
slot_getallattrs(slot);MemoryContextSwitchTo(scratch_context);
PG_TRY();
{
int i, rv;/*
* Do the work in the scratch context to avoid leaking memory from the
* datatype output function calls.
*/
for (i = 0; i < desc->natts; i++)
{
PyObject *value = NULL;if (desc->attrs[i]->attisdropped)
continue;if (myState->lists[i] == NULL)
ereport(ERROR,
(errmsg("missing list for attribute %d", i)));
/* XXX If the function can't be null, ditch that check */
if (slot->tts_isnull[i] || args->in.r.atts[i].func == NULL)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
Attachments:
plpython_callbeck.difftext/plain; charset=UTF-8; name=plpython_callbeck.diff; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 80fc4c4725..ace30d75f0 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -55,7 +55,8 @@ static void _SPI_prepare_oneshot_plan(const char *src, SPIPlanPtr plan);
static int _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
Snapshot snapshot, Snapshot crosscheck_snapshot,
- bool read_only, bool fire_triggers, uint64 tcount);
+ bool read_only, bool fire_triggers, uint64 tcount,
+ DestReceiver *callback);
static ParamListInfo _SPI_convert_params(int nargs, Oid *argtypes,
Datum *Values, const char *Nulls);
@@ -320,7 +321,34 @@ SPI_execute(const char *src, bool read_only, long tcount)
res = _SPI_execute_plan(&plan, NULL,
InvalidSnapshot, InvalidSnapshot,
- read_only, true, tcount);
+ read_only, true, tcount, NULL);
+
+ _SPI_end_call(true);
+ return res;
+}
+int
+SPI_execute_callback(const char *src, bool read_only, long tcount,
+ DestReceiver *callback)
+{
+ _SPI_plan plan;
+ int res;
+
+ if (src == NULL || tcount < 0)
+ return SPI_ERROR_ARGUMENT;
+
+ res = _SPI_begin_call(true);
+ if (res < 0)
+ return res;
+
+ memset(&plan, 0, sizeof(_SPI_plan));
+ plan.magic = _SPI_PLAN_MAGIC;
+ plan.cursor_options = 0;
+
+ _SPI_prepare_oneshot_plan(src, &plan);
+
+ res = _SPI_execute_plan(&plan, NULL,
+ InvalidSnapshot, InvalidSnapshot,
+ read_only, true, tcount, callback);
_SPI_end_call(true);
return res;
@@ -354,7 +382,34 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
_SPI_convert_params(plan->nargs, plan->argtypes,
Values, Nulls),
InvalidSnapshot, InvalidSnapshot,
- read_only, true, tcount);
+ read_only, true, tcount, NULL);
+
+ _SPI_end_call(true);
+ return res;
+}
+
+/* Execute a previously prepared plan with a callback Destination */
+int
+SPI_execute_plan_callback(SPIPlanPtr plan, Datum *Values, const char *Nulls,
+ bool read_only, long tcount, DestReceiver *callback)
+{
+ int res;
+
+ if (plan == NULL || plan->magic != _SPI_PLAN_MAGIC || tcount < 0)
+ return SPI_ERROR_ARGUMENT;
+
+ if (plan->nargs > 0 && Values == NULL)
+ return SPI_ERROR_PARAM;
+
+ res = _SPI_begin_call(true);
+ if (res < 0)
+ return res;
+
+ res = _SPI_execute_plan(plan,
+ _SPI_convert_params(plan->nargs, plan->argtypes,
+ Values, Nulls),
+ InvalidSnapshot, InvalidSnapshot,
+ read_only, true, tcount, callback);
_SPI_end_call(true);
return res;
@@ -383,7 +438,7 @@ SPI_execute_plan_with_paramlist(SPIPlanPtr plan, ParamListInfo params,
res = _SPI_execute_plan(plan, params,
InvalidSnapshot, InvalidSnapshot,
- read_only, true, tcount);
+ read_only, true, tcount, NULL);
_SPI_end_call(true);
return res;
@@ -424,7 +479,7 @@ SPI_execute_snapshot(SPIPlanPtr plan,
_SPI_convert_params(plan->nargs, plan->argtypes,
Values, Nulls),
snapshot, crosscheck_snapshot,
- read_only, fire_triggers, tcount);
+ read_only, fire_triggers, tcount, NULL);
_SPI_end_call(true);
return res;
@@ -471,7 +526,7 @@ SPI_execute_with_args(const char *src,
res = _SPI_execute_plan(&plan, paramLI,
InvalidSnapshot, InvalidSnapshot,
- read_only, true, tcount);
+ read_only, true, tcount, NULL);
_SPI_end_call(true);
return res;
@@ -1892,7 +1947,8 @@ _SPI_prepare_oneshot_plan(const char *src, SPIPlanPtr plan)
static int
_SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
Snapshot snapshot, Snapshot crosscheck_snapshot,
- bool read_only, bool fire_triggers, uint64 tcount)
+ bool read_only, bool fire_triggers, uint64 tcount,
+ DestReceiver *callback)
{
int my_res = 0;
uint64 my_processed = 0;
@@ -1903,6 +1959,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
ErrorContextCallback spierrcontext;
CachedPlan *cplan = NULL;
ListCell *lc1;
+ DestReceiver *dest = callback;
/*
* Setup error traceback support for ereport()
@@ -2020,7 +2077,6 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
{
Node *stmt = (Node *) lfirst(lc2);
bool canSetTag;
- DestReceiver *dest;
_SPI_current->processed = 0;
_SPI_current->lastoid = InvalidOid;
@@ -2072,7 +2128,8 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
UpdateActiveSnapshotCommandId();
}
- dest = CreateDestReceiver(canSetTag ? DestSPI : DestNone);
+ if (!callback)
+ dest = CreateDestReceiver(canSetTag ? DestSPI : DestNone);
if (IsA(stmt, PlannedStmt) &&
((PlannedStmt *) stmt)->utilityStmt == NULL)
@@ -2098,6 +2155,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
{
char completionTag[COMPLETION_TAG_BUFSIZE];
+ // XXX throw error if callback is set
ProcessUtility(stmt,
plansource->query_string,
PROCESS_UTILITY_QUERY,
diff --git a/src/backend/tcop/dest.c b/src/backend/tcop/dest.c
index 60a92801f1..20b6dbf4ff 100644
--- a/src/backend/tcop/dest.c
+++ b/src/backend/tcop/dest.c
@@ -81,6 +81,11 @@ static DestReceiver spi_printtupDR = {
DestSPI
};
+static DestReceiver spi_callbackDR = {
+ donothingReceive, donothingStartup, donothingCleanup, donothingCleanup,
+ DestSPICallback
+};
+
/* Globally available receiver for DestNone */
DestReceiver *None_Receiver = &donothingDR;
@@ -117,6 +122,9 @@ CreateDestReceiver(CommandDest dest)
case DestSPI:
return &spi_printtupDR;
+ case DestSPICallback:
+ return &spi_callbackDR;
+
case DestTuplestore:
return CreateTuplestoreDestReceiver();
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index 76ba394a2b..15187c47e3 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -74,11 +74,15 @@ extern PGDLLIMPORT int SPI_result;
extern int SPI_connect(void);
extern int SPI_finish(void);
extern int SPI_execute(const char *src, bool read_only, long tcount);
+extern int SPI_execute_callback(const char *src, bool read_only, long tcount,
+ DestReceiver *callback);
extern int SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
bool read_only, long tcount);
extern int SPI_execute_plan_with_paramlist(SPIPlanPtr plan,
ParamListInfo params,
bool read_only, long tcount);
+extern int SPI_execute_plan_callback(SPIPlanPtr plan, Datum *Values, const char *Nulls,
+ bool read_only, long tcount, DestReceiver *callback);
extern int SPI_exec(const char *src, long tcount);
extern int SPI_execp(SPIPlanPtr plan, Datum *Values, const char *Nulls,
long tcount);
diff --git a/src/include/tcop/dest.h b/src/include/tcop/dest.h
index dd80433f74..5389f496a2 100644
--- a/src/include/tcop/dest.h
+++ b/src/include/tcop/dest.h
@@ -90,6 +90,7 @@ typedef enum
DestRemote, /* results sent to frontend process */
DestRemoteExecute, /* sent to frontend, in Execute command */
DestSPI, /* results sent to SPI manager */
+ DestSPICallback, /* results sent to user-specified callback function */
DestTuplestore, /* results sent to Tuplestore */
DestIntoRel, /* results sent to relation (SELECT INTO) */
DestCopyOut, /* results sent to COPY TO code */
diff --git a/src/pl/plpython/plpy_main.c b/src/pl/plpython/plpy_main.c
index 860b804e54..07501f18f2 100644
--- a/src/pl/plpython/plpy_main.c
+++ b/src/pl/plpython/plpy_main.c
@@ -403,6 +403,19 @@ PLy_current_execution_context(void)
return PLy_execution_contexts;
}
+PLyExecutionContext *
+PLy_switch_execution_context(PLyExecutionContext *new)
+{
+ PLyExecutionContext *last = PLy_execution_contexts;
+
+ if (PLy_execution_contexts == NULL)
+ elog(ERROR, "no Python function is currently executing");
+
+ PLy_execution_contexts = new;
+
+ return last;
+}
+
MemoryContext
PLy_get_scratch_context(PLyExecutionContext *context)
{
diff --git a/src/pl/plpython/plpy_main.h b/src/pl/plpython/plpy_main.h
index 10426c4323..7cbe0a8d35 100644
--- a/src/pl/plpython/plpy_main.h
+++ b/src/pl/plpython/plpy_main.h
@@ -25,6 +25,9 @@ typedef struct PLyExecutionContext
/* Get the current execution context */
extern PLyExecutionContext *PLy_current_execution_context(void);
+/* Get switch execution contexts */
+extern PLyExecutionContext *PLy_switch_execution_context(PLyExecutionContext *new);
+
/* Get the scratch memory context for specified execution context */
extern MemoryContext PLy_get_scratch_context(PLyExecutionContext *context);
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 07ab6a087e..a2096b6506 100644
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -28,8 +28,27 @@
#include "plpy_procedure.h"
#include "plpy_resultobject.h"
+typedef struct
+{
+ DestReceiver pub;
+ PLyExecutionContext *exec_ctx;
+ MemoryContext mctx;
+ TupleDesc desc;
+ PLyTypeInfo *args;
+
+ /* Dictionary of Lists */
+ PyObject *dict;
+ PyObject **lists;
+} CallbackState;
+
+
+
+void PLy_CSStartup(DestReceiver *self, int operation, TupleDesc typeinfo);
+void PLy_CSDestroy(DestReceiver *self);
+static bool PLy_CSreceive(TupleTableSlot *slot, DestReceiver *self);
static PyObject *PLy_spi_execute_query(char *query, long limit);
+static PyObject *PLy_spi_execute_query2(char *query, long limit);
static PyObject *PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit);
static PyObject *PLy_spi_execute_fetch_result(SPITupleTable *tuptable,
uint64 rows, int status);
@@ -341,8 +360,156 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
return ret;
}
+void
+PLy_CSStartup(DestReceiver *self, int operation, TupleDesc typeinfo)
+{
+ PLyExecutionContext *old_exec_ctx;
+ CallbackState *myState = (CallbackState *) self;
+ PLyTypeInfo args;
+
+ MemoryContext mctx, old_mctx;
+ PyObject *dict;
+ PyObject **lists;
+ int i;
+
+ /*
+ * We may be in a different execution context when we're called, so switch
+ * back to our original one.
+ */
+ mctx = myState->mctx;
+ old_exec_ctx = PLy_switch_execution_context(myState->exec_ctx);
+ old_mctx = MemoryContextSwitchTo(mctx);
+
+ /* Store this as a sanity check */
+ myState->desc = typeinfo;
+
+ /* Setup type conversion info */
+ PLy_typeinfo_init(&args, mctx);
+ myState->args = &args;
+ PLy_input_tuple_funcs(&args, typeinfo);
+
+ /*
+ * We never palloc python objects, but this is an array of object pointers,
+ * so it's OK.
+ */
+ myState->lists = lists = palloc0(args.in.r.natts * sizeof(PyObject *));
+
+ myState->dict = dict = PyDict_New();
+ if (dict == NULL)
+ PLy_elog(ERROR, "could not create new dictionary");
+
+
+ for (i = 0; i < args.in.r.natts; i++)
+ {
+ char *key;
+ PyObject *value;
+
+ if (typeinfo->attrs[i]->attisdropped)
+ continue;
+
+ /* NOTE: If size is > 0 then the list must get initialized! */
+ value = PyList_New(0);
+ if (value == NULL)
+ PLy_elog(ERROR, "could not create new list");
+
+ key = NameStr(typeinfo->attrs[i]->attname);
+ PyDict_SetItemString(dict, key, value);
+ Py_DECREF(value);
+
+ /* We want fast access to the lists, so we store them in our array of pointers */
+ lists[i] = value;
+ }
+
+ MemoryContextSwitchTo(old_mctx);
+ PLy_switch_execution_context(old_exec_ctx);
+}
+
+void
+PLy_CSDestroy(DestReceiver *self)
+{
+ CallbackState *myState = (CallbackState *) self;
+
+ MemoryContextDelete(myState->mctx);
+}
+
+static bool
+PLy_CSreceive(TupleTableSlot *slot, DestReceiver *self)
+{
+ volatile TupleDesc desc = slot->tts_tupleDescriptor;
+ volatile CallbackState *myState = (CallbackState *) self;
+ volatile PLyTypeInfo *args = myState->args;
+
+ PLyExecutionContext *old_exec_ctx = PLy_switch_execution_context(myState->exec_ctx);
+ MemoryContext scratch_context = PLy_get_scratch_context(myState->exec_ctx);
+ MemoryContext oldcontext = CurrentMemoryContext;
+
+ /* Verify saved state matches incoming slot */
+ Assert(myState->desc == desc);
+ Assert(args->in.r.natts == desc->natts);
+
+ /* Make sure the tuple is fully deconstructed */
+ slot_getallattrs(slot);
+
+ MemoryContextSwitchTo(scratch_context);
+
+ PG_TRY();
+ {
+ int i, rv;
+
+ /*
+ * Do the work in the scratch context to avoid leaking memory from the
+ * datatype output function calls.
+ */
+ for (i = 0; i < desc->natts; i++)
+ {
+ PyObject *value = NULL;
+
+ if (desc->attrs[i]->attisdropped)
+ continue;
+
+ if (myState->lists[i] == NULL)
+ ereport(ERROR,
+ (errmsg("missing list for attribute %d", i)));
+ /* XXX If the function can't be null, ditch that check */
+ if (slot->tts_isnull[i] || args->in.r.atts[i].func == NULL)
+ {
+ Py_INCREF(Py_None);
+ value = Py_None;
+ }
+ else
+ value = (args->in.r.atts[i].func) (&args->in.r.atts[i], slot->tts_values[i]);
+
+ /*
+ * If we tried to do this in the PG_CATCH we'd have to mark value
+ * as volatile, but that won't work with PyList_Append, so just
+ * test the error code after doing Py_DECREF().
+ */
+ rv = PyList_Append(myState->lists[i], value);
+ Py_DECREF(value);
+
+ if (rv != 0)
+ ereport(ERROR,
+ (errmsg("unable to append value to list")));
+ }
+ MemoryContextSwitchTo(oldcontext);
+ /* Should we just do this once, for the whole tuple?? */
+ MemoryContextReset(scratch_context);
+ }
+ PG_CATCH();
+ {
+ MemoryContextSwitchTo(oldcontext);
+ PLy_switch_execution_context(old_exec_ctx);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+ PLy_switch_execution_context(old_exec_ctx);
+
+ /* If we get here then we were successful */
+ return true;
+}
+
static PyObject *
-PLy_spi_execute_query(char *query, long limit)
+PLy_spi_execute_query2(char *query, long limit)
{
int rv;
volatile MemoryContext oldcontext;
@@ -384,6 +551,67 @@ PLy_spi_execute_query(char *query, long limit)
}
static PyObject *
+PLy_spi_execute_query(char *query, long limit)
+{
+ PLyExecutionContext *exec_ctx = PLy_current_execution_context();
+ int rv;
+ volatile MemoryContext oldcontext, cb_ctx;
+ volatile ResourceOwner oldowner;
+ PyObject *ret = NULL;
+ CallbackState *callback;
+
+ oldowner = CurrentResourceOwner;
+
+ /* Use a new context to make cleanup easier */
+ cb_ctx = AllocSetContextCreate(CurrentMemoryContext,
+ "PL/Python callback context",
+ ALLOCSET_DEFAULT_SIZES);
+
+ oldcontext = MemoryContextSwitchTo(cb_ctx);
+ callback = palloc0(sizeof(CallbackState));
+ callback->mctx = cb_ctx;
+ memcpy(&(callback->pub), CreateDestReceiver(DestSPICallback), sizeof(DestReceiver));
+ callback->pub.receiveSlot = PLy_CSreceive;
+ callback->pub.rStartup = PLy_CSStartup;
+ callback->pub.rDestroy = PLy_CSDestroy;
+ callback->exec_ctx = exec_ctx;
+
+ PLy_spi_subtransaction_begin(oldcontext, oldowner);
+
+ PG_TRY();
+ {
+
+ pg_verifymbstr(query, strlen(query), false);
+ rv = SPI_execute_callback(query, exec_ctx->curr_proc->fn_readonly, limit,
+ (DestReceiver *) callback);
+ /*
+ * callback->dict gets set in PLy_CSStartup, which happens during
+ * executor startup. It's not valid before then.
+ */
+ ret = callback->dict;
+
+ PLy_spi_subtransaction_commit(oldcontext, oldowner);
+ }
+ PG_CATCH();
+ {
+ PLy_spi_subtransaction_abort(oldcontext, oldowner);
+ return NULL;
+ }
+ PG_END_TRY();
+
+ if (rv < 0)
+ {
+ Py_XDECREF(ret);
+ PLy_exception_set(PLy_exc_spi_error,
+ "SPI_execute failed: %s",
+ SPI_result_code_string(rv));
+ return NULL;
+ }
+
+ return ret;
+}
+
+static PyObject *
PLy_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 rows, int status)
{
PLyResultObject *result;
On Mon, Jan 2, 2017 at 11:14 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
In the attached patch (snippet below), I'm seeing something strange with
args->in.r.atts[]. Prior to entering the PG_TRY block, I can inspect things
in lldb just fine:(lldb) call args->in.r.atts[0].func
(PLyDatumToObFunc) $49 = 0x000000010fc4dc70
(plpython3.so`PLyString_FromDatum at plpy_typeio.c:621)
(lldb) call &args->in.r.atts[0]
(PLyDatumToOb *) $50 = 0x00007fd2b302f6d0
(lldb) call args->in.r.atts[0]
(PLyDatumToOb) $51 = {
func = 0x000000010fc4dc70 (plpython3.so`PLyString_FromDatum at
plpy_typeio.c:621)
typfunc = {
fn_addr = 0x000000010f478b50 (postgres`textout at varlena.c:521)
fn_oid = 47
...But I'm getting a EXC_BAD_ACCESS (code=1, address=0xb302f6d0) on the last if
in the snippet below. Looking at the variables again, I see:(lldb) call args->in.r.atts[i].func
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory
(lldb) call args->in.r.atts[i]
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory
Looks strange, what is the value of 'i'? Did you get the same result
if you try to print args->in.r.atts[0] inside PG_TRY?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/2/17 1:31 AM, Amit Kapila wrote:
On Mon, Jan 2, 2017 at 11:14 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
In the attached patch (snippet below), I'm seeing something strange with
args->in.r.atts[]. Prior to entering the PG_TRY block, I can inspect things
in lldb just fine:(lldb) call args->in.r.atts[0].func
(PLyDatumToObFunc) $49 = 0x000000010fc4dc70
(plpython3.so`PLyString_FromDatum at plpy_typeio.c:621)
(lldb) call &args->in.r.atts[0]
(PLyDatumToOb *) $50 = 0x00007fd2b302f6d0
(lldb) call args->in.r.atts[0]
(PLyDatumToOb) $51 = {
func = 0x000000010fc4dc70 (plpython3.so`PLyString_FromDatum at
plpy_typeio.c:621)
typfunc = {
fn_addr = 0x000000010f478b50 (postgres`textout at varlena.c:521)
fn_oid = 47
...But I'm getting a EXC_BAD_ACCESS (code=1, address=0xb302f6d0) on the last if
in the snippet below. Looking at the variables again, I see:(lldb) call args->in.r.atts[i].func
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory
(lldb) call args->in.r.atts[i]
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memoryLooks strange, what is the value of 'i'? Did you get the same result
if you try to print args->in.r.atts[0] inside PG_TRY?
i is 0, makes no difference:
(lldb) call i
(int) $56 = 0
(lldb) call args->in.r.atts[0]
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory
(lldb)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jan 2, 2017 at 10:43 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 1/2/17 1:31 AM, Amit Kapila wrote:
On Mon, Jan 2, 2017 at 11:14 AM, Jim Nasby <Jim.Nasby@bluetreble.com>
wrote:Looks strange, what is the value of 'i'? Did you get the same result
if you try to print args->in.r.atts[0] inside PG_TRY?i is 0, makes no difference:
(lldb) call i
(int) $56 = 0
(lldb) call args->in.r.atts[0]
error: Couldn't apply expression side effects : Couldn't dematerialize a
result variable: couldn't read its memory
(lldb)
To localize the problem you might want to try by just having the
problematic statement in PG_TRY();
PG_TRY();
{
if (args->in.r.atts[0].func == NULL)
{
}
}
PG_CATCH();
{
PG_RE_THROW();
}
PG_END_TRY();
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
In the attached patch (snippet below), I'm seeing something strange with
args->in.r.atts[].
Did you try comparing the apparent values of "args" before and after
entering PG_TRY?
I saw the comment on PG_TRY about marking things as volatile, but my
understanding from the comment is I shouldn't even need to do that,
since these variables aren't being modified.
Not sure, but if you do need to mark them volatile to prevent
misoptimization in the PG_TRY, this is not how to do it:
volatile TupleDesc desc = slot->tts_tupleDescriptor;
volatile CallbackState *myState = (CallbackState *) self;
volatile PLyTypeInfo *args = myState->args;
Correct coding would be
volatile TupleDesc desc = slot->tts_tupleDescriptor;
CallbackState * volatile myState = (CallbackState *) self;
PLyTypeInfo * volatile args = myState->args;
because what needs to be marked volatile is the pointer variable,
not what it points at. I'm a bit surprised you're not getting
"cast away volatile" warnings from the code as you have it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/2/17 9:47 PM, Tom Lane wrote:
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
In the attached patch (snippet below), I'm seeing something strange with
args->in.r.atts[].Did you try comparing the apparent values of "args" before and after
entering PG_TRY?
Yeah, see below. FWIW, when I did that just now I stepped through at the
instruction level, and things went belly up when I stepped into the
first instruction of the for (first instruction after PG_TRY was done).
I saw the comment on PG_TRY about marking things as volatile, but my
understanding from the comment is I shouldn't even need to do that,
since these variables aren't being modified.Not sure, but if you do need to mark them volatile to prevent
misoptimization in the PG_TRY, this is not how to do it:volatile TupleDesc desc = slot->tts_tupleDescriptor;
volatile CallbackState *myState = (CallbackState *) self;
volatile PLyTypeInfo *args = myState->args;Correct coding would be
volatile TupleDesc desc = slot->tts_tupleDescriptor;
CallbackState * volatile myState = (CallbackState *) self;
PLyTypeInfo * volatile args = myState->args;because what needs to be marked volatile is the pointer variable,
not what it points at. I'm a bit surprised you're not getting
"cast away volatile" warnings from the code as you have it.
Unfortunately, that didn't make a difference. Amit's suggestion of
isolating the single statement in a PG_TRY() didn't work either, but
assigning args->in.r.atts[i] to a pointer did. The volatile didn't make
a difference in this case, which if the PG_TRY() comments are to be
believed is what I'd expect. Though, it was also OK with value not being
volatile, which AFAIK isn't safe, so...
for (i = 0; i < desc->natts; i++)
{
PyObject *value = NULL;
PLyDatumToOb * volatile atts = &args->in.r.atts[i];
...
if (slot->tts_isnull[i] || atts->func == NULL)
{
Py_INCREF(Py_None);
value = Py_None;
}
else
{
PG_TRY();
{
value = (atts->func) (atts, slot->tts_values[i]);
}
PG_CATCH();
{
Py_XDECREF(value);
MemoryContextSwitchTo(oldcontext);
PLy_switch_execution_context(old_exec_ctx);
PG_RE_THROW();
}
PG_END_TRY();
}
lldb output from inspecting variables:
(lldb) call args
(volatile PLyTypeInfo *) $60 = 0x00007fff5912fbe8
(lldb) call @args
error: expected expression
(lldb) call *args
(PLyTypeInfo) $61 = {
in = {
d = {
func = 0x00007fca218f8ed0
typfunc = {
fn_addr = 0x00007fff00000001
fn_oid = 114610686
fn_nargs = 1
fn_strict = '\0'
fn_retset = '\0'
fn_stats = '0'
fn_extra = 0x00007fca2206d060
fn_mcxt = 0x00007fca22800960
fn_expr = 0x00007fff5912fbe8
}
typtransform = {
fn_addr = 0x00007fca218f8e38
fn_oid = 570849696
fn_nargs = 32714
fn_strict = '\0'
fn_retset = '\0'
fn_stats = '8'
fn_extra = 0x00007fca22066f60
fn_mcxt = 0x00007fff5912fc90
fn_expr = 0x0000000106d7df6f
}
typoid = 570830824
typmod = 32714
typioparam = 570830864
typbyval = '\0'
typlen = 0
typalign = 'h'
elm = 0x0000000000000000
}
r = {
atts = 0x00007fca218f8ed0
natts = 1
}
}
out = {
d = {
func = 0x00007fca22066f60
typfunc = {
fn_addr = 0x00007fca00000000
fn_oid = 570846544
fn_nargs = 32714
fn_strict = '\0'
fn_retset = '\0'
fn_stats = '`'
fn_extra = 0x00007fff5912fce0
fn_mcxt = 0x0000000106d4d437
fn_expr = 0x00007fca2206ce30
}
typtransform = {
fn_addr = 0x00007fca22062f90
fn_oid = 0
fn_nargs = 0
fn_strict = '\0'
fn_retset = '\0'
fn_stats = '\0'
fn_extra = 0xffffffdc22066c20
fn_mcxt = 0x0000000000000000
fn_expr = 0x00007fca22066f60
}
typoid = 3756919206
typmod = -754934605
typioparam = 1494416704
typbyval = '\xff'
typlen = 0
typalign = '^'
elm = 0x00007fff5912fd10
}
r = {
atts = 0x00007fca22066f60
natts = 0
}
}
is_rowtype = 0
typ_relid = 0
typrel_xmin = 570847072
typrel_tid = {
ip_blkid = (bi_hi = 32714, bi_lo = 0)
ip_posid = 36408
}
mcxt = 0x000000012206c6f0
}
(lldb) gu
(lldb) call args
(volatile PLyTypeInfo *) $62 = 0x00007fff5912fbe8
(lldb) call *args
(PLyTypeInfo) $63 = {
in = {
d = {
func = 0x00000000218f8ed0
typfunc = {
fn_addr = 0x00007fff5912ff08
fn_oid = 1494417744
fn_nargs = 32767
fn_strict = '\0'
fn_retset = '\0'
fn_stats = '0'
fn_extra = 0x00007fca2206d060
fn_mcxt = 0x00007fca22800960
fn_expr = 0x00007fff5912fbe8
}
typtransform = {
fn_addr = 0x00007fca218f8e38
fn_oid = 570849696
fn_nargs = 32714
fn_strict = '\0'
fn_retset = '\0'
fn_stats = '8'
fn_extra = 0x00007fca22066f60
fn_mcxt = 0x0000000000000000
fn_expr = 0x00007fff5912fce0
}
typoid = 1494416304
typmod = 32767
typioparam = 114900880
typbyval = '\x01'
typlen = 0
typalign = '\xa6'
elm = 0x000000010735c8a0
}
r = {
atts = 0x00000000218f8ed0
natts = 1494417160
}
}
out = {
d = {
func = 0x00007fff5912ff08
typfunc = {
fn_addr = 0x000000010786ac18 (plpython3.so`PLy_CSreceive + 360
at plpy_spi.c:455)
fn_oid = 570846544
fn_nargs = 32714
fn_strict = '\0'
fn_retset = '\0'
fn_stats = '\x80'
fn_extra = 0x000000005912fce0
fn_mcxt = 0x0000000106d4d437
fn_expr = 0x00007fca2206ce30
}
typtransform = {
fn_addr = 0x00007fca22062f90
fn_oid = 0
fn_nargs = 0
fn_strict = '\0'
fn_retset = '\0'
fn_stats = '\0'
fn_extra = 0xffffffdc22066c20
fn_mcxt = 0x0000000000000000
fn_expr = 0x00007fca22066f60
}
typoid = 3756919206
typmod = -754934605
typioparam = 1494416704
typbyval = '\xff'
typlen = 0
typalign = '^'
elm = 0x00007fff5912fd10
}
r = {
atts = 0x00007fff5912ff08
natts = 126266392
}
}
is_rowtype = 0
typ_relid = 0
typrel_xmin = 570847072
typrel_tid = {
ip_blkid = (bi_hi = 32714, bi_lo = 0)
ip_posid = 36408
}
mcxt = 0x000000012206c6f0
}
(lldb)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jan 4, 2017 at 3:47 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 1/2/17 9:47 PM, Tom Lane wrote:
Correct coding would be
volatile TupleDesc desc = slot->tts_tupleDescriptor;
CallbackState * volatile myState = (CallbackState *) self;
PLyTypeInfo * volatile args = myState->args;because what needs to be marked volatile is the pointer variable,
not what it points at. I'm a bit surprised you're not getting
"cast away volatile" warnings from the code as you have it.Unfortunately, that didn't make a difference. Amit's suggestion of isolating
the single statement in a PG_TRY() didn't work either, but assigning
args->in.r.atts[i] to a pointer did.
Good to know that it worked, but what is the theory? From your
experiment, it appears that in some cases accessing local pointer
variables is okay and in other cases, it is not okay.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/3/17 9:20 PM, Amit Kapila wrote:
On Wed, Jan 4, 2017 at 3:47 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 1/2/17 9:47 PM, Tom Lane wrote:
Correct coding would be
volatile TupleDesc desc = slot->tts_tupleDescriptor;
CallbackState * volatile myState = (CallbackState *) self;
PLyTypeInfo * volatile args = myState->args;because what needs to be marked volatile is the pointer variable,
not what it points at. I'm a bit surprised you're not getting
"cast away volatile" warnings from the code as you have it.Unfortunately, that didn't make a difference. Amit's suggestion of isolating
the single statement in a PG_TRY() didn't work either, but assigning
args->in.r.atts[i] to a pointer did.Good to know that it worked, but what is the theory? From your
experiment, it appears that in some cases accessing local pointer
variables is okay and in other cases, it is not okay.
I can run some other experiments if you have any to suggest.
I do think it's interesting that the data appeared to be completely fine
until I actually ran whatever the first assembly instruction of the for
loop is, so presumably it was fine after the sigsetjmp() call (which I'm
assuming is what causes all the fuss to begin with...) From my
understanding of what volatile does, I can understand why it might be
necessary for something in the CATCH block to need it, but not in the TRY.
Two other things of note that might possibly make a difference here:
- This is happening inside a function used as a DestReceiver receiver
- The original call is a plpython function, calling a plpython function,
calling a plpython function (specifically, nested_call_one() in the
plpython regression test).
That does mean that the call stack looks something like this:
plpython
SPI_execute_callback
(my customer DestReceiverer Setup function (PLy_CSSetup) is called
somewhere in here, which is what populates myState)
plpython
SPI_execute_callback
(PLy_CSSetup gets called again)
plpython (this just returns a value)
After that plpython call, the executor is going to call PLy_CSreceive,
which is the function with this problematic code. So by the time this
error happens, there are two nested levels of plpython+SPI going on. I
originally thought the re-entrant calls were causing the problem, but
after monitoring what PLy_CSSetup was doing and what PLy_CSreceive was
getting that's not the case, or at least not the only reason for this.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
I do think it's interesting that the data appeared to be completely fine
until I actually ran whatever the first assembly instruction of the for
loop is, so presumably it was fine after the sigsetjmp() call (which I'm
assuming is what causes all the fuss to begin with...) From my
understanding of what volatile does, I can understand why it might be
necessary for something in the CATCH block to need it, but not in the TRY.
While I was at Salesforce we saw some *extremely* odd behaviors in PG_TRY
blocks that we eventually tracked down to the compiler effectively trying
to store two different local variables in the same register. I don't
recall all the details anymore, but it had something to do with register
spills to stack locations and subsequent reloads happening at unfortunate
places with respect to the sigsetjmp call. I figured it was a bug in the
specific compiler we were using, because nobody in the PG community was
reporting any problems with identical code. Anyway, "volatile" was an
effective way of suppressing the issue. I wonder whether you're seeing
something similar. You'd have to grovel through the assembly code for
the function with and without "volatile" to be sure.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jan 5, 2017 at 4:18 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 1/3/17 9:20 PM, Amit Kapila wrote:
Good to know that it worked, but what is the theory? From your
experiment, it appears that in some cases accessing local pointer
variables is okay and in other cases, it is not okay.I can run some other experiments if you have any to suggest.
I do think it's interesting that the data appeared to be completely fine
until I actually ran whatever the first assembly instruction of the for loop
is, so presumably it was fine after the sigsetjmp() call (which I'm assuming
is what causes all the fuss to begin with...) From my understanding of what
volatile does, I can understand why it might be necessary for something in
the CATCH block to need it, but not in the TRY.
Memory contexts used in catch block also doesn't seem to be marked as
volatile, you might want to try by marking them as volatile. Also, it
might worth trying it on some other system to see if you are by any
chance hitting the problem similar to what Tom has faced.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jan 5, 2017 at 7:10 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Memory contexts used in catch block also doesn't seem to be marked as
volatile, you might want to try by marking them as volatile. Also, it
might worth trying it on some other system to see if you are by any
chance hitting the problem similar to what Tom has faced.
If a variable is modified within PG_TRY and then referenced in
PG_CATCH it needs to be marked as volatile to be strictly in
conformance with POSIX. This also ensures that any compiler does not
do any stupid optimizations with those variables in the way they are
referenced and used.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jan 6, 2017 at 5:43 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
If a variable is modified within PG_TRY and then referenced in
PG_CATCH it needs to be marked as volatile to be strictly in
conformance with POSIX. This also ensures that any compiler does not
do any stupid optimizations with those variables in the way they are
referenced and used.
That sort of begs the question of why PG_exception_stack is not
marked as volatile, since the macros themselves modify it within
the PG_TRY block and reference it within the PG_CATCH block. Is
there some reason this variable is immune to the problem?
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@gmail.com> writes:
On Fri, Jan 6, 2017 at 5:43 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:If a variable is modified within PG_TRY and then referenced in
PG_CATCH it needs to be marked as volatile to be strictly in
conformance with POSIX. This also ensures that any compiler does not
do any stupid optimizations with those variables in the way they are
referenced and used.
That sort of begs the question of why PG_exception_stack is not
marked as volatile, since the macros themselves modify it within
the PG_TRY block and reference it within the PG_CATCH block. Is
there some reason this variable is immune to the problem?
It's not a local variable.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers