On 9 July 2011 13:26, Thom Brown <t...@linux.com> wrote:
> On 9 July 2011 13:25, Guillaume Lelarge <guilla...@lelarge.info> wrote:
>> On Sat, 2011-07-09 at 13:14 +0100, Thom Brown wrote:
>>> > The major issue is why we use the "drop all, add all" method. Let's say
>>> > I have a composite type declared like this:
>>> >
>>> > CREATE TYPE s1.ty2 AS
>>> > (c1 integer,
>>> > c2 integer,
>>> > c3 text,
>>> > c4 integer);
>>> >
>>> > If I change c3's type (but it could be c1 or c2), I get this SQL query:
>>> >
>>> > ALTER TYPE s1.ty2
>>> > ADD ATTRIBUTE c3 xml;
>>> > ALTER TYPE s1.ty2
>>> > DROP ATTRIBUTE c3;
>>> >
>>> > Remember that, on pgAdmin, it shows the list this way:
>>> > c1 integer,
>>> > c2 integer,
>>> > c3 xml,
>>> > c4 integer
>>> >
>>> > I click OK, and get back to the properties dialog. pgAdmin now shows the
>>> > list this way:
>>> > c1 integer,
>>> > c2 integer,
>>> > c4 integer
>>> > c3 xml,
>>> >
>>> > Which is true. We drop the attribute and add another one, which will be
>>> > at the end of the list.
>>>
>>> I have a solution. It seems I overlooked the alter attribute
>>> capabilities. We can just do:
>>>
>>> ALTER TYPE s1.ty2
>>> ALTER ATTRIBUTE c3 TYPE xml;
>>>
>>> That will preserve its position without ever having to drop it. I'm
>>> not sure why I didn't see it before.
>>>
>>
>> Can you fix your patch to also use this syntax?
>
> Yes, I'll try to fix both issues. Thanks for testing it. I'll be
> back with a patch shortly.
Okay, looks like it needed more work than I realised. There are quite
a few extra changes in this one. I've moved the various checks (such
as ensuring there are at least 2 attributes) out of the block for
creating a type so that the same rules apply to modifying types. This
is because you shouldn't be able to modify a type so that there are
less than 2 attributes.
But please try to break this. I've tried various combinations of
actions (adding, renaming and changing type, just renaming, just
changing type, removing an original attribute, removing all and adding
back in, removing none and just adding additional ones) and can't find
anything wrong now. But maybe there's something I missed.
Revised patch attached.
--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/pgadmin/dlg/dlgType.cpp b/pgadmin/dlg/dlgType.cpp
index 5f3a36c..bba4a1e 100644
--- a/pgadmin/dlg/dlgType.cpp
+++ b/pgadmin/dlg/dlgType.cpp
@@ -285,6 +285,7 @@ int dlgType::Go(bool modal)
memberLengths.Add(typeLength);
memberPrecisions.Add(typePrecision);
memberCollations.Add(elements.Item(i + 2));
+ memberOriginalNames.Add(elements.Item(i));
}
cbDatatype->Enable(changeok);
@@ -446,36 +447,38 @@ void dlgType::OnSelChangeTypOrLen(wxCommandEvent &ev)
void dlgType::CheckChange()
{
+ bool enable = true;
+
+ if (rdbType->GetSelection() == TYPE_COMPOSITE)
+ {
+ CheckValid(enable, lstMembers->GetItemCount() > 1, _("Please specify at least two members."));
+ }
+ else if (rdbType->GetSelection() == TYPE_ENUM)
+ {
+ CheckValid(enable, lstLabels->GetItemCount() >= 1, _("Please specify at least one label."));
+ }
+ else
+ {
+ txtLength->Enable(!chkVariable->GetValue());
+ CheckValid(enable, cbInput->GetCurrentSelection() >= 0, _("Please specify input conversion function."));
+ CheckValid(enable, cbOutput->GetCurrentSelection() >= 0, _("Please specify output conversion function."));
+ CheckValid(enable, chkVariable->GetValue() || StrToLong(txtLength->GetValue()) > 0, _("Please specify internal storage length."));
+ }
+
if (type)
{
- EnableOK(txtComment->GetValue() != type->GetComment()
+ EnableOK(enable && (txtComment->GetValue() != type->GetComment()
|| cbOwner->GetValue() != type->GetOwner()
|| (rdbType->GetSelection() == TYPE_COMPOSITE && GetSqlForTypes() != wxEmptyString)
- || (GetSql().Length() > 0 && connection->BackendMinimumVersion(9, 1)));
+ || (GetSql().Length() > 0 && connection->BackendMinimumVersion(9, 1))));
}
else
{
wxString name = GetName();
- bool enable = true;
CheckValid(enable, !name.IsEmpty(), _("Please specify name."));
CheckValid(enable, !name.StartsWith(wxT("_")), _("Name may not start with '_'."));
- if (rdbType->GetSelection() == TYPE_COMPOSITE)
- {
- CheckValid(enable, lstMembers->GetItemCount() > 1, _("Please specify at least two members."));
- }
- else if (rdbType->GetSelection() == TYPE_ENUM)
- {
- CheckValid(enable, lstLabels->GetItemCount() >= 1, _("Please specify at least one label."));
- }
- else
- {
- txtLength->Enable(!chkVariable->GetValue());
- CheckValid(enable, cbInput->GetCurrentSelection() >= 0, _("Please specify input conversion function."));
- CheckValid(enable, cbOutput->GetCurrentSelection() >= 0, _("Please specify output conversion function."));
- CheckValid(enable, chkVariable->GetValue() || StrToLong(txtLength->GetValue()) > 0, _("Please specify internal storage length."));
- }
EnableOK(enable);
}
}
@@ -533,6 +536,7 @@ void dlgType::OnMemberAdd(wxCommandEvent &ev)
memberLengths.Add(length);
memberPrecisions.Add(precision);
memberCollations.Add(collation);
+ memberOriginalNames.Add(wxEmptyString);
}
CheckChange();
@@ -596,6 +600,7 @@ void dlgType::OnMemberRemove(wxCommandEvent &ev)
memberLengths.RemoveAt(pos);
memberPrecisions.RemoveAt(pos);
memberCollations.RemoveAt(pos);
+ memberOriginalNames.RemoveAt(pos);
}
CheckChange();
}
@@ -833,7 +838,7 @@ wxString dlgType::GetFullTypeName(int type)
typname += wxT(",") + memberPrecisions.Item(type);
typname += wxT(")");
}
- if (!memberPrecisions.Item(type).IsEmpty() && memberPrecisions.Item(type) != wxT("pg_catalog.\"default\""))
+ if (!memberCollations.Item(type).IsEmpty() && memberCollations.Item(type) != wxT("pg_catalog.\"default\""))
typname += wxT(" COLLATE ") + memberCollations.Item(type);
return typname;
@@ -842,45 +847,108 @@ wxString dlgType::GetFullTypeName(int type)
wxString dlgType::GetSqlForTypes()
{
wxString sql = wxEmptyString;
- wxString old_name, old_type, old_collation, new_name, new_type, new_collation;
+ wxString objname, old_name, old_type, old_collation, new_name, new_type, new_full_type, new_collation, original_name;
wxArrayString elements = type->GetTypesArray();
- bool modified = lstMembers->GetItemCount() * 3 != (int)elements.GetCount();
- size_t i;
+ size_t newindex;
+ size_t oldindex = 0;
+ int hold = 0;
+ objname = schema->GetQuotedPrefix() + qtIdent(GetName());
- // Check if there is a change
- for (int i = 0 ; i < lstMembers->GetItemCount() && !modified; i++)
+ for (newindex = 0 ; newindex < lstMembers->GetItemCount() ; newindex = newindex + 1 - hold)
{
- old_name = elements.Item(i * 3);
- old_type = elements.Item(i * 3 + 1);
- old_collation = elements.Item(i * 3 + 2);
- new_name = lstMembers->GetItemText(i);
- new_type = GetFullTypeName(i);
- new_collation = memberCollations.Item(i);
- modified = modified || old_name != new_name
- || old_type != new_type
- || old_collation != new_collation;
- }
+ // this will decide whether we progress to the next new item,
+ // or whether we need to continue checking the old list first
+ hold = 0;
- if (modified)
- {
- // Drop all old attributes
- for (i = 0 ; i < elements.GetCount() ; i += 3)
+ // these are a copy of the list before any changes
+ if (elements.GetCount() >= (oldindex * 3) + 3)
{
- old_name = elements.Item(i);
- sql += wxT("ALTER TYPE ") + type->GetName() + wxT("\n DROP ATTRIBUTE ") + old_name + wxT(";\n");
+ old_name = elements.Item(oldindex * 3);
+ old_type = elements.Item(oldindex * 3 + 1);
+ old_collation = elements.Item(oldindex * 3 + 2);
}
+ else
+ {
+ // we've now used up all the old attributes
+ old_name = wxEmptyString;
+ old_type = wxEmptyString;
+ old_collation = wxEmptyString;
+ }
+
+ // this is the original name of the type before editing
+ original_name = memberOriginalNames.Item(newindex);
- // Add all new attributes
- for (int i = 0 ; i < lstMembers->GetItemCount() ; i++)
+ new_name = lstMembers->GetItemText(newindex);
+ new_type = memberTypes.Item(newindex).AfterFirst(':');
+ new_full_type = GetFullTypeName(newindex);
+ new_collation = memberCollations.Item(newindex);
+
+ if (!original_name.IsEmpty() && original_name == old_name && (new_name != old_name
+ || new_type != old_type || new_collation != old_collation))
{
- new_name = lstMembers->GetItemText(i);
- new_type = GetFullTypeName(i);
- new_collation = memberCollations.Item(i);
- sql += wxT("ALTER TYPE ") + type->GetName() + wxT("\n ADD ATTRIBUTE ")
- + new_name + wxT(" ") + new_type;
- if (!new_collation.IsEmpty() && new_collation != wxT("pg_catalog.\"default\""))
- sql += wxT(" COLLATE ") + new_collation;
- sql += wxT(";\n");
+ // if this was originally in the list and the name has changed then rename it
+
+ if (new_name != old_name)
+ {
+ sql += wxT("ALTER TYPE ") + objname + wxT("\n RENAME ATTRIBUTE ")
+ + qtIdent(original_name) + wxT(" TO ") + qtIdent(new_name) + wxT(";\n");
+ }
+
+ if (new_type != old_type || new_collation != old_collation)
+ {
+ sql += wxT("ALTER TYPE ") + objname + wxT("\n ALTER ATTRIBUTE ")
+ + qtIdent(new_name);
+
+ // the syntax for alter attribute requires that we always specify the type
+ sql += wxT(" SET DATA TYPE ") + new_type;
+
+ if (new_collation != old_collation)
+ sql += wxT(" COLLATE ") + new_collation;
+
+ sql += wxT(";\n");
+ }
+ }
+ else if (!original_name.IsEmpty() && original_name != old_name)
+ {
+ // the old attribute isn't in the new list so drop it
+
+ // don't move through new list yet
+ hold = 1;
+
+ sql += wxT("ALTER TYPE ") + objname + wxT("\n DROP ATTRIBUTE ")
+ + qtIdent(old_name) + wxT(";\n");
+ }
+ else if (original_name.IsEmpty())
+ {
+ if (!old_name.IsEmpty())
+ {
+ sql += wxT("ALTER TYPE ") + objname + wxT("\n DROP ATTRIBUTE ")
+ + qtIdent(old_name) + wxT(";\n");
+ }
+
+ sql += wxT("ALTER TYPE ") + objname + wxT("\n ADD ATTRIBUTE ")
+ + qtIdent(new_name) + wxT(" ") + new_full_type + wxT(";\n");
+ }
+ else
+ {
+ // do nothing
+ }
+
+ oldindex++;
+
+ if (newindex + 1 == lstMembers->GetItemCount() && elements.GetCount() >= (oldindex * 3) + 3)
+ {
+ // remove remaining old attributes
+ for (;elements.GetCount() >= (oldindex * 3) + 3;oldindex++)
+ {
+ old_name = elements.Item(oldindex * 3);
+ old_type = elements.Item(oldindex * 3 + 1);
+ old_collation = elements.Item(oldindex * 3 + 2);
+
+ sql += wxT("ALTER TYPE ") + objname + wxT("\n DROP ATTRIBUTE ")
+ + qtIdent(old_name) + wxT(";\n");
+ }
+ break;
}
}
diff --git a/pgadmin/include/dlg/dlgType.h b/pgadmin/include/dlg/dlgType.h
index d618221..8a3481b 100644
--- a/pgadmin/include/dlg/dlgType.h
+++ b/pgadmin/include/dlg/dlgType.h
@@ -54,7 +54,7 @@ private:
void showDefinition(int panel);
wxString GetFullTypeName(int type);
- wxArrayString memberTypes, memberLengths, memberPrecisions, memberCollations;
+ wxArrayString memberTypes, memberLengths, memberPrecisions, memberCollations, memberOriginalNames;
bool queriesToBeSplitted;
DECLARE_EVENT_TABLE()
--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
