reindexdb & clusterdb broken against pre-7.3 servers
Hi,
It seems that 582edc369cd caused $subject.
Trivial fix attached, though I obviously didn't actually test it
against such server.
Attachments:
fix_appendQualifiedRelation-v1.diffapplication/octet-stream; name=fix_appendQualifiedRelation-v1.diffDownload
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index a661556ba9..2466ba7cee 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -308,6 +308,8 @@ appendQualifiedRelation(PQExpBuffer buf, const char *spec,
PGresult *res;
int ntups;
+ initPQExpBuffer(&sql);
+
/* Before 7.3, the concept of qualifying a name did not exist. */
if (PQserverVersion(conn) < 70300)
{
@@ -322,7 +324,6 @@ appendQualifiedRelation(PQExpBuffer buf, const char *spec,
* be unnecessary given a regclassin() variant taking a search_path
* argument.
*/
- initPQExpBuffer(&sql);
appendPQExpBufferStr(&sql,
"SELECT c.relname, ns.nspname\n"
" FROM pg_catalog.pg_class c,"
On Mon, May 6, 2019 at 10:04 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
Hi,
It seems that 582edc369cd caused $subject.
Trivial fix attached, though I obviously didn't actually test it
against such server.
Ahem, correct fix attached. I'm going to get a coffee and hide for
the rest of the day.
Attachments:
fix_appendQualifiedRelation-v2.diffapplication/octet-stream; name=fix_appendQualifiedRelation-v2.diffDownload
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index a661556ba9..c32ab9f936 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -311,7 +311,7 @@ appendQualifiedRelation(PQExpBuffer buf, const char *spec,
/* Before 7.3, the concept of qualifying a name did not exist. */
if (PQserverVersion(conn) < 70300)
{
- appendPQExpBufferStr(&sql, spec);
+ appendPQExpBufferStr(buf, spec);
return;
}
On 5/6/19 4:17 AM, Julien Rouhaud wrote:
On Mon, May 6, 2019 at 10:04 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
Hi,
It seems that 582edc369cd caused $subject.
Trivial fix attached, though I obviously didn't actually test it
against such server.Ahem, correct fix attached. I'm going to get a coffee and hide for
the rest of the day.
Why do we even have code referring to pre-7.3 servers? Wouldn't it be
simpler just to remove that code?
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, May 06, 2019 at 08:32:44AM -0400, Andrew Dunstan wrote:
Why do we even have code referring to pre-7.3 servers? Wouldn't it be
simpler just to remove that code?
Even for pg_dump, we only support servers down to 8.0. Let's nuke
this code.
--
Michael
Greetings,
* Michael Paquier (michael@paquier.xyz) wrote:
On Mon, May 06, 2019 at 08:32:44AM -0400, Andrew Dunstan wrote:
Why do we even have code referring to pre-7.3 servers? Wouldn't it be
simpler just to remove that code?Even for pg_dump, we only support servers down to 8.0. Let's nuke
this code.
Agreed.
Seems like we should probably have all of our client tools in-sync
regarding what version they support down to. There's at least some
code in psql that tries to work with pre-8.0 too (around tablespaces and
savepoints, specifically, it looks like), but I have doubts that recent
changes to psql have been tested back to pre-8.0.
At least... for the client tools that support multiple major versions.
Seems a bit unfortunate that we don't really define formally anywhere
which tools in src/bin/ work with multiple major versions and which
don't, even though that's a pretty big distinction and one that matters
to packagers and users.
Thanks,
Stephen
Michael Paquier <michael@paquier.xyz> writes:
On Mon, May 06, 2019 at 08:32:44AM -0400, Andrew Dunstan wrote:
Why do we even have code referring to pre-7.3 servers? Wouldn't it be
simpler just to remove that code?
Even for pg_dump, we only support servers down to 8.0. Let's nuke
this code.
+1. I think psql claims to support down to 7.4, but that's still not
a precedent for trying to handle pre-7.3. Also, the odds that we'd
not break this code path again in future seem pretty bad.
regards, tom lane
On Mon, May 6, 2019 at 3:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael@paquier.xyz> writes:
On Mon, May 06, 2019 at 08:32:44AM -0400, Andrew Dunstan wrote:
Why do we even have code referring to pre-7.3 servers? Wouldn't it be
simpler just to remove that code?Even for pg_dump, we only support servers down to 8.0. Let's nuke
this code.+1. I think psql claims to support down to 7.4, but that's still not
a precedent for trying to handle pre-7.3. Also, the odds that we'd
not break this code path again in future seem pretty bad.
WFM. Updated patch attached, I also removed another similar chunk in
the same file while at it.
Attachments:
fix_appendQualifiedRelation-v3.diffapplication/octet-stream; name=fix_appendQualifiedRelation-v3.diffDownload
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index a661556ba9..593d88eef1 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -146,10 +146,6 @@ connectDatabase(const char *dbname, const char *pghost,
exit(1);
}
- if (PQserverVersion(conn) >= 70300)
- PQclear(executeQuery(conn, ALWAYS_SECURE_SEARCH_PATH_SQL,
- progname, echo));
-
return conn;
}
@@ -308,13 +304,6 @@ appendQualifiedRelation(PQExpBuffer buf, const char *spec,
PGresult *res;
int ntups;
- /* Before 7.3, the concept of qualifying a name did not exist. */
- if (PQserverVersion(conn) < 70300)
- {
- appendPQExpBufferStr(&sql, spec);
- return;
- }
-
splitTableColumnsSpec(spec, PQclientEncoding(conn), &table, &columns);
/*
Julien Rouhaud <rjuju123@gmail.com> writes:
WFM. Updated patch attached, I also removed another similar chunk in
the same file while at it.
Uh, that looks backwards:
@@ -146,10 +146,6 @@ connectDatabase(const char *dbname, const char *pghost,
exit(1);
}
- if (PQserverVersion(conn) >= 70300)
- PQclear(executeQuery(conn, ALWAYS_SECURE_SEARCH_PATH_SQL,
- progname, echo));
-
return conn;
}
regards, tom lane
On Mon, May 6, 2019 at 5:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Julien Rouhaud <rjuju123@gmail.com> writes:
WFM. Updated patch attached, I also removed another similar chunk in
the same file while at it.Uh, that looks backwards:
Argh, sorry :(
I'm definitely done for today.
Attachments:
fix_appendQualifiedRelation-v4.diffapplication/octet-stream; name=fix_appendQualifiedRelation-v4.diffDownload
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index a661556ba9..a42afd490b 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -146,9 +146,8 @@ connectDatabase(const char *dbname, const char *pghost,
exit(1);
}
- if (PQserverVersion(conn) >= 70300)
- PQclear(executeQuery(conn, ALWAYS_SECURE_SEARCH_PATH_SQL,
- progname, echo));
+ PQclear(executeQuery(conn, ALWAYS_SECURE_SEARCH_PATH_SQL,
+ progname, echo));
return conn;
}
@@ -308,13 +307,6 @@ appendQualifiedRelation(PQExpBuffer buf, const char *spec,
PGresult *res;
int ntups;
- /* Before 7.3, the concept of qualifying a name did not exist. */
- if (PQserverVersion(conn) < 70300)
- {
- appendPQExpBufferStr(&sql, spec);
- return;
- }
-
splitTableColumnsSpec(spec, PQclientEncoding(conn), &table, &columns);
/*
On Mon, May 06, 2019 at 05:39:18PM +0200, Julien Rouhaud wrote:
I'm definitely done for today.
Looks good to me, so committed.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Mon, May 06, 2019 at 05:39:18PM +0200, Julien Rouhaud wrote:
I'm definitely done for today.
Looks good to me, so committed.
The originally-complained-of breakage exists in all active branches,
so is it really OK to commit this only in HEAD?
regards, tom lane
On Mon, May 06, 2019 at 10:23:07PM -0400, Tom Lane wrote:
The originally-complained-of breakage exists in all active branches,
so is it really OK to commit this only in HEAD?
I did not think that it would be that critical for back-branches, but
I don't mind going ahead and remove the code there as well. Are there
any objections with it?
Also, wouldn't we want instead to apply on back-branches the first
patch proposed on this thread which fixes the query generation for
this pre-7.3 related code?
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Mon, May 06, 2019 at 10:23:07PM -0400, Tom Lane wrote:
The originally-complained-of breakage exists in all active branches,
so is it really OK to commit this only in HEAD?
I did not think that it would be that critical for back-branches, but
I don't mind going ahead and remove the code there as well. Are there
any objections with it?
Also, wouldn't we want instead to apply on back-branches the first
patch proposed on this thread which fixes the query generation for
this pre-7.3 related code?
Given that we pushed out the bad code a year ago and nobody's complained,
I think it's safe to assume that no one is using any supported release
with a pre-7.3 server.
It's reasonable to doubt that this is the only problem the affected
applications would have with such a server, too. I don't see a lot
of point in "fixing" this code unless somebody actually tests that.
regards, tom lane
On Mon, May 06, 2019 at 11:24:31PM -0400, Tom Lane wrote:
It's reasonable to doubt that this is the only problem the affected
applications would have with such a server, too. I don't see a lot
of point in "fixing" this code unless somebody actually tests that.
Okay, point taken. I'll go apply that to the back-branches as well.
--
Michael