Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit e98df02

Browse files
committed
Repair memory leaks in plpython.
PLy_spi_execute_plan (PLyPlan.execute) and PLy_cursor_plan (plpy.cursor) use PLy_output_convert to convert Python values into Datums that can be passed to the query-to-execute. But they failed to pay much attention to its warning that it can leave "cruft generated along the way" behind. Repeated use of these methods can result in a substantial memory leak for the duration of the calling plpython function. To fix, make a temporary memory context to invoke PLy_output_convert in. This also lets us get rid of the rather fragile code that was here for retail pfree's of the converted Datums. Indeed, we don't need the PLyPlanObject.values field anymore at all, though I left it in place in the back branches in the name of ABI stability. Mat Arye and Tom Lane, per report from Mat Arye. Back-patch to all supported branches. Discussion: https://postgr.es/m/CADsUR0DvVgnZYWwnmKRK65MZg7YLUSTDLV61qdnrwtrAJgU6xw@mail.gmail.com
1 parent 8ed9bf0 commit e98df02

File tree

2 files changed

+49
-61
lines changed

2 files changed

+49
-61
lines changed

src/pl/plpython/plpy_cursorobject.c

+24-28
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,6 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
142142
{
143143
PLyCursorObject *cursor;
144144
volatile int nargs;
145-
int i;
146145
PLyPlanObject *plan;
147146
PLyExecutionContext *exec_ctx = PLy_current_execution_context();
148147
volatile MemoryContext oldcontext;
@@ -201,13 +200,30 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
201200
PG_TRY();
202201
{
203202
Portal portal;
203+
MemoryContext tmpcontext;
204+
Datum *volatile values;
204205
char *volatile nulls;
205206
volatile int j;
206207

208+
/*
209+
* Converted arguments and associated cruft will be in this context,
210+
* which is local to our subtransaction.
211+
*/
212+
tmpcontext = AllocSetContextCreate(CurTransactionContext,
213+
"PL/Python temporary context",
214+
ALLOCSET_SMALL_SIZES);
215+
MemoryContextSwitchTo(tmpcontext);
216+
207217
if (nargs > 0)
208-
nulls = palloc(nargs * sizeof(char));
218+
{
219+
values = (Datum *) palloc(nargs * sizeof(Datum));
220+
nulls = (char *) palloc(nargs * sizeof(char));
221+
}
209222
else
223+
{
224+
values = NULL;
210225
nulls = NULL;
226+
}
211227

212228
for (j = 0; j < nargs; j++)
213229
{
@@ -219,7 +235,7 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
219235
{
220236
bool isnull;
221237

222-
plan->values[j] = PLy_output_convert(arg, elem, &isnull);
238+
values[j] = PLy_output_convert(arg, elem, &isnull);
223239
nulls[j] = isnull ? 'n' : ' ';
224240
}
225241
PG_FINALLY(2);
@@ -229,7 +245,9 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
229245
PG_END_TRY(2);
230246
}
231247

232-
portal = SPI_cursor_open(NULL, plan->plan, plan->values, nulls,
248+
MemoryContextSwitchTo(oldcontext);
249+
250+
portal = SPI_cursor_open(NULL, plan->plan, values, nulls,
233251
exec_ctx->curr_proc->fn_readonly);
234252
if (portal == NULL)
235253
elog(ERROR, "SPI_cursor_open() failed: %s",
@@ -239,40 +257,18 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
239257

240258
PinPortal(portal);
241259

260+
MemoryContextDelete(tmpcontext);
242261
PLy_spi_subtransaction_commit(oldcontext, oldowner);
243262
}
244263
PG_CATCH();
245264
{
246-
int k;
247-
248-
/* cleanup plan->values array */
249-
for (k = 0; k < nargs; k++)
250-
{
251-
if (!plan->args[k].typbyval &&
252-
(plan->values[k] != PointerGetDatum(NULL)))
253-
{
254-
pfree(DatumGetPointer(plan->values[k]));
255-
plan->values[k] = PointerGetDatum(NULL);
256-
}
257-
}
258-
259265
Py_DECREF(cursor);
260-
266+
/* Subtransaction abort will remove the tmpcontext */
261267
PLy_spi_subtransaction_abort(oldcontext, oldowner);
262268
return NULL;
263269
}
264270
PG_END_TRY();
265271

266-
for (i = 0; i < nargs; i++)
267-
{
268-
if (!plan->args[i].typbyval &&
269-
(plan->values[i] != PointerGetDatum(NULL)))
270-
{
271-
pfree(DatumGetPointer(plan->values[i]));
272-
plan->values[i] = PointerGetDatum(NULL);
273-
}
274-
}
275-
276272
Assert(cursor->portalname != NULL);
277273
return (PyObject *) cursor;
278274
}

src/pl/plpython/plpy_spi.c

+25-33
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,7 @@ PyObject *
175175
PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
176176
{
177177
volatile int nargs;
178-
int i,
179-
rv;
178+
int rv;
180179
PLyPlanObject *plan;
181180
volatile MemoryContext oldcontext;
182181
volatile ResourceOwner oldowner;
@@ -222,13 +221,30 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
222221
PG_TRY();
223222
{
224223
PLyExecutionContext *exec_ctx = PLy_current_execution_context();
224+
MemoryContext tmpcontext;
225+
Datum *volatile values;
225226
char *volatile nulls;
226227
volatile int j;
227228

229+
/*
230+
* Converted arguments and associated cruft will be in this context,
231+
* which is local to our subtransaction.
232+
*/
233+
tmpcontext = AllocSetContextCreate(CurTransactionContext,
234+
"PL/Python temporary context",
235+
ALLOCSET_SMALL_SIZES);
236+
MemoryContextSwitchTo(tmpcontext);
237+
228238
if (nargs > 0)
229-
nulls = palloc(nargs * sizeof(char));
239+
{
240+
values = (Datum *) palloc(nargs * sizeof(Datum));
241+
nulls = (char *) palloc(nargs * sizeof(char));
242+
}
230243
else
244+
{
245+
values = NULL;
231246
nulls = NULL;
247+
}
232248

233249
for (j = 0; j < nargs; j++)
234250
{
@@ -240,7 +256,7 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
240256
{
241257
bool isnull;
242258

243-
plan->values[j] = PLy_output_convert(arg, elem, &isnull);
259+
values[j] = PLy_output_convert(arg, elem, &isnull);
244260
nulls[j] = isnull ? 'n' : ' ';
245261
}
246262
PG_FINALLY(2);
@@ -250,47 +266,23 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
250266
PG_END_TRY(2);
251267
}
252268

253-
rv = SPI_execute_plan(plan->plan, plan->values, nulls,
269+
MemoryContextSwitchTo(oldcontext);
270+
271+
rv = SPI_execute_plan(plan->plan, values, nulls,
254272
exec_ctx->curr_proc->fn_readonly, limit);
255273
ret = PLy_spi_execute_fetch_result(SPI_tuptable, SPI_processed, rv);
256274

257-
if (nargs > 0)
258-
pfree(nulls);
259-
275+
MemoryContextDelete(tmpcontext);
260276
PLy_spi_subtransaction_commit(oldcontext, oldowner);
261277
}
262278
PG_CATCH();
263279
{
264-
int k;
265-
266-
/*
267-
* cleanup plan->values array
268-
*/
269-
for (k = 0; k < nargs; k++)
270-
{
271-
if (!plan->args[k].typbyval &&
272-
(plan->values[k] != PointerGetDatum(NULL)))
273-
{
274-
pfree(DatumGetPointer(plan->values[k]));
275-
plan->values[k] = PointerGetDatum(NULL);
276-
}
277-
}
278-
280+
/* Subtransaction abort will remove the tmpcontext */
279281
PLy_spi_subtransaction_abort(oldcontext, oldowner);
280282
return NULL;
281283
}
282284
PG_END_TRY();
283285

284-
for (i = 0; i < nargs; i++)
285-
{
286-
if (!plan->args[i].typbyval &&
287-
(plan->values[i] != PointerGetDatum(NULL)))
288-
{
289-
pfree(DatumGetPointer(plan->values[i]));
290-
plan->values[i] = PointerGetDatum(NULL);
291-
}
292-
}
293-
294286
if (rv < 0)
295287
{
296288
PLy_exception_set(PLy_exc_spi_error,

0 commit comments

Comments
 (0)