Thread: PgAgent 3.3.0 batch scripts on windows always get status failed
Hi All, First post on this list so I hope I'm doing things right. We've had a problem for awhile now that any batch script run by pgagent on windows gets the status failed even if the batch file runs just fine. I downloaded the latest copy of the source code to have a look and found something strange in the code for Job::Execute in job.cpp At the start of this method the variable succeeded gets set to false, in the windows code it never gets set to true as apposed to the *nix code where the variable rc is loaded with the return status of the batch script and succeeded is assigned true if this status equals 0. In the windows code (line 268) rc is set to 1, the return status of the batch script is not returned. I do not know if this is done on purpose and that as we can't detect if the script was successful succeeded is left as failed or if not setting succeeded to true has simply been overlooked? I would suggest changing this code to: 268: // assume successful execution of script 269: rc = 0; 270: succeeded = true; Alternatively, I'm assuming the code for CloseHandle is calling pclose, the return value of pclose is the errorlevel returned by the batch script and could be assigned to rc. I couldn't find the code for CloseHandle so I'm not sure if it would be able to return this code but if so the code could be turned into: 267: rc = CloseHandle(h_script); 268: if (rc == 0) 269: succeeded = true; Cheers, Bastiaan Olij
Hi On Thu, Jan 31, 2013 at 2:03 AM, Bastiaan Olij <bastiaan@basenlily.me> wrote: > Hi All, > > First post on this list so I hope I'm doing things right. > We've had a problem for awhile now that any batch script run by pgagent > on windows gets the status failed even if the batch file runs just fine. > > I downloaded the latest copy of the source code to have a look and found > something strange in the code for Job::Execute in job.cpp > > At the start of this method the variable succeeded gets set to false, in > the windows code it never gets set to true as apposed to the *nix code > where the variable rc is loaded with the return status of the batch > script and succeeded is assigned true if this status equals 0. > > In the windows code (line 268) rc is set to 1, the return status of the > batch script is not returned. I do not know if this is done on purpose > and that as we can't detect if the script was successful succeeded is > left as failed or if not setting succeeded to true has simply been > overlooked? > > I would suggest changing this code to: > 268: // assume successful execution of script > 269: rc = 0; > 270: succeeded = true; > > Alternatively, I'm assuming the code for CloseHandle is calling pclose, > the return value of pclose is the errorlevel returned by the batch > script and could be assigned to rc. I couldn't find the code for > CloseHandle so I'm not sure if it would be able to return this code but > if so the code could be turned into: > 267: rc = CloseHandle(h_script); > 268: if (rc == 0) > 269: succeeded = true; This seems to be a previously reported issue that slipped through the net due to lack of feedback. The patch I proposed looks like: raptor:pgagent dpage$ git diff diff --git a/job.cpp b/job.cpp index a9c0c1b..937bc9c 100644 --- a/job.cpp +++ b/job.cpp @@ -264,8 +264,8 @@ int Job::Execute() } + GetExitCodeProcess(h_script, (LPDWORD)&rc); CloseHandle(h_script); - rc = 1; #else // The *nix way. Are you in a position to test it if I build a binary for Windows? -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Dave, I don't think that will work, yes it will get the proper rc value but it still leaves succeeded set to false. It needs inclusion of a check to see if rc==0 and if so set succeeded to true provided that a successful script indeed returns an error level of 0 (which I think it should). And yes, if you could send me a new binary then we are able to test this swiftly. We do currently use the enterprisedb distributable so I'm taking the assumption I'll be able to just drop the new .exe in place of the old for this. Kindest Regards, Bastiaan Olij > This seems to be a previously reported issue that slipped through the > net due to lack of feedback. The patch I proposed looks like: > > raptor:pgagent dpage$ git diff > diff --git a/job.cpp b/job.cpp > index a9c0c1b..937bc9c 100644 > --- a/job.cpp > +++ b/job.cpp > @@ -264,8 +264,8 @@ int Job::Execute() > } > > > + GetExitCodeProcess(h_script, (LPDWORD)&rc); > CloseHandle(h_script); > - rc = 1; > > #else > // The *nix way. > > Are you in a position to test it if I build a binary for Windows? > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > >
Hi On Mon, Feb 4, 2013 at 11:23 PM, Bastiaan Olij <bastiaan@basenlily.me> wrote: > Hi Dave, > > I don't think that will work, yes it will get the proper rc value but it > still leaves succeeded set to false. It needs inclusion of a check to > see if rc==0 and if so set succeeded to true provided that a successful > script indeed returns an error level of 0 (which I think it should). Yeah, of course it is somewhat more complex than that. I've attached a patch thats seems to work for me. > And yes, if you could send me a new binary then we are able to test this > swiftly. We do currently use the enterprisedb distributable so I'm > taking the assumption I'll be able to just drop the new .exe in place of > the old for this. Thanks - I'll send you a link to an updated executable off-list. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
[re-adding the list] On Wed, Feb 6, 2013 at 10:41 PM, Bastiaan Olij <bastiaan@basenlily.me> wrote: > Hey Dave, > > As you've read from David's reply the job's status now comes back as > successful which solves our immediate issue. I did a little more testing > to see if I could get it to react to different exit codes and that seems > to still be ignored so the GetExitCodeProcess command doesn't seem to do > it's thing (or I'm doing something wrong). It worked for me :-) > I simply added an EXIT /B 2 at the end of the script to see if it would > pick up the error code 2 but the error code returned remained 0. Remove the /B and it should work. GetExitCodeProcess is returning the cmd.exe return value, not the scripts ERRORLEVEL. > Mind you, reading the documentation on GetExitCodeProcess it doesn't > return an error code if the process hasn't finished running yet and it > also mentions it's only supported on desktop applications and may thus > not be available to a service. I just tested it as a service and it worked fine :-). Proof attached! (ids 32 and above are run as a service, with pgAgent running under the LocalSystem account). Ignore some of the earlier ones - they were me debugging. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
Re: PgAgent 3.3.0 batch scripts on windows always get status failed
From
"Paragon Corporation"
Date:
We're having the same issue as described in this post. http://www.postgresql.org/message-id/CA+OCxoxoCGc7d7tRdyC5-32dDMtXyT1m59s_Lk 3moHTXxfMvmw@mail.gmail.com It sounds from the post that this issue was fixed. Was this change ever pushed to stack builder? I was thinking maybe it wasn't since the latest source release of pgAdmin http://www.postgresql.org/ftp/pgadmin3/release/pgagent/ Is before this change was put into place and the pgAgent.exe packaged with stackbuilder reads File version 3.3.0.0, Product Version 3.3.0 Thanks, Regina and Leo
On Mon, Feb 10, 2014 at 4:58 AM, Paragon Corporation <lr@pcorp.us> wrote: > We're having the same issue as described in this post. > > http://www.postgresql.org/message-id/CA+OCxoxoCGc7d7tRdyC5-32dDMtXyT1m59s_Lk > 3moHTXxfMvmw@mail.gmail.com > > > It sounds from the post that this issue was fixed. Was this change ever > pushed to stack builder? > > I was thinking maybe it wasn't since the latest source release of pgAdmin > > http://www.postgresql.org/ftp/pgadmin3/release/pgagent/ > > Is before this change was put into place and the pgAgent.exe packaged with > stackbuilder reads File version 3.3.0.0, Product Version 3.3.0 No, there hasn't been a release of pgAgent since this fix was committed. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Dave, I think I speak for a number of people out there, seeing that over the past year I've received a few emails requesting for the patched version we have, that it would be nice if a new official release with the fixes from the past year would see the light of day. Cheers, Bas On 10/02/14 10:37 PM, Dave Page wrote: > No, there hasn't been a release of pgAgent since this fix was committed.
On Thu, Feb 13, 2014 at 4:45 AM, Bastiaan Olij <bastiaan@basenlily.me> wrote: > Hi Dave, > > I think I speak for a number of people out there, seeing that over the > past year I've received a few emails requesting for the patched version > we have, that it would be nice if a new official release with the fixes > from the past year would see the light of day. Hi For the record, that is the only fix in there. Once I've figured out the versioning with David, I'll look at doing a 3.4.0 release. > On 10/02/14 10:37 PM, Dave Page wrote: >> No, there hasn't been a release of pgAgent since this fix was committed. > > > > -- > Sent via pgadmin-support mailing list (pgadmin-support@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgadmin-support -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company