dblink performance regression
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
I was recently approached by someone with a dblink performance
regression, going back to somewhere between 8.3 (good) and 8.4 (bad).
They were measuring ~8% degradation in dblink connection speed. I was
able to replicate on my own hardware with the following script:
8<------------------------------
create or replace function test_dblink(loops int)
returns text as $$
declare
i int;
ret int;
begin
for i in 1..loops loop
IF i % 100 = 0 THEN
RAISE NOTICE 'connect attempt %', i;
END IF;
PERFORM dblink_connect('me','dbname=postgres host=w.x.y.z');
SELECT x into ret FROM dblink('me','SELECT 1', true) as x(x int);
PERFORM dblink_disconnect('me');
end loop;
return 'done';
end;
$$ language plpgsql;
\timing
select test_dblink(1000);
8<------------------------------
In my testing I saw a very consistent 4-5% degradation. I then did a
git bisect and traced the performance degradation to this commit:
- ------------------
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5de601267d98c5d60df6de8d436685c7105d149
committer Joe Conway <mail@joeconway.com>
Tue, 9 Jun 2009 16:35:36 +0000 (16:35 +0000)
commit e5de601267d98c5d60df6de8d436685c7105d149
"Default client encoding to server encoding for dblink connections.
Addresses issue raised by Ruzsinszky Attila and confirmed by others."
- ------------------
with this diff:
- ------------------
http://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=contrib/dblink/dblink.c;h=e709ae9cc3b9f7177eaae08d11540a8590e7dc63;hp=283b4d0b25e9924dfc2d16075e7cfc1277ce5956;hb=e5de601267d98c5d60df6de8d436685c7105d149;hpb=adaf60131f81394939b5b55f74130343d520cd2d
- ------------------
Apparently setting client encoding is an expensive operation.
My proposed fix (diff attached) is to simply skip setting client
encoding in the case where it already matches server side encoding.
With the patch applied, and client encoding matching server (both
UTF8), the performance regression is completely gone.
Possibly additional work could go into determining if
PQsetClientEncoding() can be made more efficient, but I believe it
still makes sense in the case of dblink to do nothing when nothing is
required.
If there are no objections I'll commit this to all branches this weekend.
Thanks,
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJSoTZ9AAoJEDfy90M199hlVVsP/iG6XbcXYR2G2TC9I6FIaYzQ
c9503QcewPW+tY/0TAhzOYbXfdkkmRDf+4d9HYiCnYANhJW86dxmqtFKMyASbT74
BYjrZUHhafhSobDZBtM0t/6J1CMhLB/Gy66dziqkw0/hx0NeSuQqkluDazcIh/83
34hCRB2LNE7CXx88HvIc0hd6ACk7Ecrw7W6xyoznhmajHnq2G4Mp1AKXSkOTsYcJ
GkKy1G+u42M6pcBaNp6hnPuRj3HGECz6NdeuzWvb2IKLL6cY7C0fvCccqvkhcUpl
SPfIzlzxxtavzkkkxjOQUWrUFVWulZjeTFsQz3AWPQZoLtY+YDJ513R16Jh8s4RI
XoUWXRQVcMS2kWAIck6Nt/2zpIN9Rr4MlUgqh4mXlAZ59ErigVAqbhg9SAE0N4/h
Fa31OlTousTT4Wph34n/2nZK3uF46SiOHAoFipjRtGavbFW4lXKFryz6AEJ6e1Xy
fNpfNrXbKFkmRYdcafjZ7k6+NW70iCcH98A78wgyRLy59/b/M3u/K9TH5YN5tKLH
O+fK+S+s6eA9+omeu2hLl+6CkwDvEfBvzKkLu+9+sdV0s0b7VDt2vnMx/hg6E7EG
wCKB5X451lUz2MhXqX8vKiSGLk2ShmV8o8u0ovh4kFRV4YmjPK7LyDenIdmsoYuQ
NHiORCMc3V9USObJJ8so
=E/Hx
-----END PGP SIGNATURE-----
Attachments:
fix-dblink-encoding-regression-8.4.difftext/x-patch; name=fix-dblink-encoding-regression-8.4.diffDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index cc8714d..85b058b 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -191,7 +191,8 @@ typedef struct remoteConnHashEnt
errdetail("%s", msg))); \
} \
dblink_security_check(conn, rconn); \
- PQsetClientEncoding(conn, GetDatabaseEncodingName()); \
+ if (strcmp(pg_encoding_to_char(PQclientEncoding(conn)), GetDatabaseEncodingName()) != 0) \
+ PQsetClientEncoding(conn, GetDatabaseEncodingName()); \
freeconn = true; \
} \
} while (0)
@@ -271,7 +272,8 @@ dblink_connect(PG_FUNCTION_ARGS)
dblink_security_check(conn, rconn);
/* attempt to set client encoding to match server encoding */
- PQsetClientEncoding(conn, GetDatabaseEncodingName());
+ if (strcmp(pg_encoding_to_char(PQclientEncoding(conn)), GetDatabaseEncodingName()) != 0)
+ PQsetClientEncoding(conn, GetDatabaseEncodingName());
if (connname)
{
Joe Conway <mail@joeconway.com> writes:
Apparently setting client encoding is an expensive operation.
Yeah, it requires sending a command to the remote end. I'm not
sure if it'd be a good idea for PQsetClientEncoding to try to
skip the operation when the client encoding is already the
right thing. The given name might not be spelled canonically,
for one thing.
My proposed fix (diff attached) is to simply skip setting client
encoding in the case where it already matches server side encoding.
I have to think that we shouldn't need a string compare to figure out
if one integer encoding ID is equal to another. Why not just compare
PQclientEncoding(conn) to GetDatabaseEncoding()? Otherwise, seems
reasonable.
Actually ... there's an interesting conflict here. What if libpq hasn't
got the same encoding ID number assignments as the backend? I suspect
that the proposed code is actually wrong, or at least risks being wrong,
because it's far from clear whether the linker will resolve
pg_encoding_to_char() as being libpq's version or the backend's version.
The former choice would cause things to work, as long as the textual names
agree, but the latter choice would be just as broken as if we'd simply
compared the integer IDs.
I seem to remember that at some point we realized that the encoding ID
assignments are part of libpq's ABI and so can't practically be changed
ever, so the above may be moot. Even so, I think it's a bad idea to be
depending on pg_encoding_to_char() here, given the ambiguity in what it
references. It would be unsurprising to get build-time or run-time
failures on pickier platforms, as a consequence of that ambiguity. So
I'd still recommend comparing integer IDs as above, rather than this.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 12/05/2013 06:53 PM, Tom Lane wrote:
I seem to remember that at some point we realized that the encoding
ID assignments are part of libpq's ABI and so can't practically be
changed ever, so the above may be moot. Even so, I think it's a
bad idea to be depending on pg_encoding_to_char() here, given the
ambiguity in what it references. It would be unsurprising to get
build-time or run-time failures on pickier platforms, as a
consequence of that ambiguity. So I'd still recommend comparing
integer IDs as above, rather than this.
Great feedback as always -- thanks! Will make that change.
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJSoT7qAAoJEDfy90M199hlDqUP/jyyEQhBFBLK4AosiQnnRZuc
O55j4gFGIb9jIs1nRI+ndPon0gGW2LHWuxh7sUqnpkXSmRzn07A2t8Lj+5dqEX4+
1fz7Ty/2K60Ge7klZGz0OK9YuGZLlLMLUeJJmzJaGKy+zKOv51ceS1YuhPM/Y8a6
nI1dfMxJubbVV9rYzg+B0fLcC6ygTI2fIgurcUlq1/YgbwaX3ca+yOLfXhimnEWn
pAn4sqUznzn9uTKpxCIP2MkEslKRPWj94ocAtl5/eASA0FiRbAnQFw8Rwo8D9E/c
5WnRUUYYbPK1LfJHo8Qw++XnzBQdJc8/uY3aCbBMq7mFgha+ZqFnToTO5APmoU3C
xEOM1ZY7B6Y2y00hIUrmrFKrj6RRFq1/RNmvBridKwcH9W+jV+DsmgNVfwwd/2fZ
2toRxQ/cAEp+EAmlv/9mjc5KK6rSkCtGv3X5jSqOejBmIiZ+0UJm6JhduZyPTcEg
B/WfgM5UZ7O5QQCseDX80RPr9waAwL2aH/sjMSCXPNMH1pkSL1wda5Tl6DHvKEQ3
1T7F/moW8ne9Iece6uJjVBT33N8YEiUord8m3LTdCC5MWjr17hI4hV6Ixe0cVZXZ
97OwQtliLejSKg2mWOAmTFdDhJ6JmKmMOp/GtQk46ZbBwYWzD4fBsy5Pg2cayQtX
c+gK+aZA3WI7O4pgWIxx
=NXY7
-----END PGP SIGNATURE-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 6, 2013 at 1:05 AM, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 12/05/2013 06:53 PM, Tom Lane wrote:
I seem to remember that at some point we realized that the encoding
ID assignments are part of libpq's ABI and so can't practically be
changed ever, so the above may be moot. Even so, I think it's a
bad idea to be depending on pg_encoding_to_char() here, given the
ambiguity in what it references. It would be unsurprising to get
build-time or run-time failures on pickier platforms, as a
consequence of that ambiguity. So I'd still recommend comparing
integer IDs as above, rather than this.Great feedback as always -- thanks! Will make that change.
Hi Joe, how are you?
Well, when Tom sent this email I was reviewing your patch and the main
suggestion is about use of 'pg_encoding_to_char' too... ;-)
The attached patch with my review!
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
Timbira: http://www.timbira.com.br
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
Attachments:
fix-dblink-encoding-regression-8.4-v2.difftext/plain; charset=US-ASCII; name=fix-dblink-encoding-regression-8.4-v2.diffDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d68b12a..c358734 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -99,6 +99,7 @@ static char *generate_relation_name(Relation rel);
static void dblink_connstr_check(const char *connstr);
static void dblink_security_check(PGconn *conn, remoteConn *rconn);
static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
+static void dblink_set_client_encoding(PGconn *conn);
static char *get_connect_string(const char *servername);
static char *escape_param_str(const char *from);
static void validate_pkattnums(Relation rel,
@@ -190,7 +191,7 @@ typedef struct remoteConnHashEnt
errdetail("%s", msg))); \
} \
dblink_security_check(conn, rconn); \
- PQsetClientEncoding(conn, GetDatabaseEncodingName()); \
+ dblink_set_client_encoding(conn); \
freeconn = true; \
} \
} while (0)
@@ -270,7 +271,7 @@ dblink_connect(PG_FUNCTION_ARGS)
dblink_security_check(conn, rconn);
/* attempt to set client encoding to match server encoding */
- PQsetClientEncoding(conn, GetDatabaseEncodingName());
+ dblink_set_client_encoding(conn);
if (connname)
{
@@ -2328,6 +2329,13 @@ dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_
dblink_context_conname, dblink_context_msg)));
}
+static void
+dblink_set_client_encoding(PGconn *conn)
+{
+ if (PQclientEncoding(conn) != GetDatabaseEncoding())
+ PQsetClientEncoding(conn, GetDatabaseEncodingName());
+}
+
/*
* Obtain connection string for a foreign server
*/
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 12/05/2013 07:16 PM, Fabr�zio de Royes Mello wrote:
Hi Joe, how are you?
Hi Fabrizio -- great to hear from you! I'm well.
Well, when Tom sent this email I was reviewing your patch and the
main suggestion is about use of 'pg_encoding_to_char' too... ;-)The attached patch with my review!
Awesome, thanks for the review!
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJSoVeqAAoJEDfy90M199hlDqYQAKfYRX52AGHdd2eB/JT6hmXJ
TEYWENz65Cew9t0bPn2XKAHOkpAGf6JFT/XtloKCKwgF/nE0SIB1ETXiD7DVgIFU
EeMvyFnIkfg+wKehdhATQ6c7gpmQbNqs1DPZby7O5wQFyUFZB5Y7Tz7bUyZHPjTE
ml+GUkoulj3yYjC5Uf0q79k05karXOZS5V8jxjFig+nU2kE4wgpZ7BKUQXzAVr2C
7XCGdZXQ9fewE5CXKkiRJNsp9Si0PF0ahPeP7hZ/Jd4iWDibcv+ouhhStHZLrDY5
NjGR6BoaX/pASPU2lIGbkT/xEhp3ShMtn1FHnoYDqhcp3F5DraAJzS8Rr/eP9iZO
ks8aI0tYv2nlTwm+BiwRP+oTlSfQHlCyHN+3bMFTev3L6DmB/0kp/c/RPq4NYJB4
T2Khkq/+hFXXz01PK0l95O84m2o+zTpIefbbhY/xBrv+/+caDaTdJyID26G3ULJo
BmZv7jQlmUl53JBX5J4uHxhf8ChB7wEA8nkLfNuNnDDXUUjS0DL4oi8AX24X09w4
ac0KmK2yYI4R2FjrNvXNRvVnatEuAiGqbnldBO5YFLDmMKdI8Nq3EGf2ELphBfdZ
qDL2A+qvToJKj3iHYCQdnCZLmfH9wdxcDvZ6QgPJR/sRgHWvumCr3jxzNN219Lkw
DgQI8Dud1V4GCMx4dH6K
=VVw2
-----END PGP SIGNATURE-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 6, 2013 at 2:50 AM, Joe Conway <mail@joeconway.com> wrote:
On 12/05/2013 07:16 PM, Fabrízio de Royes Mello wrote:
Hi Joe, how are you?
Hi Fabrizio -- great to hear from you! I'm well.
:-)
Well, when Tom sent this email I was reviewing your patch and the
main suggestion is about use of 'pg_encoding_to_char' too... ;-)The attached patch with my review!
Awesome, thanks for the review!
You're welcome!
Greetings,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
Timbira: http://www.timbira.com.br
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 12/05/2013 07:05 PM, Joe Conway wrote:
On 12/05/2013 06:53 PM, Tom Lane wrote:
I seem to remember that at some point we realized that the
encoding ID assignments are part of libpq's ABI and so can't
practically be changed ever, so the above may be moot. Even so,
I think it's a bad idea to be depending on pg_encoding_to_char()
here, given the ambiguity in what it references. It would be
unsurprising to get build-time or run-time failures on pickier
platforms, as a consequence of that ambiguity. So I'd still
recommend comparing integer IDs as above, rather than this.Great feedback as always -- thanks! Will make that change.
Committed to all active branches.
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJSo8ZZAAoJEDfy90M199hlCz4P/R9ngR28e3nPNOxpkKO8R2oE
t32gPhVP2fLhTstumolJHvkAQh8d+7em8aDT8lwGQ6a1DNGtwlJ2cXR64yjI9SXg
Vp4tJyuliZau7kitiDDtGXqbEyM+cCWJ8wFnckToERJtW2uXcC81kqGoac7lz1e9
o34UKizzD8PLA+N6TCG6GsOx8/W2kKEgru8up9jbUzuJDEmzUPkSn8rA2jHVl7oX
LKF0hPcFuNJept9A/iNZmBfPgXUrHL/4s9qJ0UQdzzcJDHZ3kalTgRKs9y035WXl
XQ/YbYry8/Qw5QgWz9g50/FDhK7jAP/ZBGU99lBGO9e4hxV2lTeeiz177hYzgckF
vqWNS/dKN3wtdtXmEwhwmyLodpJKJhLFWOsgfpRPPZZu5Ji+gwPYl3MQHY4THqMp
8kjzK/BL94C15NSCO5E5FBM3CGruYWPVzNZqS4PT+VqCupZ2+qxySgL/0riMbx+J
GmBF/C9EGQzrRq7idz8O7/7Frb9TwjBgj+I0wns5NKF1vJpJ2fbzafmvpLkgJ/oI
bGhJt43D645BgnAKrInIq6mMbsYAr10pK6v03gJhiJUZREeu9wX5pVIzFV1R1PgC
ZS7evjftSP5MwdGs8geEVcQKbx42jW+kQos/HKH6saLKKf5s+cdXNiXaCOj/8cI7
1TZWt31Z9PqS7FuSDkXR
=AWVh
-----END PGP SIGNATURE-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Dec 7, 2013 at 11:07 PM, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 12/05/2013 07:05 PM, Joe Conway wrote:
On 12/05/2013 06:53 PM, Tom Lane wrote:
I seem to remember that at some point we realized that the
encoding ID assignments are part of libpq's ABI and so can't
practically be changed ever, so the above may be moot. Even so,
I think it's a bad idea to be depending on pg_encoding_to_char()
here, given the ambiguity in what it references. It would be
unsurprising to get build-time or run-time failures on pickier
platforms, as a consequence of that ambiguity. So I'd still
recommend comparing integer IDs as above, rather than this.Great feedback as always -- thanks! Will make that change.
Committed to all active branches.
IMHO is more elegant create a procedure to encapsulate the code to avoid
redundancy.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
Timbira: http://www.timbira.com.br
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
On Sun, Dec 8, 2013 at 10:16 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
On Sat, Dec 7, 2013 at 11:07 PM, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 12/05/2013 07:05 PM, Joe Conway wrote:
On 12/05/2013 06:53 PM, Tom Lane wrote:
I seem to remember that at some point we realized that the
encoding ID assignments are part of libpq's ABI and so can't
practically be changed ever, so the above may be moot. Even so,
I think it's a bad idea to be depending on pg_encoding_to_char()
here, given the ambiguity in what it references. It would be
unsurprising to get build-time or run-time failures on pickier
platforms, as a consequence of that ambiguity. So I'd still
recommend comparing integer IDs as above, rather than this.Great feedback as always -- thanks! Will make that change.
Committed to all active branches.
IMHO is more elegant create a procedure to encapsulate the code to avoid
redundancy.
Yep, perhaps something like PQsetClientEncodingIfDifferent or similar
would make sense.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
All,
I tested out Joe's original patch, and it does eliminate the 8%
performance regression.
Will try the new one.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM5811ea6954b3204ba41eb514a4232a6105a23ec61c14fb5c91418af27810dd1f98c9ac6a46018cec2a5e66ebafbd8da3@asav-3.01.com
On Sat, Dec 7, 2013 at 11:20 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
IMHO is more elegant create a procedure to encapsulate the code to avoid
redundancy.Yep, perhaps something like PQsetClientEncodingIfDifferent or similar
would make sense.
Well I think at this first moment we can just create a procedure inside the
dblink contrib and not touch in libpq.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
Timbira: http://www.timbira.com.br
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 12/07/2013 05:41 PM, Fabr�zio de Royes Mello wrote:
On Sat, Dec 7, 2013 at 11:20 PM, Michael Paquier
<michael.paquier@gmail.com <mailto:michael.paquier@gmail.com>>
wrote:IMHO is more elegant create a procedure to encapsulate the code
to avoid redundancy.Yep, perhaps something like PQsetClientEncodingIfDifferent or
similar would make sense.Well I think at this first moment we can just create a procedure
inside the dblink contrib and not touch in libpq.
Maybe a libpq function could be done for 9.4, but not for back branches.
I don't think it makes sense to create a new function in dblink either
- -- we're only talking about two lines of added redundancy which is
less lines of code than a new function would add. But if we create
PQsetClientEncodingIfDifferent() (or whatever) we can remove those
extra lines in 9.4 ;-)
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJSo9BnAAoJEDfy90M199hluqYP/RyoKh9nvC49H3xDl4wBLh4n
0OQ5nKJk8RkQ5d0MLbgn9t1xiQ+RltMcHQQEDoPlrn388DNTqOP31TqCHtI11S5l
ZOjZw6eKvcp0KEzk723kZCq9d2N1uRb95K2z5jFUXbeZ2pO6yj8ohdnh8J9i1VQE
iI5F76yeUJCO8YRmHMJs39Fy3Qtq0dsXesPYBRuEJqHy7cGh9hpYHPuqHFyW19Kg
0q1nPMa7TYpKRECa1wi+Gt2BJqd70AdnOZZipqqCR2bMJqmcpBy3jo94vedaarAz
Wtunn4mk0/2LCNsAjgAdA33FYNfRpgL2c99IQ1DK5hwW9mdH2WH6G+8Eaf5zGhps
ZWXJRQSYjfUCmMOoGudEKNX3H3prrNpqltQuC978i4ddKK/78yX1wJD10I8qVNHy
MRlSoTFfomVYPW0054Jk6LR1f/RKGD4yuiIPDv8dI/gWINT1HveBGkvSf9wnY578
wjh2iLJ790o4CNK/334ooggPnlbBS4yQ+e+xsDcaJ2pc1cWJr/gCUf3cliUtv+rI
MnFvsbq4vEjhBE3Tr6LYPwivCzKpiEz2L0/oO8sShHhm/dfr9UGPUyeDO43phP+m
NFQXoh6oCuleBXk/N874yAp9EDXtu3g9E1PUMMsbplXjiH6mLp8OWmRbvQ9Qw3zu
BFtOonWViuPz5ILObc3i
=o0XG
-----END PGP SIGNATURE-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Dec 7, 2013 at 11:41 PM, Fabrízio de Royes Mello <
fabriziomello@gmail.com> wrote:
On Sat, Dec 7, 2013 at 11:20 PM, Michael Paquier <
michael.paquier@gmail.com> wrote:
IMHO is more elegant create a procedure to encapsulate the code to
avoid
redundancy.
Yep, perhaps something like PQsetClientEncodingIfDifferent or similar
would make sense.Well I think at this first moment we can just create a procedure inside
the dblink contrib and not touch in libpq.
Something like the attached.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
Timbira: http://www.timbira.com.br
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
Attachments:
dblink_set_client_encoding_v1.patchtext/x-diff; charset=US-ASCII; name=dblink_set_client_encoding_v1.patchDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index a91a547..1feee22 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -112,6 +112,7 @@ static Relation get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclM
static char *generate_relation_name(Relation rel);
static void dblink_connstr_check(const char *connstr);
static void dblink_security_check(PGconn *conn, remoteConn *rconn);
+static void dblink_set_client_encoding(PGconn *conn);
static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
static char *get_connect_string(const char *servername);
static char *escape_param_str(const char *from);
@@ -209,8 +210,7 @@ typedef struct remoteConnHashEnt
errdetail_internal("%s", msg))); \
} \
dblink_security_check(conn, rconn); \
- if (PQclientEncoding(conn) != GetDatabaseEncoding()) \
- PQsetClientEncoding(conn, GetDatabaseEncodingName()); \
+ dblink_set_client_encoding(conn); \
freeconn = true; \
} \
} while (0)
@@ -290,8 +290,7 @@ dblink_connect(PG_FUNCTION_ARGS)
dblink_security_check(conn, rconn);
/* attempt to set client encoding to match server encoding, if needed */
- if (PQclientEncoding(conn) != GetDatabaseEncoding())
- PQsetClientEncoding(conn, GetDatabaseEncodingName());
+ dblink_set_client_encoding(conn);
if (connname)
{
@@ -2622,6 +2621,13 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
}
}
+static void
+dblink_set_client_encoding(PGconn *conn)
+{
+ if (PQclientEncoding(conn) != GetDatabaseEncoding())
+ PQsetClientEncoding(conn, GetDatabaseEncodingName());
+}
+
/*
* For non-superusers, insist that the connstr specify a password. This
* prevents a password from being picked up from .pgpass, a service file,
On 2013/12/08, at 10:50, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 12/07/2013 05:41 PM, Fabrízio de Royes Mello wrote:
On Sat, Dec 7, 2013 at 11:20 PM, Michael Paquier
<michael.paquier@gmail.com <mailto:michael.paquier@gmail.com>>
wrote:IMHO is more elegant create a procedure to encapsulate the code
to avoid redundancy.Yep, perhaps something like PQsetClientEncodingIfDifferent or
similar would make sense.Well I think at this first moment we can just create a procedure
inside the dblink contrib and not touch in libpq.Maybe a libpq function could be done for 9.4, but not for back branches.
Agreed as this would change the spec of libpq between minor releases. Sorry for not being clear myself.
-- we're only talking about two lines of added redundancy which is
less lines of code than a new function would add. But if we create
PQsetClientEncodingIfDifferent() (or whatever) we can remove those
extra lines in 9.4 ;-)
--
Michael
(Sent from my mobile phone)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Joe Conway <mail@joeconway.com> writes:
I don't think it makes sense to create a new function in dblink either
-- we're only talking about two lines of added redundancy which is
less lines of code than a new function would add.
Indeed, and I think the claim that such a function "encapsulates" anything
useful is pretty silly as well. I think the committed patch is fine.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/7/13 7:50 PM, Joe Conway wrote:
On 12/07/2013 05:41 PM, Fabr�zio de Royes Mello wrote:
On Sat, Dec 7, 2013 at 11:20 PM, Michael Paquier
<michael.paquier@gmail.com <mailto:michael.paquier@gmail.com>>
wrote:IMHO is more elegant create a procedure to encapsulate the code
to avoid redundancy.Yep, perhaps something like PQsetClientEncodingIfDifferent or
similar would make sense.Well I think at this first moment we can just create a procedure
inside the dblink contrib and not touch in libpq.Maybe a libpq function could be done for 9.4, but not for back branches.
Stupid question... why don't we just pass encoding in with the other connection parameters? That eliminates any ambiguity. The only issue would be if the user also passed that in via connstr... though now that I think about it, we currently silently ignore that parameter, which IMHO is bad. We should either respect if the user passes that in (ie: not do anything at all), or we should throw an error.
--
Jim C. Nasby, Data Architect jim@nasby.net
512.569.9461 (cell) http://jim.nasby.net
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/07/2013 05:50 PM, Joe Conway wrote:
On 12/07/2013 05:41 PM, Fabrízio de Royes Mello wrote:
On Sat, Dec 7, 2013 at 11:20 PM, Michael Paquier
<michael.paquier@gmail.com <mailto:michael.paquier@gmail.com>>
wrote:IMHO is more elegant create a procedure to encapsulate the code
to avoid redundancy.Yep, perhaps something like PQsetClientEncodingIfDifferent or
similar would make sense.Well I think at this first moment we can just create a procedure
inside the dblink contrib and not touch in libpq.Maybe a libpq function could be done for 9.4, but not for back branches.
I don't think it makes sense to create a new function in dblink either
-- we're only talking about two lines of added redundancy which is
less lines of code than a new function would add. But if we create
PQsetClientEncodingIfDifferent() (or whatever) we can remove those
extra lines in 9.4 ;-)
Hey, since we're about to do 9.3.3: was this patch ever committed?
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM3119fb4a0ea49e536f544c3822da81dd339559f53ff7c5d1878a96fc092a3c2dae8e7af49c20ec1a3af0588b8a4b4812@asav-3.01.com
yes
Joe
Sent with AquaMail for Android
http://www.aqua-mail.com
On January 16, 2014 2:32:55 PM Josh Berkus <josh@agliodbs.com> wrote:
On 12/07/2013 05:50 PM, Joe Conway wrote:
On 12/07/2013 05:41 PM, Fabrízio de Royes Mello wrote:
On Sat, Dec 7, 2013 at 11:20 PM, Michael Paquier >>
<michael.paquier@gmail.com <mailto:michael.paquier@gmail.com>>
wrote:
IMHO is more elegant create a procedure to encapsulate the code
to avoid redundancy.Yep, perhaps something like PQsetClientEncodingIfDifferent or
similar would make sense.
Well I think at this first moment we can just create a procedureinside the dblink contrib and not touch in libpq.
Maybe a libpq function could be done for 9.4, but not for back branches.
I don't think it makes sense to create a new function in dblink either
-- we're only talking about two lines of added redundancy which is
less lines of code than a new function would add. But if we create
PQsetClientEncodingIfDifferent() (or whatever) we can remove those
extra lines in 9.4 ;-)Hey, since we're about to do 9.3.3: was this patch ever committed?
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers