Re: pgAdmin3 and Greenplum partitions SQL

Hi,

On Fri, Jan 18, 2013 at 12:48 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
>
>
> On Thu, Jan 17, 2013 at 2:56 AM, Lubomir Petrov <lppetrov(at)gmail(dot)com>
> wrote:
> > On Wed, Jan 16, 2013 at 8:56 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> >>
> >> Hi
> >>
> >> On Fri, Jan 11, 2013 at 7:31 PM, lpetrov <lppetrov(at)gmail(dot)com> wrote:
> >> > Hi all,
> >> >
> >> > I have identified a bug in handling Greenplum partitions SQL in
> >> > pgAdmin3.
> >> > The bug is visible when clicking on Greenplum partition table (the
> >> > partition
> >> > itself) and then the SQL for the table is displayed in the SQL pane on
> >> > the
> >> > right. The SQL for partitions represents randomly some CREATE TABLE
> >> > features
> >> > like "UNLOGGED" or "OF " (type).
> >> >
> >> > I have tracked the problem to pgTable and gpPartition. When
> >> > gpPartitionFactory::CreateObjects() creates and inserts the objects,
> it
> >> > creates gpPartition objects and gpPartition inherits pgTable.
> >> > Unfortunately
> >> > pgTable does not initialize it's members (is there any reason for
> that?)
> >> > and
> >> > relies on the caller/user to set the member variables representing
> >> > object
> >> > properties one by one.
> >> > With some recent changes introduced in new-ish Postgres versions, new
> >> > members were added to pgTable which were handled in
> >> > pgTableFactory::CreateObjects() (if/else), but not added in
> >> > gpPartitionFactory::CreateObjects(). Therefore when gpPartition object
> >> > is
> >> > created, members are uninitialized and GetSql() uses random values
> when
> >> > constructing the SQL string.
> >> >
> >> > This bug can be fixed in several ways, but I believe that the most
> >> > appropriate is to initialize member variables in pgTable. I have
> >> > attached a
> >> > patch that does this which fixes the bug mentioned above and the
> entire
> >> > class of similar bugs. Members are initialized in a way to represent
> the
> >> > default state of table properties (example: table by default is
> unlogged
> >> > unless explicitly noted, table is not 'of type' unless explicitly
> noted,
> >> > etc.).
> >> >
> >> > Let me know if you have any comments or remarks.
> >>
> >> It looks reasonable from a quick readthrough. Unfortunately the patch
> >> won't apply to either the 1.16 branch or git master - can you
> >> regenerate it against the 1.16 branch using "git diff" please? I don't
> >> have Greenplum available to test, so I'd prefer to be working with a
> >> patch that you've tested rather than manually hacking it about here
> >> and hoping I don't mess it up.
> >>
> >
> > Hi,
> >
> > Sure, attached is the patch against the 1.16 branch generated with "git
> > diff".
>
> Thanks - it still wouldn't apply though for reasons that weren't
> obvious. I've manually applied it and pushed it to the 1.16 and master
> branches though - please confirm I didn't break it.
>
>
Thanks, look good!

Regards,
Lubomir Petrov