Thread: 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.
Attachment
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.
Attachment
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
Attachment
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
Attachment
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.
Attachment
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.
Attachment
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
Attachment
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
Attachment
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
Attachment
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