Re: pgAdmin3 and Greenplum partitions SQL - Mailing list pgadmin-hackers

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.