reindexdb & clusterdb broken against pre-7.3 servers

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.

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.

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.

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

On Tue, May 07, 2019 at 12:39:13PM +0900, Michael Paquier wrote:

Okay, point taken. I'll go apply that to the back-branches as well.

And done.
--
Michael