Re: pgAdmin III commit: Lots of work on domains, and check constraints

On Tue, Sep 4, 2012 at 10:05 PM, Guillaume Lelarge
<guillaume(at)lelarge(dot)info> wrote:
> On Mon, 2012-09-03 at 22:41 +0200, Guillaume Lelarge wrote:
>> On Mon, 2012-09-03 at 08:54 +0100, Dave Page wrote:
>> > On Fri, Aug 31, 2012 at 10:57 PM, Guillaume Lelarge
>> > <guillaume(at)lelarge(dot)info> wrote:
>> > > On Sun, 2012-08-26 at 18:18 +0200, Guillaume Lelarge wrote:
>> > >> On Mon, 2012-07-23 at 08:38 +0100, Dave Page wrote:
>> > >> > On Sat, Jul 21, 2012 at 1:40 PM, Guillaume Lelarge
>> > >> > <guillaume(at)lelarge(dot)info> wrote:
>> > >> > > On Wed, 2012-06-06 at 10:50 +0600, Timon wrote:
>> > >> > >> seems that this commit broke reindexing of selected index. steps to reproduce:
>> > >> > >> 1) create table
>> > >> > >> 2) create index
>> > >> > >> 3) select index in object inspector
>> > >> > >> 4) try to reindex it via maintenance menu item
>> > >> > >> 5) got error : ERROR: schema "table_name" does not exist
>> > >> > >>
>> > >> > >> and one more crash here
>> > >> > >> .. same steps as before
>> > >> > >> 4) try to CLUSTER index
>> > >> > >> 5) pgadmin simply crashed
>> > >> > >>
>> > >> > >
>> > >> > > OK, I finally got some time to work on this. As Timon said, these bugs
>> > >> > > come from the patch "Lots of work on domains, and check constraints". In
>> > >> > > this patch, I changed some objects parent class from pgTableObject to
>> > >> > > pgSchemaObject. Due to this change, the GetTable() method returns NULL,
>> > >> > > which segfaults all statements that try to use the return value without
>> > >> > > checking. The two examples above from Timon are exactly this.
>> > >> > >
>> > >> > > I don't see many ways to get out of this issue.
>> > >> > >
>> > >> > > We could use GetSchema() instead of GetTable(). It works, it's an easy
>> > >> > > and small patch. But it'll certainly be a maintenance nightmare (at
>> > >> > > least without any comments)
>> > >> > >
>> > >> > > We could also revert my patch. It's simple, we loose the feature of
>> > >> > > adding as many check constraints as we want to a domain, we loose the
>> > >> > > feature of renaming and validating constraints, and we gain a few bugs.
>> > >> > >
>> > >> > > I don't see any other options. My own personal choice would be the first
>> > >> > > one (see attached patch). But it's a tough call.
>> > >> >
>> > >> > We've run into problems in the past every time we've tried to share a
>> > >> > sub-class between two parents. I think we should stop trying to do
>> > >> > that, and just resign ourselves to having to duplicate the class - I
>> > >> > guess pgCheckConstraint and pgDomainCheckConstraint is the way to go.
>> > >>
>> > >> I don't think I'll have the time and motivation to work on this before
>> > >> we go GA. I guess I'll have to do this later on but in the mean time,
>> > >> should I revert my commit or apply this patch?
>> > >>
>> > >
>> > > Dave, any comment?
>> >
>> > What does the patch look like? As long as it's safe, well commented,
>> > and overall the new code is an improvement, it seems like it's the
>> > best option.
>> >
>>
>> I'll work on it tomorrow. If it sounds good enough to me, I'll apply it.
>> Otherwise, I'll revert my old patch.
>>
>
> Done. I cannot say that it will fix all issues, but at least it fixes
> the one I know. There's still an issue found by Erwin, that I can't be
> sure because the trac website is still down.

Sorry - for some reason when you reported this before I got it into my
head that you meant redmine.postgresql.org. Trac is fixed now.

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

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