Skip site navigation (1) Skip section navigation (2)

Re: Ticket 269: Add support for 9.1 ALTER TYPE new syntax for enum

From: Guillaume Lelarge <guillaume(at)lelarge(dot)info>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: pgadmin-hackers(at)postgresql(dot)org
Subject: Re: Ticket 269: Add support for 9.1 ALTER TYPE new syntax for enum
Date: 2010-11-02 18:38:38
Message-ID: 4CD05AAE.2040904@lelarge.info (view raw or flat)
Thread:
Lists: pgadmin-hackers
Le 02/11/2010 07:18, Guillaume Lelarge a écrit :
> Le 02/11/2010 05:49, Dave Page a écrit :
>> On Mon, Nov 1, 2010 at 2:51 PM, Guillaume Lelarge
>> <guillaume(at)lelarge(dot)info> wrote:
>>> Le 31/10/2010 09:44, Guillaume Lelarge a écrit :
>>>> Le 31/10/2010 00:39, Dave Page a écrit :
>>>>> On Sun, Oct 31, 2010 at 1:56 AM, Guillaume Lelarge
>>>>> <guillaume(at)lelarge(dot)info> wrote:
>>>>>> Le 30/10/2010 10:25, Dave Page a écrit :
>>>>>>
>>>>>>> Yeah, that's really nasty. I guess we need split the commands at ;.
>>>>>>
>>>>>> Yeah. If it's not between quotes. I don't like it at all, but I don't
>>>>>> see another way of doing it.
>>>>>>
>>>>>>> I guess we should pass a flag down somehow to tell the function that
>>>>>>> executes the query to do that and then we could also potentially get
>>>>>>> rid of the double SQL boxes.  I'm not looking at the code, but I
>>>>>>> suspect that'll be nasty.
>>>>>>>
>>>>>>
>>>>>> We actually aren't required to add such a flag. We can check if the
>>>>>> query contains "ALTER TYPE", and "ADD AFTER" or "ADD BEFORE".
>>>>>
>>>>> That's knowledge I'd rather avoid hardwiring into the lower level
>>>>> machinery here.
>>>>>
>>>>
>>>> So do I. I tried a few things yesterday. Changing the apply() and
>>>> GetSql() parameters imply to change all GetSql for all dlg* source code.
>>>> That will be quite an invasive patch.
>>>>
>>>
>>> I've done the "split-the-queries" function. Seems to work great, but
>>> still doesn't cover dollar quoting. Anyway, it's less ugly than I
>>> thought. The interesting part is dlgProperty::SplitQueries(). Would love
>>> to get comments :)
>>
>> I just eyeballed the patch - if I'm reading it right, it splits *all*
>> queries and executes each part individually. Is that correct?
>>
> 
> Right.
> 
>> If so, we're going to need that flag. Most of the time, we want all
>> the query parts to be executed atomically, otherwise if we get and
>> error (particularly when using the Apply button where there is one),
>> the dialogue won't know what parts of the update work and what didn't,
>> and thus will have a difficult job refreshing the display
>> appropriately.
>>
> 
> Yeah, that was the part I wanted to work on yesterday. I have an idea on
> this, I actually have the code, but it doesn't work :-/ Need some more work.
> 

Done. I'm actually quite happy with how it turns out. Less nasty then I
previously thought. Complete patch attached (alterenum_v1.patch). The
last part of the work is available on the patch 0001* attached to this
email. Or you can get a look at my branch on github
(http://github.com/gleu/pgadmin3/commits/ticket269).

Probably I should change the method name... WannaSplitQueries is
probably not the best one we could find :)

Comments welcome.


-- 
Guillaume
 http://www.postgresql.fr
 http://dalibo.com

Attachment: 0001-The-split-the-query-should-only-fire-if-necessary.patch
Description: text/x-diff (0 bytes)

In response to

Responses

pgadmin-hackers by date

Next:From: Guillaume LelargeDate: 2010-11-02 21:49:34
Subject: Re: 4 spaces versus tabs
Previous:From: Guillaume LelargeDate: 2010-11-02 14:18:24
Subject: Re: Ticket 269: Add support for 9.1 ALTER TYPE new syntax for enum

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group