Thread: Object translation mechanism is, umm, broken.

Object translation mechanism is, umm, broken.

From
Dave Page
Date:
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

Re: Object translation mechanism is, umm, broken.

From
Guillaume Lelarge
Date:
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


Re: Object translation mechanism is, umm, broken.

From
Dave Page
Date:
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

Re: Object translation mechanism is, umm, broken.

From
Guillaume Lelarge
Date:
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

Re: Object translation mechanism is, umm, broken.

From
Guillaume Lelarge
Date:
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

Re: Object translation mechanism is, umm, broken.

From
Dave Page
Date:
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

Re: Object translation mechanism is, umm, broken.

From
Guillaume Lelarge
Date:
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


Re: Object translation mechanism is, umm, broken.

From
Dave Page
Date:
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