Thread: pgAgent reports failure upon success
<p><font face="sans-serif" size="2">pgAgent is reporting that the job running this query:</font><br /><br /><font
face="sans-serif"size="2"> insert into dba.pg_stat_bgwriter_data</font><br /><font face="sans-serif" size="2">
(</font><br/><font face="sans-serif" size="2"> select *, now() from pg_catalog.pg_stat_bgwriter</font><br /><font
face="sans-serif"size="2"> );</font><br /><br /><font face="sans-serif" size="2">is failing with a return code of 1,
despitehaving completed the insert ok.</font><br /><br /><font face="sans-serif" size="2">When I wrap the code in a
"do"statement, it reports success:</font><br /><br /><font face="sans-serif" size="2"> do $$</font><br /><font
face="sans-serif"size="2"> begin</font><br /><font face="sans-serif" size="2"> insert into
dba.pg_stat_bgwriter_data</font><br/><font face="sans-serif" size="2"> (</font><br /><font face="sans-serif" size="2">
select*, now() from pg_catalog.pg_stat_bgwriter</font><br /><font face="sans-serif" size="2"> );</font><br /><font
face="sans-serif"size="2"> end;</font><br /><font face="sans-serif" size="2"> $$ language plpgsql</font><br /><br /><br
/><fontface="sans-serif" size="2">I've been through the code, and the return of 1 is coming back from the number of
rowsinserted after ExecuteVoid so the logic following in job.cpp seems incorrect:</font><br /><br /><font
face="sans-serif"size="2"> wxString stepstatus;</font><br /><font face="sans-serif" size="2">
if (rc == 0)</font><br /><font face="sans-serif" size="2"> stepstatus = wxT("s");</font><br
/><fontface="sans-serif" size="2"> else</font><br /><font face="sans-serif" size="2">
stepstatus = steps->GetString(wxT("jstonerror"));</font><br /><br /><font face="sans-serif" size="2">"rc" is
comingback as the number of rows, so in this case; 1. I assume because the "do" statement returns 0 it's reporting
ok.</font><br/><br /><font face="sans-serif" size="2">pgAgent Version is: pgAgent-3.2.0</font><br /><font
face="sans-serif"size="2">OS is: CentOS 6.2</font><br /><font face="sans-serif" size="2">Jobs ran from: pgAdmin
1.14.3</font><br/><br /><font face="sans-serif" size="2">If it's agreed that this is a bug, then I will have a go at a
patchif I get time later today...</font><br /><br /><font face="sans-serif" size="2">Cheers</font><br /><br /><font
face="sans-serif"size="2">Martin</font><br /><br /><font
face="sans-serif">=============================================Romax Technology Limited Rutherford House Nottingham
Science& Technology Park Nottingham, NG7 2PZ England Telephone numbers: +44 (0)115 951 88 00 (main) For other
officelocations see: http://www.romaxtech.com/Contact ================================= =============== E-mail:
info@romaxtech.comWebsite: www.romaxtech.com ================================= ================ Confidentiality
StatementThis transmission is for the addressee only and contains information that is confidential and privileged.
Unlessyou are the named addressee, or authorised to receive it on behalf of the addressee you may not copy or use it,
ordisclose it to anyone else. If you have received this transmission in error please delete from your system and
contactthe sender. Thank you for your cooperation. =================================================</font>
Hi
On Fri, Jul 6, 2012 at 8:04 AM, Martin French
<Martin.French@romaxtech.com> wrote:
>
> I've been through the code, and the return of 1 is coming back from the
> number of rows inserted after ExecuteVoid so the logic following in job.cpp
> seems incorrect:
>
> wxString stepstatus;
> if (rc == 0)
> stepstatus = wxT("s");
> else
> stepstatus = steps->GetString(wxT("jstonerror"));
>
> "rc" is coming back as the number of rows, so in this case; 1. I assume
> because the "do" statement returns 0 it's reporting ok.
Yeah, that is a bug; though I don't think the issue is in that code
snippet. I think it's here:
stepConn = DBconn::Get(jstconnstr, jstdbname);
if (stepConn)
{ LogMessage(wxString::Format(_("Executing SQL step %s (part of job
%s)"), stepid.c_str(), jobid.c_str()), LOG_DEBUG); rc = stepConn->ExecuteVoid(steps->GetString(wxT("jstcode")));
output= stepConn->GetLastError(); stepConn->Return();
}
I think rc should be set to either 0 or -1, depending on whether an
error was returned, not based on the return value from ExecuteVoid.
Seem reasonable?
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
<p><font face="sans-serif" size="2">Hi Dave,</font><br /><br /><font face="sans-serif" size="2">That seems reasonable
enoughto me. </font><br /><br /><font face="sans-serif" size="2">GetLastError only returns a wxString though, fed from
PQerrorMessageso I guess it would be the string that would be tested for content?</font><br /><br /><font
face="sans-serif"size="2">Maybe passing the </font><tt><font size="1">ExecStatusType</font></tt><font face="sans-serif"
size="2"> intoa DBConn/DBResult member variable that could be tested would work better?</font><br /><br /><font
face="sans-serif"size="2">Thoughts? :)</font><br /><br /><font face="sans-serif" size="2">Cheers</font><br /><br
/><fontface="sans-serif" size="2">Martin</font><br /><br /><br /><br /><br /><br /><br /><img alt="Inactive hide
detailsfor Dave Page ---06/07/2012 11:06:04---Hi On Fri, Jul 6, 2012 at 8:04 AM, Martin French" border="0" height="16"
src="cid:0__=0FBBF0A0DFA427128f9e8a93df@romaxtech.com"width="16" /><font color="#424282" face="sans-serif"
size="2">DavePage ---06/07/2012 11:06:04---Hi On Fri, Jul 6, 2012 at 8:04 AM, Martin French</font><br /><br /><font
color="#5F5F5F"face="sans-serif" size="1">From: </font><font face="sans-serif" size="1">Dave Page
<dpage@pgadmin.org></font><br/><font color="#5F5F5F" face="sans-serif" size="1">To: </font><font
face="sans-serif"size="1">Martin French <Martin.French@romaxtech.com>, </font><br /><font color="#5F5F5F"
face="sans-serif"size="1">Cc: </font><font face="sans-serif" size="1">pgadmin-support@postgresql.org</font><br /><font
color="#5F5F5F"face="sans-serif" size="1">Date: </font><font face="sans-serif" size="1">06/07/2012 11:06</font><br
/><fontcolor="#5F5F5F" face="sans-serif" size="1">Subject: </font><font face="sans-serif" size="1">Re:
[pgadmin-support]pgAgent reports failure upon success</font><br /><hr align="left" noshade size="2"
style="color:#8091A5;" width="100%" /><br /><br /><br /><tt><font size="2">Hi<br /><br />On Fri, Jul 6, 2012 at 8:04
AM,Martin French<br /><Martin.French@romaxtech.com> wrote:<br />><br />> I've been through the code, and
thereturn of 1 is coming back from the<br />> number of rows inserted after ExecuteVoid so the logic following in
job.cpp<br/>> seems incorrect:<br />><br />> wxString stepstatus;<br />> if
(rc== 0)<br />> stepstatus = wxT("s");<br />> else<br />>
stepstatus = steps->GetString(wxT("jstonerror"));<br />><br />> "rc" is coming back as the number
ofrows, so in this case; 1. I assume<br />> because the "do" statement returns 0 it's reporting ok.<br /><br />Yeah,
thatis a bug; though I don't think the issue is in that code<br />snippet. I think it's here:<br /><br />stepConn =
DBconn::Get(jstconnstr,jstdbname);<br />if (stepConn)<br />{<br /> LogMessage(wxString::Format(_("Executing SQL step
%s(part of job<br />%s)"), stepid.c_str(), jobid.c_str()), LOG_DEBUG);<br /> rc =
stepConn->ExecuteVoid(steps->GetString(wxT("jstcode")));<br/> output = stepConn->GetLastError();<br />
stepConn->Return();<br/>}<br /><br />I think rc should be set to either 0 or -1, depending on whether an<br />error
wasreturned, not based on the return value from ExecuteVoid.<br /><br />Seem reasonable?<br /></font></tt><tt><font
size="2"><br/>-- <br />Dave Page<br />Blog: </font></tt><tt><font size="2"><a
href="http://pgsnake.blogspot.com">http://pgsnake.blogspot.com</a></font></tt><tt><fontsize="2"><br />Twitter:
@pgsnake<br/><br />EnterpriseDB UK: </font></tt><tt><font size="2"><a
href="http://www.enterprisedb.com">http://www.enterprisedb.com</a></font></tt><tt><fontsize="2"><br />The Enterprise
PostgreSQLCompany<br /></font></tt><tt><font size="2"><br /></font></tt><br /><font
face="sans-serif">=============================================Romax Technology Limited Rutherford House Nottingham
Science& Technology Park Nottingham, NG7 2PZ England Telephone numbers: +44 (0)115 951 88 00 (main) For other
officelocations see: http://www.romaxtech.com/Contact ================================= =============== E-mail:
info@romaxtech.comWebsite: www.romaxtech.com ================================= ================ Confidentiality
StatementThis transmission is for the addressee only and contains information that is confidential and privileged.
Unlessyou are the named addressee, or authorised to receive it on behalf of the addressee you may not copy or use it,
ordisclose it to anyone else. If you have received this transmission in error please delete from your system and
contactthe sender. Thank you for your cooperation. =================================================</font>
Hi
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, Jul 6, 2012 at 11:36 AM, Martin French <Martin.French@romaxtech.com> wrote:
Hi Dave,
That seems reasonable enough to me.
GetLastError only returns a wxString though, fed from PQerrorMessage so I guess it would be the string that would be tested for content?
Maybe passing the ExecStatusType into a DBConn/DBResult member variable that could be tested would work better?
Yes, that seems like a good solution. Can/will you work up a patch for that?
Thanks, Dave.
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
<p><font face="sans-serif" size="2">Yep, can do, and will do Dave.</font><br /><br /><font face="sans-serif" size="2">Willbe after the weekend now, but should have something Monday eve.</font><br /><br /><font face="sans-serif" size="2">Havea good weekend mate.</font><br /><br /><font face="sans-serif" size="2">Cheers</font><br /><br /><br /><br /><imgalt="Inactive hide details for Dave Page ---06/07/2012 12:22:56---Hi On Fri, Jul 6, 2012 at 11:36 AM, Martin French"border="0" height="16" src="cid:0__=0FBBF0A0DFAD106B8f9e8a93df@romaxtech.com" width="16" /><font color="#424282" face="sans-serif"size="2">Dave Page ---06/07/2012 12:22:56---Hi On Fri, Jul 6, 2012 at 11:36 AM, Martin French</font><br/><br /><font color="#5F5F5F" face="sans-serif" size="1">From: </font><font face="sans-serif" size="1">DavePage <dpage@pgadmin.org></font><br /><font color="#5F5F5F" face="sans-serif" size="1">To: </font><fontface="sans-serif" size="1">Martin French <Martin.French@romaxtech.com>, </font><br /><font color="#5F5F5F"face="sans-serif" size="1">Cc: </font><font face="sans-serif" size="1">pgadmin-support@postgresql.org</font><br/><font color="#5F5F5F" face="sans-serif" size="1">Date: </font><font face="sans-serif"size="1">06/07/2012 12:22</font><br /><font color="#5F5F5F" face="sans-serif" size="1">Subject: </font><fontface="sans-serif" size="1">Re: [pgadmin-support] pgAgent reports failure upon success</font><br /><hr align="left"noshade size="2" style="color:#8091A5; " width="100%" /><br /><br /><br /><font face="serif" size="3">Hi<br /></font><br/><font face="serif" size="3">On Fri, Jul 6, 2012 at 11:36 AM, Martin French <</font><a href="mailto:Martin.French@romaxtech.com"target="_blank"><font color="#0000FF" face="serif" size="3"><u>Martin.French@romaxtech.com</u></font></a><fontface="serif" size="3">> wrote:</font><ul style="padding-left:9pt"><font face="sans-serif" size="3">Hi Dave,</font><font face="serif" size="3"><br /></font><font face="sans-serif"size="3"><br />That seems reasonable enough to me. </font><font face="serif" size="3"><br /></font><fontface="sans-serif" size="3"><br />GetLastError only returns a wxString though, fed from PQerrorMessage so Iguess it would be the string that would be tested for content?</font><font face="serif" size="3"><br /></font><font face="sans-serif"size="3"><br />Maybe passing the </font><tt><font size="1">ExecStatusType</font></tt><font face="sans-serif"size="3"> into a DBConn/DBResult member variable that could be tested would work better?</font><p></ul><br/><font face="serif" size="3">Yes, that seems like a good solution. Can/will you work up a patchfor that?</font><br /><br /><font face="serif" size="3">Thanks, Dave. </font><br /><br /><br /><font face="serif" size="3">--<br />Dave Page<br />Blog: </font><a href="http://pgsnake.blogspot.com/" target="_blank"><font color="#0000FF"face="serif" size="3"><u>http://pgsnake.blogspot.com</u></font></a><font face="serif" size="3"><br />Twitter:@pgsnake<br /><br />EnterpriseDB UK: </font><a href="http://www.enterprisedb.com/" target="_blank"><font color="#0000FF"face="serif" size="3"><u>http://www.enterprisedb.com</u></font></a><font face="serif" size="3"><br />The EnterprisePostgreSQL Company<br /></font><br /><font face="sans-serif">============================================= RomaxTechnology Limited Rutherford House Nottingham Science & Technology Park Nottingham, NG7 2PZ England Telephone numbers:+44 (0)115 951 88 00 (main) For other office locations see: http://www.romaxtech.com/Contact ================================================ E-mail: info@romaxtech.com Website: www.romaxtech.com ================================================= Confidentiality Statement This transmission is for the addressee only andcontains information that is confidential and privileged. Unless you are the named addressee, or authorised to receiveit on behalf of the addressee you may not copy or use it, or disclose it to anyone else. If you have received thistransmission in error please delete from your system and contact the sender. Thank you for your cooperation. =================================================</font>
On Fri, Jul 6, 2012 at 12:26 PM, Martin French <Martin.French@romaxtech.com> wrote:
Yep, can do, and will do Dave.
Will be after the weekend now, but should have something Monday eve.
Great.
Have a good weekend mate.
Cheers
And you!
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
<p><font face="sans-serif" size="2">Proposed patch for this bug in attached files.</font><br /><br /><i>(See attached file:connection.cpp)</i><i>(See attached file: connection.h)</i><i>(See attached file: job.cpp)</i><br /><br /><font face="sans-serif"size="2">Tested against PG 9.1.4, all seems fine, success/failure reported correctly even when multiplejobs ran concurrently.</font><br /><br /><font face="sans-serif" size="2">Please test and feed back.</font><br /><br/><font face="sans-serif" size="2">Thanks.</font><br /><br /><tt><font size="2">pgadmin-support-owner@postgresql.orgwrote on 06/07/2012 12:29:56:<br /><br />> From: Dave Page <dpage@pgadmin.org></font></tt><br/><tt><font size="2">> To: Martin French <Martin.French@romaxtech.com>,</font></tt><br /><tt><font size="2">> Cc: pgadmin-support@postgresql.org</font></tt><br/><tt><font size="2">> Date: 06/07/2012 15:35</font></tt><br /><tt><fontsize="2">> Subject: Re: [pgadmin-support] pgAgent reports failure upon success</font></tt><br /><tt><font size="2">>Sent by: pgadmin-support-owner@postgresql.org</font></tt><br /><tt><font size="2">> <br />> <br /></font></tt><br/><tt><font size="2">> On Fri, Jul 6, 2012 at 12:26 PM, Martin French <Martin.French@romaxtech.com<br/>> > wrote:</font></tt><br /><tt><font size="2">> Yep, can do, and will do Dave.<br/>> <br />> Will be after the weekend now, but should have something Monday eve.</font></tt><br /><tt><fontsize="2">> Great.</font></tt><br /><tt><font size="2">> </font></tt><br /><tt><font size="2">> Havea good weekend mate.<br />> <br />> Cheers</font></tt><br /><tt><font size="2">> <br />> And you! </font></tt><br/><tt><font size="2">> <br />> Cheers, Dave.</font></tt><br /><tt><font size="2">> <br />>-- <br />> Dave Page<br />> Blog: <a href="http://pgsnake.blogspot.com">http://pgsnake.blogspot.com</a><br />>Twitter: @pgsnake<br />> <br />> EnterpriseDB UK: <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>> The Enterprise PostgreSQL Company<br /></font></tt><fontface="sans-serif">============================================= Romax Technology Limited Rutherford HouseNottingham Science & Technology Park Nottingham, NG7 2PZ England Telephone numbers: +44 (0)115 951 88 00 (main)For other office locations see: http://www.romaxtech.com/Contact ================================= ===============E-mail: info@romaxtech.com Website: www.romaxtech.com ================================= ================ ConfidentialityStatement This transmission is for the addressee only and contains information that is confidential and privileged.Unless you are the named addressee, or authorised to receive it on behalf of the addressee you may not copy oruse it, or disclose it to anyone else. If you have received this transmission in error please delete from your system andcontact the sender. Thank you for your cooperation. =================================================</font>
Hi On Mon, Jul 9, 2012 at 10:11 AM, Martin French <Martin.French@romaxtech.com> wrote: > Proposed patch for this bug in attached files. > > (See attached file: connection.cpp)(See attached file: connection.h)(See > attached file: job.cpp) > > Tested against PG 9.1.4, all seems fine, success/failure reported correctly > even when multiple jobs ran concurrently. Thanks - but can you provide the changes as a diff please? Otherwise it's much harder to properly review. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
<p><font face="sans-serif" size="2">Apologies wrong files attached! It is Monday after all... ;o)</font><br /><br /><br /><i>(Seeattached file: connection.cpp.patch)</i><i>(See attached file: connection.h.patch)</i><i>(See attached file: job.cpp.patch)</i><br/><br /><font face="sans-serif" size="2">Cheers</font><br /><br /><tt><font size="2">Dave Page <dpage@pgadmin.org>wrote on 09/07/2012 10:16:47:<br /><br />> From: Dave Page <dpage@pgadmin.org></font></tt><br/><tt><font size="2">> To: Martin French <Martin.French@romaxtech.com>,</font></tt><br /><tt><font size="2">> Cc: pgadmin-support@postgresql.org, pgadmin-support-owner@postgresql.org</font></tt><br/><tt><font size="2">> Date: 09/07/2012 10:16</font></tt><br /><tt><fontsize="2">> Subject: Re: [pgadmin-support] pgAgent reports failure upon success</font></tt><br /><tt><font size="2">><br />> Hi<br />> <br />> On Mon, Jul 9, 2012 at 10:11 AM, Martin French<br />> <Martin.French@romaxtech.com>wrote:<br />> > Proposed patch for this bug in attached files.<br />> ><br/>> > (See attached file: connection.cpp)(See attached file: connection.h)(See<br />> > attached file:job.cpp)<br />> ><br />> > Tested against PG 9.1.4, all seems fine, success/failure reported correctly<br/>> > even when multiple jobs ran concurrently.<br />> <br />> Thanks - but can you provide the changesas a diff please? Otherwise<br />> it's much harder to properly review.<br />> <br />> -- <br />> DavePage<br />> Blog: <a href="http://pgsnake.blogspot.com">http://pgsnake.blogspot.com</a><br />> Twitter: @pgsnake<br/>> <br />> EnterpriseDB UK: <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br />>The Enterprise PostgreSQL Company<br />> <br /></font></tt><font face="sans-serif">=============================================Romax Technology Limited Rutherford House Nottingham Science& Technology Park Nottingham, NG7 2PZ England Telephone numbers: +44 (0)115 951 88 00 (main) For other officelocations see: http://www.romaxtech.com/Contact ================================= =============== E-mail: info@romaxtech.comWebsite: www.romaxtech.com ================================= ================ Confidentiality StatementThis transmission is for the addressee only and contains information that is confidential and privileged. Unlessyou are the named addressee, or authorised to receive it on behalf of the addressee you may not copy or use it, ordisclose it to anyone else. If you have received this transmission in error please delete from your system and contactthe sender. Thank you for your cooperation. =================================================</font>
<p><tt><font size="2">Tree Diff.</font></tt><br /><br /><br /><tt><font size="2"> diff -r pgAgent-3.2.1-Source
pgAgent-3.2.2-Source</font></tt><br/><tt><font size="2">diff -r pgAgent-3.2.1-Source/connection.cpp
pgAgent-3.2.2-Source/connection.cpp</font></tt><br/><tt><font size="2">28a29</font></tt><br /><tt><font size="2">>
lastStatus = 0;</font></tt><br /><tt><font size="2">339a341</font></tt><br /><tt><font size="2">>
{</font></tt><br/><tt><font size="2">340a343,344</font></tt><br /><tt><font size="2">>
conn->SetLastResult(0);</font></tt><br/><tt><font size="2">> }</font></tt><br /><tt><font
size="2">344a349</font></tt><br/><tt><font size="2">>
conn->SetLastResult(-1);</font></tt><br/><tt><font size="2">349a355</font></tt><br /><tt><font size="2">>
{</font></tt><br/><tt><font size="2">351c357,358</font></tt><br /><tt><font size="2"><</font></tt><br /><tt><font
size="2">---</font></tt><br/><tt><font size="2">> conn->SetLastResult(-1);</font></tt><br
/><tt><fontsize="2">> }</font></tt><br /><tt><font size="2">diff -r pgAgent-3.2.1-Source/include/connection.h
pgAgent-3.2.2-Source/include/connection.h</font></tt><br/><tt><font size="2">54c54,61</font></tt><br /><tt><font
size="2"><</font></tt><br/><tt><font size="2">---</font></tt><br /><tt><font size="2">> void
SetLastResult(signedint result )</font></tt><br /><tt><font size="2">> {</font></tt><br /><tt><font
size="2">> lastStatus = result;</font></tt><br /><tt><font size="2">> }</font></tt><br
/><tt><fontsize="2">> int GetLastResult()</font></tt><br /><tt><font size="2">> {</font></tt><br
/><tt><fontsize="2">> return lastStatus;</font></tt><br /><tt><font size="2">>
}</font></tt><br/><tt><font size="2">72a80</font></tt><br /><tt><font size="2">> signed int
lastStatus;</font></tt><br/><tt><font size="2">diff -r pgAgent-3.2.1-Source/job.cpp
pgAgent-3.2.2-Source/job.cpp</font></tt><br/><tt><font size="2">140c140</font></tt><br /><tt><font size="2"><
rc = stepConn->ExecuteVoid(steps->GetString(wxT("jstcode")));</font></tt><br
/><tt><fontsize="2">---</font></tt><br /><tt><font size="2">>
stepConn->ExecuteVoid(steps->GetString(wxT("jstcode")));</font></tt><br/><tt><font
size="2">141a142</font></tt><br/><tt><font size="2">> rc =
stepConn->GetLastResult();</font></tt><br/><font face="sans-serif">=============================================
RomaxTechnology Limited Rutherford House Nottingham Science & Technology Park Nottingham, NG7 2PZ England Telephone
numbers:+44 (0)115 951 88 00 (main) For other office locations see: http://www.romaxtech.com/Contact
================================================ E-mail: info@romaxtech.com Website: www.romaxtech.com
================================================= Confidentiality Statement This transmission is for the addressee only
andcontains information that is confidential and privileged. Unless you are the named addressee, or authorised to
receiveit on behalf of the addressee you may not copy or use it, or disclose it to anyone else. If you have received
thistransmission in error please delete from your system and contact the sender. Thank you for your cooperation.
=================================================</font>
Hi On Mon, Jul 9, 2012 at 10:30 AM, Martin French <Martin.French@romaxtech.com> wrote: > Apologies wrong files attached! It is Monday after all... ;o) > > > (See attached file: connection.cpp.patch)(See attached file: > connection.h.patch)(See attached file: job.cpp.patch) OK, first of all, thanks for the patch. Second; please don't be discouraged by the feedback, but if you can update the patch, I'd appreciate it: - You have member name of [Get|Set]LastResult which set/return a member variable called lastStatus. These should be consistent to avoid confusion. - Rather than setting lastStatus to 0 or -1, why not set it to PQresultStatus(). That seems like it would be more useful in the future (and potentially in the log files). - When creating the diff, please use "git diff", or if working from a source tarball (not recommended for development) - take 2 copies and then diff them, eg "diff -c -r pgagent-src/ pgagent-src.orig/ > mypatch.diff". That'll put the entire patch in one file, which makes it much easier to handle and reapply. Thanks! -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
<p><font face="sans-serif" size="2">Hi</font><br /><br /><tt><font size="2">Thanks for the feedback. I certainly won't
bediscouraged by it! :)</font></tt><br /><br /><tt><font size="2">> - You have member name of [Get|Set]LastResult
whichset/return a<br />> member variable called lastStatus. These should be consistent to avoid<br />>
confusion.</font></tt><br/><br /><tt><font size="2">Will amend accordingly.</font></tt><br /><br /><tt><font
size="2">>- Rather than setting lastStatus to 0 or -1, why not set it to<br />> PQresultStatus(). That seems like
itwould be more useful in the<br />> future (and potentially in the log files).</font></tt><br /><br /><tt><font
size="2">I'dset it to only two values, as it made the comparison easier. As I see it, ExecStatusType ENUM can be anyone
of0 - 8 inclusive, but some of those are error messages, so it'd either have to determine which ones are OK and which
arenot either with a case statement or with an extended "if". Happy to code that in along the lines of (or inverse,
testingfor fails...):</font></tt><br /><br /><tt><font size="2">switch (rc)</font></tt><br /><tt><font
size="2">{</font></tt><br/><tt><font size="2"> case PGRES_COMMAND_OK:</font></tt><br /><tt><font size="2"> case
PGRES_TUPLES_OK:</font></tt><br/><tt><font size="2"> case PGRES_COPY_OUT:</font></tt><br /><tt><font size="2"> case
PGRES_COPY_IN:</font></tt><br/><tt><font size="2"> case PGRES_COPY_BOTH:</font></tt><br /><tt><font size="2">
</font></tt><tt><fontsize="2">stepstatus = wxT("s");</font></tt><br /><tt><font size="2"> break;</font></tt><br
/><tt><fontsize="2"> default:</font></tt><br /><tt><font size="2"> </font></tt><tt><font size="2">stepstatus =
steps->GetString(wxT("jstonerror"));</font></tt><br/><tt><font size="2"> break</font></tt><br /><tt><font
size="2">}</font></tt><br/><br /><tt><font size="2">If you think that's a more viable option? Let me know and I'll sort
it:)</font></tt><br /><br /><tt><font size="2">> - When creating the diff, please use "git diff", or if working from
a<br/>> source tarball (not recommended for development) - take 2 copies and<br />> then diff them, eg "diff -c
-rpgagent-src/ pgagent-src.orig/ ><br />> mypatch.diff". That'll put the entire patch in one file, which makes<br
/>>it much easier to handle and reapply.</font></tt><br /><br /><tt><font size="2">I'm afraid I'm limited to source
tarballsat the moment. Attempting to pull from git gives me "Connection Refused". So will create a single patch file
usingthe method suggested.</font></tt><br /><br /><tt><font size="2">Cheers</font></tt><br /><br /><tt><font
size="2">Martin.</font></tt><br /><font face="sans-serif">============================================= Romax
TechnologyLimited Rutherford House Nottingham Science & Technology Park Nottingham, NG7 2PZ England Telephone
numbers:+44 (0)115 951 88 00 (main) For other office locations see: http://www.romaxtech.com/Contact
================================================ E-mail: info@romaxtech.com Website: www.romaxtech.com
================================================= Confidentiality Statement This transmission is for the addressee only
andcontains information that is confidential and privileged. Unless you are the named addressee, or authorised to
receiveit on behalf of the addressee you may not copy or use it, or disclose it to anyone else. If you have received
thistransmission in error please delete from your system and contact the sender. Thank you for your cooperation.
=================================================</font>
On Tue, Jul 10, 2012 at 1:57 PM, Martin French
<Martin.French@romaxtech.com> wrote:
> Hi
>
> Thanks for the feedback. I certainly won't be discouraged by it! :)
:-)
>> - You have member name of [Get|Set]LastResult which set/return a
>> member variable called lastStatus. These should be consistent to avoid
>> confusion.
>
> Will amend accordingly.
Thanks.
>> - Rather than setting lastStatus to 0 or -1, why not set it to
>> PQresultStatus(). That seems like it would be more useful in the
>> future (and potentially in the log files).
>
> I'd set it to only two values, as it made the comparison easier. As I see
> it, ExecStatusType ENUM can be anyone of 0 - 8 inclusive, but some of those
> are error messages, so it'd either have to determine which ones are OK and
> which are not either with a case statement or with an extended "if". Happy
> to code that in along the lines of (or inverse, testing for fails...):
>
> switch (rc)
> {
> case PGRES_COMMAND_OK:
> case PGRES_TUPLES_OK:
> case PGRES_COPY_OUT:
> case PGRES_COPY_IN:
> case PGRES_COPY_BOTH:
>
> stepstatus = wxT("s");
> break;
> default:
>
> stepstatus = steps->GetString(wxT("jstonerror"));
> break
> }
>
> If you think that's a more viable option? Let me know and I'll sort it :)
That's a little more complex for this case, but I think it'll make the
dbconn class more useful for the future. You could of course wrap that
switch statement up in the class as well, and offer another public
member that returns a simple boolean indicating if the last status was
a success or failure.
>> - When creating the diff, please use "git diff", or if working from a
>> source tarball (not recommended for development) - take 2 copies and
>> then diff them, eg "diff -c -r pgagent-src/ pgagent-src.orig/ >
>> mypatch.diff". That'll put the entire patch in one file, which makes
>> it much easier to handle and reapply.
>
> I'm afraid I'm limited to source tarballs at the moment. Attempting to pull
> from git gives me "Connection Refused". So will create a single patch file
> using the method suggested.
Hmm, this works for me:
git clone git://git.postgresql.org/git/pgagent.git
or, if you're on a network that blocks the git protocol, you might want to try:
git clone http://git.postgresql.org/git/pgagent.git
Please let me know if that doesn't work, as I'm one of the admins for
that server as well so obviously want to fix any problems.
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
<p><font face="sans-serif" size="2">Hi</font><br /><br /><tt><font size="2">pgadmin-support-owner@postgresql.org wrote
on10/07/2012 16:05:18:<br /><br />> From: Dave Page <dpage@pgadmin.org></font></tt><br /><tt><font
size="2">>To: Martin French <Martin.French@romaxtech.com>, </font></tt><br /><tt><font size="2">> Cc:
pgadmin-support@postgresql.org,pgadmin-support-owner@postgresql.org</font></tt><br /><tt><font size="2">> Date:
10/07/201219:12</font></tt><br /><tt><font size="2">> Subject: Re: [pgadmin-support] pgAgent reports failure upon
success</font></tt><br/><tt><font size="2">> Sent by: pgadmin-support-owner@postgresql.org</font></tt><br
/><tt><fontsize="2">> <br />> On Tue, Jul 10, 2012 at 1:57 PM, Martin French<br />>
<Martin.French@romaxtech.com>wrote:<br />> > Hi<br />> ><br />> > Thanks for the feedback. I
certainlywon't be discouraged by it! :)<br />> <br />> :-)<br />> <br />> >> - You have member name
of[Get|Set]LastResult which set/return a<br />> >> member variable called lastStatus. These should be
consistentto avoid<br />> >> confusion.<br />> ><br />> > Will amend accordingly.<br />> <br
/>>Thanks.<br />> <br />> >> - Rather than setting lastStatus to 0 or -1, why not set it to<br />>
>>PQresultStatus(). That seems like it would be more useful in the<br />> >> future (and potentially in
thelog files).<br />> ><br />> > I'd set it to only two values, as it made the comparison easier. As I
see<br/>> > it, ExecStatusType ENUM can be anyone of 0 - 8 inclusive, but some of those<br />> > are error
messages,so it'd either have to determine which ones are OK and<br />> > which are not either with a case
statementor with an extended "if". Happy<br />> > to code that in along the lines of (or inverse, testing for
fails...):<br/>> ><br />> > switch (rc)<br />> > {<br />> > case PGRES_COMMAND_OK:<br />>
>case PGRES_TUPLES_OK:<br />> > case PGRES_COPY_OUT:<br />> > case PGRES_COPY_IN:<br />> > case
PGRES_COPY_BOTH:<br/>> ><br />> > stepstatus = wxT("s");<br />> > break;<br />> > default:<br
/>>><br />> > stepstatus = steps->GetString(wxT("jstonerror"));<br />> > break<br />> > }<br
/>>><br />> > If you think that's a more viable option? Let me know and I'll sort it :)<br />> <br
/>>That's a little more complex for this case, but I think it'll make the<br />> dbconn class more useful for the
future.You could of course wrap that<br />> switch statement up in the class as well, and offer another public<br
/>>member that returns a simple boolean indicating if the last status was<br />> a success or failure.<br
/></font></tt><br/><tt><font size="2">Agreed, I'll see what I can put together today (time permitting). </font></tt><br
/><br/><tt><font size="2">> <br />> >> - When creating the diff, please use "git diff", or if working from
a<br/>> >> source tarball (not recommended for development) - take 2 copies and<br />> >> then diff
them,eg "diff -c -r pgagent-src/ pgagent-src.orig/ ><br />> >> mypatch.diff". That'll put the entire patch
inone file, which makes<br />> >> it much easier to handle and reapply.<br />> ><br />> > I'm
afraidI'm limited to source tarballs at the moment. Attempting to pull<br />> > from git gives me "Connection
Refused".So will create a single patch file<br />> > using the method suggested.<br />> <br />> Hmm, this
worksfor me:<br />> <br />> git clone git://git.postgresql.org/git/pgagent.git<br />> <br />> or, if you're
ona network that blocks the git protocol, you might <br />> want to try:<br />> <br />> git clone <a
href="http://git.postgresql.org/git/pgagent.git">http://git.postgresql.org/git/pgagent.git</a><br/>> <br />>
Pleaselet me know if that doesn't work, as I'm one of the admins for<br />> that server as well so obviously want to
fixany problems.<br />> </font></tt><br /><tt><font size="2">The http link works now. I think the problem was this
end,as it's a new VM I'm using, so is not configured perhaps as well as It should be!</font></tt><br /><tt><font
size="2"><br/>Cheers</font></tt><br /><br /><tt><font size="2">Martin.</font></tt><font
face="sans-serif">=============================================Romax Technology Limited Rutherford House Nottingham
Science& Technology Park Nottingham, NG7 2PZ England Telephone numbers: +44 (0)115 951 88 00 (main) For other
officelocations see: http://www.romaxtech.com/Contact ================================= =============== E-mail:
info@romaxtech.comWebsite: www.romaxtech.com ================================= ================ Confidentiality
StatementThis transmission is for the addressee only and contains information that is confidential and privileged.
Unlessyou are the named addressee, or authorised to receive it on behalf of the addressee you may not copy or use it,
ordisclose it to anyone else. If you have received this transmission in error please delete from your system and
contactthe sender. Thank you for your cooperation. =================================================</font>