Thread: pgAdmin III commit: Lots of work on domains, and check constraints
Lots of work on domains, and check constraints Check constraints on domains are now sub-nodes. A user can add as many check constraints as he wants. He can rename and validate them on 9.2. He can add a not-yet-valid check constraint. He can also add a NO INHERIT check constraint on a new table. Branch ------ master Details ------- http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=02b60e5a8b4f6c3cfafca1b93eff4833ddc7ae4f Modified Files -------------- pgadmin/dlg/dlgCheck.cpp | 44 ++++-- pgadmin/dlg/dlgDomain.cpp | 214 ++++++++++++++++++++++------ pgadmin/dlg/dlgProperty.cpp | 13 +- pgadmin/frm/events.cpp | 8 +- pgadmin/include/dlg/dlgCheck.h | 6 +- pgadmin/include/dlg/dlgDomain.h | 6 + pgadmin/include/schema/pgCheck.h | 42 ++++-- pgadmin/include/schema/pgConstraints.h | 10 +- pgadmin/include/schema/pgDomain.h | 1 + pgadmin/include/schema/pgForeignKey.h | 6 +- pgadmin/include/schema/pgIndex.h | 14 +- pgadmin/include/schema/pgIndexConstraint.h | 16 +- pgadmin/include/utils/misc.h | 1 + pgadmin/schema/pgCheck.cpp | 80 +++++++---- pgadmin/schema/pgConstraints.cpp | 51 +++++--- pgadmin/schema/pgDomain.cpp | 95 ++++++++----- pgadmin/schema/pgForeignKey.cpp | 15 ++- pgadmin/schema/pgIndex.cpp | 28 ++-- pgadmin/schema/pgTable.cpp | 2 + pgadmin/ui/dlgCheck.xrc | 14 ++ pgadmin/ui/dlgDomain.xrc | 95 ++++++++----- 21 files changed, 517 insertions(+), 244 deletions(-)
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 2012/5/1 Guillaume Lelarge <guillaume@lelarge.info>: > Lots of work on domains, and check constraints > > Check constraints on domains are now sub-nodes. A user can add as many check > constraints as he wants. He can rename and validate them on 9.2. He can add > a not-yet-valid check constraint. He can also add a NO INHERIT check constraint > on a new table. > > Branch > ------ > master > > Details > ------- > http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=02b60e5a8b4f6c3cfafca1b93eff4833ddc7ae4f > > Modified Files > -------------- > pgadmin/dlg/dlgCheck.cpp | 44 ++++-- > pgadmin/dlg/dlgDomain.cpp | 214 ++++++++++++++++++++++------ > pgadmin/dlg/dlgProperty.cpp | 13 +- > pgadmin/frm/events.cpp | 8 +- > pgadmin/include/dlg/dlgCheck.h | 6 +- > pgadmin/include/dlg/dlgDomain.h | 6 + > pgadmin/include/schema/pgCheck.h | 42 ++++-- > pgadmin/include/schema/pgConstraints.h | 10 +- > pgadmin/include/schema/pgDomain.h | 1 + > pgadmin/include/schema/pgForeignKey.h | 6 +- > pgadmin/include/schema/pgIndex.h | 14 +- > pgadmin/include/schema/pgIndexConstraint.h | 16 +- > pgadmin/include/utils/misc.h | 1 + > pgadmin/schema/pgCheck.cpp | 80 +++++++---- > pgadmin/schema/pgConstraints.cpp | 51 +++++--- > pgadmin/schema/pgDomain.cpp | 95 ++++++++----- > pgadmin/schema/pgForeignKey.cpp | 15 ++- > pgadmin/schema/pgIndex.cpp | 28 ++-- > pgadmin/schema/pgTable.cpp | 2 + > pgadmin/ui/dlgCheck.xrc | 14 ++ > pgadmin/ui/dlgDomain.xrc | 95 ++++++++----- > 21 files changed, 517 insertions(+), 244 deletions(-) > > > -- > Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgadmin-hackers -- All bugs reserved
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. Comments? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
Attachment
On Sat, Jul 21, 2012 at 1:40 PM, Guillaume Lelarge <guillaume@lelarge.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. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I tested this patch. It fixes my problem. Also it fixes one annoying crash when you click on indexes list. thanks for you work :) 2012/7/21 Guillaume Lelarge <guillaume@lelarge.info>: > 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. > > Comments? > > > -- > Guillaume > http://blog.guillaume.lelarge.info > http://www.dalibo.com -- All bugs reserved
and, one more little bug altering index fillfactor produces wrong sql code steps to reproduce: 1. create table 2. add index 3. chage fillfactor on created index 4. got error like "scheme name <table name> doesn't exists" example of sql: ALTER INDEX gallery.idx_gallery_regionid_itemiddesc SET (FILLFACTOR=80); gallery is the table name 2012/8/17 Timon <timosha@gmail.com>: > I tested this patch. It fixes my problem. > Also it fixes one annoying crash when you click on indexes list. > thanks for you work :) > > 2012/7/21 Guillaume Lelarge <guillaume@lelarge.info>: >> 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. >> >> Comments? >> >> >> -- >> Guillaume >> http://blog.guillaume.lelarge.info >> http://www.dalibo.com > > > > -- > All bugs reserved -- All bugs reserved
On Mon, 2012-07-23 at 08:38 +0100, Dave Page wrote: > On Sat, Jul 21, 2012 at 1:40 PM, Guillaume Lelarge > <guillaume@lelarge.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? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Fri, 2012-08-17 at 11:39 +0600, Timon wrote: > and, one more little bug > altering index fillfactor produces wrong sql code > steps to reproduce: > > 1. create table > 2. add index > 3. chage fillfactor on created index > 4. got error like "scheme name <table name> doesn't exists" > example of sql: ALTER INDEX gallery.idx_gallery_regionid_itemiddesc > SET (FILLFACTOR=80); > gallery is the table name > Thanks, I modified my patch to take care of this too. New (rebased) patch attached. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
Attachment
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@lelarge.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? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Fri, Aug 31, 2012 at 10:57 PM, Guillaume Lelarge <guillaume@lelarge.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@lelarge.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. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, 2012-09-03 at 08:54 +0100, Dave Page wrote: > On Fri, Aug 31, 2012 at 10:57 PM, Guillaume Lelarge > <guillaume@lelarge.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@lelarge.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. Thanks. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
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@lelarge.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@lelarge.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. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Tue, Sep 4, 2012 at 10:05 PM, Guillaume Lelarge <guillaume@lelarge.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@lelarge.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@lelarge.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
On Wed, 2012-09-05 at 08:25 +0100, Dave Page wrote: > On Tue, Sep 4, 2012 at 10:05 PM, Guillaume Lelarge > <guillaume@lelarge.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@lelarge.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@lelarge.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. > Oh OK, no problem. Thanks for fixing this. The issue found by Erwin is fixed by the patch I applied yesterday. So we're all good (or more, as good as we could :-/ ). -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com