Re: pgAdmin3 and Greenplum partitions SQL

From: Lubomir Petrov <lppetrov(at)gmail(dot)com>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: pgAdmin3 and Greenplum partitions SQL
Date: 2013-01-17 02:56:19
Message-ID: CA+Z0ECfuQFzy444WtjuA_AJkRHrXurPorJOFrOwF=dCC_8+4sQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

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!

Regards,
Lubomir Petrov

Attachment Content-Type Size
pgadmin_pgTable.diff application/octet-stream 1.7 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Ian Lawrence Barwick 2013-01-18 02:42:29 Tiny documentation patch
Previous Message Dave Page 2013-01-16 16:56:56 Re: pgAdmin3 and Greenplum partitions SQL