Re: Invalid pg_upgrade error message during live check

[Bruce Momjian]

On Mon, Jan  8, 2018 at 06:26:21PM -0500, Bruce Momjian wrote:
> On Mon, Jan  8, 2018 at 10:43:00AM -0500, Robert Haas wrote:
> > On Fri, Jan 5, 2018 at 9:11 PM, Bruce Momjian  wrote:
> > > On Fri, Jan  5, 2018 at 04:58:39PM -0500, Bruce Momjian wrote:
> > >> Pg_upgrade is able to run in --check mode when the old server is still
> > >> running.  Unfortunately, all supported versions of pg_upgrade generate
> > >> an incorrect (but harmless) "failure" message when doing this:
> > >
> > > Based on recent discussions, I am thinking we would just fix this in PG
> > > 10 and HEAD and leave the harmless error message in the other branches,
> > > right?
> > 
> > Hmm, not sure why you say that.  If the message is buggy and the fix
> > isn't too risky, might as well fix it all the way back.
> 
> OK, I will do that then.  The message is harmless, but confusing, so I
> would like to fix it, but it requires changing the API of two functions.
> I will see how cleanly it applies back to 9.3 and act accordingly.

I ended up just doing HEAD and PG10.  When you change a function API,
you have to change every call site, and that can cause the patch to be
large and make backpatching too far difficult, which was the case for
this patch.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Invalid pg_upgrade error message during live check

[Bruce Momjian]

On Mon, Jan  8, 2018 at 10:43:00AM -0500, Robert Haas wrote:
> On Fri, Jan 5, 2018 at 9:11 PM, Bruce Momjian  wrote:
> > On Fri, Jan  5, 2018 at 04:58:39PM -0500, Bruce Momjian wrote:
> >> Pg_upgrade is able to run in --check mode when the old server is still
> >> running.  Unfortunately, all supported versions of pg_upgrade generate
> >> an incorrect (but harmless) "failure" message when doing this:
> >
> > Based on recent discussions, I am thinking we would just fix this in PG
> > 10 and HEAD and leave the harmless error message in the other branches,
> > right?
> 
> Hmm, not sure why you say that.  If the message is buggy and the fix
> isn't too risky, might as well fix it all the way back.

OK, I will do that then.  The message is harmless, but confusing, so I
would like to fix it, but it requires changing the API of two functions.
I will see how cleanly it applies back to 9.3 and act accordingly.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Invalid pg_upgrade error message during live check

[Robert Haas]

On Fri, Jan 5, 2018 at 9:11 PM, Bruce Momjian  wrote:
> On Fri, Jan  5, 2018 at 04:58:39PM -0500, Bruce Momjian wrote:
>> Pg_upgrade is able to run in --check mode when the old server is still
>> running.  Unfortunately, all supported versions of pg_upgrade generate
>> an incorrect (but harmless) "failure" message when doing this:
>
> Based on recent discussions, I am thinking we would just fix this in PG
> 10 and HEAD and leave the harmless error message in the other branches,
> right?

Hmm, not sure why you say that.  If the message is buggy and the fix
isn't too risky, might as well fix it all the way back.

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

Re: Invalid pg_upgrade error message during live check

[Bruce Momjian]

On Fri, Jan  5, 2018 at 04:58:39PM -0500, Bruce Momjian wrote:
> Pg_upgrade is able to run in --check mode when the old server is still
> running.  Unfortunately, all supported versions of pg_upgrade generate
> an incorrect (but harmless) "failure" message when doing this:

Based on recent discussions, I am thinking we would just fix this in PG
10 and HEAD and leave the harmless error message in the other branches,
right?

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Invalid pg_upgrade error message during live check

[Bruce Momjian]

Pg_upgrade is able to run in --check mode when the old server is still
running.  Unfortunately, all supported versions of pg_upgrade generate
an incorrect (but harmless) "failure" message when doing this:

$ pg_upgrade -b /u/pgsql.old/bin -B /u/pgsql/bin \
-d /u/pgsql.old/data/ -D /u/pgsql/data --link --check

--> *failure*
--> Consult the last few lines of "pg_upgrade_server.log" for
--> the probable cause of the failure.
Performing Consistency Checks on Old Live Server

Checking cluster versions   ok
Checking database user is the install user  ok
Checking database connection settings   ok
Checking for prepared transactions  ok
Checking for reg* data types in user tables ok
Checking for contrib/isn with bigint-passing mismatch   ok
Checking for invalid "unknown" user columns ok
Checking for hash indexes   ok
Checking for roles starting with "pg_"  ok
Checking for presence of required libraries ok
Checking database user is the install user  ok
Checking for prepared transactions  ok

*Clusters are compatible*

Embarrassingly, I found out about this bug when watching a presentation in
Frankfurt:

PostgreSQL-Upgrade: Best Practices

https://www.ittage.informatik-aktuell.de/programm/2017/postgresql-upgrade-best-practices/

and there is a blog entry about it too:


https://blog.dbi-services.com/does-pg_upgrade-in-check-mode-raises-a-failure-when-the-old-cluster-is-running/

(Yeah, it stinks to be me.  :-) )

So, I looked into the code and it turns out that start_postmaster()
needs to run exec_prog() in a way that allows it to fail and continue
_without_ generating an error message.  There are other places that need
to run exec_prog() and allow it to fail and continue _and_ generate an
error message.

To fix this, I modified the exec_prog() API to separately control
reporting and exiting-on errors.  The attached patch does this and
generates the proper output:

$ pg_upgrade -b /u/pgsql.old/bin -B /u/pgsql/bin \
-d /u/pgsql.old/data/ -D /u/pgsql/data --link --check

Performing Consistency Checks on Old Live Server

Checking cluster versions   ok
Checking database user is the install user  ok
Checking database connection settings   ok
Checking for prepared transactions  ok
Checking for reg* data types in user tables ok
Checking for contrib/isn with bigint-passing mismatch   ok
Checking for invalid "unknown" user columns ok
Checking for hash indexes   ok
Checking for roles starting with "pg_"  ok
Checking for presence of required libraries ok
Checking database user is the install user  ok
Checking for prepared transactions  ok

*Clusters are compatible*

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +
diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c
new file mode 100644
index 5ed6b78..8a662e9
*** a/src/bin/pg_upgrade/dump.c
--- b/src/bin/pg_upgrade/dump.c
*** generate_old_dump(void)
*** 23,29 
  	prep_status("Creating dump of global objects");
  
  	/* run new pg_dumpall binary for globals */
! 	exec_prog(UTILITY_LOG_FILE, NULL, true,
  			  "\"%s/pg_dumpall\" %s --globals-only --quote-all-identifiers "
  			  "--binary-upgrade %s -f %s",
  			  new_cluster.bindir, cluster_conn_opts(_cluster),
--- 23,29 
  	prep_status("Creating dump of global objects");
  
  	/* run new pg_dumpall binary for globals */
! 	exec_prog(UTILITY_LOG_FILE, NULL, true, true,
  			  "\"%s/pg_dumpall\" %s --globals-only --quote-all-identifiers "
  			  "--binary-upgrade %s -f %s",
  			  new_cluster.bindir, cluster_conn_opts(_cluster),
diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
new file mode 100644
index ea45434..9122e27
*** a/src/bin/pg_upgrade/exec.c
--- b/src/bin/pg_upgrade/exec.c
*** get_bin_version(ClusterInfo *cluster)
*** 71,86 
   * and attempts to execute that command.  If the command executes
   * successfully, exec_prog() returns true.
   *
!  * If the command fails, an