[HACKERS] pg_upgrade exit_nicely()

2011-03-31 Thread Peter Eisentraut
While reading around in pg_upgrade code I came across the slightly
bizarre function

void exit_nicely(bool need_cleanup)

The parameter doesn't actually determine whether any cleanup is done.
The cleanup is done anyway and the parameter only controls the exit
code in a backwards way.

Also most of the cleanup appears to be useless, because you don't need
to close files or free memory before the program exits.

I figured this could be written more cleanly with an exit hook, so here
is a patch.  I don't care much whether this patch is for now or later,
just wanted to throw it out there.

diff --git i/contrib/pg_upgrade/check.c w/contrib/pg_upgrade/check.c
index 1c4847a..05aac8f 100644
--- i/contrib/pg_upgrade/check.c
+++ w/contrib/pg_upgrade/check.c
@@ -131,7 +131,7 @@ report_clusters_compatible(void)
 		pg_log(PG_REPORT, \n*Clusters are compatible*\n);
 		/* stops new cluster */
 		stop_postmaster(false, false);
-		exit_nicely(false);
+		exit(0);
 	}
 
 	pg_log(PG_REPORT, \n
diff --git i/contrib/pg_upgrade/option.c w/contrib/pg_upgrade/option.c
index c984535..857d652 100644
--- i/contrib/pg_upgrade/option.c
+++ w/contrib/pg_upgrade/option.c
@@ -77,12 +77,12 @@ parseCommandLine(int argc, char *argv[])
 			strcmp(argv[1], -?) == 0)
 		{
 			usage();
-			exit_nicely(false);
+			exit(0);
 		}
 		if (strcmp(argv[1], --version) == 0 || strcmp(argv[1], -V) == 0)
 		{
 			pg_log(PG_REPORT, pg_upgrade  PG_VERSION \n);
-			exit_nicely(false);
+			exit(0);
 		}
 	}
 
@@ -125,7 +125,7 @@ parseCommandLine(int argc, char *argv[])
 if ((log_opts.debug_fd = fopen(optarg, w)) == NULL)
 {
 	pg_log(PG_FATAL, cannot open debug file\n);
-	exit_nicely(false);
+	exit(1);
 }
 break;
 
@@ -141,7 +141,7 @@ parseCommandLine(int argc, char *argv[])
 if ((old_cluster.port = atoi(optarg)) = 0)
 {
 	pg_log(PG_FATAL, invalid old port number\n);
-	exit_nicely(false);
+	exit(1);
 }
 break;
 
@@ -149,7 +149,7 @@ parseCommandLine(int argc, char *argv[])
 if ((new_cluster.port = atoi(optarg)) = 0)
 {
 	pg_log(PG_FATAL, invalid new port number\n);
-	exit_nicely(false);
+	exit(1);
 }
 break;
 
diff --git i/contrib/pg_upgrade/pg_upgrade.h w/contrib/pg_upgrade/pg_upgrade.h
index 4461952..8f72ea8 100644
--- i/contrib/pg_upgrade/pg_upgrade.h
+++ w/contrib/pg_upgrade/pg_upgrade.h
@@ -363,7 +363,6 @@ void		check_for_libpq_envvars(void);
 
 /* util.c */
 
-void		exit_nicely(bool need_cleanup);
 char	   *quote_identifier(const char *s);
 int			get_user_info(char **user_name);
 void		check_ok(void);
diff --git i/contrib/pg_upgrade/server.c w/contrib/pg_upgrade/server.c
index 4d5f373..a155f90 100644
--- i/contrib/pg_upgrade/server.c
+++ w/contrib/pg_upgrade/server.c
@@ -23,7 +23,7 @@ static bool test_server_conn(ClusterInfo *cluster, int timeout);
  *
  *	Connects to the desired database on the designated server.
  *	If the connection attempt fails, this function logs an error
- *	message and calls exit_nicely() to kill the program.
+ *	message and calls exit() to kill the program.
  */
 PGconn *
 connectToServer(ClusterInfo *cluster, const char *db_name)
@@ -45,7 +45,8 @@ connectToServer(ClusterInfo *cluster, const char *db_name)
 		if (conn)
 			PQfinish(conn);
 
-		exit_nicely(true);
+		printf(Failure, exiting\n);
+		exit(1);
 	}
 
 	return conn;
@@ -57,7 +58,7 @@ connectToServer(ClusterInfo *cluster, const char *db_name)
  *
  *	Formats a query string from the given arguments and executes the
  *	resulting query.  If the query fails, this function logs an error
- *	message and calls exit_nicely() to kill the program.
+ *	message and calls exit() to kill the program.
  */
 PGresult *
 executeQueryOrDie(PGconn *conn, const char *fmt,...)
@@ -81,8 +82,8 @@ executeQueryOrDie(PGconn *conn, const char *fmt,...)
 			   PQerrorMessage(conn));
 		PQclear(result);
 		PQfinish(conn);
-		exit_nicely(true);
-		return NULL;			/* Never get here, but keeps compiler happy */
+		printf(Failure, exiting\n);
+		exit(1);
 	}
 	else
 		return result;
@@ -152,6 +153,18 @@ get_major_server_version(ClusterInfo *cluster)
 }
 
 
+static void
+#ifdef HAVE_ATEXIT
+stop_postmaster_exit_hook(void)
+#else
+stop_postmaster_exit_hook(int exitstatus, void *arg)
+#endif
+{
+	stop_postmaster(true, true);
+
+}
+
+
 void
 start_postmaster(ClusterInfo *cluster, bool quiet)
 {
@@ -159,11 +172,22 @@ start_postmaster(ClusterInfo *cluster, bool quiet)
 	const char *bindir;
 	const char *datadir;
 	unsigned short port;
+	bool		exit_hook_registered = false;
 
 	bindir = cluster-bindir;
 	datadir = cluster-pgdata;
 	port = cluster-port;
 
+	if (!exit_hook_registered)
+	{
+#ifdef HAVE_ATEXIT
+		atexit(stop_postmaster_exit_hook);
+#else
+		on_exit(stop_postmaster_exit_hook);
+#endif
+		exit_hook_registered = true;
+	}
+
 	/*
 	 * On Win32, we can't send both pg_upgrade output and pg_ctl output to the
 	 * same file because we get the error: The process cannot access the file
diff --git i/contrib/pg_upgrade/util.c 

Re: [HACKERS] pg_upgrade exit_nicely()

2011-03-31 Thread Bruce Momjian

Feel free to apply this to HEAD.

---

Peter Eisentraut wrote:
 While reading around in pg_upgrade code I came across the slightly
 bizarre function
 
 void exit_nicely(bool need_cleanup)
 
 The parameter doesn't actually determine whether any cleanup is done.
 The cleanup is done anyway and the parameter only controls the exit
 code in a backwards way.
 
 Also most of the cleanup appears to be useless, because you don't need
 to close files or free memory before the program exits.
 
 I figured this could be written more cleanly with an exit hook, so here
 is a patch.  I don't care much whether this patch is for now or later,
 just wanted to throw it out there.
 

[ Attachment, skipping... ]

 
 -- 
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers