Thread: Object translation mechanism is, umm, broken.
Hi Guillaume, This commit: http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4 (particularly the code in pgObject.cpp) is causing problems. I keep running into objects for which we now get messages like "Refreshing unknown object of type Replication", which I believe used to work just fine. I've just hit it with the Slony replication cluster object, and now have a sneaking suspicion it's also going to affect all the other slony object types. Aside from the fact that the code is incomplete, it's a serious modularity violation. We shouldn't have a giant conditional statement in a parent class that has knowledge of each of the different classes that might be derived from that object. Instead, each of the child classes should override a common function. Can you look at this please? -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On Thu, 2011-06-02 at 08:54 +0000, Dave Page wrote: > [...] > This commit: http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4 > (particularly the code in pgObject.cpp) is causing problems. I keep > running into objects for which we now get messages like "Refreshing > unknown object of type Replication", which I believe used to work just > fine. I've just hit it with the Slony replication cluster object, and > now have a sneaking suspicion it's also going to affect all the other > slony object types. > Right. > Aside from the fact that the code is incomplete, it's a serious > modularity violation. We shouldn't have a giant conditional statement > in a parent class that has knowledge of each of the different classes > that might be derived from that object. Instead, each of the child > classes should override a common function. > > Can you look at this please? > Working on it right now. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Fri, Jun 3, 2011 at 1:45 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > > Hi, > > On Thu, 2011-06-02 at 08:54 +0000, Dave Page wrote: > > [...] > > This commit: http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4 > > (particularly the code in pgObject.cpp) is causing problems. I keep > > running into objects for which we now get messages like "Refreshing > > unknown object of type Replication", which I believe used to work just > > fine. I've just hit it with the Slony replication cluster object, and > > now have a sneaking suspicion it's also going to affect all the other > > slony object types. > > > > Right. > > > Aside from the fact that the code is incomplete, it's a serious > > modularity violation. We shouldn't have a giant conditional statement > > in a parent class that has knowledge of each of the different classes > > that might be derived from that object. Instead, each of the child > > classes should override a common function. > > > > Can you look at this please? > > > > Working on it right now. > Thanks - I appreciate it. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, 2011-06-03 at 14:24 +0100, Dave Page wrote: > On Fri, Jun 3, 2011 at 1:45 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > > > > Hi, > > > > On Thu, 2011-06-02 at 08:54 +0000, Dave Page wrote: > > > [...] > > > This commit: http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4 > > > (particularly the code in pgObject.cpp) is causing problems. I keep > > > running into objects for which we now get messages like "Refreshing > > > unknown object of type Replication", which I believe used to work just > > > fine. I've just hit it with the Slony replication cluster object, and > > > now have a sneaking suspicion it's also going to affect all the other > > > slony object types. > > > > > > > Right. > > > > > Aside from the fact that the code is incomplete, it's a serious > > > modularity violation. We shouldn't have a giant conditional statement > > > in a parent class that has knowledge of each of the different classes > > > that might be derived from that object. Instead, each of the child > > > classes should override a common function. > > > > > > Can you look at this please? > > > > > > > Working on it right now. > > > > Thanks - I appreciate it. > So I finally have something. Can you get a quick look at it to tell me if that's what you were looking for? I guess you'll be particularly interested in pgadmin/schema/pgObject.cpp. Thanks. BTW, still two issues on this (big) patch: * Greenplum objects are taken care of, but I have no way to check them * EDB objects still have no support. I can add that, but can't check the code -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Sat, 2011-06-11 at 11:22 +0200, Guillaume Lelarge wrote: > On Fri, 2011-06-03 at 14:24 +0100, Dave Page wrote: > > On Fri, Jun 3, 2011 at 1:45 PM, Guillaume Lelarge > > <guillaume@lelarge.info> wrote: > > > > > > Hi, > > > > > > On Thu, 2011-06-02 at 08:54 +0000, Dave Page wrote: > > > > [...] > > > > This commit: http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4 > > > > (particularly the code in pgObject.cpp) is causing problems. I keep > > > > running into objects for which we now get messages like "Refreshing > > > > unknown object of type Replication", which I believe used to work just > > > > fine. I've just hit it with the Slony replication cluster object, and > > > > now have a sneaking suspicion it's also going to affect all the other > > > > slony object types. > > > > > > > > > > Right. > > > > > > > Aside from the fact that the code is incomplete, it's a serious > > > > modularity violation. We shouldn't have a giant conditional statement > > > > in a parent class that has knowledge of each of the different classes > > > > that might be derived from that object. Instead, each of the child > > > > classes should override a common function. > > > > > > > > Can you look at this please? > > > > > > > > > > Working on it right now. > > > > > > > Thanks - I appreciate it. > > > > So I finally have something. Can you get a quick look at it to tell me > if that's what you were looking for? I guess you'll be particularly > interested in pgadmin/schema/pgObject.cpp. > > Thanks. > > BTW, still two issues on this (big) patch: > * Greenplum objects are taken care of, but I have no way to check them > * EDB objects still have no support. I can add that, but can't check > the code > Same patch with EDB objects support (but still no tests on them). And this time, compressed. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
Attachment
On Sat, Jun 11, 2011 at 1:37 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Sat, 2011-06-11 at 11:22 +0200, Guillaume Lelarge wrote: >> On Fri, 2011-06-03 at 14:24 +0100, Dave Page wrote: >> > On Fri, Jun 3, 2011 at 1:45 PM, Guillaume Lelarge >> > <guillaume@lelarge.info> wrote: >> > > >> > > Hi, >> > > >> > > On Thu, 2011-06-02 at 08:54 +0000, Dave Page wrote: >> > > > [...] >> > > > This commit: http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4 >> > > > (particularly the code in pgObject.cpp) is causing problems. I keep >> > > > running into objects for which we now get messages like "Refreshing >> > > > unknown object of type Replication", which I believe used to work just >> > > > fine. I've just hit it with the Slony replication cluster object, and >> > > > now have a sneaking suspicion it's also going to affect all the other >> > > > slony object types. >> > > > >> > > >> > > Right. >> > > >> > > > Aside from the fact that the code is incomplete, it's a serious >> > > > modularity violation. We shouldn't have a giant conditional statement >> > > > in a parent class that has knowledge of each of the different classes >> > > > that might be derived from that object. Instead, each of the child >> > > > classes should override a common function. >> > > > >> > > > Can you look at this please? >> > > > >> > > >> > > Working on it right now. >> > > >> > >> > Thanks - I appreciate it. >> > >> >> So I finally have something. Can you get a quick look at it to tell me >> if that's what you were looking for? I guess you'll be particularly >> interested in pgadmin/schema/pgObject.cpp. >> >> Thanks. >> >> BTW, still two issues on this (big) patch: >> * Greenplum objects are taken care of, but I have no way to check them >> * EDB objects still have no support. I can add that, but can't check >> the code >> > > Same patch with EDB objects support (but still no tests on them). And > this time, compressed. That seems like a much cleaner design, though I must admit I'm surprised by the size of the patch. I wonder if we should get it applied, and push out another beta ASAP? Thanks. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, 2011-06-13 at 10:18 +0100, Dave Page wrote: > On Sat, Jun 11, 2011 at 1:37 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > > On Sat, 2011-06-11 at 11:22 +0200, Guillaume Lelarge wrote: > >> On Fri, 2011-06-03 at 14:24 +0100, Dave Page wrote: > >> > On Fri, Jun 3, 2011 at 1:45 PM, Guillaume Lelarge > >> > <guillaume@lelarge.info> wrote: > >> > > > >> > > Hi, > >> > > > >> > > On Thu, 2011-06-02 at 08:54 +0000, Dave Page wrote: > >> > > > [...] > >> > > > This commit: http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4 > >> > > > (particularly the code in pgObject.cpp) is causing problems. I keep > >> > > > running into objects for which we now get messages like "Refreshing > >> > > > unknown object of type Replication", which I believe used to work just > >> > > > fine. I've just hit it with the Slony replication cluster object, and > >> > > > now have a sneaking suspicion it's also going to affect all the other > >> > > > slony object types. > >> > > > > >> > > > >> > > Right. > >> > > > >> > > > Aside from the fact that the code is incomplete, it's a serious > >> > > > modularity violation. We shouldn't have a giant conditional statement > >> > > > in a parent class that has knowledge of each of the different classes > >> > > > that might be derived from that object. Instead, each of the child > >> > > > classes should override a common function. > >> > > > > >> > > > Can you look at this please? > >> > > > > >> > > > >> > > Working on it right now. > >> > > > >> > > >> > Thanks - I appreciate it. > >> > > >> > >> So I finally have something. Can you get a quick look at it to tell me > >> if that's what you were looking for? I guess you'll be particularly > >> interested in pgadmin/schema/pgObject.cpp. > >> > >> Thanks. > >> > >> BTW, still two issues on this (big) patch: > >> * Greenplum objects are taken care of, but I have no way to check them > >> * EDB objects still have no support. I can add that, but can't check > >> the code > >> > > > > Same patch with EDB objects support (but still no tests on them). And > > this time, compressed. > > That seems like a much cleaner design, though I must admit I'm > surprised by the size of the patch. > Yeah, I was also surprised. I mostly had two different kinds of things to do: add messages to fully unsupported objects (meaning something like 200 more lines in their respective files), and adding a CreateCollection() function to partially supported objects (so, declaration in .h, and code in .cpp). The sum of all these changes is huge, but it's not a difficult code. > I wonder if we should get it applied, and push out another beta ASAP? > I'm not against pushing out another beta. But I'm not sure we really need it just for this patch. We seem to have some users that work on the last git release. Let's see if they find something with this patch. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Mon, Jun 13, 2011 at 1:50 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Mon, 2011-06-13 at 10:18 +0100, Dave Page wrote: >> On Sat, Jun 11, 2011 at 1:37 PM, Guillaume Lelarge >> <guillaume@lelarge.info> wrote: >> > On Sat, 2011-06-11 at 11:22 +0200, Guillaume Lelarge wrote: >> >> On Fri, 2011-06-03 at 14:24 +0100, Dave Page wrote: >> >> > On Fri, Jun 3, 2011 at 1:45 PM, Guillaume Lelarge >> >> > <guillaume@lelarge.info> wrote: >> >> > > >> >> > > Hi, >> >> > > >> >> > > On Thu, 2011-06-02 at 08:54 +0000, Dave Page wrote: >> >> > > > [...] >> >> > > > This commit: http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4 >> >> > > > (particularly the code in pgObject.cpp) is causing problems. I keep >> >> > > > running into objects for which we now get messages like "Refreshing >> >> > > > unknown object of type Replication", which I believe used to work just >> >> > > > fine. I've just hit it with the Slony replication cluster object, and >> >> > > > now have a sneaking suspicion it's also going to affect all the other >> >> > > > slony object types. >> >> > > > >> >> > > >> >> > > Right. >> >> > > >> >> > > > Aside from the fact that the code is incomplete, it's a serious >> >> > > > modularity violation. We shouldn't have a giant conditional statement >> >> > > > in a parent class that has knowledge of each of the different classes >> >> > > > that might be derived from that object. Instead, each of the child >> >> > > > classes should override a common function. >> >> > > > >> >> > > > Can you look at this please? >> >> > > > >> >> > > >> >> > > Working on it right now. >> >> > > >> >> > >> >> > Thanks - I appreciate it. >> >> > >> >> >> >> So I finally have something. Can you get a quick look at it to tell me >> >> if that's what you were looking for? I guess you'll be particularly >> >> interested in pgadmin/schema/pgObject.cpp. >> >> >> >> Thanks. >> >> >> >> BTW, still two issues on this (big) patch: >> >> * Greenplum objects are taken care of, but I have no way to check them >> >> * EDB objects still have no support. I can add that, but can't check >> >> the code >> >> >> > >> > Same patch with EDB objects support (but still no tests on them). And >> > this time, compressed. >> >> That seems like a much cleaner design, though I must admit I'm >> surprised by the size of the patch. >> > > Yeah, I was also surprised. I mostly had two different kinds of things > to do: add messages to fully unsupported objects (meaning something like > 200 more lines in their respective files), and adding a > CreateCollection() function to partially supported objects (so, > declaration in .h, and code in .cpp). The sum of all these changes is > huge, but it's not a difficult code. > >> I wonder if we should get it applied, and push out another beta ASAP? >> > > I'm not against pushing out another beta. But I'm not sure we really > need it just for this patch. We seem to have some users that work on the > last git release. Let's see if they find something with this patch. OK, cool. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company