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(at)momjian(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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