Re: [GENERAL] trouble with pg_upgrade 9.0 -> 9.1

Applied.  We can mark this report closed.  Groshev, let us know if you
have any further problems.

---------------------------------------------------------------------------

On Thu, Dec 20, 2012 at 07:19:48AM -0500, Bruce Momjian wrote:
> On Wed, Dec 19, 2012 at 10:19:30PM -0500, Bruce Momjian wrote:
> > On Wed, Dec 19, 2012 at 12:56:05PM -0500, Kevin Grittner wrote:
> > > Groshev Andrey wrote:
> > > 
> > > > >>>>>   Mismatch of relation names: database "database", old rel public.lob.ВерсияВнешнегоДокумента$Документ_pkey, new rel public.plob.ВерсияВнешнегоДокумента$Документ
> > > 
> > > There is a limit on identifiers of 63 *bytes* (not characters)
> > > after which the name is truncated. In UTF8 encoding, the underscore
> > > would be in the 64th position.
> > 
> > OK, Kevin is certainly pointing out a bug in the pg_upgrade code, though
> > I am unclear how it would exhibit the mismatch error reported.
> > 
> > pg_upgrade uses NAMEDATALEN for database, schema, and relation name
> > storage lengths.  While NAMEDATALEN works fine in the backend, it is
> > possible that a frontend client, like pg_upgrade, could retrieve a name
> > in the client encoding whose length exceeds NAMEDATALEN if the client
> > encoding did not match the database encoding (or is it the cluster
> > encoding for system tables).  This would cause truncation of these
> > values.  The truncation would not cause crashes, but might cause
> > failures by not being able to connect to overly-long database names, and
> > it weakens the checking of relation/schema names --- the same check that
> > is reported above.
> > 
> > (I believe initdb.c also erroneously uses NAMEDATALEN.)
> 
> I have developed the attached patch to pg_strdup() the string returned
> from libpq, rather than use a fixed NAMEDATALEN buffer to store the
> value.  I am only going to apply this to 9.3 because I can't see this
> causing problems except for weaker comparisons for very long identifiers
> where the client encoding is longer than the server encoding, and
> failures for very long database names, no of which we have gotten bug
> reports about.
> 
> Turns out initdb.c was fine because it expects only collation names to
> be only in ASCII;   I added a comment to that effect.
> 
> -- 
>   Bruce Momjian  <bruce@momjian.us>        http://momjian.us
>   EnterpriseDB                             http://enterprisedb.com
> 
>   + It's impossible for everything to be true. +

> diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
> new file mode 100644
> index 2250442..5cb9b61
> *** a/contrib/pg_upgrade/info.c
> --- b/contrib/pg_upgrade/info.c
> *************** static void get_db_infos(ClusterInfo *cl
> *** 23,29 ****
>   static void get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo);
>   static void free_rel_infos(RelInfoArr *rel_arr);
>   static void print_db_infos(DbInfoArr *dbinfo);
> ! static void print_rel_infos(RelInfoArr *arr);
>   
>   
>   /*
> --- 23,29 ----
>   static void get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo);
>   static void free_rel_infos(RelInfoArr *rel_arr);
>   static void print_db_infos(DbInfoArr *dbinfo);
> ! static void print_rel_infos(RelInfoArr *rel_arr);
>   
>   
>   /*
> *************** create_rel_filename_map(const char *old_
> *** 127,134 ****
>   	map->new_relfilenode = new_rel->relfilenode;
>   
>   	/* used only for logging and error reporing, old/new are identical */
> ! 	snprintf(map->nspname, sizeof(map->nspname), "%s", old_rel->nspname);
> ! 	snprintf(map->relname, sizeof(map->relname), "%s", old_rel->relname);
>   }
>   
>   
> --- 127,134 ----
>   	map->new_relfilenode = new_rel->relfilenode;
>   
>   	/* used only for logging and error reporing, old/new are identical */
> ! 	map->nspname = old_rel->nspname;
> ! 	map->relname = old_rel->relname;
>   }
>   
>   
> *************** get_db_infos(ClusterInfo *cluster)
> *** 220,227 ****
>   	for (tupnum = 0; tupnum < ntups; tupnum++)
>   	{
>   		dbinfos[tupnum].db_oid = atooid(PQgetvalue(res, tupnum, i_oid));
> ! 		snprintf(dbinfos[tupnum].db_name, sizeof(dbinfos[tupnum].db_name), "%s",
> ! 				 PQgetvalue(res, tupnum, i_datname));
>   		snprintf(dbinfos[tupnum].db_tblspace, sizeof(dbinfos[tupnum].db_tblspace), "%s",
>   				 PQgetvalue(res, tupnum, i_spclocation));
>   	}
> --- 220,226 ----
>   	for (tupnum = 0; tupnum < ntups; tupnum++)
>   	{
>   		dbinfos[tupnum].db_oid = atooid(PQgetvalue(res, tupnum, i_oid));
> ! 		dbinfos[tupnum].db_name = pg_strdup(PQgetvalue(res, tupnum, i_datname));
>   		snprintf(dbinfos[tupnum].db_tblspace, sizeof(dbinfos[tupnum].db_tblspace), "%s",
>   				 PQgetvalue(res, tupnum, i_spclocation));
>   	}
> *************** get_rel_infos(ClusterInfo *cluster, DbIn
> *** 346,355 ****
>   		curr->reloid = atooid(PQgetvalue(res, relnum, i_oid));
>   
>   		nspname = PQgetvalue(res, relnum, i_nspname);
> ! 		strlcpy(curr->nspname, nspname, sizeof(curr->nspname));
>   
>   		relname = PQgetvalue(res, relnum, i_relname);
> ! 		strlcpy(curr->relname, relname, sizeof(curr->relname));
>   
>   		curr->relfilenode = atooid(PQgetvalue(res, relnum, i_relfilenode));
>   
> --- 345,354 ----
>   		curr->reloid = atooid(PQgetvalue(res, relnum, i_oid));
>   
>   		nspname = PQgetvalue(res, relnum, i_nspname);
> ! 		curr->nspname = pg_strdup(nspname);
>   
>   		relname = PQgetvalue(res, relnum, i_relname);
> ! 		curr->relname = pg_strdup(relname);
>   
>   		curr->relfilenode = atooid(PQgetvalue(res, relnum, i_relfilenode));
>   
> *************** free_db_and_rel_infos(DbInfoArr *db_arr)
> *** 377,383 ****
> --- 376,385 ----
>   	int			dbnum;
>   
>   	for (dbnum = 0; dbnum < db_arr->ndbs; dbnum++)
> + 	{
>   		free_rel_infos(&db_arr->dbs[dbnum].rel_arr);
> + 		pg_free(db_arr->dbs[dbnum].db_name);
> + 	}
>   	pg_free(db_arr->dbs);
>   	db_arr->dbs = NULL;
>   	db_arr->ndbs = 0;
> *************** free_db_and_rel_infos(DbInfoArr *db_arr)
> *** 387,392 ****
> --- 389,401 ----
>   static void
>   free_rel_infos(RelInfoArr *rel_arr)
>   {
> + 	int			relnum;
> + 
> + 	for (relnum = 0; relnum < rel_arr->nrels; relnum++)
> + 	{
> + 		pg_free(rel_arr->rels[relnum].nspname);
> + 		pg_free(rel_arr->rels[relnum].relname);
> + 	}
>   	pg_free(rel_arr->rels);
>   	rel_arr->nrels = 0;
>   }
> *************** print_db_infos(DbInfoArr *db_arr)
> *** 407,418 ****
>   
>   
>   static void
> ! print_rel_infos(RelInfoArr *arr)
>   {
>   	int			relnum;
>   
> ! 	for (relnum = 0; relnum < arr->nrels; relnum++)
>   		pg_log(PG_VERBOSE, "relname: %s.%s: reloid: %u reltblspace: %s\n",
> ! 			   arr->rels[relnum].nspname, arr->rels[relnum].relname,
> ! 			   arr->rels[relnum].reloid, arr->rels[relnum].tablespace);
>   }
> --- 416,427 ----
>   
>   
>   static void
> ! print_rel_infos(RelInfoArr *rel_arr)
>   {
>   	int			relnum;
>   
> ! 	for (relnum = 0; relnum < rel_arr->nrels; relnum++)
>   		pg_log(PG_VERBOSE, "relname: %s.%s: reloid: %u reltblspace: %s\n",
> ! 			   rel_arr->rels[relnum].nspname, rel_arr->rels[relnum].relname,
> ! 			   rel_arr->rels[relnum].reloid, rel_arr->rels[relnum].tablespace);
>   }
> diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
> new file mode 100644
> index 972e8e9..cae1e46
> *** a/contrib/pg_upgrade/pg_upgrade.h
> --- b/contrib/pg_upgrade/pg_upgrade.h
> *************** extern char *output_files[];
> *** 114,121 ****
>    */
>   typedef struct
>   {
> ! 	char		nspname[NAMEDATALEN];	/* namespace name */
> ! 	char		relname[NAMEDATALEN];	/* relation name */
>   	Oid			reloid;			/* relation oid */
>   	Oid			relfilenode;	/* relation relfile node */
>   	/* relation tablespace path, or "" for the cluster default */
> --- 114,122 ----
>    */
>   typedef struct
>   {
> ! 	/* Can't use NAMEDATALEN;  not guaranteed to fit on client */
> ! 	char		*nspname;		/* namespace name */
> ! 	char		*relname;		/* relation name */
>   	Oid			reloid;			/* relation oid */
>   	Oid			relfilenode;	/* relation relfile node */
>   	/* relation tablespace path, or "" for the cluster default */
> *************** typedef struct
> *** 143,150 ****
>   	Oid			old_relfilenode;
>   	Oid			new_relfilenode;
>   	/* the rest are used only for logging and error reporting */
> ! 	char		nspname[NAMEDATALEN];	/* namespaces */
> ! 	char		relname[NAMEDATALEN];
>   } FileNameMap;
>   
>   /*
> --- 144,151 ----
>   	Oid			old_relfilenode;
>   	Oid			new_relfilenode;
>   	/* the rest are used only for logging and error reporting */
> ! 	char		*nspname;		/* namespaces */
> ! 	char		*relname;
>   } FileNameMap;
>   
>   /*
> *************** typedef struct
> *** 153,159 ****
>   typedef struct
>   {
>   	Oid			db_oid;			/* oid of the database */
> ! 	char		db_name[NAMEDATALEN];	/* database name */
>   	char		db_tblspace[MAXPGPATH]; /* database default tablespace path */
>   	RelInfoArr	rel_arr;		/* array of all user relinfos */
>   } DbInfo;
> --- 154,160 ----
>   typedef struct
>   {
>   	Oid			db_oid;			/* oid of the database */
> ! 	char		*db_name;		/* database name */
>   	char		db_tblspace[MAXPGPATH]; /* database default tablespace path */
>   	RelInfoArr	rel_arr;		/* array of all user relinfos */
>   } DbInfo;
> diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
> new file mode 100644
> index 40740dc..3e05ac3
> *** a/src/bin/initdb/initdb.c
> --- b/src/bin/initdb/initdb.c
> *************** setup_collation(void)
> *** 1836,1842 ****
>   #if defined(HAVE_LOCALE_T) && !defined(WIN32)
>   	int			i;
>   	FILE	   *locale_a_handle;
> ! 	char		localebuf[NAMEDATALEN];
>   	int			count = 0;
>   
>   	PG_CMD_DECL;
> --- 1836,1842 ----
>   #if defined(HAVE_LOCALE_T) && !defined(WIN32)
>   	int			i;
>   	FILE	   *locale_a_handle;
> ! 	char		localebuf[NAMEDATALEN]; /* we assume ASCII so this is fine */
>   	int			count = 0;
>   
>   	PG_CMD_DECL;

> 
> -- 
> 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  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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