Thread: pg_upgrade code questions
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
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
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/
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
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/
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
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
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
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
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
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 ----
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
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
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
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
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
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
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
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/
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