Обсуждение: 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
On Wed, Jan 16, 2013 at 8:56 AM, Dave Page <dpage@pgadmin.org> wrote:
HiIt looks reasonable from a quick readthrough. Unfortunately the patch
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.
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
Вложения
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
Hi,
On Fri, Jan 18, 2013 at 12:48 AM, Dave Page <dpage@pgadmin.org> wrote:
HiThanks - it still wouldn't apply though for reasons that weren't
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".
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