Re: [HACKERS] [PATCH] pg_upgrade: Split off pg_fatal() from pg_log()

[Robert Haas]

On Tue, Oct 1, 2013 at 9:33 PM, Peter Eisentraut pete...@gmx.net wrote:
 On Sun, 2013-09-15 at 18:27 +0200, Marko Tiikkaja wrote:
 I think the reasoning behind this patch is sound.  However, I would like
 to raise a couple of small questions:

1) Is there a reason for the fmt string not being const char*?  You
 changed it for pg_log_v(), but not for pg_log() and pg_fatal().

 Good catch.  I think I just left the existing code alone.  I'll fix it.

2) Allowing PG_FATAL to be passed to pg_log() seems weird, but I
 don't feel strongly about that.

 Yeah, it's a bit weird.  It's just because it all ends up in pg_log_v().
 We could have pg_log() error about it, but that seems a bit excessive.
 It's not a public API or anything.

Since there are several votes in favor of this patch and none opposed,
I'm marking it Ready for Committer.

Peter, do you plan to commit it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] [PATCH] pg_upgrade: Split off pg_fatal() from pg_log()

[Peter Eisentraut]

On Sun, 2013-09-15 at 18:27 +0200, Marko Tiikkaja wrote:
 I think the reasoning behind this patch is sound.  However, I would like 
 to raise a couple of small questions:
 
1) Is there a reason for the fmt string not being const char*?  You 
 changed it for pg_log_v(), but not for pg_log() and pg_fatal().

Good catch.  I think I just left the existing code alone.  I'll fix it.

2) Allowing PG_FATAL to be passed to pg_log() seems weird, but I 
 don't feel strongly about that.

Yeah, it's a bit weird.  It's just because it all ends up in pg_log_v().
We could have pg_log() error about it, but that seems a bit excessive.
It's not a public API or anything.



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

Re: [HACKERS] [PATCH] pg_upgrade: Split off pg_fatal() from pg_log()

[Bruce Momjian]

On Thu, Sep 12, 2013 at 10:50:42PM -0400, Peter Eisentraut wrote:
 The experiences with elog() and ereport() have shown that having one
 function that can return or not depending on some log level parameter
 isn't a good idea when you want to communicate well with the compiler.
 In pg_upgrade, there is a similar case with the pg_log() function.
 Since that isn't a public API, I'm proposing to change it and introduce
 a separate function pg_fatal() for those cases where the calls don't
 return.

Fine with me.

-- 
  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

Re: [HACKERS] [PATCH] pg_upgrade: Split off pg_fatal() from pg_log()

[Marko Tiikkaja]

Hi Peter,

On 2013-09-13 04:50, Peter Eisentraut wrote:

The experiences with elog() and ereport() have shown that having one
function that can return or not depending on some log level parameter
isn't a good idea when you want to communicate well with the compiler.
In pg_upgrade, there is a similar case with the pg_log() function.
Since that isn't a public API, I'm proposing to change it and introduce
a separate function pg_fatal() for those cases where the calls don't
return.


I think the reasoning behind this patch is sound.  However, I would like 
to raise a couple of small questions:


  1) Is there a reason for the fmt string not being const char*?  You 
changed it for pg_log_v(), but not for pg_log() and pg_fatal().
  2) Allowing PG_FATAL to be passed to pg_log() seems weird, but I 
don't feel strongly about that.


Other than that, the patch looks good to me.


Regards,
Marko Tiikkaja


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

[HACKERS] [PATCH] pg_upgrade: Split off pg_fatal() from pg_log()

[Peter Eisentraut]

The experiences with elog() and ereport() have shown that having one
function that can return or not depending on some log level parameter
isn't a good idea when you want to communicate well with the compiler.
In pg_upgrade, there is a similar case with the pg_log() function.
Since that isn't a public API, I'm proposing to change it and introduce
a separate function pg_fatal() for those cases where the calls don't
return.

From 12ae7ed2b4174f694f67f31ac18eb1fe22d30ef9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut pete...@gmx.net
Date: Wed, 11 Sep 2013 22:43:03 -0400
Subject: [PATCH] pg_upgrade: Split off pg_fatal() from pg_log()

This allows decorating pg_fatal() with noreturn compiler hints, leading
to better diagnostics.
---
 contrib/pg_upgrade/check.c   | 68 --
 contrib/pg_upgrade/controldata.c | 92 
 contrib/pg_upgrade/exec.c| 21 
 contrib/pg_upgrade/file.c|  3 +-
 contrib/pg_upgrade/function.c| 11 ++---
 contrib/pg_upgrade/info.c|  6 +--
 contrib/pg_upgrade/option.c  | 23 +
 contrib/pg_upgrade/page.c|  6 +--
 contrib/pg_upgrade/parallel.c| 12 ++---
 contrib/pg_upgrade/pg_upgrade.c  |  8 ++--
 contrib/pg_upgrade/pg_upgrade.h  |  3 ++
 contrib/pg_upgrade/relfilenode.c | 11 ++---
 contrib/pg_upgrade/server.c  | 11 ++---
 contrib/pg_upgrade/tablespace.c  |  3 +-
 contrib/pg_upgrade/util.c| 35 +++---
 contrib/pg_upgrade/version.c |  2 +-
 contrib/pg_upgrade/version_old_8_3.c | 23 -
 17 files changed, 165 insertions(+), 173 deletions(-)

diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
index 0376fcb..1a37b79 100644
--- a/contrib/pg_upgrade/check.c
+++ b/contrib/pg_upgrade/check.c
@@ -158,8 +158,7 @@ static void check_locale_and_encoding(ControlData *oldctrl,
 	 * matching install-user oids.
 	 */
 	if (old_cluster.install_role_oid != new_cluster.install_role_oid)
-		pg_log(PG_FATAL,
-			   Old and new cluster install users have different values for pg_authid.oid.\n);
+		pg_fatal(Old and new cluster install users have different values for pg_authid.oid.\n);
 
 	/*
 	 * We only allow the install user in the new cluster because other defined
@@ -167,7 +166,7 @@ static void check_locale_and_encoding(ControlData *oldctrl,
 	 * error during pg_dump restore.
 	 */
 	if (new_cluster.role_count != 1)
-		pg_log(PG_FATAL, Only the install user can be defined in the new cluster.\n);
+		pg_fatal(Only the install user can be defined in the new cluster.\n);
 
 	check_for_prepared_transactions(new_cluster);
 }
@@ -271,11 +270,11 @@ static void check_locale_and_encoding(ControlData *oldctrl,
 	 */
 
 	if (GET_MAJOR_VERSION(old_cluster.major_version)  803)
-		pg_log(PG_FATAL, This utility can only upgrade from PostgreSQL version 8.3 and later.\n);
+		pg_fatal(This utility can only upgrade from PostgreSQL version 8.3 and later.\n);
 
 	/* Only current PG version is supported as a target */
 	if (GET_MAJOR_VERSION(new_cluster.major_version) != GET_MAJOR_VERSION(PG_VERSION_NUM))
-		pg_log(PG_FATAL, This utility can only upgrade to PostgreSQL version %s.\n,
+		pg_fatal(This utility can only upgrade to PostgreSQL version %s.\n,
 			   PG_MAJORVERSION);
 
 	/*
@@ -284,7 +283,7 @@ static void check_locale_and_encoding(ControlData *oldctrl,
 	 * versions.
 	 */
 	if (old_cluster.major_version  new_cluster.major_version)
-		pg_log(PG_FATAL, This utility cannot be used to downgrade to older major PostgreSQL versions.\n);
+		pg_fatal(This utility cannot be used to downgrade to older major PostgreSQL versions.\n);
 
 	/* get old and new binary versions */
 	get_bin_version(old_cluster);
@@ -293,12 +292,10 @@ static void check_locale_and_encoding(ControlData *oldctrl,
 	/* Ensure binaries match the designated data directories */
 	if (GET_MAJOR_VERSION(old_cluster.major_version) !=
 		GET_MAJOR_VERSION(old_cluster.bin_version))
-		pg_log(PG_FATAL,
-			   Old cluster data and binary directories are from different major versions.\n);
+		pg_fatal(Old cluster data and binary directories are from different major versions.\n);
 	if (GET_MAJOR_VERSION(new_cluster.major_version) !=
 		GET_MAJOR_VERSION(new_cluster.bin_version))
-		pg_log(PG_FATAL,
-			   New cluster data and binary directories are from different major versions.\n);
+		pg_fatal(New cluster data and binary directories are from different major versions.\n);
 
 	check_ok();
 }
@@ -315,17 +312,17 @@ static void check_locale_and_encoding(ControlData *oldctrl,
 	/* Is it 9.0 but without tablespace directories? */
 	if (GET_MAJOR_VERSION(new_cluster.major_version) == 900 
 		new_cluster.controldata.cat_ver  TABLE_SPACE_SUBDIRS_CAT_VER)
-		pg_log(PG_FATAL, This utility can only upgrade to PostgreSQL version 9.0 after 2010-01-11\n
+		pg_fatal(This utility can only upgrade to PostgreSQL version 9.0 after 2010-01-11\n
 			   because of backend API