On 04.10.22 10:15, Peter Eisentraut wrote:
I wanted to propose the attached patch to get rid of the custom pgpid_t
typedef in pg_ctl. Since we liberally use pid_t elsewhere, this seemed
plausible.
However, this patch fails the CompilerWarnings job on Cirrus, because
apparently under mingw, pid_t is "volatile long long int", so all the
printf placeholders mismatch. However, we print pid_t as %d in a lot of
other places, so I'm confused why this fails here.
I figured out that in most places we actually store PIDs in int, and in
the cases where we use pid_t, casts before printing are indeed used and
necessary. So nevermind that.
In any case, I took this opportunity to standardize the printing of PIDs
as %d. There were a few stragglers.
And then the original patch to get rid of pgpid_t in pg_ctl, now updated
with the correct casts for printing. I confirmed that this now passes
the CompilerWarnings job.
From 72b0620f3824ef91bf9f64b4814e5874f8152322 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 10 Oct 2022 10:04:45 +0200
Subject: [PATCH v2 1/3] doc: Correct type of bgw_notify_pid
This has apparently been wrong since the beginning
(090d0f2050647958865cb495dff74af7257d2bb4).
---
doc/src/sgml/bgworker.sgml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 3d4e4afcf919..7ba5da27e50a 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -63,7 +63,7 @@ <title>Background Worker Processes</title>
char bgw_function_name[BGW_MAXLEN];
Datum bgw_main_arg;
char bgw_extra[BGW_EXTRALEN];
- int bgw_notify_pid;
+ pid_t bgw_notify_pid;
} BackgroundWorker;
</programlisting>
</para>
base-commit: 06dbd619bfbfe03fefa7223838690d4012f874ad
--
2.37.3
From c405c4a942c861a720662035983dca237fb92527 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 10 Oct 2022 10:07:14 +0200
Subject: [PATCH v2 2/3] Standardize format for printing PIDs
Most code prints PIDs as %d, but some code tried to print them as long
or unsigned long. While this is in theory allowed, the fact that PIDs
fit into int is deeply baked into all PostgreSQL code, so these random
deviations don't accomplish anything except confusion.
Note that we still need casts from pid_t to int, because on 64-bit
MinGW, pid_t is long long int. (But per above, actually supporting
that range in PostgreSQL code would be major surgery and probably not
useful.)
---
contrib/pg_prewarm/autoprewarm.c | 20 ++++++++++----------
src/backend/postmaster/bgworker.c | 4 ++--
src/backend/storage/ipc/procsignal.c | 4 ++--
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index c8d673a20e36..1843b1862e57 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -193,8 +193,8 @@ autoprewarm_main(Datum main_arg)
{
LWLockRelease(&apw_state->lock);
ereport(LOG,
- (errmsg("autoprewarm worker is already running
under PID %lu",
- (unsigned long)
apw_state->bgworker_pid)));
+ (errmsg("autoprewarm worker is already running
under PID %d",
+ (int)
apw_state->bgworker_pid)));
return;
}
apw_state->bgworker_pid = MyProcPid;
@@ -303,8 +303,8 @@ apw_load_buffers(void)
{
LWLockRelease(&apw_state->lock);
ereport(LOG,
- (errmsg("skipping prewarm because block dump
file is being written by PID %lu",
- (unsigned long)
apw_state->pid_using_dumpfile)));
+ (errmsg("skipping prewarm because block dump
file is being written by PID %d",
+ (int)
apw_state->pid_using_dumpfile)));
return;
}
LWLockRelease(&apw_state->lock);
@@ -599,12 +599,12 @@ apw_dump_now(bool is_bgworker, bool dump_unlogged)
{
if (!is_bgworker)
ereport(ERROR,
- (errmsg("could not perform block dump
because dump file is being used by PID %lu",
- (unsigned long)
apw_state->pid_using_dumpfile)));
+ (errmsg("could not perform block dump
because dump file is being used by PID %d",
+ (int)
apw_state->pid_using_dumpfile)));
ereport(LOG,
- (errmsg("skipping block dump because it is
already being performed by PID %lu",
- (unsigned long)
apw_state->pid_using_dumpfile)));
+ (errmsg("skipping block dump because it is
already being performed by PID %d",
+ (int)
apw_state->pid_using_dumpfile)));
return 0;
}
@@ -737,8 +737,8 @@ autoprewarm_start_worker(PG_FUNCTION_ARGS)
if (pid != InvalidPid)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("autoprewarm worker is already running
under PID %lu",
- (unsigned long) pid)));
+ errmsg("autoprewarm worker is already running
under PID %d",
+ (int) pid)));
apw_start_leader_worker();
diff --git a/src/backend/postmaster/bgworker.c
b/src/backend/postmaster/bgworker.c
index 8dd7d64630c4..0d72de24b02b 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -389,8 +389,8 @@ BackgroundWorkerStateChange(bool allow_new_workers)
rw->rw_worker.bgw_notify_pid = slot->worker.bgw_notify_pid;
if
(!PostmasterMarkPIDForWorkerNotify(rw->rw_worker.bgw_notify_pid))
{
- elog(DEBUG1, "worker notification PID %ld is not valid",
- (long) rw->rw_worker.bgw_notify_pid);
+ elog(DEBUG1, "worker notification PID %d is not valid",
+ (int) rw->rw_worker.bgw_notify_pid);
rw->rw_worker.bgw_notify_pid = 0;
}
diff --git a/src/backend/storage/ipc/procsignal.c
b/src/backend/storage/ipc/procsignal.c
index 21a9fc0fdd2e..7767657f2713 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -416,8 +416,8 @@ WaitForProcSignalBarrier(uint64 generation)
5000,
WAIT_EVENT_PROC_SIGNAL_BARRIER))
ereport(LOG,
- (errmsg("still waiting for
backend with PID %lu to accept ProcSignalBarrier",
- (unsigned long)
slot->pss_pid)));
+ (errmsg("still waiting for
backend with PID %d to accept ProcSignalBarrier",
+ (int)
slot->pss_pid)));
oldval =
pg_atomic_read_u64(&slot->pss_barrierGeneration);
}
ConditionVariableCancelSleep();
--
2.37.3
From 079b46b7d48b803b1252a1e3f5fe370943487e40 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 10 Oct 2022 10:13:36 +0200
Subject: [PATCH v2 3/3] Remove pgpid_t type, use pid_t instead
It's unclear why a separate type would be needed here. We use plain
pid_t everywhere else.
Reverts 66fa6eba5a61be740a6c07de92c42221fae79e9c.
---
src/bin/pg_ctl/pg_ctl.c | 135 +++++++++++++++----------------
src/tools/pgindent/typedefs.list | 1 -
2 files changed, 66 insertions(+), 70 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 9291c61d000f..a509fc9109e8 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -33,9 +33,6 @@
#include "pqexpbuffer.h"
#endif
-/* PID can be negative for standalone backend */
-typedef long pgpid_t;
-
typedef enum
{
@@ -103,7 +100,7 @@ static char backup_file[MAXPGPATH];
static char promote_file[MAXPGPATH];
static char logrotate_file[MAXPGPATH];
-static volatile pgpid_t postmasterPID = -1;
+static volatile pid_t postmasterPID = -1;
#ifdef WIN32
static DWORD pgctl_start_type = SERVICE_AUTO_START;
@@ -129,7 +126,7 @@ static void do_reload(void);
static void do_status(void);
static void do_promote(void);
static void do_logrotate(void);
-static void do_kill(pgpid_t pid);
+static void do_kill(pid_t pid);
static void print_msg(const char *msg);
static void adjust_data_dir(void);
@@ -147,13 +144,13 @@ static int CreateRestrictedProcess(char *cmd,
PROCESS_INFORMATION *processInfo,
static PTOKEN_PRIVILEGES GetPrivilegesToDelete(HANDLE hToken);
#endif
-static pgpid_t get_pgpid(bool is_status_request);
+static pid_t get_pgpid(bool is_status_request);
static char **readfile(const char *path, int *numlines);
static void free_readfile(char **optlines);
-static pgpid_t start_postmaster(void);
+static pid_t start_postmaster(void);
static void read_post_opts(void);
-static WaitPMResult wait_for_postmaster_start(pgpid_t pm_pid, bool
do_checkpoint);
+static WaitPMResult wait_for_postmaster_start(pid_t pm_pid, bool
do_checkpoint);
static bool wait_for_postmaster_stop(void);
static bool wait_for_postmaster_promote(void);
static bool postmaster_is_alive(pid_t pid);
@@ -245,11 +242,11 @@ print_msg(const char *msg)
}
}
-static pgpid_t
+static pid_t
get_pgpid(bool is_status_request)
{
FILE *pidf;
- long pid;
+ int pid;
struct stat statbuf;
if (stat(pg_data, &statbuf) != 0)
@@ -289,7 +286,7 @@ get_pgpid(bool is_status_request)
exit(1);
}
}
- if (fscanf(pidf, "%ld", &pid) != 1)
+ if (fscanf(pidf, "%d", &pid) != 1)
{
/* Is the file empty? */
if (ftell(pidf) == 0 && feof(pidf))
@@ -301,7 +298,7 @@ get_pgpid(bool is_status_request)
exit(1);
}
fclose(pidf);
- return (pgpid_t) pid;
+ return (pid_t) pid;
}
@@ -439,13 +436,13 @@ free_readfile(char **optlines)
* On Windows, we also save aside a handle to the shell process in
* "postmasterProcess", which the caller should close when done with it.
*/
-static pgpid_t
+static pid_t
start_postmaster(void)
{
char *cmd;
#ifndef WIN32
- pgpid_t pm_pid;
+ pid_t pm_pid;
/* Flush stdio channels just before fork, to avoid double-output
problems */
fflush(NULL);
@@ -593,7 +590,7 @@ start_postmaster(void)
* manager checkpoint, it's got nothing to do with database checkpoints!!
*/
static WaitPMResult
-wait_for_postmaster_start(pgpid_t pm_pid, bool do_checkpoint)
+wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
{
int i;
@@ -610,7 +607,7 @@ wait_for_postmaster_start(pgpid_t pm_pid, bool
do_checkpoint)
numlines >= LOCK_FILE_LINE_PM_STATUS)
{
/* File is complete enough for us, parse it */
- pgpid_t pmpid;
+ pid_t pmpid;
time_t pmstart;
/*
@@ -665,7 +662,7 @@ wait_for_postmaster_start(pgpid_t pm_pid, bool
do_checkpoint)
{
int exitstatus;
- if (waitpid((pid_t) pm_pid, &exitstatus, WNOHANG) ==
(pid_t) pm_pid)
+ if (waitpid(pm_pid, &exitstatus, WNOHANG) == pm_pid)
return POSTMASTER_FAILED;
}
#else
@@ -716,12 +713,12 @@ wait_for_postmaster_stop(void)
for (cnt = 0; cnt < wait_seconds * WAITS_PER_SEC; cnt++)
{
- pgpid_t pid;
+ pid_t pid;
if ((pid = get_pgpid(false)) == 0)
return true; /* pid file is gone */
- if (kill((pid_t) pid, 0) != 0)
+ if (kill(pid, 0) != 0)
{
/*
* Postmaster seems to have died. Check the pid file
once more to
@@ -753,12 +750,12 @@ wait_for_postmaster_promote(void)
for (cnt = 0; cnt < wait_seconds * WAITS_PER_SEC; cnt++)
{
- pgpid_t pid;
+ pid_t pid;
DBState state;
if ((pid = get_pgpid(false)) == 0)
return false; /* pid file is gone */
- if (kill((pid_t) pid, 0) != 0)
+ if (kill(pid, 0) != 0)
return false; /* postmaster died */
state = get_control_dbstate();
@@ -855,8 +852,8 @@ trap_sigint_during_startup(SIGNAL_ARGS)
if (postmasterPID != -1)
{
if (kill(postmasterPID, SIGINT) != 0)
- write_stderr(_("%s: could not send stop signal (PID:
%ld): %s\n"),
- progname, (pgpid_t)
postmasterPID, strerror(errno));
+ write_stderr(_("%s: could not send stop signal (PID:
%d): %s\n"),
+ progname, (int) postmasterPID,
strerror(errno));
}
/*
@@ -926,8 +923,8 @@ do_init(void)
static void
do_start(void)
{
- pgpid_t old_pid = 0;
- pgpid_t pm_pid;
+ pid_t old_pid = 0;
+ pid_t pm_pid;
if (ctl_command != RESTART_COMMAND)
{
@@ -1018,7 +1015,7 @@ do_start(void)
static void
do_stop(void)
{
- pgpid_t pid;
+ pid_t pid;
pid = get_pgpid(false);
@@ -1032,14 +1029,14 @@ do_stop(void)
{
pid = -pid;
write_stderr(_("%s: cannot stop server; "
- "single-user server is running (PID:
%ld)\n"),
- progname, pid);
+ "single-user server is running (PID:
%d)\n"),
+ progname, (int) pid);
exit(1);
}
- if (kill((pid_t) pid, sig) != 0)
+ if (kill(pid, sig) != 0)
{
- write_stderr(_("%s: could not send stop signal (PID: %ld):
%s\n"), progname, pid,
+ write_stderr(_("%s: could not send stop signal (PID: %d):
%s\n"), progname, (int) pid,
strerror(errno));
exit(1);
}
@@ -1077,7 +1074,7 @@ do_stop(void)
static void
do_restart(void)
{
- pgpid_t pid;
+ pid_t pid;
pid = get_pgpid(false);
@@ -1093,21 +1090,21 @@ do_restart(void)
else if (pid < 0) /* standalone backend, not
postmaster */
{
pid = -pid;
- if (postmaster_is_alive((pid_t) pid))
+ if (postmaster_is_alive(pid))
{
write_stderr(_("%s: cannot restart server; "
- "single-user server is
running (PID: %ld)\n"),
- progname, pid);
+ "single-user server is
running (PID: %d)\n"),
+ progname, (int) pid);
write_stderr(_("Please terminate the single-user server
and try again.\n"));
exit(1);
}
}
- if (postmaster_is_alive((pid_t) pid))
+ if (postmaster_is_alive(pid))
{
- if (kill((pid_t) pid, sig) != 0)
+ if (kill(pid, sig) != 0)
{
- write_stderr(_("%s: could not send stop signal (PID:
%ld): %s\n"), progname, pid,
+ write_stderr(_("%s: could not send stop signal (PID:
%d): %s\n"), progname, (int) pid,
strerror(errno));
exit(1);
}
@@ -1131,8 +1128,8 @@ do_restart(void)
}
else
{
- write_stderr(_("%s: old server process (PID: %ld) seems to be
gone\n"),
- progname, pid);
+ write_stderr(_("%s: old server process (PID: %d) seems to be
gone\n"),
+ progname, (int) pid);
write_stderr(_("starting server anyway\n"));
}
@@ -1142,7 +1139,7 @@ do_restart(void)
static void
do_reload(void)
{
- pgpid_t pid;
+ pid_t pid;
pid = get_pgpid(false);
if (pid == 0) /* no pid file */
@@ -1155,16 +1152,16 @@ do_reload(void)
{
pid = -pid;
write_stderr(_("%s: cannot reload server; "
- "single-user server is running (PID:
%ld)\n"),
- progname, pid);
+ "single-user server is running (PID:
%d)\n"),
+ progname, (int) pid);
write_stderr(_("Please terminate the single-user server and try
again.\n"));
exit(1);
}
- if (kill((pid_t) pid, sig) != 0)
+ if (kill(pid, sig) != 0)
{
- write_stderr(_("%s: could not send reload signal (PID: %ld):
%s\n"),
- progname, pid, strerror(errno));
+ write_stderr(_("%s: could not send reload signal (PID: %d):
%s\n"),
+ progname, (int) pid, strerror(errno));
exit(1);
}
@@ -1180,7 +1177,7 @@ static void
do_promote(void)
{
FILE *prmfile;
- pgpid_t pid;
+ pid_t pid;
pid = get_pgpid(false);
@@ -1194,8 +1191,8 @@ do_promote(void)
{
pid = -pid;
write_stderr(_("%s: cannot promote server; "
- "single-user server is running (PID:
%ld)\n"),
- progname, pid);
+ "single-user server is running (PID:
%d)\n"),
+ progname, (int) pid);
exit(1);
}
@@ -1223,10 +1220,10 @@ do_promote(void)
}
sig = SIGUSR1;
- if (kill((pid_t) pid, sig) != 0)
+ if (kill(pid, sig) != 0)
{
- write_stderr(_("%s: could not send promote signal (PID: %ld):
%s\n"),
- progname, pid, strerror(errno));
+ write_stderr(_("%s: could not send promote signal (PID: %d):
%s\n"),
+ progname, (int) pid, strerror(errno));
if (unlink(promote_file) != 0)
write_stderr(_("%s: could not remove promote signal
file \"%s\": %s\n"),
progname, promote_file,
strerror(errno));
@@ -1261,7 +1258,7 @@ static void
do_logrotate(void)
{
FILE *logrotatefile;
- pgpid_t pid;
+ pid_t pid;
pid = get_pgpid(false);
@@ -1275,8 +1272,8 @@ do_logrotate(void)
{
pid = -pid;
write_stderr(_("%s: cannot rotate log file; "
- "single-user server is running (PID:
%ld)\n"),
- progname, pid);
+ "single-user server is running (PID:
%d)\n"),
+ progname, (int) pid);
exit(1);
}
@@ -1296,10 +1293,10 @@ do_logrotate(void)
}
sig = SIGUSR1;
- if (kill((pid_t) pid, sig) != 0)
+ if (kill(pid, sig) != 0)
{
- write_stderr(_("%s: could not send log rotation signal (PID:
%ld): %s\n"),
- progname, pid, strerror(errno));
+ write_stderr(_("%s: could not send log rotation signal (PID:
%d): %s\n"),
+ progname, (int) pid, strerror(errno));
if (unlink(logrotate_file) != 0)
write_stderr(_("%s: could not remove log rotation
signal file \"%s\": %s\n"),
progname, logrotate_file,
strerror(errno));
@@ -1341,7 +1338,7 @@ postmaster_is_alive(pid_t pid)
static void
do_status(void)
{
- pgpid_t pid;
+ pid_t pid;
pid = get_pgpid(true);
/* Is there a pid file? */
@@ -1351,24 +1348,24 @@ do_status(void)
if (pid < 0)
{
pid = -pid;
- if (postmaster_is_alive((pid_t) pid))
+ if (postmaster_is_alive(pid))
{
- printf(_("%s: single-user server is running
(PID: %ld)\n"),
- progname, pid);
+ printf(_("%s: single-user server is running
(PID: %d)\n"),
+ progname, (int) pid);
return;
}
}
else
/* must be a postmaster */
{
- if (postmaster_is_alive((pid_t) pid))
+ if (postmaster_is_alive(pid))
{
char **optlines;
char **curr_line;
int numlines;
- printf(_("%s: server is running (PID: %ld)\n"),
- progname, pid);
+ printf(_("%s: server is running (PID: %d)\n"),
+ progname, (int) pid);
optlines = readfile(postopts_file, &numlines);
if (optlines != NULL)
@@ -1396,12 +1393,12 @@ do_status(void)
static void
-do_kill(pgpid_t pid)
+do_kill(pid_t pid)
{
- if (kill((pid_t) pid, sig) != 0)
+ if (kill(pid, sig) != 0)
{
- write_stderr(_("%s: could not send signal %d (PID: %ld): %s\n"),
- progname, sig, pid, strerror(errno));
+ write_stderr(_("%s: could not send signal %d (PID: %d): %s\n"),
+ progname, sig, (int) pid,
strerror(errno));
exit(1);
}
}
@@ -2214,7 +2211,7 @@ main(int argc, char **argv)
char *env_wait;
int option_index;
int c;
- pgpid_t killproc = 0;
+ pid_t killproc = 0;
pg_logging_init(argv[0]);
progname = get_progname(argv[0]);
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 97c9bc186157..3e8cead90f48 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3513,7 +3513,6 @@ pg_wc_probefunc
pg_wchar
pg_wchar_tbl
pgp_armor_headers_state
-pgpid_t
pgsocket
pgsql_thing_t
pgssEntry
--
2.37.3
