Re: [HACKERS] some dblink refactoring

[Peter Eisentraut]

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

Re: [HACKERS] some dblink refactoring

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@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




dblink_strinfo_refactor.patch
Description: dblink_strinfo_refactor.patch

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

Re: [HACKERS] some dblink refactoring

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@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

Re: [HACKERS] some dblink refactoring

[Peter Eisentraut]

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

Re: [HACKERS] some dblink refactoring

[Corey Huinker]

On Tue, Feb 28, 2017 at 10:09 PM, Peter Eisentraut <
peter.eisentr...@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?

[HACKERS] some dblink refactoring

[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.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 2e1fc0b0c50452bac91461a2317c28a8718fe89f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
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 =