Re: Object translation mechanism is, umm, broken.

On Mon, Jun 13, 2011 at 1:50 PM, Guillaume Lelarge
<guillaume(at)lelarge(dot)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(at)lelarge(dot)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(at)lelarge(dot)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