Subject: Remove "unsupported version" warning for Greenplum

From: Andreas 'ads' Scherbaum <[email-masked]>
Date: Mon, 18 Jan 2016 02:44:53 +0100

Hello,

we tested pgAdminIII 1.22 internally and found that it works sufficient 
with Greenplum. A few minor problems are identified and we plan to work 
on them as well, but nothing serious. We however know that there are a 
number more serious problems with HAWQ (SQL on Hadoop), we plan to work 
on this as well.

As a first step, attached is a patch which removes the "unsupported 
version" warning for Greenplum. The patch copies a bit infrastructure 
and prepares us to support later versions of Greenplum when we bump our 
version number after merging with later PostgreSQL versions. One 
remaining question is if the supported versions should stay in 
pgConn.cpp or move to pgAdmin3.h.

Thank you,

-- 
				Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project
diff --git a/pgadmin/schema/pgServer.cpp b/pgadmin/schema/pgServer.cpp
index b263f7c..259da73 100644
--- a/pgadmin/schema/pgServer.cpp
+++ b/pgadmin/schema/pgServer.cpp
@@ -835,14 +835,47 @@ int pgServer::Connect(frmMain *form, bool askPassword, const wxString &pwd, bool
 		dbOid = conn->GetDbOid();
 
 		// Check the server version
-		if (!(conn->BackendMinimumVersion(SERVER_MIN_VERSION_N >> 8, SERVER_MIN_VERSION_N & 0x00FF)) ||
-		        (conn->BackendMinimumVersion(SERVER_MAX_VERSION_N >> 8, (SERVER_MAX_VERSION_N & 0x00FF) + 1)))
+		if (conn->GetIsGreenplum())
 		{
-			wxLogWarning(_("The server you are connecting to is not a version that is supported by this release of %s.\n\n%s may not function as expected.\n\nSupported server versions are %s to %s."),
-			             appearanceFactory->GetLongAppName().c_str(),
-			             appearanceFactory->GetLongAppName().c_str(),
-			             wxString(SERVER_MIN_VERSION_T).c_str(),
-			             wxString(SERVER_MAX_VERSION_T).c_str());
+			// Check for Greenplum specific version
+			// Greenplum always shows PG version "8.2.15" for now
+			// this might change once the merge with recent PG versions makes progress
+			// therefore also check for the max version
+			const short GP_MIN_VERSION_N = 0x0802;
+			const wxString GP_MIN_VERSION_T = wxT("8.2");
+			const short GP_MAX_VERSION_N = 0x0802;
+			const wxString GP_MAX_VERSION_T = wxT("8.2");
+			if (!(conn->BackendMinimumVersion(GP_MIN_VERSION_N >> 8, GP_MIN_VERSION_N & 0x00FF)) ||
+		        	(conn->BackendMinimumVersion(GP_MAX_VERSION_N >> 8, (GP_MAX_VERSION_N & 0x00FF) + 1)))
+			{
+				if (GP_MIN_VERSION_N == GP_MAX_VERSION_N)
+				{
+					wxLogWarning(_("The server you are connecting to is not a version that is supported by this release of %s.\n\n%s may not function as expected.\n\nSupported server version is %s."),
+					             appearanceFactory->GetLongAppName().c_str(),
+					             appearanceFactory->GetLongAppName().c_str(),
+					             wxString(GP_MIN_VERSION_T).c_str());
+				}
+				else
+				{
+					wxLogWarning(_("The server you are connecting to is not a version that is supported by this release of %s.\n\n%s may not function as expected.\n\nSupported server versions are %s to %s."),
+					             appearanceFactory->GetLongAppName().c_str(),
+					             appearanceFactory->GetLongAppName().c_str(),
+					             wxString(GP_MIN_VERSION_T).c_str(),
+					             wxString(GP_MAX_VERSION_T).c_str());
+				}
+			}
+		}
+		else
+		{
+			if (!(conn->BackendMinimumVersion(SERVER_MIN_VERSION_N >> 8, SERVER_MIN_VERSION_N & 0x00FF)) ||
+			        (conn->BackendMinimumVersion(SERVER_MAX_VERSION_N >> 8, (SERVER_MAX_VERSION_N & 0x00FF) + 1)))
+			{
+				wxLogWarning(_("The server you are connecting to is not a version that is supported by this release of %s.\n\n%s may not function as expected.\n\nSupported server versions are %s to %s."),
+				             appearanceFactory->GetLongAppName().c_str(),
+				             appearanceFactory->GetLongAppName().c_str(),
+				             wxString(SERVER_MIN_VERSION_T).c_str(),
+				             wxString(SERVER_MAX_VERSION_T).c_str());
+			}
 		}
 
 		connected = true;

-- 
Sent via pgadmin-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

---

From: Dave Page <[email-masked]>
Date: Tue, 19 Jan 2016 15:03:03 +0000

Hi

On Mon, Jan 18, 2016 at 1:44 AM, Andreas 'ads' Scherbaum
<[email-masked]> wrote:
>
> Hello,
>
> we tested pgAdminIII 1.22 internally and found that it works sufficient with
> Greenplum. A few minor problems are identified and we plan to work on them
> as well, but nothing serious. We however know that there are a number more
> serious problems with HAWQ (SQL on Hadoop), we plan to work on this as well.
>
> As a first step, attached is a patch which removes the "unsupported version"
> warning for Greenplum. The patch copies a bit infrastructure and prepares us
> to support later versions of Greenplum when we bump our version number after
> merging with later PostgreSQL versions. One remaining question is if the
> supported versions should stay in pgConn.cpp or move to pgAdmin3.h.

Your patch won't apply again. I have no idea why - I'm trying to do it
on my Mac, which is a *nix under the hood (they don't use Mac line
endings any more - that was the old Mac OS 9 and earlier from a decade
or so ago iirc). How are you creating them? The normal way is to do
something like:

git pull --rebase
git diff > foo.diff

My git config for the repo is basically what you get on a fresh
installation of git, with a git clone, e.g.

[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
        ignorecase = true
        precomposeunicode = true
[remote "origin"]
        url = ssh://[email-masked]/pgadmin3.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master

Anyway - patch issues aside, isn't your patch essentially the same as
the one I was looking for feedback on just before release?

http://www.postgresql.org/message-id/flat/CACjysiUQLnddN6n=[email-masked]#CACjysiUQLnddN6n=[email-masked]

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Andreas 'ads' Scherbaum <[email-masked]>
Date: Tue, 19 Jan 2016 21:48:47 +0100

> or so ago iirc). How are you creating them? The normal way is to do

That is strange. Can someone else on the list please check if the patch 
applies (and then roll it back)? I did a fresh clone (again) and it 
applies clean.

> http://www.postgresql.org/message-id/flat/CACjysiUQLnddN6n=[email-masked]#CACjysiUQLnddN6n=[email-masked]

I clearly have missed this one. And I see that you prefer the version in 
the header file.

Also I plan to add extra support for HAWQ (SQL on Hadoop, also based on 
the Greenplum engine), but this has a few issues.

Ok, let me please rework this patch, and do another check for the git 
config.

Regards,

---

From: Andreas 'ads' Scherbaum <[email-masked]>
Date: Tue, 19 Jan 2016 23:54:26 +0100

> or so ago iirc). How are you creating them? The normal way is to do

And how does the attached work? Fresh clone again, only difference is a 
warning (not an error) for whitespaces removed.

Regards

index b263f7c..ad47851 100644

 		connected = true;

---

From: Dave Page <[email-masked]>
Date: Thu, 21 Jan 2016 09:31:33 +0000

> warning (not an error) for whitespaces removed.

Still doesn't apply. I tried on the following systems:

Mac OS X 10.11.1 - git version 2.5.4 (Apple Git-61)
Windows 7 Enterprise SP1 - git version 1.8.1.msysgit.1
CentOS release 6.7 (Final) - git version 1.7.1

I'm fairly convinced at this stage that there's something funky on
your system. Perhaps we should take a look next week when we're both
in Brussels?

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 21 Jan 2016 23:13:16 +0100

> your system. Perhaps we should take a look next week when we're both
Tried off a fresh clone of HEAD and it works perfectly:

mha@mha-laptop:~/postgresql/pgadmin3$ patch -p1 < /tmp/gp-warning2.diff
(Stripping trailing CRs from patch; use --binary to disable.)
patching file pgadmin/schema/pgServer.cpp

git 2.1.4 on Debian Jessie

Dave - have you tried off a completely clean checkout as well? In case some
metadata has gotten screwed?

The settings that you have that I don't (so they're not the ones you get by
default) are:

[core]
        ignorecase = true
        precomposeunicode = true

I wonder if it's the unicode one..

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 21 Jan 2016 23:25:43 +0100

> git 2.1.4 on Debian Jessie

Actually, since I'm using patch to apply it, the relevant version is
probably:
$ patch --version
GNU patch 2.7.5
Copyright (C) 2003, 2009-2012 Free Software Foundation, Inc.
Copyright (C) 1988 Larry Wall

---

From: Andreas 'ads' Scherbaum <[email-masked]>
Date: Thu, 21 Jan 2016 23:35:43 +0100

> your system. Perhaps we should take a look next week when we're both

After debugging back and forth with Magnus, it looks like that Google 
Mail is fooling you. Your downloaded file has a different line ending, 
and your file is 3269 bytes, where the original file is 3210 bytes. 
That's 59 additional line breaks.

4fa0990a1020e425fe95b99ea9f186de  gp-warning2.diff

The file you download from the archive:

http://www.postgresql.org/message-id/[email-masked]

is also correct.

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 21 Jan 2016 23:54:52 +0100

On Thu, Jan 21, 2016 at 11:35 PM, Andreas 'ads' Scherbaum <
[email-masked]> wrote:

> After debugging back and forth with Magnus, it looks like that Google Mail
> is fooling you. Your downloaded file has a different line ending, and your
> file is 3269 bytes, where the original file is 3210 bytes. That's 59
> additional line breaks.
>

It's also worth noticing that "patch" has no problem with either of the two
files, but "git apply" does. At least on my system.

---

From: Dave Page <[email-masked]>
Date: Fri, 22 Jan 2016 09:43:37 +0000

On Thu, Jan 21, 2016 at 10:54 PM, Magnus Hagander <[email-masked]> wrote:
>
>
> On Thu, Jan 21, 2016 at 11:35 PM, Andreas 'ads' Scherbaum

Well that's weird. But why is it only happening with your patches? I
apply patches from others constantly without issues.

> files, but "git apply" does. At least on my system.

Yeah, patch does work for me. I'm so used to using 'git apply' these
days that I didn't think of trying that.

So what am I applying? Does my patch work for you Ads, or do you want
to update yours to move the version numbers?

---

From: Magnus Hagander <[email-masked]>
Date: Fri, 22 Jan 2016 10:57:24 +0100

On Fri, Jan 22, 2016 at 10:43 AM, Dave Page <[email-masked]> wrote:

> Mail

What MIME types do you get those in typically? Could be that gmail is
reacting to it being text/x-patch and not application/x-patch or something?

---

From: Dave Page <[email-masked]>
Date: Fri, 22 Jan 2016 10:04:16 +0000

> reacting to it being text/x-patch and not application/x-patch or something?

text/x-patch or application/octet-stream seem to be fine from others.
The one thing I noticed is the majority of patches I've received from
others tend to be base64 encoded, whilst Ads' is not.

I'd be interested to try a patch that's been zipped before attachment.

---

From: Andreas 'ads' Scherbaum <[email-masked]>
Date: Fri, 22 Jan 2016 11:12:58 +0100

> I'd be interested to try a patch that's been zipped before attachment.

Yes, I decided to gzip them next time, before sending them.

Honestly I have no idea why only my patches. There is nothing unusual in 
my Thunderbird, pretty much a default installation. And downloading my 
patch from Thunderbird or the PG Archive results in the correct file. So 
it must be a Google thing.

Ok, now that this problem is solved, I will move the versions into the 
header files and send you a new version soon.

Thanks,

---

From: Andreas 'ads' Scherbaum <[email-masked]>
Date: Thu, 4 Feb 2016 13:22:09 +0100

> pgConn.cpp or move to pgAdmin3.h.

Next attempt (this time packed diff).

The attached patch adds recognition for Greenplum HAWQ (SQL on Hadoop), 
where I know that it has problems with some catalog tables. An error 
message is still shown for HAWQ. The error message during connect for 
GPDB is removed. Version information is moved to header file.

Regards,

---

From: Dave Page <[email-masked]>
Date: Mon, 8 Feb 2016 10:23:26 +0000

> Next attempt (this time packed diff).

And this time it works perfectly :-)

> where I know that it has problems with some catalog tables. An error message
> is still shown for HAWQ. The error message during connect for GPDB is
> removed. Version information is moved to header file.

Thanks - applied to the 1.22 and master branches.

---

From: Andreas 'ads' Scherbaum <[email-masked]>
Date: Mon, 8 Feb 2016 12:30:41 +0100

> Thanks - applied to the 1.22 and master branches.

Thanks for submitting the patch.

Just that you know (because I've seen your commit message): it will barf 
when connecting to HAWQ - because there are catalog errors. The patch 
removes the warning for Greenplum Database only. I'm not going to fix 
this in III, but keep an eye on it for 4.