Обсуждение: Ticket 269: Add support for 9.1 ALTER TYPE new syntax for enum

Hi,

This patch adds support to the new ALTER TYPE syntax in 9.1 for enums.

It's working great except one thing. If a user wants to add two labels,
we're screwed because we can't do two "ALTER TYPE ... ADD" statements in
the same query execution. Any idea how to solve this? the only way I
found would be to disallow adding two labels at once but it results on a
less interesting feature.

Comments?


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

Le 29/10/2010 21:11, Guillaume Lelarge a écrit :
> [...]
> This patch adds support to the new ALTER TYPE syntax in 9.1 for enums.
>
> It's working great except one thing. If a user wants to add two labels,
> we're screwed because we can't do two "ALTER TYPE ... ADD" statements in
> the same query execution. Any idea how to solve this? the only way I
> found would be to disallow adding two labels at once but it results on a
> less interesting feature.
>

So I was wrong. The issue is that we can't issue this statement in a
explicit transaction. Any idea how to solve the "don't send begin/end
statements"?


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

Le 29/10/2010 21:56, Guillaume Lelarge a écrit :
> Le 29/10/2010 21:11, Guillaume Lelarge a écrit :
>> [...]
>> This patch adds support to the new ALTER TYPE syntax in 9.1 for enums.
>>
>> It's working great except one thing. If a user wants to add two labels,
>> we're screwed because we can't do two "ALTER TYPE ... ADD" statements in
>> the same query execution. Any idea how to solve this? the only way I
>> found would be to disallow adding two labels at once but it results on a
>> less interesting feature.
>>
>
> So I was wrong. The issue is that we can't issue this statement in a
> explicit transaction. Any idea how to solve the "don't send begin/end
> statements"?
>

The only idea I have is to make dlgType a two-SQL-boxes dialog and
modify the dlgProperty::apply() method so that if there is "ALTER TYPE
... ADD {BEFORE | AFTER}" statements, they get splitted and fired
individualy. I didn't yet write the code to split the statement, but it
will surely be ugly.

Any objection on doing this? or better idea to fix this issue?


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

On Sat, Oct 30, 2010 at 4:07 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
> Le 29/10/2010 21:56, Guillaume Lelarge a écrit :
>> Le 29/10/2010 21:11, Guillaume Lelarge a écrit :
>>> [...]
>>> This patch adds support to the new ALTER TYPE syntax in 9.1 for enums.
>>>
>>> It's working great except one thing. If a user wants to add two labels,
>>> we're screwed because we can't do two "ALTER TYPE ... ADD" statements in
>>> the same query execution. Any idea how to solve this? the only way I
>>> found would be to disallow adding two labels at once but it results on a
>>> less interesting feature.
>>>
>>
>> So I was wrong. The issue is that we can't issue this statement in a
>> explicit transaction. Any idea how to solve the "don't send begin/end
>> statements"?
>>
>
> The only idea I have is to make dlgType a two-SQL-boxes dialog and
> modify the dlgProperty::apply() method so that if there is "ALTER TYPE
> ... ADD {BEFORE | AFTER}" statements, they get splitted and fired
> individualy. I didn't yet write the code to split the statement, but it
> will surely be ugly.
>
> Any objection on doing this? or better idea to fix this issue?

I don't understand why we can't do all this at once - what's the
problem exactly? Normally we only split statements when they're not
atomic and cannot be rolled back.

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

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

Le 30/10/2010 08:18, Dave Page a écrit :
> On Sat, Oct 30, 2010 at 4:07 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>> Le 29/10/2010 21:56, Guillaume Lelarge a écrit :
>>> Le 29/10/2010 21:11, Guillaume Lelarge a écrit :
>>>> [...]
>>>> This patch adds support to the new ALTER TYPE syntax in 9.1 for enums.
>>>>
>>>> It's working great except one thing. If a user wants to add two labels,
>>>> we're screwed because we can't do two "ALTER TYPE ... ADD" statements in
>>>> the same query execution. Any idea how to solve this? the only way I
>>>> found would be to disallow adding two labels at once but it results on a
>>>> less interesting feature.
>>>>
>>>
>>> So I was wrong. The issue is that we can't issue this statement in a
>>> explicit transaction. Any idea how to solve the "don't send begin/end
>>> statements"?
>>>
>>
>> The only idea I have is to make dlgType a two-SQL-boxes dialog and
>> modify the dlgProperty::apply() method so that if there is "ALTER TYPE
>> ... ADD {BEFORE | AFTER}" statements, they get splitted and fired
>> individualy. I didn't yet write the code to split the statement, but it
>> will surely be ugly.
>>
>> Any objection on doing this? or better idea to fix this issue?
>
> I don't understand why we can't do all this at once - what's the
> problem exactly? Normally we only split statements when they're not
> atomic and cannot be rolled back.
>

Here is the error message I got:

  ERROR:  ALTER TYPE ... ADD cannot be executed from a function or
multi-command string

So, the issue we have is that we can't execute two of them at the same
time. We need to split the ALTER TYPE statements to make sure we execute
them one by one. We could have used the two-sql-textboxes without
actually splitting the query but in this case, a user will only be able
to add two new labels to an enum datatype. What happens if he wants to
add three of them?

So I really think we need to split the ALTER TYPE statements. We
probably can borrow the code from psql for this.


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

On Sat, Oct 30, 2010 at 4:51 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
> Le 30/10/2010 08:18, Dave Page a écrit :
>> On Sat, Oct 30, 2010 at 4:07 PM, Guillaume Lelarge
>> <guillaume@lelarge.info> wrote:
>>> Le 29/10/2010 21:56, Guillaume Lelarge a écrit :
>>>> Le 29/10/2010 21:11, Guillaume Lelarge a écrit :
>>>>> [...]
>>>>> This patch adds support to the new ALTER TYPE syntax in 9.1 for enums.
>>>>>
>>>>> It's working great except one thing. If a user wants to add two labels,
>>>>> we're screwed because we can't do two "ALTER TYPE ... ADD" statements in
>>>>> the same query execution. Any idea how to solve this? the only way I
>>>>> found would be to disallow adding two labels at once but it results on a
>>>>> less interesting feature.
>>>>>
>>>>
>>>> So I was wrong. The issue is that we can't issue this statement in a
>>>> explicit transaction. Any idea how to solve the "don't send begin/end
>>>> statements"?
>>>>
>>>
>>> The only idea I have is to make dlgType a two-SQL-boxes dialog and
>>> modify the dlgProperty::apply() method so that if there is "ALTER TYPE
>>> ... ADD {BEFORE | AFTER}" statements, they get splitted and fired
>>> individualy. I didn't yet write the code to split the statement, but it
>>> will surely be ugly.
>>>
>>> Any objection on doing this? or better idea to fix this issue?
>>
>> I don't understand why we can't do all this at once - what's the
>> problem exactly? Normally we only split statements when they're not
>> atomic and cannot be rolled back.
>>
>
> Here is the error message I got:
>
>  ERROR:  ALTER TYPE ... ADD cannot be executed from a function or
> multi-command string

I just checked the docs, and wow - that seems like a major step
backwards compared to our normal strive for transactional DDL.

> So, the issue we have is that we can't execute two of them at the same
> time. We need to split the ALTER TYPE statements to make sure we execute
> them one by one. We could have used the two-sql-textboxes without
> actually splitting the query but in this case, a user will only be able
> to add two new labels to an enum datatype. What happens if he wants to
> add three of them?

Yeah, that's really nasty. I guess we need split the commands at ;. 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.

> So I really think we need to split the ALTER TYPE statements. We
> probably can borrow the code from psql for this.

Well psql has a full blown parser doesn't it? I'm not sure we want to
go that far if we can help it.


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

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

Le 30/10/2010 10:25, Dave Page a écrit :
> On Sat, Oct 30, 2010 at 4:51 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>> Le 30/10/2010 08:18, Dave Page a écrit :
>>> On Sat, Oct 30, 2010 at 4:07 PM, Guillaume Lelarge
>>> <guillaume@lelarge.info> wrote:
>>>> Le 29/10/2010 21:56, Guillaume Lelarge a écrit :
>>>>> Le 29/10/2010 21:11, Guillaume Lelarge a écrit :
>>>>>> [...]
>>>>>> This patch adds support to the new ALTER TYPE syntax in 9.1 for enums.
>>>>>>
>>>>>> It's working great except one thing. If a user wants to add two labels,
>>>>>> we're screwed because we can't do two "ALTER TYPE ... ADD" statements in
>>>>>> the same query execution. Any idea how to solve this? the only way I
>>>>>> found would be to disallow adding two labels at once but it results on a
>>>>>> less interesting feature.
>>>>>>
>>>>>
>>>>> So I was wrong. The issue is that we can't issue this statement in a
>>>>> explicit transaction. Any idea how to solve the "don't send begin/end
>>>>> statements"?
>>>>>
>>>>
>>>> The only idea I have is to make dlgType a two-SQL-boxes dialog and
>>>> modify the dlgProperty::apply() method so that if there is "ALTER TYPE
>>>> ... ADD {BEFORE | AFTER}" statements, they get splitted and fired
>>>> individualy. I didn't yet write the code to split the statement, but it
>>>> will surely be ugly.
>>>>
>>>> Any objection on doing this? or better idea to fix this issue?
>>>
>>> I don't understand why we can't do all this at once - what's the
>>> problem exactly? Normally we only split statements when they're not
>>> atomic and cannot be rolled back.
>>>
>>
>> Here is the error message I got:
>>
>>  ERROR:  ALTER TYPE ... ADD cannot be executed from a function or
>> multi-command string
>
> I just checked the docs, and wow - that seems like a major step
> backwards compared to our normal strive for transactional DDL.
>

Yeah. I wonder why they did it this way, but had no time yet to check
that on -hackers.

>> So, the issue we have is that we can't execute two of them at the same
>> time. We need to split the ALTER TYPE statements to make sure we execute
>> them one by one. We could have used the two-sql-textboxes without
>> actually splitting the query but in this case, a user will only be able
>> to add two new labels to an enum datatype. What happens if he wants to
>> add three of them?
>
> 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".

But the flag would be a good idea if we need to split the statements for
other kind of queries (we don't need now, but how can I be sure it won't
happen in a new patch?).

I'm going to work on this before having dinner.

>> So I really think we need to split the ALTER TYPE statements. We
>> probably can borrow the code from psql for this.
>
> Well psql has a full blown parser doesn't it? I'm not sure we want to
> go that far if we can help it.
>

Yes, it has. I checked after sending the mail. And I don't want to add
that code to pgAdmin :)


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

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.


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

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

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.


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

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 :)


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

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?

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.

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

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

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.


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

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 :)

Comments welcome.


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

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

Le 03/11/2010 06:31, Dave Page a écrit :
> 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 :-)
>

Commited. I you came with another name, it'll be a simple change.


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