Remove repeated calls to PQserverVersion
Started by Peter Smithover 4 years ago6 messages
Hi,
I found a few functions making unnecessary repeated calls to
PQserverVersion(conn); instead of just calling once and assigning to a
local variable.
PSA a little patch which culls those extra calls.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Avoid-unnecessary-calls-to-PGserverVersion.patchapplication/octet-stream; name=v1-0001-Avoid-unnecessary-calls-to-PGserverVersion.patchDownload
From 634f1e47c51121dc754f651e8e57d8bd5d7a077c Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 13 Jul 2021 18:51:37 +1000
Subject: [PATCH v1] Avoid unnecessary calls to PGserverVersion.
Some functions were found to be making repeated calls to PQserverVersion(conn), instead of just calling once and assigning the result to a local variable. This patch culls those extra calls.
---
contrib/postgres_fdw/postgres_fdw.c | 6 ++++--
.../replication/libpqwalreceiver/libpqwalreceiver.c | 7 ++++---
src/bin/pg_basebackup/pg_basebackup.c | 15 ++++++++++-----
src/bin/scripts/reindexdb.c | 6 ++++--
src/bin/scripts/vacuumdb.c | 20 +++++++++++---------
src/fe_utils/string_utils.c | 9 +++++----
6 files changed, 38 insertions(+), 25 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index f15c97a..e0466c4 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5191,6 +5191,7 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
ForeignServer *server;
UserMapping *mapping;
PGconn *conn;
+ int server_version;
StringInfoData buf;
PGresult *volatile res = NULL;
int numrows,
@@ -5221,9 +5222,10 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
server = GetForeignServer(serverOid);
mapping = GetUserMapping(GetUserId(), server->serverid);
conn = GetConnection(mapping, false, NULL);
+ server_version = PQserverVersion(conn);
/* Don't attempt to import collation if remote server hasn't got it */
- if (PQserverVersion(conn) < 90100)
+ if (server_version < 90100)
import_collate = false;
/* Create workspace for strings */
@@ -5318,7 +5320,7 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
deparseStringLiteral(&buf, stmt->remote_schema);
/* Partitions are supported since Postgres 10 */
- if (PQserverVersion(conn) >= 100000 &&
+ if (server_version >= 100000 &&
stmt->list_type != FDW_IMPORT_SCHEMA_LIMIT_TO)
appendStringInfoString(&buf, " AND NOT c.relispartition ");
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 19ea159..111aba0 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -427,6 +427,7 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
char *pubnames_str;
List *pubnames;
char *pubnames_literal;
+ int server_version = PQserverVersion(conn->streamConn);
appendStringInfoString(&cmd, " (");
@@ -434,11 +435,11 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
options->proto.logical.proto_version);
if (options->proto.logical.streaming &&
- PQserverVersion(conn->streamConn) >= 140000)
+ server_version >= 140000)
appendStringInfoString(&cmd, ", streaming 'on'");
if (options->proto.logical.twophase &&
- PQserverVersion(conn->streamConn) >= 150000)
+ server_version >= 150000)
appendStringInfoString(&cmd, ", two_phase 'on'");
pubnames = options->proto.logical.publication_names;
@@ -460,7 +461,7 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
pfree(pubnames_str);
if (options->proto.logical.binary &&
- PQserverVersion(conn->streamConn) >= 140000)
+ server_version >= 140000)
appendStringInfoString(&cmd, ", binary 'true'");
appendStringInfoChar(&cmd, ')');
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8bb0acf..e08fcb9 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -597,6 +597,7 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
uint32 hi,
lo;
char statusdir[MAXPGPATH];
+ int server_version;
param = pg_malloc0(sizeof(logstreamer_param));
param->timeline = timeline;
@@ -628,14 +629,16 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
/* Error message already written in GetConnection() */
exit(1);
+ server_version = PQserverVersion(conn);
+
/* In post-10 cluster, pg_xlog has been renamed to pg_wal */
snprintf(param->xlog, sizeof(param->xlog), "%s/%s",
basedir,
- PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
+ server_version < MINIMUM_VERSION_FOR_PG_WAL ?
"pg_xlog" : "pg_wal");
/* Temporary replication slots are only supported in 10 and newer */
- if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_TEMP_SLOTS)
+ if (server_version < MINIMUM_VERSION_FOR_TEMP_SLOTS)
temp_replication_slot = false;
/*
@@ -670,7 +673,7 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier)
*/
snprintf(statusdir, sizeof(statusdir), "%s/%s/archive_status",
basedir,
- PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
+ server_version < MINIMUM_VERSION_FOR_PG_WAL ?
"pg_xlog" : "pg_wal");
if (pg_mkdir_p(statusdir, pg_dir_create_mode) != 0 && errno != EEXIST)
@@ -2262,6 +2265,7 @@ main(int argc, char **argv)
int c;
int option_index;
+ int server_version;
pg_logging_init(argv[0]);
progname = get_progname(argv[0]);
@@ -2595,6 +2599,7 @@ main(int argc, char **argv)
exit(1);
}
atexit(disconnect_atexit);
+ server_version = PQserverVersion(conn);
/*
* Set umask so that directories/files are created with the same
@@ -2607,7 +2612,7 @@ main(int argc, char **argv)
umask(pg_mode_mask);
/* Backup manifests are supported in 13 and newer versions */
- if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_MANIFESTS)
+ if (server_version < MINIMUM_VERSION_FOR_MANIFESTS)
manifest = false;
/*
@@ -2634,7 +2639,7 @@ main(int argc, char **argv)
* renamed to pg_wal in post-10 clusters.
*/
linkloc = psprintf("%s/%s", basedir,
- PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ?
+ server_version < MINIMUM_VERSION_FOR_PG_WAL ?
"pg_xlog" : "pg_wal");
#ifdef HAVE_SYMLINK
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index fc06815..52f9b48 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -344,10 +344,12 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
ParallelSlotArray *sa;
bool failed = false;
int items_count = 0;
+ int server_version;
conn = connectDatabase(cparams, progname, echo, false, false);
+ server_version = PQserverVersion(conn);
- if (concurrently && PQserverVersion(conn) < 120000)
+ if (concurrently && server_version < 120000)
{
PQfinish(conn);
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
@@ -355,7 +357,7 @@ reindex_one_database(ConnParams *cparams, ReindexType type,
exit(1);
}
- if (tablespace && PQserverVersion(conn) < 140000)
+ if (tablespace && server_version < 140000)
{
PQfinish(conn);
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 61974ba..df5d4c7 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -448,6 +448,7 @@ vacuum_one_database(ConnParams *cparams,
bool failed = false;
bool tables_listed = false;
bool has_where = false;
+ int server_version;
const char *initcmd;
const char *stage_commands[] = {
"SET default_statistics_target=1; SET vacuum_cost_delay=0;",
@@ -464,8 +465,9 @@ vacuum_one_database(ConnParams *cparams,
(stage >= 0 && stage < ANALYZE_NUM_STAGES));
conn = connectDatabase(cparams, progname, echo, false, true);
+ server_version = PQserverVersion(conn);
- if (vacopts->disable_page_skipping && PQserverVersion(conn) < 90600)
+ if (vacopts->disable_page_skipping && server_version < 90600)
{
PQfinish(conn);
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
@@ -473,7 +475,7 @@ vacuum_one_database(ConnParams *cparams,
exit(1);
}
- if (vacopts->no_index_cleanup && PQserverVersion(conn) < 120000)
+ if (vacopts->no_index_cleanup && server_version < 120000)
{
PQfinish(conn);
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
@@ -481,7 +483,7 @@ vacuum_one_database(ConnParams *cparams,
exit(1);
}
- if (vacopts->force_index_cleanup && PQserverVersion(conn) < 120000)
+ if (vacopts->force_index_cleanup && server_version < 120000)
{
PQfinish(conn);
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
@@ -489,7 +491,7 @@ vacuum_one_database(ConnParams *cparams,
exit(1);
}
- if (!vacopts->do_truncate && PQserverVersion(conn) < 120000)
+ if (!vacopts->do_truncate && server_version < 120000)
{
PQfinish(conn);
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
@@ -497,7 +499,7 @@ vacuum_one_database(ConnParams *cparams,
exit(1);
}
- if (!vacopts->process_toast && PQserverVersion(conn) < 140000)
+ if (!vacopts->process_toast && server_version < 140000)
{
PQfinish(conn);
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
@@ -505,7 +507,7 @@ vacuum_one_database(ConnParams *cparams,
exit(1);
}
- if (vacopts->skip_locked && PQserverVersion(conn) < 120000)
+ if (vacopts->skip_locked && server_version < 120000)
{
PQfinish(conn);
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
@@ -513,21 +515,21 @@ vacuum_one_database(ConnParams *cparams,
exit(1);
}
- if (vacopts->min_xid_age != 0 && PQserverVersion(conn) < 90600)
+ if (vacopts->min_xid_age != 0 && server_version < 90600)
{
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
"--min-xid-age", "9.6");
exit(1);
}
- if (vacopts->min_mxid_age != 0 && PQserverVersion(conn) < 90600)
+ if (vacopts->min_mxid_age != 0 && server_version < 90600)
{
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
"--min-mxid-age", "9.6");
exit(1);
}
- if (vacopts->parallel_workers >= 0 && PQserverVersion(conn) < 130000)
+ if (vacopts->parallel_workers >= 0 && server_version < 130000)
{
pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
"--parallel", "13");
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 3efee4e..222845d 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -832,6 +832,7 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
PQExpBufferData schemabuf;
PQExpBufferData namebuf;
bool added_clause = false;
+ int server_version = PQserverVersion(conn);
#define WHEREAND() \
(appendPQExpBufferStr(buf, have_where ? " AND " : "WHERE "), \
@@ -883,13 +884,13 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
appendPQExpBuffer(buf,
"(%s OPERATOR(pg_catalog.~) ", namevar);
appendStringLiteralConn(buf, namebuf.data, conn);
- if (PQserverVersion(conn) >= 120000)
+ if (server_version >= 120000)
appendPQExpBufferStr(buf, " COLLATE pg_catalog.default");
appendPQExpBuffer(buf,
"\n OR %s OPERATOR(pg_catalog.~) ",
altnamevar);
appendStringLiteralConn(buf, namebuf.data, conn);
- if (PQserverVersion(conn) >= 120000)
+ if (server_version >= 120000)
appendPQExpBufferStr(buf, " COLLATE pg_catalog.default");
appendPQExpBufferStr(buf, ")\n");
}
@@ -897,7 +898,7 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
{
appendPQExpBuffer(buf, "%s OPERATOR(pg_catalog.~) ", namevar);
appendStringLiteralConn(buf, namebuf.data, conn);
- if (PQserverVersion(conn) >= 120000)
+ if (server_version >= 120000)
appendPQExpBufferStr(buf, " COLLATE pg_catalog.default");
appendPQExpBufferChar(buf, '\n');
}
@@ -914,7 +915,7 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
WHEREAND();
appendPQExpBuffer(buf, "%s OPERATOR(pg_catalog.~) ", schemavar);
appendStringLiteralConn(buf, schemabuf.data, conn);
- if (PQserverVersion(conn) >= 120000)
+ if (server_version >= 120000)
appendPQExpBufferStr(buf, " COLLATE pg_catalog.default");
appendPQExpBufferChar(buf, '\n');
}
--
1.8.3.1
Re: Remove repeated calls to PQserverVersion
On Tue, Jul 13, 2021 at 07:02:27PM +1000, Peter Smith wrote:
I found a few functions making unnecessary repeated calls to
PQserverVersion(conn); instead of just calling once and assigning to a
local variable.
Does it really matter? PQserverVersion() does a simple lookup at the
internals of PGconn.
--
Michael
Re: Remove repeated calls to PQserverVersion
Michael Paquier <michael@paquier.xyz> writes:
On Tue, Jul 13, 2021 at 07:02:27PM +1000, Peter Smith wrote:
I found a few functions making unnecessary repeated calls to
PQserverVersion(conn); instead of just calling once and assigning to a
local variable.
Does it really matter? PQserverVersion() does a simple lookup at the
internals of PGconn.
Yeah, it'd have to be mighty hot code to be worth caring about that,
and none of these spots look like it could be worth it.
regards, tom lane
Re: Remove repeated calls to PQserverVersion
On Wed, Jul 14, 2021 at 12:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael@paquier.xyz> writes:
On Tue, Jul 13, 2021 at 07:02:27PM +1000, Peter Smith wrote:
I found a few functions making unnecessary repeated calls to
PQserverVersion(conn); instead of just calling once and assigning to a
local variable.Does it really matter? PQserverVersion() does a simple lookup at the
internals of PGconn.Yeah, it'd have to be mighty hot code to be worth caring about that,
and none of these spots look like it could be worth it.
I agree there would be no observable performance improvements.
But I never made any claims about performance; my motivation for this
trivial patch was more like just "code tidy" or "refactor", so
applying performance as the only worthiness criteria for a "code tidy"
patch seemed like a misrepresentation here.
Of course you can judge the patch is still not worthwhile for other
reasons. So be it.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Remove repeated calls to PQserverVersion
On 2021-Jul-14, Peter Smith wrote:
But I never made any claims about performance; my motivation for this
trivial patch was more like just "code tidy" or "refactor", so
applying performance as the only worthiness criteria for a "code tidy"
patch seemed like a misrepresentation here.Of course you can judge the patch is still not worthwhile for other
reasons. So be it.
Personally, I like the simplicity of the function call in those places,
because when reading just that line one immediately knows where the
value is coming from. If you assign it to a variable, the line is not
standalone and I have to find out where the assignment is, and verify
that there hasn't been any other assignment in between.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)
Re: Remove repeated calls to PQserverVersion
On 14 Jul 2021, at 02:19, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Personally, I like the simplicity of the function call in those places,
+1
--
Daniel Gustafsson https://vmware.com/