New pg_ctl has retrogressed in error messages

Tom Lane wrote:

7.4, on not finding a postmaster:

[tgl@rh1 pgsql]$ pg_ctl stop
/home/tgl/version74/bin/pg_ctl: line 274: kill: (15273) - No such process
waiting for postmaster to shut down................................................................ failed
pg_ctl: postmaster does not shut down

CVS tip, same scenario:

[tgl@rh1 pgsql]$ pg_ctl stop
stop signal failed

Not waiting for the results of a failed kill() is good, but the error
message is exceedingly unhelpful. It should mention the PID it tried to
kill and give the errno string. Perhaps

failed to signal postmaster process 15273: No such process

OK, new output is:

$ aspg pg_ctl stop
stop signal failed (PID: 3603): No such process

I also changed all the pid variables to use pid_t.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I also changed all the pid variables to use pid_t.

Good, but ...

! fscanf(pidf, "%u", &pid);

this code will fail rather horribly if sizeof(pid_t) != sizeof(int).
Even more to the point, I believe a standalone backend will put
the negative of its PID into the file, and the revised code will fail
to parse that at all.

I think the safest code would be like

long tmp;

fscanf(pidf, "%ld", &tmp);
if (tmp < 0)
{
tmp = -tmp;
// do anything else needed for backend case
}
pid = (pid_t) tmp;

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I also changed all the pid variables to use pid_t.

Good, but ...

! fscanf(pidf, "%u", &pid);

this code will fail rather horribly if sizeof(pid_t) != sizeof(int).
Even more to the point, I believe a standalone backend will put
the negative of its PID into the file, and the revised code will fail
to parse that at all.

I think the safest code would be like

long tmp;

fscanf(pidf, "%ld", &tmp);
if (tmp < 0)
{
tmp = -tmp;
// do anything else needed for backend case
}
pid = (pid_t) tmp;

I deliberately used a signed long for these reasons in the first place.

The number of places we actually need to use this value as a pid is
small (3 by my count - in calls to kill() ), and it was cast there to
pid_t, I think. I still don't see what's wrong with that.

cheers

andrew

Andrew Dunstan wrote:

this code will fail rather horribly if sizeof(pid_t) != sizeof(int).
Even more to the point, I believe a standalone backend will put
the negative of its PID into the file, and the revised code will fail
to parse that at all.

I think the safest code would be like

long tmp;

fscanf(pidf, "%ld", &tmp);
if (tmp < 0)
{
tmp = -tmp;
// do anything else needed for backend case
}
pid = (pid_t) tmp;

I deliberately used a signed long for these reasons in the first place.

The number of places we actually need to use this value as a pid is
small (3 by my count - in calls to kill() ), and it was cast there to
pid_t, I think. I still don't see what's wrong with that.

OK, I created a typedef pgpid_t equal to long and used that in the code,
change format to %ld, and documented why negative values are an issue.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073