Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit232d8ca

Browse files
committed
Fix memory leakage in postgres_fdw's DirectModify code path.
postgres_fdw tries to use PG_TRY blocks to ensure that it willeventually free the PGresult created by the remote modify command.However, it's fundamentally impossible for this scheme to workreliably when there's RETURNING data, because the query could failin between invocations of postgres_fdw's DirectModify methods.There is at least one instance of exactly this situation in theregression tests, and the ensuing session-lifespan leak is visibleunder Valgrind.We can improve matters by using a memory context reset callbackattached to the ExecutorState context. That ensures that thePGresult will be freed when the ExecutorState context is torndown, even if control never reaches postgresEndDirectModify.I have little faith that there aren't other potential PGresultleakages in the backend modules that use libpq. So I think it'dbe a good idea to apply this concept universally by creatinginfrastructure that attaches a reset callback to every PGresultgenerated in the backend. However, that seems too invasive forv18 at this point, let alone the back branches. So for themoment, apply this narrow fix that just makes DirectModify safe.I have a patch in the queue for the more general idea, but itwill have to wait for v19.Author: Tom Lane <tgl@sss.pgh.pa.us>Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>Discussion:https://postgr.es/m/2976982.1748049023@sss.pgh.pa.usBackpatch-through: 13
1 parentd98cefe commit232d8ca

File tree

1 file changed

+35
-27
lines changed

1 file changed

+35
-27
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ typedef struct PgFdwDirectModifyState
240240
PGresult*result;/* result for query */
241241
intnum_tuples;/* # of result tuples */
242242
intnext_tuple;/* index of next one to return */
243+
MemoryContextCallbackresult_cb;/* ensures result will get freed */
243244
RelationresultRel;/* relcache entry for the target relation */
244245
AttrNumber*attnoMap;/* array of attnums of input user columns */
245246
AttrNumberctidAttno;/* attnum of input ctid column */
@@ -2670,6 +2671,17 @@ postgresBeginDirectModify(ForeignScanState *node, int eflags)
26702671
dmstate= (PgFdwDirectModifyState*)palloc0(sizeof(PgFdwDirectModifyState));
26712672
node->fdw_state=dmstate;
26722673

2674+
/*
2675+
* We use a memory context callback to ensure that the dmstate's PGresult
2676+
* (if any) will be released, even if the query fails somewhere that's
2677+
* outside our control. The callback is always armed for the duration of
2678+
* the query; this relies on PQclear(NULL) being a no-op.
2679+
*/
2680+
dmstate->result_cb.func= (MemoryContextCallbackFunction)PQclear;
2681+
dmstate->result_cb.arg=NULL;
2682+
MemoryContextRegisterResetCallback(CurrentMemoryContext,
2683+
&dmstate->result_cb);
2684+
26732685
/*
26742686
* Identify which user to do the remote access as. This should match what
26752687
* ExecCheckPermissions() does.
@@ -2817,7 +2829,13 @@ postgresEndDirectModify(ForeignScanState *node)
28172829
return;
28182830

28192831
/* Release PGresult */
2820-
PQclear(dmstate->result);
2832+
if (dmstate->result)
2833+
{
2834+
PQclear(dmstate->result);
2835+
dmstate->result=NULL;
2836+
/* ... and don't forget to disable the callback */
2837+
dmstate->result_cb.arg=NULL;
2838+
}
28212839

28222840
/* Release remote connection */
28232841
ReleaseConnection(dmstate->conn);
@@ -4591,13 +4609,17 @@ execute_dml_stmt(ForeignScanState *node)
45914609
/*
45924610
* Get the result, and check for success.
45934611
*
4594-
* We don't use a PG_TRY block here, so be careful not to throw error
4595-
* without releasing the PGresult.
4612+
* We use a memory context callback to ensure that the PGresult will be
4613+
* released, even if the query fails somewhere that's outside our control.
4614+
* The callback is already registered, just need to fill in its arg.
45964615
*/
4616+
Assert(dmstate->result==NULL);
45974617
dmstate->result=pgfdw_get_result(dmstate->conn);
4618+
dmstate->result_cb.arg=dmstate->result;
4619+
45984620
if (PQresultStatus(dmstate->result)!=
45994621
(dmstate->has_returning ?PGRES_TUPLES_OK :PGRES_COMMAND_OK))
4600-
pgfdw_report_error(ERROR,dmstate->result,dmstate->conn,true,
4622+
pgfdw_report_error(ERROR,dmstate->result,dmstate->conn,false,
46014623
dmstate->query);
46024624

46034625
/* Get the number of rows affected. */
@@ -4641,30 +4663,16 @@ get_returning_data(ForeignScanState *node)
46414663
}
46424664
else
46434665
{
4644-
/*
4645-
* On error, be sure to release the PGresult on the way out. Callers
4646-
* do not have PG_TRY blocks to ensure this happens.
4647-
*/
4648-
PG_TRY();
4649-
{
4650-
HeapTuplenewtup;
4651-
4652-
newtup=make_tuple_from_result_row(dmstate->result,
4653-
dmstate->next_tuple,
4654-
dmstate->rel,
4655-
dmstate->attinmeta,
4656-
dmstate->retrieved_attrs,
4657-
node,
4658-
dmstate->temp_cxt);
4659-
ExecStoreHeapTuple(newtup,slot, false);
4660-
}
4661-
PG_CATCH();
4662-
{
4663-
PQclear(dmstate->result);
4664-
PG_RE_THROW();
4665-
}
4666-
PG_END_TRY();
4666+
HeapTuplenewtup;
46674667

4668+
newtup=make_tuple_from_result_row(dmstate->result,
4669+
dmstate->next_tuple,
4670+
dmstate->rel,
4671+
dmstate->attinmeta,
4672+
dmstate->retrieved_attrs,
4673+
node,
4674+
dmstate->temp_cxt);
4675+
ExecStoreHeapTuple(newtup,slot, false);
46684676
/* Get the updated/deleted tuple. */
46694677
if (dmstate->rel)
46704678
resultSlot=slot;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp