Thread: [pgScript patch] Output + bug fix
Hi pgAdmin hackers, Here is a patch for pgAdmin. I have finally completed the patch for the pgScript outputs and am sorry for the delay. This patch also corrects a bug in pgsPrintStmt.cpp because of a thread double lock in case of exception. I have other updates to send but I would like this one to be committed first. I have noticed while making this patch that the pgQueryThread::GetMessagesAndClear() method does not return anything when my query returns a warning. It did before. The line 185 is: conn->SetLastResultError(NULL); Before, it was: appendMessage(conn->GetLastError() + wxT("\n")); When I put this line back to what it was, I do not have the problem anymore. Is this a problem or is there another way to get the last warning message? Best regards, Mickael
Attachment
On Fri, Mar 6, 2009 at 7:31 PM, Mickael Deloison <mdeloison@gmail.com> wrote: > Hi pgAdmin hackers, > > Here is a patch for pgAdmin. I have finally completed the patch for > the pgScript outputs and am sorry for the delay. > This patch also corrects a bug in pgsPrintStmt.cpp because of a thread > double lock in case of exception. > I have other updates to send but I would like this one to be committed first. Please be quick - I want to get the first beta out early next week. Magnus; can you review the patch please? You're already familiar with pgScript. > I have noticed while making this patch that the > pgQueryThread::GetMessagesAndClear() method does not return anything > when my query returns a warning. It did before. > The line 185 is: > conn->SetLastResultError(NULL); > Before, it was: > appendMessage(conn->GetLastError() + wxT("\n")); > When I put this line back to what it was, I do not have the problem > anymore. Is this a problem or is there another way to get the last > warning message? There were some cases where error messages would be lost, and part of the fix was to try to make errors all go through SetLastResultError() for consistency in output. I modified SetLastResultError() so that if it's passed NULL, it gets the message via: lastResultError.msg_primary = GetLastError(); instead, so it /should/ get the same message. Can you supply an example of how to trigger the warning please? I assume it's a pgScript thing, as I get notices and errors from the backend just fine. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
2009/3/6 Dave Page <dpage@pgadmin.org>: > There were some cases where error messages would be lost, and part of > the fix was to try to make errors all go through SetLastResultError() > for consistency in output. I modified SetLastResultError() so that if > it's passed NULL, it gets the message via: lastResultError.msg_primary > = GetLastError(); instead, so it /should/ get the same message. > > Can you supply an example of how to trigger the warning please? I > assume it's a pgScript thing, as I get notices and errors from the > backend just fine. > Here is the code you requested, I removed the useless parts: pgQueryThread thread(m_app->connection(), stmt); if (thread.Create() == wxTHREAD_NO_ERROR) { if (thread.Run() == wxTHREAD_NO_ERROR) { // ... } if (thread.ReturnCode() != PGRES_COMMAND_OK && thread.ReturnCode() != PGRES_TUPLES_OK) { // ... wxString message(stmt + wxT("\n") + --> thread.GetMessagesAndClear().Strip(wxString::both)); // ... (*m_cout) << message << wxT("\n"); // ... } Now, I always have nothing when I call GetMessagesAndClear().
Sorry - I meant some pgScript code that would create a message so i can figure out where it gets lost. On 3/6/09, Mickael Deloison <mdeloison@gmail.com> wrote: > 2009/3/6 Dave Page <dpage@pgadmin.org>: >> There were some cases where error messages would be lost, and part of >> the fix was to try to make errors all go through SetLastResultError() >> for consistency in output. I modified SetLastResultError() so that if >> it's passed NULL, it gets the message via: lastResultError.msg_primary >> = GetLastError(); instead, so it /should/ get the same message. >> >> Can you supply an example of how to trigger the warning please? I >> assume it's a pgScript thing, as I get notices and errors from the >> backend just fine. >> > > Here is the code you requested, I removed the useless parts: > > pgQueryThread thread(m_app->connection(), stmt); > > if (thread.Create() == wxTHREAD_NO_ERROR) > { > if (thread.Run() == wxTHREAD_NO_ERROR) > { > // ... > } > > if (thread.ReturnCode() != PGRES_COMMAND_OK > && thread.ReturnCode() != PGRES_TUPLES_OK) > { > // ... > wxString message(stmt + wxT("\n") + > --> thread.GetMessagesAndClear().Strip(wxString::both)); > // ... > (*m_cout) << message << wxT("\n"); > // ... > } > > > Now, I always have nothing when I call GetMessagesAndClear(). > -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
2009/3/6 Dave Page <dpage@pgadmin.org>: > Sorry - I meant some pgScript code that would create a message so i > can figure out where it gets lost. >> Here is the code you requested, I removed the useless parts: >> >> pgQueryThread thread(m_app->connection(), stmt); >> >> if (thread.Create() == wxTHREAD_NO_ERROR) >> { >> if (thread.Run() == wxTHREAD_NO_ERROR) >> { >> // ... >> } >> >> if (thread.ReturnCode() != PGRES_COMMAND_OK >> && thread.ReturnCode() != PGRES_TUPLES_OK) >> { >> // ... >> wxString message(stmt + wxT("\n") + >> --> thread.GetMessagesAndClear().Strip(wxString::both)); >> // ... >> (*m_cout) << message << wxT("\n"); >> // ... >> } This was pgScript code. It's in pgadmin/pgscript/expressions/pgsExecute.cpp. This is the expression that executes a query: it displays the warning if the return code was not PGRES_COMMAND_OK or PGRES_TUPLES_OK.
I meant 'code written in pgScript' that would exercise that c++ codepath, but i think i can work something out from your description :) On 3/6/09, Mickael Deloison <mdeloison@gmail.com> wrote: > 2009/3/6 Dave Page <dpage@pgadmin.org>: >> Sorry - I meant some pgScript code that would create a message so i >> can figure out where it gets lost. >>> Here is the code you requested, I removed the useless parts: >>> >>> pgQueryThread thread(m_app->connection(), stmt); >>> >>> if (thread.Create() == wxTHREAD_NO_ERROR) >>> { >>> if (thread.Run() == wxTHREAD_NO_ERROR) >>> { >>> // ... >>> } >>> >>> if (thread.ReturnCode() != PGRES_COMMAND_OK >>> && thread.ReturnCode() != PGRES_TUPLES_OK) >>> { >>> // ... >>> wxString message(stmt + wxT("\n") + >>> --> >>> thread.GetMessagesAndClear().Strip(wxString::both)); >>> // ... >>> (*m_cout) << message << wxT("\n"); >>> // ... >>> } > > This was pgScript code. It's in > pgadmin/pgscript/expressions/pgsExecute.cpp. This is the expression > that executes a query: it displays the warning if the return code was > not PGRES_COMMAND_OK or PGRES_TUPLES_OK. > -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
2009/3/7 Dave Page <dpage@pgadmin.org>: > I meant 'code written in pgScript' that would exercise that c++ > codepath, but i think i can work something out from your description > :) > Ok, you can put anything that returns something else than PGRES_COMMAND_OK or PGRES_TUPLES_OK. For example: SELECT 1 FROM Table_that_does_not_exist; Mickael
On Sat, Mar 7, 2009 at 11:31 AM, Mickael Deloison <mdeloison@gmail.com> wrote: > 2009/3/7 Dave Page <dpage@pgadmin.org>: >> I meant 'code written in pgScript' that would exercise that c++ >> codepath, but i think i can work something out from your description >> :) >> > > Ok, you can put anything that returns something else than > PGRES_COMMAND_OK or PGRES_TUPLES_OK. > For example: > SELECT 1 FROM Table_that_does_not_exist; Yeah, that's what I figured :-). I've reverted the change for now - it doesn't really affect the work I'd done, except to show some messages twice (which always used to be the case). We really need to re-design the error handling here to make things more consistent and less chaotic.... -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Hi! Attached is a modified version of this patch, with one question left to solve. Changes are: * Moved the space after the ] in the prefix into the prefix instead, so it doesn't have to be included in every string everywhere. * Made strings translatable (excluding the prefix) * Moved generate_spaces() into misc.cpp as it can be useful in many places outside pgScript, and reimplemented it using Pad() to make it simpler. One question that remains: please see pgsParameterException.cpp line 28. Should that not also use generate_spaces() and be dependent on the prefix length? //Magnus Mickael Deloison wrote: > Hi pgAdmin hackers, > > Here is a patch for pgAdmin. I have finally completed the patch for > the pgScript outputs and am sorry for the delay. > This patch also corrects a bug in pgsPrintStmt.cpp because of a thread > double lock in case of exception. > I have other updates to send but I would like this one to be committed first. > > I have noticed while making this patch that the > pgQueryThread::GetMessagesAndClear() method does not return anything > when my query returns a warning. It did before. > The line 185 is: > conn->SetLastResultError(NULL); > Before, it was: > appendMessage(conn->GetLastError() + wxT("\n")); > When I put this line back to what it was, I do not have the problem > anymore. Is this a problem or is there another way to get the last > warning message? > > Best regards, > Mickael > > > ------------------------------------------------------------------------ > > Index: include/pgscript/pgScript.h =================================================================== --- include/pgscript/pgScript.h (revision 7650) +++ include/pgscript/pgScript.h (working copy) @@ -28,6 +28,12 @@ #include <wx/txtstrm.h> #define pgsOutputStream wxTextOutputStream +const wxString PGSOUTPGSCRIPT (wxT("[PGSCRIPT ] ")); +const wxString PGSOUTEXCEPTION(wxT("[EXCEPTION] ")); +const wxString PGSOUTQUERY (wxT("[QUERY ] ")); +const wxString PGSOUTWARNING (wxT("[WARNING ] ")); +const wxString PGSOUTERROR (wxT("[ERROR ] ")); + /*** LOGGING ***/ #include "utils/sysLogger.h" Index: include/utils/misc.h =================================================================== --- include/utils/misc.h (revision 7650) +++ include/utils/misc.h (working copy) @@ -100,6 +100,8 @@ wxDateTime StrToDateTime(const wxString &value); OID StrToOid(const wxString& value); +wxString generate_spaces(int length); + // nls aware wxString BoolToYesNo(bool value); wxString NumToStr(long value); Index: pgscript/exceptions/pgsArithmeticException.cpp =================================================================== --- pgscript/exceptions/pgsArithmeticException.cpp (revision 7650) +++ pgscript/exceptions/pgsArithmeticException.cpp (working copy) @@ -24,6 +24,7 @@ const wxString pgsArithmeticException::message() const { - return wxString() << wxT("[EXCEPT] Arithmetic Exception - Operation impossible between ") - << m_left << wxT(" and ") << m_right; + return wxString() << PGSOUTEXCEPTION << + wxString::Format(_("Arithmetic Exception - Operation impossible between '%s' and '%s'"), + m_left.c_str(), m_right.c_str()); } Index: pgscript/exceptions/pgsCastException.cpp =================================================================== --- pgscript/exceptions/pgsCastException.cpp (revision 7650) +++ pgscript/exceptions/pgsCastException.cpp (working copy) @@ -24,6 +24,7 @@ const wxString pgsCastException::message() const { - return wxString() << wxT("[EXCEPT] Cast Exception - Cannot convert ") - << m_value << wxT(" to ") << m_type; + return wxString() << PGSOUTEXCEPTION << + wxString::Format(_(" Cast Exception - Cannot convert '%s' to '%s'"), + m_value.c_str(), m_type.c_str()); } Index: pgscript/exceptions/pgsInterruptException.cpp =================================================================== --- pgscript/exceptions/pgsInterruptException.cpp (revision 7650) +++ pgscript/exceptions/pgsInterruptException.cpp (working copy) @@ -24,5 +24,5 @@ const wxString pgsInterruptException::message() const { - return wxT("[EXCEPT] pgScript interrupted"); + return wxString() << PGSOUTEXCEPTION << _("pgScript interrupted"); } Index: pgscript/exceptions/pgsParameterException.cpp =================================================================== --- pgscript/exceptions/pgsParameterException.cpp (revision 7650) +++ pgscript/exceptions/pgsParameterException.cpp (working copy) @@ -25,7 +25,8 @@ const wxString pgsParameterException::message() const { wxString message(m_message); - message.Replace(wxT("\n"), wxT("\n ")); - return wxString() << wxT("[EXCEPT] Parameter Exception - Some parameters ") - << wxT("are invalid:\n>> ") << message; + message.Replace(wxT("\n"), wxT("\n ")); // FIXME: use length of PGSOUTEXCEPTION? + return wxString() << PGSOUTEXCEPTION << + wxString::Format(_("Parameter Exception - Some parameters are invalid:\n>> %s"), + message.c_str()); } Index: pgscript/exceptions/pgsAssertException.cpp =================================================================== --- pgscript/exceptions/pgsAssertException.cpp (revision 7650) +++ pgscript/exceptions/pgsAssertException.cpp (working copy) @@ -24,5 +24,5 @@ const wxString pgsAssertException::message() const { - return wxString() << wxT("[EXCEPT] Assert Exception - ") << m_message; + return wxString() << PGSOUTEXCEPTION << _("Assert Exception - ") << m_message; } Index: pgscript/expressions/pgsExecute.cpp =================================================================== --- pgscript/expressions/pgsExecute.cpp (revision 7650) +++ pgscript/expressions/pgsExecute.cpp (working copy) @@ -116,12 +116,13 @@ { m_app->LockOutput(); - (*m_cout) << wxT("[WRNING] "); + (*m_cout) << PGSOUTWARNING; wxString message(stmt + wxT("\n") + thread .GetMessagesAndClear().Strip(wxString::both)); wxRegEx multilf(wxT("(\n)+")); multilf.ReplaceAll(&message, wxT("\n")); - message.Replace(wxT("\n"), wxT("\n ")); + message.Replace(wxT("\n"), wxT("\n") + + generate_spaces(PGSOUTWARNING.Length() + 1)); (*m_cout) << message << wxT("\n"); m_app->UnlockOutput(); @@ -133,7 +134,7 @@ { m_app->LockOutput(); - (*m_cout) << wxT("[NOTICE] "); + (*m_cout) << PGSOUTQUERY; wxString message(thread.GetMessagesAndClear() .Strip(wxString::both)); if (!message.IsEmpty()) @@ -142,7 +143,8 @@ message = stmt; wxRegEx multilf(wxT("(\n)+")); multilf.ReplaceAll(&message, wxT("\n")); - message.Replace(wxT("\n"), wxT("\n ")); + message.Replace(wxT("\n"), wxT("\n") + + generate_spaces(PGSOUTQUERY.Length() + 1)); (*m_cout) << message << wxT("\n"); m_app->UnlockOutput(); Index: pgscript/statements/pgsPrintStmt.cpp =================================================================== --- pgscript/statements/pgsPrintStmt.cpp (revision 7650) +++ pgscript/statements/pgsPrintStmt.cpp (working copy) @@ -11,6 +11,7 @@ #include "pgAdmin3.h" #include "pgscript/statements/pgsPrintStmt.h" +#include "pgscript/exceptions/pgsException.h" #include "pgscript/utilities/pgsThread.h" #include "pgscript/utilities/pgsUtilities.h" @@ -33,8 +34,20 @@ m_app->LockOutput(); } - m_cout << wxT("[OUTPUT] ") << wx_static_cast(const wxString, - m_var->eval(vars)->value()) << wxT("\n"); + try + { + m_cout << PGSOUTPGSCRIPT << wx_static_cast(const wxString, + m_var->eval(vars)->value()) << wxT("\n"); + } + catch (const pgsException &) + { + if (m_app != 0) + { + m_app->UnlockOutput(); + } + + throw; + } if (m_app != 0) { Index: pgscript/statements/pgsStmtList.cpp =================================================================== --- pgscript/statements/pgsStmtList.cpp (revision 7650) +++ pgscript/statements/pgsStmtList.cpp (working copy) @@ -82,7 +82,7 @@ m_app->LockOutput(); } - m_cout << wxT("[ERROR] Unknown exception:\n") + m_cout << PGSOUTERROR << _("Unknown exception:\n") << wx_static_cast(const wxString, wxString(e.what(), wxConvUTF8)); m_exception_thrown = true; Index: utils/misc.cpp =================================================================== --- utils/misc.cpp (revision 7650) +++ utils/misc.cpp (working copy) @@ -132,6 +132,10 @@ return (OID)strtoul(value.ToAscii(), 0, 10); } +wxString generate_spaces(int length) +{ + return wxString().Pad(length); +} wxString NumToStr(double value) {
On Mon, Mar 9, 2009 at 1:29 PM, Magnus Hagander <magnus@hagander.net> wrote: > Hi! > > Attached is a modified version of this patch, with one question left to > solve. Changes are: > > * Moved the space after the ] in the prefix into the prefix instead, so > it doesn't have to be included in every string everywhere. pgsCastException.cpp still appears to have a leading space on it's error message. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page wrote: > On Mon, Mar 9, 2009 at 1:29 PM, Magnus Hagander <magnus@hagander.net> wrote: >> Hi! >> >> Attached is a modified version of this patch, with one question left to >> solve. Changes are: >> >> * Moved the space after the ] in the prefix into the prefix instead, so >> it doesn't have to be included in every string everywhere. > > pgsCastException.cpp still appears to have a leading space on it's > error message. Meh. I fixed that in my tree, but forgot to do a new "svn diff" before I sent it to the list... Thx for pointing it out though :-) //Magnus
2009/3/9 Magnus Hagander <magnus@hagander.net>: > Meh. I fixed that in my tree, but forgot to do a new "svn diff" before I > sent it to the list... > > Thx for pointing it out though :-) > Updated version of the patch with: * The extra space in pgsCastException.cpp has been removed * The padding spaces in pgsParameterException have been added * In frm/plugins.cpp, "pgconn.h" was included, however it did not compile on my Slackware system... Changed to "pgConn.h" and now it works fine. Best regards, Mickael
Attachment
On Mon, Mar 9, 2009 at 9:59 PM, Mickael Deloison <mdeloison@gmail.com> wrote: > * In frm/plugins.cpp, "pgconn.h" was included, however it did not > compile on my Slackware system... Changed to "pgConn.h" and > now it works fine. Crap - my bad. Sorry. I tested on Windows and Mac, both of which are case-insensitive by default. I've fixed that so it doesn't cause others pain, but have left the rest of the patch for Magnus. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Mickael Deloison wrote: > 2009/3/9 Magnus Hagander <magnus@hagander.net>: >> Meh. I fixed that in my tree, but forgot to do a new "svn diff" before I >> sent it to the list... >> >> Thx for pointing it out though :-) >> > > Updated version of the patch with: > * The extra space in pgsCastException.cpp has been removed > * The padding spaces in pgsParameterException have been added > * In frm/plugins.cpp, "pgconn.h" was included, however it did not > compile on my Slackware system... Changed to "pgConn.h" and > now it works fine. Applied. I hand-merged the pgsParameterException stuff in with my existing patch instead of using yours directly, so feel free to point out any mistake I made there :-) //Magnus
2009/3/10 Magnus Hagander <magnus@hagander.net>: > Applied. I hand-merged the pgsParameterException stuff in with my > existing patch instead of using yours directly, so feel free to point > out any mistake I made there :-) > Hi, You forgot to remove two characters (">>") in the exception message. The atch that fixes that is attached to this email. I re-ran every test (unit and integration) under Linux and Windows, loaded each test file directly in pgAdmin. The only remaining thing to fix is in this patch. So, after it has been applied, I think pgScript will be ready for pgAdmin 1.10 beta 1. Best regards, Mickael
Attachment
On Thu, Mar 12, 2009 at 11:38 PM, Mickael Deloison <mdeloison@gmail.com> wrote: > 2009/3/10 Magnus Hagander <magnus@hagander.net>: >> Applied. I hand-merged the pgsParameterException stuff in with my >> existing patch instead of using yours directly, so feel free to point >> out any mistake I made there :-) >> > > Hi, > > You forgot to remove two characters (">>") in the exception message. > The atch that fixes that is attached to this email. Thanks, applied. > I re-ran every test (unit and integration) under Linux and Windows, > loaded each test file directly in pgAdmin. The only remaining thing to > fix is in this patch. So, after it has been applied, I think pgScript > will be ready for pgAdmin 1.10 beta 1. Little late for beta 1 unfortunately, but it'll be in beta 2. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com