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