Re: Overhaul of type attributes modification

On Thu, 2011-07-14 at 12:30 +0100, Thom Brown wrote:
> On 14 July 2011 10:28, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> > On Thu, Jul 14, 2011 at 10:22 AM, Guillaume Lelarge
> > <guillaume(at)lelarge(dot)info> wrote:
> >> On Mon, 2011-07-11 at 15:29 +0100, Thom Brown wrote:
> >>> On 11 July 2011 15:21, Guillaume Lelarge <guillaume(at)lelarge(dot)info> wrote:
> >>> > On Sun, 2011-07-10 at 01:05 +0100, Thom Brown wrote:
> >>> >> On 9 July 2011 23:58, Thom Brown <thom(at)linux(dot)com> wrote:
> >>> >> > I've found a corner-case bug btw, which requires a tiny amendment.
> >>> >> > But I'm also rebasing the patch for current master and 1.14_patches
> >>> >> > (or whatever it's called), so you'll get these shortly
> >>> >>
> >>> >> Okay, here are the two rebased patches, both with the extra fix:
> >>> >>
> >>> >> Patch for REL-1_14_0_PATCHES:
> >>> >> update_composite_type_attributes_sql_v2_rebased_1.14.patch
> >>> >>
> >>> >> Patch for master: update_composite_type_attributes_sql_v2_rebased_master.patch
> >>> >>
> >>> >
> >>> > Thanks. Can't reproduce the bugs I found earlier, but found a new one.
> >>> >
> >>> > Suppose the following type:
> >>> >
> >>> > CREATE TYPE s1.ty3 AS
> >>> > (c1 uuid,
> >>> > c2 uuid);
> >>> >
> >>> > If I remove the first element, the SQL textbox is empty. It will
> >>> > probably help you to know that I get the same behaviour when I delete
> >>> > the three first elements on a 4-elements type, but not if I delete less
> >>> > than three for this type.
> >>> >
> >>> > Sorry. Hope you won't kill me next time we meet :)
> >>>
> >>> No, that's not a bug. That's something I changed. This is what I
> >>> posted before:
> >>>
> >>> "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."
> >>>
> >>
> >> Oh OK, you're definitely right. Sorry about this.
> >>
> >> I don't like the checks but, if they were already there, it makes sense
> >> to apply them on creation and alteration of a type.
> >>
> >>> If my assumption is flawed, please feel free to remove that part of
> >>> the change. In fact I'm wondering if we should have that restriction
> >>> for either case (CREATE and ALTER) since you can actually create
> >>> composite types with no sub-types within them. I just wanted to unify
> >>> the logic between the two.
> >>>
> >>
> >> And that's a good idea.
> >>
> >> Well, your patch seems good to me. I've commited the bug fix (on the
> >> collation clause) on the 1.14 master, and the complete patch on the
> >> master branch. The patch is pretty invasive for something that's not a
> >> bugfix, so I don't commit the complete patch to the 1.14 branch. Dave
> >> and Thom, maybe you have a different opinion on this?
> >
> > I'd rather keep it for master only.
>
> If that's the case, someone will need to fix the bugs in 1.14 where
> changing attributes results in possibly using the wrong data types in
> the add attribute definitions as I posted in my 2nd message.
>

I don't have the issue at all. I tried a few other things, I looked into
the code, nothing feels wrong to me. Must have been fixed somehow, I
guess.

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