Thread: pgAdmin3 and Greenplum partitions SQL

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.

Regards,
Lubomir Petrov

Hi

On Fri, Jan 11, 2013 at 7:31 PM, lpetrov <lppetrov@gmail.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.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi



On Thu, Jan 17, 2013 at 2:56 AM, Lubomir Petrov <lppetrov@gmail.com> wrote:
> On Wed, Jan 16, 2013 at 8:56 AM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> On Fri, Jan 11, 2013 at 7:31 PM, lpetrov <lppetrov@gmail.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.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company