Thread: pg_upgrade code questions

pg_upgrade code questions

From
Takahiro Itagaki
Date:
I read pg_upgrade code glance over, and found 4 issues in it.
Are there any issues to be fixed before 9.0 release?
   1. NAMEDATASIZE   2. extern PGDLLIMPORT   3. pathSeparator   4. EDB_NATIVE_LANG

==== 1. NAMEDATASIZE ====
pg_upgrade has the following definition, but should it be just NAMEDATALEN?
   /* Allocate for null byte */   #define NAMEDATASIZE        (NAMEDATALEN + 1)

Table names should be in NAMEDATELEN - 1 bytes. At least 64th bytes in 
"name" data is always '\0'.
   =# CREATE TABLE "1234567890...(total 70 chars)...1234567890" (i int);   NOTICE:  identifier "123...890" will be
truncatedto "123...0123"
 

==== 2. extern PGDLLIMPORT ====
pg_upgrade has own definitions of   extern PGDLLIMPORT Oid binary_upgrade_next_xxx
in pg_upgrade_sysoids.c. But those variables are not declared as
PGDLLIMPORT in the core. Can we access unexported variables here?

==== 3. pathSeparator ====
Path separator for Windows is not only \ but also /. The current code
ignores /. Also, it might not work if the path string including multi-byte
characters that have \ (0x5c) in the second byte.

==== 4. EDB_NATIVE_LANG ====
Of course it is commented out with #ifdef, but do we have codes
for EDB in core?

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center



Re: pg_upgrade code questions

From
Devrim GÜNDÜZ
Date:
On Thu, 2010-05-13 at 15:13 +0900, Takahiro Itagaki wrote:
> ==== 4. EDB_NATIVE_LANG ====
> Of course it is commented out with #ifdef, but do we have codes
> for EDB in core?

I was about to raise similar thing, for the documentation:

http://developer.postgresql.org/pgdocs/postgres/pgupgrade.html

This includes some references to EDB AS, which should be removed from
PostgreSQL official documentation, IMHO.

Regards,
--
Devrim GÜNDÜZ
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
PostgreSQL RPM Repository: http://yum.pgrpms.org
Community: devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr
http://www.gunduz.org  Twitter: http://twitter.com/devrimgunduz

Re: pg_upgrade code questions

From
Magnus Hagander
Date:
On Thu, May 13, 2010 at 8:22 AM, Devrim GÜNDÜZ <devrim@gunduz.org> wrote:
> On Thu, 2010-05-13 at 15:13 +0900, Takahiro Itagaki wrote:
>> ==== 4. EDB_NATIVE_LANG ====
>> Of course it is commented out with #ifdef, but do we have codes
>> for EDB in core?
>
> I was about to raise similar thing, for the documentation:
>
> http://developer.postgresql.org/pgdocs/postgres/pgupgrade.html
>
> This includes some references to EDB AS, which should be removed from
> PostgreSQL official documentation, IMHO.

+1 on getting rid of those references.


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/


Re: pg_upgrade code questions

From
Bruce Momjian
Date:
Magnus Hagander wrote:
> On Thu, May 13, 2010 at 8:22 AM, Devrim G?ND?Z <devrim@gunduz.org> wrote:
> > On Thu, 2010-05-13 at 15:13 +0900, Takahiro Itagaki wrote:
> >> ==== 4. EDB_NATIVE_LANG ====
> >> Of course it is commented out with #ifdef, but do we have codes
> >> for EDB in core?
> >
> > I was about to raise similar thing, for the documentation:
> >
> > http://developer.postgresql.org/pgdocs/postgres/pgupgrade.html
> >
> > This includes some references to EDB AS, which should be removed from
> > PostgreSQL official documentation, IMHO.
> 
> +1 on getting rid of those references.

Agreed.  When it was on pgFoundry, I had to mention that because it was
unclear who would be using it, but in /contrib we know this is for
community Postgres.  EnterpriseDB did contribute the code so I would
like to keep the code working for EnterpriseDB Advanced Server if that
is easy.

I have added SGML comments to comment out the text that mentions EDB
Advanced Server.  Is that enough?  Should I remove the text from the
SGML?  Should I move it to the bottom of the SGML?  Should I remove the
EnterpriseDB Advanced Server checks from the C code too?  I don't
remember having to deal with anything like this before, so I am unclear
how to proceed.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com


Re: pg_upgrade code questions

From
Magnus Hagander
Date:
On Thu, May 13, 2010 at 5:06 PM, Bruce Momjian <bruce@momjian.us> wrote:
> Magnus Hagander wrote:
>> On Thu, May 13, 2010 at 8:22 AM, Devrim G?ND?Z <devrim@gunduz.org> wrote:
>> > On Thu, 2010-05-13 at 15:13 +0900, Takahiro Itagaki wrote:
>> >> ==== 4. EDB_NATIVE_LANG ====
>> >> Of course it is commented out with #ifdef, but do we have codes
>> >> for EDB in core?
>> >
>> > I was about to raise similar thing, for the documentation:
>> >
>> > http://developer.postgresql.org/pgdocs/postgres/pgupgrade.html
>> >
>> > This includes some references to EDB AS, which should be removed from
>> > PostgreSQL official documentation, IMHO.
>>
>> +1 on getting rid of those references.
>
> Agreed.  When it was on pgFoundry, I had to mention that because it was
> unclear who would be using it, but in /contrib we know this is for
> community Postgres.  EnterpriseDB did contribute the code so I would
> like to keep the code working for EnterpriseDB Advanced Server if that
> is easy.
>
> I have added SGML comments to comment out the text that mentions EDB
> Advanced Server.  Is that enough?  Should I remove the text from the
> SGML?  Should I move it to the bottom of the SGML?  Should I remove the
> EnterpriseDB Advanced Server checks from the C code too?  I don't
> remember having to deal with anything like this before, so I am unclear
> how to proceed.

I say remove it. On all accounts.

There's a fork of postgres for EDB AS, shouldn't there be a fork of
pg_upgrade the same way, if it requires special code? The code in
community postgresql certainly shouldn't have any EDB AS code in it.


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/


Re: pg_upgrade code questions

From
Tom Lane
Date:
Magnus Hagander <magnus@hagander.net> writes:
> On Thu, May 13, 2010 at 5:06 PM, Bruce Momjian <bruce@momjian.us> wrote:
>> I have added SGML comments to comment out the text that mentions EDB
>> Advanced Server. �Is that enough? �Should I remove the text from the
>> SGML? �Should I move it to the bottom of the SGML? �Should I remove the
>> EnterpriseDB Advanced Server checks from the C code too? �I don't
>> remember having to deal with anything like this before, so I am unclear
>> how to proceed.

> I say remove it. On all accounts.

> There's a fork of postgres for EDB AS, shouldn't there be a fork of
> pg_upgrade the same way, if it requires special code? The code in
> community postgresql certainly shouldn't have any EDB AS code in it.

Indeed.  Given the (presumably large) delta between EDB's code and ours,
having to have some delta in pg_upgrade isn't going to make much
difference for them.  I think the community code and docs should
completely omit any mention of that.
        regards, tom lane


Re: pg_upgrade code questions

From
Bruce Momjian
Date:
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
> > On Thu, May 13, 2010 at 5:06 PM, Bruce Momjian <bruce@momjian.us> wrote:
> >> I have added SGML comments to comment out the text that mentions EDB
> >> Advanced Server. �Is that enough? �Should I remove the text from the
> >> SGML? �Should I move it to the bottom of the SGML? �Should I remove the
> >> EnterpriseDB Advanced Server checks from the C code too? �I don't
> >> remember having to deal with anything like this before, so I am unclear
> >> how to proceed.
> 
> > I say remove it. On all accounts.
> 
> > There's a fork of postgres for EDB AS, shouldn't there be a fork of
> > pg_upgrade the same way, if it requires special code? The code in
> > community postgresql certainly shouldn't have any EDB AS code in it.
> 
> Indeed.  Given the (presumably large) delta between EDB's code and ours,
> having to have some delta in pg_upgrade isn't going to make much
> difference for them.  I think the community code and docs should
> completely omit any mention of that.

I am trying to think of this as a non-EnterpriseDB employee.  If suppose
Greenplum had given us a utility and they wanted it to work with their
version of the database, what accommodation would we make for them?  I
agree on the documentation, but would we allow #ifdefs that were only
used by them if there were only a few of them?  Could we treat it as an
operating system that none of us use?  I don't think Greenplum would
require us to keep support for their database, but they would prefer it,
and it might encourage more contributions from them.  Maybe we would
just tell them to keep their own patches, but I figured I would ask
specifically so we have a policy for next time.

I guess another question is whether we would accept a patch that was
useful only for a Greenplum build?  And does removing such code use the
same criteria?

I know pgAdmin supports Greenplum, but that is an external tool so it
makes more sense there.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com


Re: pg_upgrade code questions

From
"Joshua D. Drake"
Date:
On Thu, 2010-05-13 at 17:19 +0200, Magnus Hagander wrote:

> I say remove it. On all accounts.
>
> There's a fork of postgres for EDB AS, shouldn't there be a fork of
> pg_upgrade the same way, if it requires special code? The code in
> community postgresql certainly shouldn't have any EDB AS code in it.

If the code would be useful for other "projects" then keep it. If it is
only for a closed source product, dump it.

Joshua D. Drake


--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering

Re: pg_upgrade code questions

From
Andrew Dunstan
Date:

Bruce Momjian wrote:
>> Indeed.  Given the (presumably large) delta between EDB's code and ours,
>> having to have some delta in pg_upgrade isn't going to make much
>> difference for them.  I think the community code and docs should
>> completely omit any mention of that.
>>     
>
> I am trying to think of this as a non-EnterpriseDB employee.  If suppose
> Greenplum had given us a utility and they wanted it to work with their
> version of the database, what accommodation would we make for them?  I
> agree on the documentation, but would we allow #ifdefs that were only
> used by them if there were only a few of them?  Could we treat it as an
> operating system that none of us use?  I don't think Greenplum would
> require us to keep support for their database, but they would prefer it,
> and it might encourage more contributions from them.  Maybe we would
> just tell them to keep their own patches, but I figured I would ask
> specifically so we have a policy for next time.
>
> I guess another question is whether we would accept a patch that was
> useful only for a Greenplum build?  And does removing such code use the
> same criteria?
>
> I know pgAdmin supports Greenplum, but that is an external tool so it
> makes more sense there.
>
>   

What if several vendors want the same thing? The code will quickly 
become spaghetti.

AFAIK the Linux kernel expects distros to keep their patchsets 
separately, and I rather think we should too.

cheers

andrew


Re: pg_upgrade code questions

From
Josh Berkus
Date:
On 5/13/10 10:14 AM, Bruce Momjian wrote:
> I am trying to think of this as a non-EnterpriseDB employee.  If suppose
> Greenplum had given us a utility and they wanted it to work with their
> version of the database, what accommodation would we make for them?  I
> agree on the documentation, but would we allow #ifdefs that were only
> used by them if there were only a few of them?  Could we treat it as an
> operating system that none of us use?  I don't think Greenplum would
> require us to keep support for their database, but they would prefer it,
> and it might encourage more contributions from them.  Maybe we would
> just tell them to keep their own patches, but I figured I would ask
> specifically so we have a policy for next time.

My $0.021746:

If something is going to be included in /contrib, it should only include
code which relates to standard PostgreSQL.  The independant pg_migrator
project can be a PG/EDBAS tool; the contrib module needs to be
vanilla-postgres only.  If the donor of the code wants to keep the
specific fork support, then it should remain an independant project.

I'm not just referring to EDB here, or even just proprietary forks; even
open source forks (like PostgresXC or pgCluster) shouldn't have specific
code in /contrib.  Within the limits of reasonableness, of course.

My argument isn't based on purity, but is rather based on:
(a) avoiding confusing the users, and
(b) avoiding bulking code with lots of ifdefs if we can avoid it, and
(c) fork release cycles are often different from pgsql-core, and EDB's
certainly is.

--                                  -- Josh Berkus                                    PostgreSQL Experts Inc.
                        http://www.pgexperts.com
 


Re: pg_upgrade code questions

From
Bruce Momjian
Date:
Josh Berkus wrote:
> On 5/13/10 10:14 AM, Bruce Momjian wrote:
> > I am trying to think of this as a non-EnterpriseDB employee.  If suppose
> > Greenplum had given us a utility and they wanted it to work with their
> > version of the database, what accommodation would we make for them?  I
> > agree on the documentation, but would we allow #ifdefs that were only
> > used by them if there were only a few of them?  Could we treat it as an
> > operating system that none of us use?  I don't think Greenplum would
> > require us to keep support for their database, but they would prefer it,
> > and it might encourage more contributions from them.  Maybe we would
> > just tell them to keep their own patches, but I figured I would ask
> > specifically so we have a policy for next time.
>
> My $0.021746:
>
> If something is going to be included in /contrib, it should only include
> code which relates to standard PostgreSQL.  The independant pg_migrator
> project can be a PG/EDBAS tool; the contrib module needs to be
> vanilla-postgres only.  If the donor of the code wants to keep the
> specific fork support, then it should remain an independant project.
>
> I'm not just referring to EDB here, or even just proprietary forks; even
> open source forks (like PostgresXC or pgCluster) shouldn't have specific
> code in /contrib.  Within the limits of reasonableness, of course.
>
> My argument isn't based on purity, but is rather based on:
> (a) avoiding confusing the users, and
> (b) avoiding bulking code with lots of ifdefs if we can avoid it, and
> (c) fork release cycles are often different from pgsql-core, and EDB's
> certainly is.

I was more interested in understanding our policy rather than how to
handle this specific issue.  I have removed all mentions of EnterpriseDB
Advanced Server from pg_upgrade with the attached patch.  I will keep
the patch for submission back to EnterpriseDB when they want it, or they
can just pull it from CVS.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com
Index: contrib/pg_upgrade/check.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/check.c,v
retrieving revision 1.3
diff -c -c -r1.3 check.c
*** contrib/pg_upgrade/check.c    13 May 2010 15:58:15 -0000    1.3
--- contrib/pg_upgrade/check.c    13 May 2010 22:48:06 -0000
***************
*** 149,158 ****
          {
              prep_status(ctx, "Adjusting sequences");
              exec_prog(ctx, true,
!                     SYSTEMQUOTE "\"%s/%s\" --set ON_ERROR_STOP=on --port %d "
                        "-f \"%s\" --dbname template1 >> \"%s\"" SYSTEMQUOTE,
!                       ctx->new.bindir, ctx->new.psql_exe, ctx->new.port,
!                       sequence_script_file_name, ctx->logfile);
              unlink(sequence_script_file_name);
              pg_free(sequence_script_file_name);
              check_ok(ctx);
--- 149,158 ----
          {
              prep_status(ctx, "Adjusting sequences");
              exec_prog(ctx, true,
!                     SYSTEMQUOTE "\"%s/psql\" --set ON_ERROR_STOP=on --port %d "
                        "-f \"%s\" --dbname template1 >> \"%s\"" SYSTEMQUOTE,
!                       ctx->new.bindir, ctx->new.port, sequence_script_file_name,
!                       ctx->logfile);
              unlink(sequence_script_file_name);
              pg_free(sequence_script_file_name);
              check_ok(ctx);
Index: contrib/pg_upgrade/controldata.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/controldata.c,v
retrieving revision 1.1
diff -c -c -r1.1 controldata.c
*** contrib/pg_upgrade/controldata.c    12 May 2010 02:19:10 -0000    1.1
--- contrib/pg_upgrade/controldata.c    13 May 2010 22:48:06 -0000
***************
*** 9,18 ****
  #include <ctype.h>
  #include <stdlib.h>

- #ifdef EDB_NATIVE_LANG
- #include "access/tuptoaster.h"
- #endif
-

  /*
   * get_control_data()
--- 9,14 ----
***************
*** 88,102 ****
          got_float8_pass_by_value = true;
      }

- #ifdef EDB_NATIVE_LANG
-     /* EDB AS 8.3 is an 8.2 code base */
-     if (cluster->is_edb_as && GET_MAJOR_VERSION(cluster->major_version) <= 803)
-     {
-         cluster->controldata.toast = TOAST_MAX_CHUNK_SIZE;
-         got_toast = true;
-     }
- #endif
-
      /* we have the result of cmd in "output". so parse it line by line now */
      while (fgets(bufin, sizeof(bufin), output))
      {
--- 84,89 ----
***************
*** 140,148 ****
              p++;                /* removing ':' char */
              cluster->controldata.cat_ver = (uint32) atol(p);
          }
!         else if ((p = strstr(bufin, "First log file ID after reset:")) != NULL ||
!                  (cluster->is_edb_as && GET_MAJOR_VERSION(cluster->major_version) <= 803 &&
!                   (p = strstr(bufin, "Current log file ID:")) != NULL))
          {
              p = strchr(p, ':');

--- 127,133 ----
              p++;                /* removing ':' char */
              cluster->controldata.cat_ver = (uint32) atol(p);
          }
!         else if ((p = strstr(bufin, "First log file ID after reset:")) != NULL)
          {
              p = strchr(p, ':');

***************
*** 153,161 ****
              cluster->controldata.logid = (uint32) atol(p);
              got_log_id = true;
          }
!         else if ((p = strstr(bufin, "First log file segment after reset:")) != NULL ||
!                  (cluster->is_edb_as && GET_MAJOR_VERSION(cluster->major_version) <= 803 &&
!                   (p = strstr(bufin, "Next log file segment:")) != NULL))
          {
              p = strchr(p, ':');

--- 138,144 ----
              cluster->controldata.logid = (uint32) atol(p);
              got_log_id = true;
          }
!         else if ((p = strstr(bufin, "First log file segment after reset:")) != NULL)
          {
              p = strchr(p, ':');

Index: contrib/pg_upgrade/exec.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/exec.c,v
retrieving revision 1.2
diff -c -c -r1.2 exec.c
*** contrib/pg_upgrade/exec.c    13 May 2010 15:58:15 -0000    1.2
--- contrib/pg_upgrade/exec.c    13 May 2010 22:48:06 -0000
***************
*** 11,18 ****


  static void checkBinDir(migratorContext *ctx, ClusterInfo *cluster);
! static int check_exec(migratorContext *ctx, const char *dir, const char *cmdName,
!            const char *alternative);
  static const char *validate_exec(const char *path);
  static int    check_data_dir(migratorContext *ctx, const char *pg_data);

--- 11,17 ----


  static void checkBinDir(migratorContext *ctx, ClusterInfo *cluster);
! static int check_exec(migratorContext *ctx, const char *dir, const char *cmdName);
  static const char *validate_exec(const char *path);
  static int    check_data_dir(migratorContext *ctx, const char *pg_data);

***************
*** 89,110 ****
  static void
  checkBinDir(migratorContext *ctx, ClusterInfo *cluster)
  {
!     check_exec(ctx, cluster->bindir, "postgres", "edb-postgres");
!     check_exec(ctx, cluster->bindir, "pg_ctl", NULL);
!     check_exec(ctx, cluster->bindir, "pg_dumpall", NULL);
!
! #ifdef EDB_NATIVE_LANG
!     /* check for edb-psql first because we need to detect EDB AS */
!     if (check_exec(ctx, cluster->bindir, "edb-psql", "psql") == 1)
!     {
!         cluster->psql_exe = "edb-psql";
!         cluster->is_edb_as = true;
!     }
!     else
! #else
!     if (check_exec(ctx, cluster->bindir, "psql", NULL) == 1)
! #endif
!         cluster->psql_exe = "psql";
  }


--- 88,97 ----
  static void
  checkBinDir(migratorContext *ctx, ClusterInfo *cluster)
  {
!     check_exec(ctx, cluster->bindir, "postgres");
!     check_exec(ctx, cluster->bindir, "psql");
!     check_exec(ctx, cluster->bindir, "pg_ctl");
!     check_exec(ctx, cluster->bindir, "pg_dumpall");
  }


***************
*** 146,153 ****
   *    a valid executable, this function returns 0 to indicated failure.
   */
  static int
! check_exec(migratorContext *ctx, const char *dir, const char *cmdName,
!            const char *alternative)
  {
      char        path[MAXPGPATH];
      const char *errMsg;
--- 133,139 ----
   *    a valid executable, this function returns 0 to indicated failure.
   */
  static int
! check_exec(migratorContext *ctx, const char *dir, const char *cmdName)
  {
      char        path[MAXPGPATH];
      const char *errMsg;
***************
*** 155,175 ****
      snprintf(path, sizeof(path), "%s%c%s", dir, pathSeparator, cmdName);

      if ((errMsg = validate_exec(path)) == NULL)
-     {
          return 1;                /* 1 -> first alternative OK */
-     }
      else
!     {
!         if (alternative)
!         {
!             report_status(ctx, PG_WARNING, "check for %s warning:  %s",
!                           cmdName, errMsg);
!             if (check_exec(ctx, dir, alternative, NULL) == 1)
!                 return 2;        /* 2 -> second alternative OK */
!         }
!         else
!             pg_log(ctx, PG_FATAL, "check for %s failed - %s\n", cmdName, errMsg);
!     }

      return 0;                    /* 0 -> neither alternative is acceptable */
  }
--- 141,149 ----
      snprintf(path, sizeof(path), "%s%c%s", dir, pathSeparator, cmdName);

      if ((errMsg = validate_exec(path)) == NULL)
          return 1;                /* 1 -> first alternative OK */
      else
!         pg_log(ctx, PG_FATAL, "check for %s failed - %s\n", cmdName, errMsg);

      return 0;                    /* 0 -> neither alternative is acceptable */
  }
Index: contrib/pg_upgrade/file.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/file.c,v
retrieving revision 1.3
diff -c -c -r1.3 file.c
*** contrib/pg_upgrade/file.c    13 May 2010 22:07:42 -0000    1.3
--- contrib/pg_upgrade/file.c    13 May 2010 22:48:06 -0000
***************
*** 9,18 ****
  #include <sys/types.h>
  #include <fcntl.h>

- #ifdef EDB_NATIVE_LANG
- #include <fcntl.h>
- #endif
-
  #ifdef WIN32
  #include <windows.h>
  #endif
--- 9,14 ----
Index: contrib/pg_upgrade/info.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/info.c,v
retrieving revision 1.2
diff -c -c -r1.2 info.c
*** contrib/pg_upgrade/info.c    12 May 2010 11:07:24 -0000    1.2
--- contrib/pg_upgrade/info.c    13 May 2010 22:48:06 -0000
***************
*** 291,298 ****
                RelInfoArr *relarr, Cluster whichCluster)
  {
      PGconn       *conn = connectToServer(ctx, dbinfo->db_name, whichCluster);
-     bool        is_edb_as = (whichCluster == CLUSTER_OLD) ?
-                     ctx->old.is_edb_as : ctx->new.is_edb_as;
      PGresult   *res;
      RelInfo    *relinfos;
      int            ntups;
--- 291,296 ----
***************
*** 341,378 ****
               FirstNormalObjectId,
      /* see the comment at the top of v8_3_create_sequence_script() */
               (GET_MAJOR_VERSION(ctx->old.major_version) <= 803) ?
!              "" : " OR relkind = 'S'",
!
!     /*
!      * EDB AS installs pgagent by default via initdb. We have to ignore it,
!      * and not migrate any old table contents.
!      */
!              (is_edb_as && strcmp(dbinfo->db_name, "edb") == 0) ?
!              "     AND "
!              "    n.nspname != 'pgagent' AND "
!     /* skip pgagent TOAST tables */
!              "    c.oid NOT IN "
!              "    ( "
!              "        SELECT c2.reltoastrelid "
!              "        FROM pg_catalog.pg_class c2 JOIN "
!              "                pg_catalog.pg_namespace n2 "
!              "            ON c2.relnamespace = n2.oid "
!              "        WHERE n2.nspname = 'pgagent' AND "
!              "              c2.reltoastrelid != 0 "
!              "    ) AND "
!     /* skip pgagent TOAST table indexes */
!              "    c.oid NOT IN "
!              "    ( "
!              "        SELECT c3.reltoastidxid "
!              "        FROM pg_catalog.pg_class c2 JOIN "
!              "                pg_catalog.pg_namespace n2 "
!              "            ON c2.relnamespace = n2.oid JOIN "
!              "                pg_catalog.pg_class c3 "
!              "            ON c2.reltoastrelid = c3.oid "
!              "        WHERE n2.nspname = 'pgagent' AND "
!              "              c2.reltoastrelid != 0 AND "
!              "              c3.reltoastidxid != 0 "
!              "    ) " : "");

      res = executeQueryOrDie(ctx, conn, query);

--- 339,345 ----
               FirstNormalObjectId,
      /* see the comment at the top of v8_3_create_sequence_script() */
               (GET_MAJOR_VERSION(ctx->old.major_version) <= 803) ?
!              "" : " OR relkind = 'S'");

      res = executeQueryOrDie(ctx, conn, query);

Index: contrib/pg_upgrade/pg_upgrade.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/pg_upgrade.c,v
retrieving revision 1.1
diff -c -c -r1.1 pg_upgrade.c
*** contrib/pg_upgrade/pg_upgrade.c    12 May 2010 02:19:11 -0000    1.1
--- contrib/pg_upgrade/pg_upgrade.c    13 May 2010 22:48:06 -0000
***************
*** 196,205 ****
       */
      prep_status(ctx, "Creating databases in the new cluster");
      exec_prog(ctx, true,
!               SYSTEMQUOTE "\"%s/%s\" --set ON_ERROR_STOP=on --port %d "
                "-f \"%s/%s\" --dbname template1 >> \"%s\"" SYSTEMQUOTE,
!               ctx->new.bindir, ctx->new.psql_exe, ctx->new.port,
!               ctx->output_dir, GLOBALS_DUMP_FILE, ctx->logfile);
      check_ok(ctx);

      get_db_and_rel_infos(ctx, &ctx->new.dbarr, CLUSTER_NEW);
--- 196,205 ----
       */
      prep_status(ctx, "Creating databases in the new cluster");
      exec_prog(ctx, true,
!               SYSTEMQUOTE "\"%s/psql\" --set ON_ERROR_STOP=on --port %d "
                "-f \"%s/%s\" --dbname template1 >> \"%s\"" SYSTEMQUOTE,
!               ctx->new.bindir, ctx->new.port, ctx->output_dir,
!               GLOBALS_DUMP_FILE, ctx->logfile);
      check_ok(ctx);

      get_db_and_rel_infos(ctx, &ctx->new.dbarr, CLUSTER_NEW);
***************
*** 218,227 ****

      prep_status(ctx, "Restoring database schema to new cluster");
      exec_prog(ctx, true,
!               SYSTEMQUOTE "\"%s/%s\" --set ON_ERROR_STOP=on --port %d "
                "-f \"%s/%s\" --dbname template1 >> \"%s\"" SYSTEMQUOTE,
!               ctx->new.bindir, ctx->new.psql_exe, ctx->new.port,
!               ctx->output_dir, DB_DUMP_FILE, ctx->logfile);
      check_ok(ctx);

      /* regenerate now that we have db schemas */
--- 218,227 ----

      prep_status(ctx, "Restoring database schema to new cluster");
      exec_prog(ctx, true,
!               SYSTEMQUOTE "\"%s/psql\" --set ON_ERROR_STOP=on --port %d "
                "-f \"%s/%s\" --dbname template1 >> \"%s\"" SYSTEMQUOTE,
!               ctx->new.bindir, ctx->new.port, ctx->output_dir,
!               DB_DUMP_FILE, ctx->logfile);
      check_ok(ctx);

      /* regenerate now that we have db schemas */
Index: contrib/pg_upgrade/pg_upgrade.h
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/pg_upgrade.h,v
retrieving revision 1.3
diff -c -c -r1.3 pg_upgrade.h
*** contrib/pg_upgrade/pg_upgrade.h    13 May 2010 15:58:15 -0000    1.3
--- contrib/pg_upgrade/pg_upgrade.h    13 May 2010 22:48:06 -0000
***************
*** 200,214 ****
      DbInfoArr    dbarr;        /* dbinfos array */
      char       *pgdata;        /* pathname for cluster's $PGDATA directory */
      char       *bindir;        /* pathname for cluster's executable directory */
-     const char *psql_exe;    /* name of the psql command to execute
-                              * in the cluster */
      unsigned short port;    /* port number where postmaster is waiting */
      uint32        major_version;        /* PG_VERSION of cluster */
      char       *major_version_str;    /* string PG_VERSION of cluster */
      Oid            pg_database_oid;        /* OID of pg_database relation */
      char       *libpath;    /* pathname for cluster's pkglibdir */
-     /* EDB AS is PG 8.2 with 8.3 enhancements backpatched. */
-     bool        is_edb_as;    /* EnterpriseDB's Postgres Plus Advanced Server? */
      char       *tablespace_suffix;    /* directory specification */
  } ClusterInfo;

--- 200,210 ----
Index: contrib/pg_upgrade/relfilenode.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/relfilenode.c,v
retrieving revision 1.2
diff -c -c -r1.2 relfilenode.c
*** contrib/pg_upgrade/relfilenode.c    12 May 2010 16:50:00 -0000    1.2
--- contrib/pg_upgrade/relfilenode.c    13 May 2010 22:48:07 -0000
***************
*** 6,15 ****

  #include "pg_upgrade.h"

- #ifdef EDB_NATIVE_LANG
- #include <fcntl.h>
- #endif
-
  #include "catalog/pg_class.h"
  #include "access/transam.h"

--- 6,11 ----
Index: doc/src/sgml/pgupgrade.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/pgupgrade.sgml,v
retrieving revision 1.3
diff -c -c -r1.3 pgupgrade.sgml
*** doc/src/sgml/pgupgrade.sgml    13 May 2010 15:03:24 -0000    1.3
--- doc/src/sgml/pgupgrade.sgml    13 May 2010 22:48:07 -0000
***************
*** 23,33 ****
     pg_upgrade supports upgrades from 8.3.X and later to the current
     major release of Postgres, including snapshot and alpha releases.

- <!--
-    pg_upgrade also supports upgrades from EnterpriseDB's Postgres Plus
-    Advanced Server.
- -->
-
    </para>

   </sect2>
--- 23,28 ----
***************
*** 120,144 ****
       start the new cluster.
      </para>

- <!--
-     <para>
-      If migrating EnterpriseDB's Postgres Plus Advanced Server, you must:
-      <itemizedlist>
-       <listitem>
-        <para>
-         <emphasis>not</> install <literal>sample tables and procedures/functions</>
-         in the new server
-        </para>
-       </listitem>
-       <listitem>
-        <para>
-         delete the empty <literal>edb</> schema in the <literal>enterprisedb</> database
-        </para>
-       </listitem>
-      </itemizedlist>
-     </para>
- -->
-
     </listitem>

     <listitem>
--- 115,120 ----

Re: pg_upgrade code questions

From
Bruce Momjian
Date:
Takahiro Itagaki wrote:
> I read pg_upgrade code glance over, and found 4 issues in it.
> Are there any issues to be fixed before 9.0 release?
> 
>     1. NAMEDATASIZE
>     2. extern PGDLLIMPORT
>     3. pathSeparator
>     4. EDB_NATIVE_LANG
> 
> ==== 1. NAMEDATASIZE ====
> pg_upgrade has the following definition, but should it be just NAMEDATALEN?
> 
>     /* Allocate for null byte */
>     #define NAMEDATASIZE        (NAMEDATALEN + 1)
> 
> Table names should be in NAMEDATELEN - 1 bytes. At least 64th bytes in 
> "name" data is always '\0'.
> 
>     =# CREATE TABLE "1234567890...(total 70 chars)...1234567890" (i int);
>     NOTICE:  identifier "123...890" will be truncated to "123...0123"

Agreed.  I have changed the code to use NAMEDATALEN.

> ==== 2. extern PGDLLIMPORT ====
> pg_upgrade has own definitions of
>     extern PGDLLIMPORT Oid binary_upgrade_next_xxx
> in pg_upgrade_sysoids.c. But those variables are not declared as
> PGDLLIMPORT in the core. Can we access unexported variables here?

The issue here is that you use PGDLLIMPORT where you are importing the
variable, not where it is defined.  For example, look at
'seq_page_cost'.  You can see PGDLLIMPORT used where it is imported with
'extern', but not where is it defined.

> ==== 3. pathSeparator ====
> Path separator for Windows is not only \ but also /. The current code
> ignores /. Also, it might not work if the path string including multi-byte
> characters that have \ (0x5c) in the second byte.

Agreed.  I have modified the code to use only "/" and check for "/" and
"\".  It is used only for checking the last byte so I didn't think it
would affect a multi-byte sequence.  I am actually unclear on that issue
though.  Can you review the new code to see if it is OK.

> ==== 4. EDB_NATIVE_LANG ====
> Of course it is commented out with #ifdef, but do we have codes
> for EDB in core?

Yeah, removed.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com


Re: pg_upgrade code questions

From
"Joshua D. Drake"
Date:
On Thu, 2010-05-13 at 17:19 +0200, Magnus Hagander wrote:

> I say remove it. On all accounts.
> 
> There's a fork of postgres for EDB AS, shouldn't there be a fork of
> pg_upgrade the same way, if it requires special code? The code in
> community postgresql certainly shouldn't have any EDB AS code in it.

If the code would be useful for other "projects" then keep it. If it is
only for a closed source product, dump it.

Joshua D. Drake


-- 
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering




Re: pg_upgrade code questions

From
Tom Lane
Date:
Bruce Momjian <bruce@momjian.us> writes:
> Takahiro Itagaki wrote:
>> ==== 2. extern PGDLLIMPORT ====
>> pg_upgrade has own definitions of
>> extern PGDLLIMPORT Oid binary_upgrade_next_xxx
>> in pg_upgrade_sysoids.c. But those variables are not declared as
>> PGDLLIMPORT in the core. Can we access unexported variables here?

> The issue here is that you use PGDLLIMPORT where you are importing the
> variable, not where it is defined.  For example, look at
> 'seq_page_cost'.  You can see PGDLLIMPORT used where it is imported with
> 'extern', but not where is it defined.

Right.  Also we are intentionally not exposing those variables in any
backend .h file, because they are not meant for general use.  So the
"extern PGDLLIMPORT" isn't going to be in the main backend and has to
be in pg_upgrade.  This was discussed awhile ago when we put in those
variables, I believe.
        regards, tom lane


Re: pg_upgrade code questions

From
Bruce Momjian
Date:
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Takahiro Itagaki wrote:
> >> ==== 2. extern PGDLLIMPORT ====
> >> pg_upgrade has own definitions of
> >> extern PGDLLIMPORT Oid binary_upgrade_next_xxx
> >> in pg_upgrade_sysoids.c. But those variables are not declared as
> >> PGDLLIMPORT in the core. Can we access unexported variables here?
> 
> > The issue here is that you use PGDLLIMPORT where you are importing the
> > variable, not where it is defined.  For example, look at
> > 'seq_page_cost'.  You can see PGDLLIMPORT used where it is imported with
> > 'extern', but not where is it defined.
> 
> Right.  Also we are intentionally not exposing those variables in any
> backend .h file, because they are not meant for general use.  So the
> "extern PGDLLIMPORT" isn't going to be in the main backend and has to
> be in pg_upgrade.  This was discussed awhile ago when we put in those
> variables, I believe.

Yes, this was discussed.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com


Re: pg_upgrade code questions

From
Takahiro Itagaki
Date:
Bruce Momjian <bruce@momjian.us> wrote:

> > >> ==== 2. extern PGDLLIMPORT ====
> > >> pg_upgrade has own definitions of
> > >> extern PGDLLIMPORT Oid binary_upgrade_next_xxx
> > 
> > > The issue here is that you use PGDLLIMPORT where you are importing the
> > > variable, not where it is defined.  For example, look at
> > > 'seq_page_cost'.  You can see PGDLLIMPORT used where it is imported with
> > > 'extern', but not where is it defined.
> > 
> > Right.  Also we are intentionally not exposing those variables in any
> > backend .h file, because they are not meant for general use.  So the
> > "extern PGDLLIMPORT" isn't going to be in the main backend and has to
> > be in pg_upgrade.  This was discussed awhile ago when we put in those
> > variables, I believe.
> 
> Yes, this was discussed.

I wonder some compilers or linkers might hide unexported global variables
from postgres.lib as if they are declared with 'static' specifiers.
I'm especially worried about Windows and MSVC. So, if Windows testers
can see it works, there was nothing to worry about.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center




Re: pg_upgrade code questions

From
Bruce Momjian
Date:
Takahiro Itagaki wrote:
> 
> Bruce Momjian <bruce@momjian.us> wrote:
> 
> > > >> ==== 2. extern PGDLLIMPORT ====
> > > >> pg_upgrade has own definitions of
> > > >> extern PGDLLIMPORT Oid binary_upgrade_next_xxx
> > > 
> > > > The issue here is that you use PGDLLIMPORT where you are importing the
> > > > variable, not where it is defined.  For example, look at
> > > > 'seq_page_cost'.  You can see PGDLLIMPORT used where it is imported with
> > > > 'extern', but not where is it defined.
> > > 
> > > Right.  Also we are intentionally not exposing those variables in any
> > > backend .h file, because they are not meant for general use.  So the
> > > "extern PGDLLIMPORT" isn't going to be in the main backend and has to
> > > be in pg_upgrade.  This was discussed awhile ago when we put in those
> > > variables, I believe.
> > 
> > Yes, this was discussed.
> 
> I wonder some compilers or linkers might hide unexported global variables
> from postgres.lib as if they are declared with 'static' specifiers.
> I'm especially worried about Windows and MSVC. So, if Windows testers
> can see it works, there was nothing to worry about.

Yes, none of the variables pg_upgrade is referencing are 'static', and
Magnus tested MSVC and checked MinGW compiles.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com


Re: pg_upgrade code questions

From
Devrim GÜNDÜZ
Date:
On Thu, 2010-05-13 at 17:19 +0200, Magnus Hagander wrote:
> I say remove it. On all accounts.
>
> There's a fork of postgres for EDB AS, shouldn't there be a fork of
> pg_upgrade the same way, if it requires special code? The code in
> community postgresql certainly shouldn't have any EDB AS code in it.

Agreed.
--
Devrim GÜNDÜZ
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
PostgreSQL RPM Repository: http://yum.pgrpms.org
Community: devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr
http://www.gunduz.org  Twitter: http://twitter.com/devrimgunduz

Re: pg_upgrade code questions

From
Magnus Hagander
Date:
On Fri, May 14, 2010 at 5:34 AM, Bruce Momjian <bruce@momjian.us> wrote:
> Takahiro Itagaki wrote:
>>
>> Bruce Momjian <bruce@momjian.us> wrote:
>>
>> > > >> ==== 2. extern PGDLLIMPORT ====
>> > > >> pg_upgrade has own definitions of
>> > > >> extern PGDLLIMPORT Oid binary_upgrade_next_xxx
>> > >
>> > > > The issue here is that you use PGDLLIMPORT where you are importing the
>> > > > variable, not where it is defined.  For example, look at
>> > > > 'seq_page_cost'.  You can see PGDLLIMPORT used where it is imported with
>> > > > 'extern', but not where is it defined.
>> > >
>> > > Right.  Also we are intentionally not exposing those variables in any
>> > > backend .h file, because they are not meant for general use.  So the
>> > > "extern PGDLLIMPORT" isn't going to be in the main backend and has to
>> > > be in pg_upgrade.  This was discussed awhile ago when we put in those
>> > > variables, I believe.
>> >
>> > Yes, this was discussed.
>>
>> I wonder some compilers or linkers might hide unexported global variables
>> from postgres.lib as if they are declared with 'static' specifiers.
>> I'm especially worried about Windows and MSVC. So, if Windows testers
>> can see it works, there was nothing to worry about.
>
> Yes, none of the variables pg_upgrade is referencing are 'static', and
> Magnus tested MSVC and checked MinGW compiles.

Just to be clear, I only verified that it *built*, didn't have time to
check if it actually *worked*.


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/


Re: pg_upgrade code questions

From
Heikki Linnakangas
Date:
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Thu, May 13, 2010 at 5:06 PM, Bruce Momjian <bruce@momjian.us> wrote:
>>> I have added SGML comments to comment out the text that mentions EDB
>>> Advanced Server.  Is that enough?  Should I remove the text from the
>>> SGML?  Should I move it to the bottom of the SGML?  Should I remove the
>>> EnterpriseDB Advanced Server checks from the C code too?  I don't
>>> remember having to deal with anything like this before, so I am unclear
>>> how to proceed.
> 
>> I say remove it. On all accounts.
> 
>> There's a fork of postgres for EDB AS, shouldn't there be a fork of
>> pg_upgrade the same way, if it requires special code? The code in
>> community postgresql certainly shouldn't have any EDB AS code in it.
> 
> Indeed.  Given the (presumably large) delta between EDB's code and ours,
> having to have some delta in pg_upgrade isn't going to make much
> difference for them.  I think the community code and docs should
> completely omit any mention of that.

Speaking as the person who has been doing the EDB AS merges recently, I
agree. It was helpful to have that stuff there when it was in pgfoundry,
but now that it's part of the main repository, it just gets in the way.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com