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

On Tue, Nov 2, 2010 at 11:38 AM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
> 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@lelarge.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@lelarge.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 :)

Seems like a nice solution. And the name is descriptive, if nothing else :-)



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

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