Prev: Stefan's bug (was: max_standby_delay considered harmful)
Next: How to know killed by pg_terminate_backend
From: Bruce Momjian on 13 May 2010 20:13
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(a)momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 13 May 2010 21:01
Bruce Momjian <bruce(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Bruce Momjian on 13 May 2010 21:13
Tom Lane wrote:
> Bruce Momjian <bruce(a)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(a)momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Takahiro Itagaki on 13 May 2010 22:24

Bruce Momjian <bruce(a)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



--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Bruce Momjian on 13 May 2010 23:34
Takahiro Itagaki wrote:
>
> Bruce Momjian <bruce(a)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(a)momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: Stefan's bug (was: max_standby_delay considered harmful)
Next: How to know killed by pg_terminate_backend