Skip to content

Commit 80aa984

Browse files
tglsfdctglsfdc
committed
Reap the benefits of not having to avoid leaking PGresults.
Remove a bunch of PG_TRY constructs, de-volatilize related variables, remove some PQclear calls in error paths. Aside from making the code simpler and shorter, this should provide some marginal performance gains. For ease of review, I did not re-indent code within the removed PG_TRY constructs. That'll be done in a separate patch. 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.us
1 parent 7d8f595 commit 80aa984

File tree

6 files changed

+83
-274
lines changed

6 files changed

+83
-274
lines changed

‎contrib/dblink/dblink.c

Lines changed: 27 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,8 @@ static void materializeQueryResult(FunctionCallInfo fcinfo,
101101
const char *conname,
102102
const char *sql,
103103
bool fail);
104-
static PGresult *storeQueryResult(volatile storeInfo *sinfo, PGconn *conn, const char *sql);
105-
static void storeRow(volatile storeInfo *sinfo, PGresult *res, bool first);
104+
static PGresult *storeQueryResult(storeInfo *sinfo, PGconn *conn, const char *sql);
105+
static void storeRow(storeInfo *sinfo, PGresult *res, bool first);
106106
static remoteConn *getConnectionByName(const char *name);
107107
static HTAB *createConnHash(void);
108108
static remoteConn *createNewConnection(const char *name);
@@ -169,14 +169,6 @@ typedef struct remoteConnHashEnt
169169
/* initial number of connection hashes */
170170
#define NUMCONN 16
171171

172-
static char *
173-
xpstrdup(const char *in)
174-
{
175-
if (in == NULL)
176-
return NULL;
177-
return pstrdup(in);
178-
}
179-
180172
pg_noreturn static void
181173
dblink_res_internalerror(PGconn *conn, PGresult *res, const char *p2)
182174
{
@@ -870,17 +862,14 @@ static void
870862
materializeResult(FunctionCallInfo fcinfo, PGconn *conn, PGresult *res)
871863
{
872864
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
865+
TupleDesc tupdesc;
866+
bool is_sql_cmd;
867+
int ntuples;
868+
int nfields;
873869

874870
/* prepTuplestoreResult must have been called previously */
875871
Assert(rsinfo->returnMode == SFRM_Materialize);
876872

877-
PG_TRY();
878-
{
879-
TupleDesc tupdesc;
880-
bool is_sql_cmd;
881-
int ntuples;
882-
int nfields;
883-
884873
if (PQresultStatus(res) == PGRES_COMMAND_OK)
885874
{
886875
is_sql_cmd = true;
@@ -988,13 +977,8 @@ materializeResult(FunctionCallInfo fcinfo, PGconn *conn, PGresult *res)
988977
/* clean up GUC settings, if we changed any */
989978
restoreLocalGucs(nestlevel);
990979
}
991-
}
992-
PG_FINALLY();
993-
{
994-
/* be sure to release the libpq result */
980+
995981
PQclear(res);
996-
}
997-
PG_END_TRY();
998982
}
999983

1000984
/*
@@ -1013,16 +997,17 @@ materializeQueryResult(FunctionCallInfo fcinfo,
1013997
bool fail)
1014998
{
1015999
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
1016-
PGresult *volatile res = NULL;
1017-
volatile storeInfo sinfo = {0};
10181000

10191001
/* prepTuplestoreResult must have been called previously */
10201002
Assert(rsinfo->returnMode == SFRM_Materialize);
10211003

1022-
sinfo.fcinfo = fcinfo;
1023-
1004+
/* Use a PG_TRY block to ensure we pump libpq dry of results */
10241005
PG_TRY();
10251006
{
1007+
storeInfo sinfo = {0};
1008+
PGresult *res;
1009+
1010+
sinfo.fcinfo = fcinfo;
10261011
/* Create short-lived memory context for data conversions */
10271012
sinfo.tmpcontext = AllocSetContextCreate(CurrentMemoryContext,
10281013
"dblink temporary context",
@@ -1035,14 +1020,7 @@ materializeQueryResult(FunctionCallInfo fcinfo,
10351020
(PQresultStatus(res) != PGRES_COMMAND_OK &&
10361021
PQresultStatus(res) != PGRES_TUPLES_OK))
10371022
{
1038-
/*
1039-
* dblink_res_error will clear the passed PGresult, so we need
1040-
* this ugly dance to avoid doing so twice during error exit
1041-
*/
1042-
PGresult *res1 = res;
1043-
1044-
res = NULL;
1045-
dblink_res_error(conn, conname, res1, fail,
1023+
dblink_res_error(conn, conname, res, fail,
10461024
"while executing query");
10471025
/* if fail isn't set, we'll return an empty query result */
10481026
}
@@ -1081,7 +1059,6 @@ materializeQueryResult(FunctionCallInfo fcinfo,
10811059
tuplestore_puttuple(tupstore, tuple);
10821060

10831061
PQclear(res);
1084-
res = NULL;
10851062
}
10861063
else
10871064
{
@@ -1090,26 +1067,20 @@ materializeQueryResult(FunctionCallInfo fcinfo,
10901067
Assert(rsinfo->setResult != NULL);
10911068

10921069
PQclear(res);
1093-
res = NULL;
10941070
}
10951071

10961072
/* clean up data conversion short-lived memory context */
10971073
if (sinfo.tmpcontext != NULL)
10981074
MemoryContextDelete(sinfo.tmpcontext);
1099-
sinfo.tmpcontext = NULL;
11001075

11011076
PQclear(sinfo.last_res);
1102-
sinfo.last_res = NULL;
11031077
PQclear(sinfo.cur_res);
1104-
sinfo.cur_res = NULL;
11051078
}
11061079
PG_CATCH();
11071080
{
1108-
/* be sure to release any libpq result we collected */
1109-
PQclear(res);
1110-
PQclear(sinfo.last_res);
1111-
PQclear(sinfo.cur_res);
1112-
/* and clear out any pending data in libpq */
1081+
PGresult *res;
1082+
1083+
/* be sure to clear out any pending data in libpq */
11131084
while ((res = libpqsrv_get_result(conn, dblink_we_get_result)) !=
11141085
NULL)
11151086
PQclear(res);
@@ -1122,7 +1093,7 @@ materializeQueryResult(FunctionCallInfo fcinfo,
11221093
* Execute query, and send any result rows to sinfo->tuplestore.
11231094
*/
11241095
static PGresult *
1125-
storeQueryResult(volatile storeInfo *sinfo, PGconn *conn, const char *sql)
1096+
storeQueryResult(storeInfo *sinfo, PGconn *conn, const char *sql)
11261097
{
11271098
bool first = true;
11281099
int nestlevel = -1;
@@ -1190,7 +1161,7 @@ storeQueryResult(volatile storeInfo *sinfo, PGconn *conn, const char *sql)
11901161
* (in this case the PGresult might contain either zero or one row).
11911162
*/
11921163
static void
1193-
storeRow(volatile storeInfo *sinfo, PGresult *res, bool first)
1164+
storeRow(storeInfo *sinfo, PGresult *res, bool first)
11941165
{
11951166
int nfields = PQnfields(res);
11961167
HeapTuple tuple;
@@ -2795,26 +2766,25 @@ dblink_connstr_check(const char *connstr)
27952766
/*
27962767
* Report an error received from the remote server
27972768
*
2798-
* res: the received error result (will be freed)
2769+
* res: the received error result
27992770
* fail: true for ERROR ereport, false for NOTICE
28002771
* fmt and following args: sprintf-style format and values for errcontext;
28012772
* the resulting string should be worded like "while <some action>"
2773+
*
2774+
* If "res" is not NULL, it'll be PQclear'ed here (unless we throw error,
2775+
* in which case memory context cleanup will clear it eventually).
28022776
*/
28032777
static void
28042778
dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
28052779
bool fail, const char *fmt,...)
28062780
{
28072781
int level;
28082782
char *pg_diag_sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
2809-
char *pg_diag_message_primary = PQresultErrorField(res, PG_DIAG_MESSAGE_PRIMARY);
2810-
char *pg_diag_message_detail = PQresultErrorField(res, PG_DIAG_MESSAGE_DETAIL);
2811-
char *pg_diag_message_hint = PQresultErrorField(res, PG_DIAG_MESSAGE_HINT);
2812-
char *pg_diag_context = PQresultErrorField(res, PG_DIAG_CONTEXT);
2783+
char *message_primary = PQresultErrorField(res, PG_DIAG_MESSAGE_PRIMARY);
2784+
char *message_detail = PQresultErrorField(res, PG_DIAG_MESSAGE_DETAIL);
2785+
char *message_hint = PQresultErrorField(res, PG_DIAG_MESSAGE_HINT);
2786+
char *message_context = PQresultErrorField(res, PG_DIAG_CONTEXT);
28132787
int sqlstate;
2814-
char *message_primary;
2815-
char *message_detail;
2816-
char *message_hint;
2817-
char *message_context;
28182788
va_list ap;
28192789
char dblink_context_msg[512];
28202790

@@ -2832,11 +2802,6 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
28322802
else
28332803
sqlstate = ERRCODE_CONNECTION_FAILURE;
28342804

2835-
message_primary = xpstrdup(pg_diag_message_primary);
2836-
message_detail = xpstrdup(pg_diag_message_detail);
2837-
message_hint = xpstrdup(pg_diag_message_hint);
2838-
message_context = xpstrdup(pg_diag_context);
2839-
28402805
/*
28412806
* If we don't get a message from the PGresult, try the PGconn. This is
28422807
* needed because for connection-level failures, PQgetResult may just
@@ -2845,14 +2810,6 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
28452810
if (message_primary == NULL)
28462811
message_primary = pchomp(PQerrorMessage(conn));
28472812

2848-
/*
2849-
* Now that we've copied all the data we need out of the PGresult, it's
2850-
* safe to free it. We must do this to avoid PGresult leakage. We're
2851-
* leaking all the strings too, but those are in palloc'd memory that will
2852-
* get cleaned up eventually.
2853-
*/
2854-
PQclear(res);
2855-
28562813
/*
28572814
* Format the basic errcontext string. Below, we'll add on something
28582815
* about the connection name. That's a violation of the translatability
@@ -2877,6 +2834,7 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
28772834
dblink_context_msg, conname)) :
28782835
(errcontext("%s on unnamed dblink connection",
28792836
dblink_context_msg))));
2837+
PQclear(res);
28802838
}
28812839

28822840
/*

‎contrib/postgres_fdw/connection.c

Lines changed: 16 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -815,7 +815,7 @@ static void
815815
do_sql_command_begin(PGconn *conn, const char *sql)
816816
{
817817
if (!PQsendQuery(conn, sql))
818-
pgfdw_report_error(ERROR, NULL, conn, false, sql);
818+
pgfdw_report_error(ERROR, NULL, conn, sql);
819819
}
820820

821821
static void
@@ -830,10 +830,10 @@ do_sql_command_end(PGconn *conn, const char *sql, bool consume_input)
830830
* would be large compared to the overhead of PQconsumeInput.)
831831
*/
832832
if (consume_input && !PQconsumeInput(conn))
833-
pgfdw_report_error(ERROR, NULL, conn, false, sql);
833+
pgfdw_report_error(ERROR, NULL, conn, sql);
834834
res = pgfdw_get_result(conn);
835835
if (PQresultStatus(res) != PGRES_COMMAND_OK)
836-
pgfdw_report_error(ERROR, res, conn, true, sql);
836+
pgfdw_report_error(ERROR, res, conn, sql);
837837
PQclear(res);
838838
}
839839

@@ -967,22 +967,21 @@ pgfdw_get_result(PGconn *conn)
967967
* Report an error we got from the remote server.
968968
*
969969
* elevel: error level to use (typically ERROR, but might be less)
970-
* res: PGresult containing the error
970+
* res: PGresult containing the error (might be NULL)
971971
* conn: connection we did the query on
972-
* clear: if true, PQclear the result (otherwise caller will handle it)
973972
* sql: NULL, or text of remote command we tried to execute
974973
*
974+
* If "res" is not NULL, it'll be PQclear'ed here (unless we throw error,
975+
* in which case memory context cleanup will clear it eventually).
976+
*
975977
* Note: callers that choose not to throw ERROR for a remote error are
976978
* responsible for making sure that the associated ConnCacheEntry gets
977979
* marked with have_error = true.
978980
*/
979981
void
980982
pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
981-
bool clear, const char *sql)
983+
const char *sql)
982984
{
983-
/* If requested, PGresult must be released before leaving this function. */
984-
PG_TRY();
985-
{
986985
char *diag_sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
987986
char *message_primary = PQresultErrorField(res, PG_DIAG_MESSAGE_PRIMARY);
988987
char *message_detail = PQresultErrorField(res, PG_DIAG_MESSAGE_DETAIL);
@@ -1016,13 +1015,7 @@ pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
10161015
message_hint ? errhint("%s", message_hint) : 0,
10171016
message_context ? errcontext("%s", message_context) : 0,
10181017
sql ? errcontext("remote SQL command: %s", sql) : 0));
1019-
}
1020-
PG_FINALLY();
1021-
{
1022-
if (clear)
10231018
PQclear(res);
1024-
}
1025-
PG_END_TRY();
10261019
}
10271020

10281021
/*
@@ -1545,7 +1538,7 @@ pgfdw_exec_cleanup_query_begin(PGconn *conn, const char *query)
15451538
*/
15461539
if (!PQsendQuery(conn, query))
15471540
{
1548-
pgfdw_report_error(WARNING, NULL, conn, false, query);
1541+
pgfdw_report_error(WARNING, NULL, conn, query);
15491542
return false;
15501543
}
15511544

@@ -1570,7 +1563,7 @@ pgfdw_exec_cleanup_query_end(PGconn *conn, const char *query,
15701563
*/
15711564
if (consume_input && !PQconsumeInput(conn))
15721565
{
1573-
pgfdw_report_error(WARNING, NULL, conn, false, query);
1566+
pgfdw_report_error(WARNING, NULL, conn, query);
15741567
return false;
15751568
}
15761569

@@ -1582,15 +1575,15 @@ pgfdw_exec_cleanup_query_end(PGconn *conn, const char *query,
15821575
(errmsg("could not get query result due to timeout"),
15831576
errcontext("remote SQL command: %s", query)));
15841577
else
1585-
pgfdw_report_error(WARNING, NULL, conn, false, query);
1578+
pgfdw_report_error(WARNING, NULL, conn, query);
15861579

15871580
return false;
15881581
}
15891582

15901583
/* Issue a warning if not successful. */
15911584
if (PQresultStatus(result) != PGRES_COMMAND_OK)
15921585
{
1593-
pgfdw_report_error(WARNING, result, conn, true, query);
1586+
pgfdw_report_error(WARNING, result, conn, query);
15941587
return ignore_errors;
15951588
}
15961589
PQclear(result);
@@ -1618,17 +1611,12 @@ pgfdw_get_cleanup_result(PGconn *conn, TimestampTz endtime,
16181611
PGresult **result,
16191612
bool *timed_out)
16201613
{
1621-
volatile bool failed = false;
1622-
PGresult *volatile last_res = NULL;
1614+
bool failed = false;
1615+
PGresult *last_res = NULL;
1616+
int canceldelta = RETRY_CANCEL_TIMEOUT * 2;
16231617

16241618
*result = NULL;
16251619
*timed_out = false;
1626-
1627-
/* In what follows, do not leak any PGresults on an error. */
1628-
PG_TRY();
1629-
{
1630-
int canceldelta = RETRY_CANCEL_TIMEOUT * 2;
1631-
16321620
for (;;)
16331621
{
16341622
PGresult *res;
@@ -1706,15 +1694,7 @@ pgfdw_get_cleanup_result(PGconn *conn, TimestampTz endtime,
17061694
PQclear(last_res);
17071695
last_res = res;
17081696
}
1709-
exit: ;
1710-
}
1711-
PG_CATCH();
1712-
{
1713-
PQclear(last_res);
1714-
PG_RE_THROW();
1715-
}
1716-
PG_END_TRY();
1717-
1697+
exit:
17181698
if (failed)
17191699
PQclear(last_res);
17201700
else

0 commit comments

Comments
 (0)