Re: 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 truncated to "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(at)gunduz(dot)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 Hagander
Me: 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(at)gunduz(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

On Thu, May 13, 2010 at 5:06 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Magnus Hagander wrote:
>> On Thu, May 13, 2010 at 8:22 AM, Devrim G?ND?Z <devrim(at)gunduz(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Thu, May 13, 2010 at 5:06 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)hagander(dot)net> writes:
> > On Thu, May 13, 2010 at 5:06 PM, Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> wrote:
> Takahiro Itagaki wrote:
>>
>> Bruce Momjian <bruce(at)momjian(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Thu, May 13, 2010 at 5:06 PM, Bruce Momjian <bruce(at)momjian(dot)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