Re: Overhaul of type attributes modification - Mailing list pgadmin-hackers

On Fri, 2011-07-08 at 16:01 +0100, Thom Brown wrote:
> On 8 July 2011 15:38, Thom Brown <thom@linux.com> wrote:
> > On 8 July 2011 15:19, Thom Brown <thom@linux.com> wrote:
> >> Hi all,
> >>
> >> I noticed that if you add, delete, rename or change the type,
> >> collation or precision of a composite type attribute, it deletes all
> >> of them then adds them all back in.  Obviously attribute additions,
> >> deletions and modifications may only occur for types under PostgreSQL
> >> 9.1, but I thought it a bit extreme to actually drop all types if
> >> you're adding a new one in, or just removing one.  Also, if you rename
> >> a type, it drops all attributes again then re-adds them, just to have
> >> an attribute with a different name.
> >>
> >> So I've attached a patch which "does the right thing".  Take the
> >> example of the following type:
> >>
> >> CREATE TYPE bark AS
> >>   (one text,
> >>    two text,
> >>    three text,
> >>    four text,
> >>    five text);
> >>
> >> Say we wanted to remove "two", change the type of three to uuid,
> >> rename four to forty and add an extra text type of six.
> >>
> >> Normally we'd just get:
> >>
> >> ALTER TYPE bark DROP ATTRIBUTE one;
> >> ALTER TYPE bark DROP ATTRIBUTE two;
> >> ALTER TYPE bark DROP ATTRIBUTE three;
> >> ALTER TYPE bark DROP ATTRIBUTE four;
> >> ALTER TYPE bark DROP ATTRIBUTE five;
> >> ALTER TYPE bark ADD ATTRIBUTE one text;
> >> ALTER TYPE bark ADD ATTRIBUTE three text;
> >> ALTER TYPE bark ADD ATTRIBUTE forty uuid;
> >> ALTER TYPE bark ADD ATTRIBUTE five text;
> >> ALTER TYPE bark ADD ATTRIBUTE six uuid;
> >>
> >> With these changes we'd now get:
> >>
> >> ALTER TYPE bark DROP ATTRIBUTE two;
> >> ALTER TYPE bark RENAME ATTRIBUTE four TO forty;
> >> ALTER TYPE bark ADD ATTRIBUTE six text;
> >>
> >> .. except now those are also nicely indented to be more readable for
> >> types with long schemas/type names.
> >>
> >> e.g.
> >> ALTER TYPE long_schema_name.quite_a_long_table_name
> >>  ADD ATTRIBUTE "suspiciously long attribute name"?
> >>
> >> It also fixes a bug whereby if you have a precision specified, the
> >> word COLLATE mysteriously appears after the type whether or not you
> >> have a collation assigned because the collation condition was based on
> >> the precision being present for some reason.  And if you actually
> >> assigned a collation for a valid data type, it wouldn't appear at all,
> >> so that's fixed too.
> >
> > Actually I noticed that the example I gave of what would occur before
> > the patch highlights another bug.  Notice that forty and six have the
> > type of uuid, and three doesn't?  Three should have been the one with
> > uuid, and forty and sixty should have been text.  That's not my typo
> > as that's come from PgAdmin 1.14 beta 2.  So it was actually assigning
> > the wrong data types in those case too.  So the patch fixes those
> > problems too.
>
> Also I noticed that my example of the new output doesn't show the
> three datatype being changed to uuid.  This was just me forgetting to
> click the Change button when I changed the type, so it would actually
> come out as:
>
> ALTER TYPE bark DROP ATTRIBUTE two;
> ALTER TYPE bark DROP ATTRIBUTE three;
> ALTER TYPE bark ADD ATTRIBUTE three uuid;
> ALTER TYPE bark RENAME ATTRIBUTE four TO forty;
> ALTER TYPE bark ADD ATTRIBUTE six text;
>

I took a look at this patch. The new function is really good (I love the
"hold" trick), but I have found two issues, one major and one minor.

The minor issue is simple, and could probably get fixed easily. 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 remove c4, and change c3's name and type (say c3b and xml type), I
get this SQL query:

ALTER TYPE s1.ty2
  ADD ATTRIBUTE c3b xml;
ALTER TYPE s1.ty2
  DROP ATTRIBUTE c3;

I'm not sure why, but it forgets about the c4 removal. I kind of fixed
it, but then I stumbled onto another issue, the major one.

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.

One solution, if we don't want to keep the "drop all, add all" method,
is that an existing member with a new type or collation should always be
added at the end of the list. Another solution, we can allow someone to
change a member position. PostgreSQL doesn't allow that, but pgAdmin
could do it with ADD and DROP nicely combined. But I guess the
GetSqlForTypes() method will be pretty hard to write.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com