some dblink refactoring

Started by Peter Eisentrautalmost 9 years ago6 messages
#1Peter Eisentraut
peter.eisentraut@2ndquadrant.com
1 attachment(s)

Here is a patch to refactor some macro hell in dblink.

This patch was discussed in the background sessions thread as a
prerequisite for some work there, but I figure I'll make a separate
thread for it to give everyone interested in dblink a chance to respond
separate from the other thread.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

v2-0001-dblink-Replace-some-macros-by-static-functions.patchtext/x-patch; name=v2-0001-dblink-Replace-some-macros-by-static-functions.patchDownload
From 2e1fc0b0c50452bac91461a2317c28a8718fe89f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sun, 25 Dec 2016 12:00:00 -0500
Subject: [PATCH v2 1/2] dblink: Replace some macros by static functions

Also remove some unused code and the no longer useful dblink.h file.
---
 contrib/dblink/dblink.c | 290 +++++++++++++++++++++++-------------------------
 contrib/dblink/dblink.h |  39 -------
 2 files changed, 137 insertions(+), 192 deletions(-)
 delete mode 100644 contrib/dblink/dblink.h

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index e0d6778a08..67d6699066 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -61,8 +61,6 @@
 #include "utils/tqual.h"
 #include "utils/varlena.h"
 
-#include "dblink.h"
-
 PG_MODULE_MAGIC;
 
 typedef struct remoteConn
@@ -146,98 +144,102 @@ typedef struct remoteConnHashEnt
 /* initial number of connection hashes */
 #define NUMCONN 16
 
-/* general utility */
-#define xpfree(var_) \
-	do { \
-		if (var_ != NULL) \
-		{ \
-			pfree(var_); \
-			var_ = NULL; \
-		} \
-	} while (0)
-
-#define xpstrdup(var_c, var_) \
-	do { \
-		if (var_ != NULL) \
-			var_c = pstrdup(var_); \
-		else \
-			var_c = NULL; \
-	} while (0)
-
-#define DBLINK_RES_INTERNALERROR(p2) \
-	do { \
-			msg = pchomp(PQerrorMessage(conn)); \
-			if (res) \
-				PQclear(res); \
-			elog(ERROR, "%s: %s", p2, msg); \
-	} while (0)
-
-#define DBLINK_CONN_NOT_AVAIL \
-	do { \
-		if(conname) \
-			ereport(ERROR, \
-					(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST), \
-					 errmsg("connection \"%s\" not available", conname))); \
-		else \
-			ereport(ERROR, \
-					(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST), \
-					 errmsg("connection not available"))); \
-	} while (0)
-
-#define DBLINK_GET_CONN \
-	do { \
-			char *conname_or_str = text_to_cstring(PG_GETARG_TEXT_PP(0)); \
-			rconn = getConnectionByName(conname_or_str); \
-			if (rconn) \
-			{ \
-				conn = rconn->conn; \
-				conname = conname_or_str; \
-			} \
-			else \
-			{ \
-				connstr = get_connect_string(conname_or_str); \
-				if (connstr == NULL) \
-				{ \
-					connstr = conname_or_str; \
-				} \
-				dblink_connstr_check(connstr); \
-				conn = PQconnectdb(connstr); \
-				if (PQstatus(conn) == CONNECTION_BAD) \
-				{ \
-					msg = pchomp(PQerrorMessage(conn)); \
-					PQfinish(conn); \
-					ereport(ERROR, \
-							(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION), \
-							 errmsg("could not establish connection"), \
-							 errdetail_internal("%s", msg))); \
-				} \
-				dblink_security_check(conn, rconn); \
-				if (PQclientEncoding(conn) != GetDatabaseEncoding()) \
-					PQsetClientEncoding(conn, GetDatabaseEncodingName()); \
-				freeconn = true; \
-			} \
-	} while (0)
-
-#define DBLINK_GET_NAMED_CONN \
-	do { \
-			conname = text_to_cstring(PG_GETARG_TEXT_PP(0)); \
-			rconn = getConnectionByName(conname); \
-			if (rconn) \
-				conn = rconn->conn; \
-			else \
-				DBLINK_CONN_NOT_AVAIL; \
-	} while (0)
-
-#define DBLINK_INIT \
-	do { \
-			if (!pconn) \
-			{ \
-				pconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext, sizeof(remoteConn)); \
-				pconn->conn = NULL; \
-				pconn->openCursorCount = 0; \
-				pconn->newXactForCursor = FALSE; \
-			} \
-	} while (0)
+static char *
+xpstrdup(const char *in)
+{
+	if (in == NULL)
+		return NULL;
+	return pstrdup(in);
+}
+
+static void
+dblink_res_internalerror(PGconn *conn, PGresult *res, const char *p2)
+{
+	char	   *msg = pchomp(PQerrorMessage(conn));
+	if (res)
+		PQclear(res);
+	elog(ERROR, "%s: %s", p2, msg);
+}
+
+static void pg_attribute_noreturn()
+dblink_conn_not_avail(const char *conname)
+{
+	if (conname)
+		ereport(ERROR,
+				(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
+				 errmsg("connection \"%s\" not available", conname)));
+	else
+		ereport(ERROR,
+				(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
+				 errmsg("connection not available")));
+}
+
+static void
+dblink_get_conn(char *conname_or_str,
+				PGconn * volatile *conn_p, char **conname_p, volatile bool *freeconn_p)
+{
+	remoteConn *rconn = getConnectionByName(conname_or_str);
+	PGconn	   *conn;
+	char	   *conname;
+	bool		freeconn;
+
+	if (rconn)
+	{
+		conn = rconn->conn;
+		conname = conname_or_str;
+		freeconn = false;
+	}
+	else
+	{
+		const char *connstr;
+
+		connstr = get_connect_string(conname_or_str);
+		if (connstr == NULL)
+			connstr = conname_or_str;
+		dblink_connstr_check(connstr);
+		conn = PQconnectdb(connstr);
+		if (PQstatus(conn) == CONNECTION_BAD)
+		{
+			char	   *msg = pchomp(PQerrorMessage(conn));
+			PQfinish(conn);
+			ereport(ERROR,
+					(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+					 errmsg("could not establish connection"),
+					 errdetail_internal("%s", msg)));
+		}
+		dblink_security_check(conn, rconn);
+		if (PQclientEncoding(conn) != GetDatabaseEncoding())
+			PQsetClientEncoding(conn, GetDatabaseEncodingName());
+		freeconn = true;
+		conname = NULL;
+	}
+
+	*conn_p = conn;
+	*conname_p = conname;
+	*freeconn_p = freeconn;
+}
+
+static PGconn *
+dblink_get_named_conn(const char *conname)
+{
+	remoteConn *rconn = getConnectionByName(conname);
+	if (rconn)
+		return rconn->conn;
+	else
+		dblink_conn_not_avail(conname);
+}
+
+static void
+dblink_init(void)
+{
+	if (!pconn)
+	{
+		pconn = (remoteConn *) MemoryContextAlloc(TopMemoryContext, sizeof(remoteConn));
+		pconn->conn = NULL;
+		pconn->openCursorCount = 0;
+		pconn->newXactForCursor = FALSE;
+	}
+}
 
 /*
  * Create a persistent connection to another database
@@ -253,7 +255,7 @@ dblink_connect(PG_FUNCTION_ARGS)
 	PGconn	   *conn = NULL;
 	remoteConn *rconn = NULL;
 
-	DBLINK_INIT;
+	dblink_init();
 
 	if (PG_NARGS() == 2)
 	{
@@ -318,7 +320,7 @@ dblink_disconnect(PG_FUNCTION_ARGS)
 	remoteConn *rconn = NULL;
 	PGconn	   *conn = NULL;
 
-	DBLINK_INIT;
+	dblink_init();
 
 	if (PG_NARGS() == 1)
 	{
@@ -331,7 +333,7 @@ dblink_disconnect(PG_FUNCTION_ARGS)
 		conn = pconn->conn;
 
 	if (!conn)
-		DBLINK_CONN_NOT_AVAIL;
+		dblink_conn_not_avail(conname);
 
 	PQfinish(conn);
 	if (rconn)
@@ -352,7 +354,6 @@ PG_FUNCTION_INFO_V1(dblink_open);
 Datum
 dblink_open(PG_FUNCTION_ARGS)
 {
-	char	   *msg;
 	PGresult   *res = NULL;
 	PGconn	   *conn = NULL;
 	char	   *curname = NULL;
@@ -362,7 +363,7 @@ dblink_open(PG_FUNCTION_ARGS)
 	remoteConn *rconn = NULL;
 	bool		fail = true;	/* default to backward compatible behavior */
 
-	DBLINK_INIT;
+	dblink_init();
 	initStringInfo(&buf);
 
 	if (PG_NARGS() == 2)
@@ -401,7 +402,7 @@ dblink_open(PG_FUNCTION_ARGS)
 	}
 
 	if (!rconn || !rconn->conn)
-		DBLINK_CONN_NOT_AVAIL;
+		dblink_conn_not_avail(conname);
 	else
 		conn = rconn->conn;
 
@@ -410,7 +411,7 @@ dblink_open(PG_FUNCTION_ARGS)
 	{
 		res = PQexec(conn, "BEGIN");
 		if (PQresultStatus(res) != PGRES_COMMAND_OK)
-			DBLINK_RES_INTERNALERROR("begin error");
+			dblink_res_internalerror(conn, res, "begin error");
 		PQclear(res);
 		rconn->newXactForCursor = TRUE;
 
@@ -450,11 +451,10 @@ dblink_close(PG_FUNCTION_ARGS)
 	char	   *curname = NULL;
 	char	   *conname = NULL;
 	StringInfoData buf;
-	char	   *msg;
 	remoteConn *rconn = NULL;
 	bool		fail = true;	/* default to backward compatible behavior */
 
-	DBLINK_INIT;
+	dblink_init();
 	initStringInfo(&buf);
 
 	if (PG_NARGS() == 1)
@@ -489,7 +489,7 @@ dblink_close(PG_FUNCTION_ARGS)
 	}
 
 	if (!rconn || !rconn->conn)
-		DBLINK_CONN_NOT_AVAIL;
+		dblink_conn_not_avail(conname);
 	else
 		conn = rconn->conn;
 
@@ -517,7 +517,7 @@ dblink_close(PG_FUNCTION_ARGS)
 
 			res = PQexec(conn, "COMMIT");
 			if (PQresultStatus(res) != PGRES_COMMAND_OK)
-				DBLINK_RES_INTERNALERROR("commit error");
+				dblink_res_internalerror(conn, res, "commit error");
 			PQclear(res);
 		}
 	}
@@ -543,7 +543,7 @@ dblink_fetch(PG_FUNCTION_ARGS)
 
 	prepTuplestoreResult(fcinfo);
 
-	DBLINK_INIT;
+	dblink_init();
 
 	if (PG_NARGS() == 4)
 	{
@@ -587,7 +587,7 @@ dblink_fetch(PG_FUNCTION_ARGS)
 	}
 
 	if (!conn)
-		DBLINK_CONN_NOT_AVAIL;
+		dblink_conn_not_avail(conname);
 
 	initStringInfo(&buf);
 	appendStringInfo(&buf, "FETCH %d FROM %s", howmany, curname);
@@ -633,15 +633,13 @@ PG_FUNCTION_INFO_V1(dblink_send_query);
 Datum
 dblink_send_query(PG_FUNCTION_ARGS)
 {
-	char	   *conname = NULL;
-	PGconn	   *conn = NULL;
-	char	   *sql = NULL;
-	remoteConn *rconn = NULL;
+	PGconn	   *conn;
+	char	   *sql;
 	int			retval;
 
 	if (PG_NARGS() == 2)
 	{
-		DBLINK_GET_NAMED_CONN;
+		conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
 		sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
 	}
 	else
@@ -671,15 +669,12 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
 
 	prepTuplestoreResult(fcinfo);
 
-	DBLINK_INIT;
+	dblink_init();
 
 	PG_TRY();
 	{
-		char	   *msg;
-		char	   *connstr = NULL;
 		char	   *sql = NULL;
 		char	   *conname = NULL;
-		remoteConn *rconn = NULL;
 		bool		fail = true;	/* default to backward compatible */
 
 		if (!is_async)
@@ -687,7 +682,7 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
 			if (PG_NARGS() == 3)
 			{
 				/* text,text,bool */
-				DBLINK_GET_CONN;
+				dblink_get_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)), &conn, &conname, &freeconn);
 				sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
 				fail = PG_GETARG_BOOL(2);
 			}
@@ -702,7 +697,7 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
 				}
 				else
 				{
-					DBLINK_GET_CONN;
+					dblink_get_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)), &conn, &conname, &freeconn);
 					sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
 				}
 			}
@@ -722,13 +717,13 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
 			if (PG_NARGS() == 2)
 			{
 				/* text,bool */
-				DBLINK_GET_NAMED_CONN;
+				conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
 				fail = PG_GETARG_BOOL(1);
 			}
 			else if (PG_NARGS() == 1)
 			{
 				/* text */
-				DBLINK_GET_NAMED_CONN;
+				conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
 			}
 			else
 				/* shouldn't happen */
@@ -736,7 +731,7 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
 		}
 
 		if (!conn)
-			DBLINK_CONN_NOT_AVAIL;
+			dblink_conn_not_avail(conname);
 
 		if (!is_async)
 		{
@@ -1297,12 +1292,10 @@ PG_FUNCTION_INFO_V1(dblink_is_busy);
 Datum
 dblink_is_busy(PG_FUNCTION_ARGS)
 {
-	char	   *conname = NULL;
-	PGconn	   *conn = NULL;
-	remoteConn *rconn = NULL;
+	PGconn	   *conn;
 
-	DBLINK_INIT;
-	DBLINK_GET_NAMED_CONN;
+	dblink_init();
+	conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
 
 	PQconsumeInput(conn);
 	PG_RETURN_INT32(PQisBusy(conn));
@@ -1323,15 +1316,13 @@ PG_FUNCTION_INFO_V1(dblink_cancel_query);
 Datum
 dblink_cancel_query(PG_FUNCTION_ARGS)
 {
-	int			res = 0;
-	char	   *conname = NULL;
-	PGconn	   *conn = NULL;
-	remoteConn *rconn = NULL;
+	int			res;
+	PGconn	   *conn;
 	PGcancel   *cancel;
 	char		errbuf[256];
 
-	DBLINK_INIT;
-	DBLINK_GET_NAMED_CONN;
+	dblink_init();
+	conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
 	cancel = PQgetCancel(conn);
 
 	res = PQcancel(cancel, errbuf, 256);
@@ -1359,12 +1350,10 @@ Datum
 dblink_error_message(PG_FUNCTION_ARGS)
 {
 	char	   *msg;
-	char	   *conname = NULL;
-	PGconn	   *conn = NULL;
-	remoteConn *rconn = NULL;
+	PGconn	   *conn;
 
-	DBLINK_INIT;
-	DBLINK_GET_NAMED_CONN;
+	dblink_init();
+	conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
 
 	msg = PQerrorMessage(conn);
 	if (msg == NULL || msg[0] == '\0')
@@ -1384,22 +1373,19 @@ dblink_exec(PG_FUNCTION_ARGS)
 	PGconn	   *volatile conn = NULL;
 	volatile bool freeconn = false;
 
-	DBLINK_INIT;
+	dblink_init();
 
 	PG_TRY();
 	{
-		char	   *msg;
 		PGresult   *res = NULL;
-		char	   *connstr = NULL;
 		char	   *sql = NULL;
 		char	   *conname = NULL;
-		remoteConn *rconn = NULL;
 		bool		fail = true;	/* default to backward compatible behavior */
 
 		if (PG_NARGS() == 3)
 		{
 			/* must be text,text,bool */
-			DBLINK_GET_CONN;
+			dblink_get_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)), &conn, &conname, &freeconn);
 			sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
 			fail = PG_GETARG_BOOL(2);
 		}
@@ -1414,7 +1400,7 @@ dblink_exec(PG_FUNCTION_ARGS)
 			}
 			else
 			{
-				DBLINK_GET_CONN;
+				dblink_get_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)), &conn, &conname, &freeconn);
 				sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
 			}
 		}
@@ -1429,7 +1415,7 @@ dblink_exec(PG_FUNCTION_ARGS)
 			elog(ERROR, "wrong number of arguments");
 
 		if (!conn)
-			DBLINK_CONN_NOT_AVAIL;
+			dblink_conn_not_avail(conname);
 
 		res = PQexec(conn, sql);
 		if (!res ||
@@ -1880,9 +1866,7 @@ PG_FUNCTION_INFO_V1(dblink_get_notify);
 Datum
 dblink_get_notify(PG_FUNCTION_ARGS)
 {
-	char	   *conname = NULL;
-	PGconn	   *conn = NULL;
-	remoteConn *rconn = NULL;
+	PGconn	   *conn;
 	PGnotify   *notify;
 	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
 	TupleDesc	tupdesc;
@@ -1892,9 +1876,9 @@ dblink_get_notify(PG_FUNCTION_ARGS)
 
 	prepTuplestoreResult(fcinfo);
 
-	DBLINK_INIT;
+	dblink_init();
 	if (PG_NARGS() == 1)
-		DBLINK_GET_NAMED_CONN;
+		conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
 	else
 		conn = pconn->conn;
 
@@ -2698,10 +2682,10 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
 	else
 		sqlstate = ERRCODE_CONNECTION_FAILURE;
 
-	xpstrdup(message_primary, pg_diag_message_primary);
-	xpstrdup(message_detail, pg_diag_message_detail);
-	xpstrdup(message_hint, pg_diag_message_hint);
-	xpstrdup(message_context, pg_diag_context);
+	message_primary = xpstrdup(pg_diag_message_primary);
+	message_detail = xpstrdup(pg_diag_message_detail);
+	message_hint = xpstrdup(pg_diag_message_hint);
+	message_context = xpstrdup(pg_diag_context);
 
 	/*
 	 * If we don't get a message from the PGresult, try the PGconn.  This
diff --git a/contrib/dblink/dblink.h b/contrib/dblink/dblink.h
deleted file mode 100644
index c96dbe28a8..0000000000
--- a/contrib/dblink/dblink.h
+++ /dev/null
@@ -1,39 +0,0 @@
-/*
- * dblink.h
- *
- * Functions returning results from a remote database
- *
- * Joe Conway <mail@joeconway.com>
- * And contributors:
- * Darko Prenosil <Darko.Prenosil@finteh.hr>
- * Shridhar Daithankar <shridhar_daithankar@persistent.co.in>
- *
- * contrib/dblink/dblink.h
- * Copyright (c) 2001-2017, PostgreSQL Global Development Group
- * ALL RIGHTS RESERVED;
- *
- * Permission to use, copy, modify, and distribute this software and its
- * documentation for any purpose, without fee, and without a written agreement
- * is hereby granted, provided that the above copyright notice and this
- * paragraph and the following two paragraphs appear in all copies.
- *
- * IN NO EVENT SHALL THE AUTHOR OR DISTRIBUTORS BE LIABLE TO ANY PARTY FOR
- * DIRECT, INDIRECT, SPECIAL, INCIDENTAL, OR CONSEQUENTIAL DAMAGES, INCLUDING
- * LOST PROFITS, ARISING OUT OF THE USE OF THIS SOFTWARE AND ITS
- * DOCUMENTATION, EVEN IF THE AUTHOR OR DISTRIBUTORS HAVE BEEN ADVISED OF THE
- * POSSIBILITY OF SUCH DAMAGE.
- *
- * THE AUTHOR AND DISTRIBUTORS SPECIFICALLY DISCLAIMS ANY WARRANTIES,
- * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY
- * AND FITNESS FOR A PARTICULAR PURPOSE.  THE SOFTWARE PROVIDED HEREUNDER IS
- * ON AN "AS IS" BASIS, AND THE AUTHOR AND DISTRIBUTORS HAS NO OBLIGATIONS TO
- * PROVIDE MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
- *
- */
-
-#ifndef DBLINK_H
-#define DBLINK_H
-
-#include "fmgr.h"
-
-#endif   /* DBLINK_H */
-- 
2.12.0

#2Corey Huinker
corey.huinker@gmail.com
In reply to: Peter Eisentraut (#1)
Re: some dblink refactoring

On Tue, Feb 28, 2017 at 10:09 PM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

Here is a patch to refactor some macro hell in dblink.

This patch was discussed in the background sessions thread as a
prerequisite for some work there, but I figure I'll make a separate
thread for it to give everyone interested in dblink a chance to respond
separate from the other thread.

+1

Any chance we can make get_connect_string() a core function or at least
externally accessible?

#3Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Corey Huinker (#2)
Re: some dblink refactoring

On 2/28/17 22:22, Corey Huinker wrote:

Any chance we can make get_connect_string() a core function or at least
externally accessible?

[get_connect_string() gets the connection string for a foreign server]

The connection string for a foreign server depends on the nature of the
foreign server. dblink can assume it's a PostgreSQL server, but it's
not clear how to generalize that.

Some kind of node or connection registry (i.e., "native" servers) might
be a better feature to think about here.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Tsunakawa, Takayuki
tsunakawa.takay@jp.fujitsu.com
In reply to: Peter Eisentraut (#1)
Re: some dblink refactoring

From: pgsql-hackers-owner@postgresql.org

[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Peter Eisentraut
Here is a patch to refactor some macro hell in dblink.

This patch was discussed in the background sessions thread as a prerequisite
for some work there, but I figure I'll make a separate thread for it to
give everyone interested in dblink a chance to respond separate from the
other thread.

I changed the status to ready for committer. The patch applied cleanly, passed the regression test (make installcheck in contrib/dblink/), and the code looks perfect.

How about applying the attached small patch for another refactoring? This merely changes makeStringInfo() to initStringInfo() at two sites just other places in the same file. makeStringInfo() on the function local variables leaves memory for StringInfoData allocated unnecessarily (which may be automatically reclaimed some time after.)

Regards
Takayuki Tsunakawa

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Tsunakawa, Takayuki
tsunakawa.takay@jp.fujitsu.com
In reply to: Tsunakawa, Takayuki (#4)
1 attachment(s)
Re: some dblink refactoring

From: pgsql-hackers-owner@postgresql.org

[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tsunakawa,
Takayuki
How about applying the attached small patch for another refactoring? This
merely changes makeStringInfo() to initStringInfo() at two sites just other
places in the same file. makeStringInfo() on the function local variables
leaves memory for StringInfoData allocated unnecessarily (which may be
automatically reclaimed some time after.)

Sorry, I forgot to attach the file.

Regards
Takayuki Tsunakawa

Attachments:

dblink_strinfo_refactor.patchapplication/octet-stream; name=dblink_strinfo_refactor.patchDownload
diff -rc a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
*** a/contrib/dblink/dblink.c	2017-03-08 13:44:16.000000000 +0900
--- b/contrib/dblink/dblink.c	2017-03-08 13:57:06.000000000 +0900
***************
*** 2721,2733 ****
  	ForeignServer *foreign_server = NULL;
  	UserMapping *user_mapping;
  	ListCell   *cell;
! 	StringInfo	buf = makeStringInfo();
  	ForeignDataWrapper *fdw;
  	AclResult	aclresult;
  	char	   *srvname;
  
  	static const PQconninfoOption *options = NULL;
  
  	/*
  	 * Get list of valid libpq options.
  	 *
--- 2721,2735 ----
  	ForeignServer *foreign_server = NULL;
  	UserMapping *user_mapping;
  	ListCell   *cell;
! 	StringInfoData	buf;
  	ForeignDataWrapper *fdw;
  	AclResult	aclresult;
  	char	   *srvname;
  
  	static const PQconninfoOption *options = NULL;
  
+ 	initStringInfo(&buf);
+ 
  	/*
  	 * Get list of valid libpq options.
  	 *
***************
*** 2769,2775 ****
  			DefElem    *def = lfirst(cell);
  
  			if (is_valid_dblink_option(options, def->defname, ForeignDataWrapperRelationId))
! 				appendStringInfo(buf, "%s='%s' ", def->defname,
  								 escape_param_str(strVal(def->arg)));
  		}
  
--- 2771,2777 ----
  			DefElem    *def = lfirst(cell);
  
  			if (is_valid_dblink_option(options, def->defname, ForeignDataWrapperRelationId))
! 				appendStringInfo(&buf, "%s='%s' ", def->defname,
  								 escape_param_str(strVal(def->arg)));
  		}
  
***************
*** 2778,2784 ****
  			DefElem    *def = lfirst(cell);
  
  			if (is_valid_dblink_option(options, def->defname, ForeignServerRelationId))
! 				appendStringInfo(buf, "%s='%s' ", def->defname,
  								 escape_param_str(strVal(def->arg)));
  		}
  
--- 2780,2786 ----
  			DefElem    *def = lfirst(cell);
  
  			if (is_valid_dblink_option(options, def->defname, ForeignServerRelationId))
! 				appendStringInfo(&buf, "%s='%s' ", def->defname,
  								 escape_param_str(strVal(def->arg)));
  		}
  
***************
*** 2788,2798 ****
  			DefElem    *def = lfirst(cell);
  
  			if (is_valid_dblink_option(options, def->defname, UserMappingRelationId))
! 				appendStringInfo(buf, "%s='%s' ", def->defname,
  								 escape_param_str(strVal(def->arg)));
  		}
  
! 		return buf->data;
  	}
  	else
  		return NULL;
--- 2790,2800 ----
  			DefElem    *def = lfirst(cell);
  
  			if (is_valid_dblink_option(options, def->defname, UserMappingRelationId))
! 				appendStringInfo(&buf, "%s='%s' ", def->defname,
  								 escape_param_str(strVal(def->arg)));
  		}
  
! 		return buf.data;
  	}
  	else
  		return NULL;
***************
*** 2807,2822 ****
  escape_param_str(const char *str)
  {
  	const char *cp;
! 	StringInfo	buf = makeStringInfo();
  
  	for (cp = str; *cp; cp++)
  	{
  		if (*cp == '\\' || *cp == '\'')
! 			appendStringInfoChar(buf, '\\');
! 		appendStringInfoChar(buf, *cp);
  	}
  
! 	return buf->data;
  }
  
  /*
--- 2809,2826 ----
  escape_param_str(const char *str)
  {
  	const char *cp;
! 	StringInfoData	buf;
! 
! 	initStringInfo(&buf);
  
  	for (cp = str; *cp; cp++)
  	{
  		if (*cp == '\\' || *cp == '\'')
! 			appendStringInfoChar(&buf, '\\');
! 		appendStringInfoChar(&buf, *cp);
  	}
  
! 	return buf.data;
  }
  
  /*
#6Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Tsunakawa, Takayuki (#4)
Re: some dblink refactoring

On 3/8/17 00:10, Tsunakawa, Takayuki wrote:

I changed the status to ready for committer. The patch applied cleanly, passed the regression test (make installcheck in contrib/dblink/), and the code looks perfect.

How about applying the attached small patch for another refactoring? This merely changes makeStringInfo() to initStringInfo() at two sites just other places in the same file. makeStringInfo() on the function local variables leaves memory for StringInfoData allocated unnecessarily (which may be automatically reclaimed some time after.)

Committed both. Thanks.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers