Обсуждение: Patch for column properties
Hi, When someone set a default value for a column and set NOT NULL at the same time, the query could fail if the table already contains NULL values for this column. This patch adds an UPDATE statement to put the DEFAULT value in this column for all NULL values (but only when you SET NULL and SET DEFAULT the column at the same time). I've done this patch for a friend but I don't quite like this patch because of its behaviour. It doesn't warn the user. I think it would be better to show a dialog telling that an UPDATE statement will be executed because otherwise the whole action could fail. So here are some questions : * do you think this patch is worth it ? * do I need to show a confirmation dialog ? * do I need to check first if there's NULL values on this column ? Thanks. Regards. -- Guillaume. <!-- http://abs.traduc.org/ http://lfs.traduc.org/ http://docs.postgresqlfr.org/ --> Index: dlgColumn.cpp =================================================================== --- dlgColumn.cpp (révision 5962) +++ dlgColumn.cpp (copie de travail) @@ -252,6 +252,17 @@ sql += wxT(";\n"); } + + if (txtDefault->GetValue() != column->GetDefault() + && !txtDefault->GetValue().IsEmpty() + && chkNotNull->GetValue() != column->GetNotNull() + && chkNotNull->GetValue()) + { + sql += wxT("UPDATE ") + table->GetQuotedFullIdentifier() + + wxT("\n SET ") + qtIdent(name) + wxT("=") + txtDefault->GetValue() + + wxT("\n WHERE ") + qtIdent(name) + wxT(" IS NULL;\n"); + } + if (chkNotNull->GetValue() != column->GetNotNull()) { sql += wxT("ALTER TABLE ") + table->GetQuotedFullIdentifier()
Guillaume Lelarge wrote: > Hi, > > When someone set a default value for a column and set NOT NULL at the > same time, the query could fail if the table already contains NULL > values for this column. This patch adds an UPDATE statement to put the > DEFAULT value in this column for all NULL values (but only when you SET > NULL and SET DEFAULT the column at the same time). > > I've done this patch for a friend but I don't quite like this patch > because of its behaviour. It doesn't warn the user. I think it would be > better to show a dialog telling that an UPDATE statement will be > executed because otherwise the whole action could fail. > > So here are some questions : > * do you think this patch is worth it ? I'm not keen on the idea.I prefer to leave that sort of behaviour to the server, *if* thats the way it gets written. Regards, Dave
Dave Page wrote: > Guillaume Lelarge wrote: >> Hi, >> >> When someone set a default value for a column and set NOT NULL at the >> same time, the query could fail if the table already contains NULL >> values for this column. This patch adds an UPDATE statement to put the >> DEFAULT value in this column for all NULL values (but only when you >> SET NULL and SET DEFAULT the column at the same time). >> >> I've done this patch for a friend but I don't quite like this patch >> because of its behaviour. It doesn't warn the user. I think it would >> be better to show a dialog telling that an UPDATE statement will be >> executed because otherwise the whole action could fail. >> >> So here are some questions : >> * do you think this patch is worth it ? > > I'm not keen on the idea.I prefer to leave that sort of behaviour to the > server, *if* thats the way it gets written. You could first execute "alter table .. set default ...; alter table ..set not null" first, and only if the "set not null" fails ask the user if he wants the update to be performed. I personally would like the feature I think, though I can easily live without it ;-) greetings, Florian Pflug
Dave Page a écrit : > Guillaume Lelarge wrote: >> Hi, >> >> When someone set a default value for a column and set NOT NULL at the >> same time, the query could fail if the table already contains NULL >> values for this column. This patch adds an UPDATE statement to put the >> DEFAULT value in this column for all NULL values (but only when you >> SET NULL and SET DEFAULT the column at the same time). >> >> I've done this patch for a friend but I don't quite like this patch >> because of its behaviour. It doesn't warn the user. I think it would >> be better to show a dialog telling that an UPDATE statement will be >> executed because otherwise the whole action could fail. >> >> So here are some questions : >> * do you think this patch is worth it ? > > I'm not keen on the idea.I prefer to leave that sort of behaviour to the > server, *if* thats the way it gets written. > I don't think this is something that will be done ever by the server. We have everything we want to be able to dot it. Each statement is available. I think this is more a user app issue than a server issue. But I understand you're not confortable adding this behavior. I know I'm not. Perhaps this can be available as on option, disabled by default ? Regards. -- Guillaume. <!-- http://abs.traduc.org/ http://lfs.traduc.org/ http://docs.postgresqlfr.org/ -->
Florian G. Pflug a écrit : > Dave Page wrote: >> Guillaume Lelarge wrote: >>> Hi, >>> >>> When someone set a default value for a column and set NOT NULL at the >>> same time, the query could fail if the table already contains NULL >>> values for this column. This patch adds an UPDATE statement to put >>> the DEFAULT value in this column for all NULL values (but only when >>> you SET NULL and SET DEFAULT the column at the same time). >>> >>> I've done this patch for a friend but I don't quite like this patch >>> because of its behaviour. It doesn't warn the user. I think it would >>> be better to show a dialog telling that an UPDATE statement will be >>> executed because otherwise the whole action could fail. >>> >>> So here are some questions : >>> * do you think this patch is worth it ? >> >> I'm not keen on the idea.I prefer to leave that sort of behaviour to >> the server, *if* thats the way it gets written. > > You could first execute "alter table .. set default ...; alter table > ..set not null" first, and only if the "set not null" fails ask the user > if he wants > the update to be performed. I personally would like the feature I think, > though I can easily live without it ;-) > Perhaps we can do this with a better error message. There's something I really like with EMS SQL Manager. When the user change a property, the SQL is shown and can be changed. If the COMMIT failed, they show you the SQL and it also can be modified. Perhaps we just need this ? When there is a PostgreSQL related error message, pgAdmin shows the SQL it tried to execute and let the user change it. I think it would be a better behavior than what my patch implied. Would something like this be possible ? And one more question, I wonder why the SQL tab is not editable ? I can think of one reason but I'm not sure :) Regards. -- Guillaume. <!-- http://abs.traduc.org/ http://lfs.traduc.org/ http://docs.postgresqlfr.org/ -->
Guillaume Lelarge wrote: > There's something I really like with EMS SQL Manager. When the user > change a property, the SQL is shown and can be changed. If the COMMIT > failed, they show you the SQL and it also can be modified. Perhaps we > just need this ? When there is a PostgreSQL related error message, > pgAdmin shows the SQL it tried to execute and let the user change it. I > think it would be a better behavior than what my patch implied. Would > something like this be possible ? Probably - but it seems it would be a fair amount of work. Feel free to look into it if you like. > And one more question, I wonder why > the SQL tab is not editable ? I can think of one reason but I'm not sure :) Because it's dynamically generated, and any changes would either be lost if the user changed tab, or would have to be reverse engineered back into the other dialog controls. Also, on some dialogs there are placeholders included in the code for cases when we have multi-step queries that are tied together by a generated ID. IF the user mucked about with those, it could break things particularly spectaularly. Regards, Dave
Dave Page a écrit : > Guillaume Lelarge wrote: >> There's something I really like with EMS SQL Manager. When the user >> change a property, the SQL is shown and can be changed. If the COMMIT >> failed, they show you the SQL and it also can be modified. Perhaps we >> just need this ? When there is a PostgreSQL related error message, >> pgAdmin shows the SQL it tried to execute and let the user change it. >> I think it would be a better behavior than what my patch implied. >> Would something like this be possible ? > > Probably - but it seems it would be a fair amount of work. Feel free to > look into it if you like. > I'll get a look at it. But I understand this could be a lot of work. >> And one more question, I wonder why the SQL tab is not editable ? I >> can think of one reason but I'm not sure :) > > Because it's dynamically generated, and any changes would either be lost > if the user changed tab, or would have to be reverse engineered back > into the other dialog controls. That's what I thought. > Also, on some dialogs there are > placeholders included in the code for cases when we have multi-step > queries that are tied together by a generated ID. IF the user mucked > about with those, it could break things particularly spectaularly. > Can you tell me where I can find such multi-step queries ? Thanks. Regards. -- Guillaume. <!-- http://abs.traduc.org/ http://lfs.traduc.org/ http://docs.postgresqlfr.org/ -->
Dave Page a écrit : > [...] >> And one more question, I wonder why the SQL tab is not editable ? I >> can think of one reason but I'm not sure :) > > Because it's dynamically generated, and any changes would either be lost > if the user changed tab, or would have to be reverse engineered back > into the other dialog controls. There's probably another way of doing it. We can add a checkbox on the SQL tab. If the user clicks on it, it enables editing the SQL text field and pgAdmin disables the other tabs so we don't have to reverse engineer. If the user clicks on it once again, pgAdmin re-enables the other tabs, disables editing the SQL text field and refresh its content. > Also, on some dialogs there are > placeholders included in the code for cases when we have multi-step > queries that are tied together by a generated ID. IF the user mucked > about with those, it could break things particularly spectaularly. > But this would not work with such a checkbox. Regards. -- Guillaume. <!-- http://abs.traduc.org/ http://lfs.traduc.org/ http://docs.postgresqlfr.org/ -->
Guillaume Lelarge wrote: > Can you tell me where I can find such multi-step queries ? Thanks. Look at the pgAgent job code - for example, dlgJob::GetInsertSql() in http://svn.pgadmin.org/cgi-bin/viewcvs.cgi/trunk/pgadmin3/pgadmin/agent/dlgJob.cpp?rev=5828&view=markup Regards Dave