Обсуждение: [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

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

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

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