Re: pgScript patch

Hi Mickael

On Sun, Jul 27, 2008 at 3:34 PM, Mickael Deloison <mdeloison(at)gmail(dot)com> wrote:
> Hi pgadmin hackers,
>
> pgScript can now be integrated into pgAdmin3. I have made a patch on
> revision 7394 of pgAdmin. This patch is big therefore I do not post it
> in this email, it is instead available on the following server:
> http://pgscript.projects.postgresql.org/pgadmin

Cool. It is indeed a huge patch, so I'll have to leave it to your
mentor to undertake a more extensive code review, but here are a few
points I noticed whilst testing on Mac:

- Please include "Copyright (C) 2002 - 2008, The pgAdmin Development
Team" in the copyright notices at the top of each source file. I'm
happy for you to include your own copyright there also, but it makes
things much easier from a legal POV if you list us as well.

- The build failed initially with:

./pgscript/statements/pgsStmtList.cpp: In member function 'virtual
void pgsStmtList::eval(pgsVarMap&) const':
./pgscript/statements/pgsStmtList.cpp:56: error: cannot use typeid
with -fno-rtti
./pgscript/statements/pgsStmtList.cpp:56: error: cannot use typeid
with -fno-rtti
./pgscript/statements/pgsStmtList.cpp:57: error: cannot use typeid
with -fno-rtti
./pgscript/statements/pgsStmtList.cpp:57: error: cannot use typeid
with -fno-rtti
./pgscript/statements/pgsStmtList.cpp: In member function 'virtual
void pgsStmtList::eval(pgsVarMap&) const':
./pgscript/statements/pgsStmtList.cpp:56: error: cannot use typeid
with -fno-rtti
./pgscript/statements/pgsStmtList.cpp:56: error: cannot use typeid
with -fno-rtti
./pgscript/statements/pgsStmtList.cpp:57: error: cannot use typeid
with -fno-rtti
./pgscript/statements/pgsStmtList.cpp:57: error: cannot use typeid
with -fno-rtti
lipo: can't figure out the architecture type of:
/var/folders/uk/ukdzizfJHxe07gKAk8a+NE+++TI/-Tmp-//ccIbnqxz.out
make[2]: *** [pgsStmtList.o] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

After removing -fno-rtti from acinclude.m4:

- I see the following warnings:

./pgadmin/include/frm/frmQuery.h: In constructor
'frmQuery::frmQuery(frmMain*, const wxString&, pgConn*, const
wxString&, const wxString&)':
../pgadmin/include/frm/frmQuery.h:75: warning: 'frmQuery::pgscript'
will be initialized after
../pgadmin/include/frm/frmQuery.h:71: warning: 'wxTimer frmQuery::timer'
./frm/frmQuery.cpp:130: warning: when initialized here
../pgadmin/include/frm/frmQuery.h: In constructor
'frmQuery::frmQuery(frmMain*, const wxString&, pgConn*, const
wxString&, const wxString&)':
../pgadmin/include/frm/frmQuery.h:75: warning: 'frmQuery::pgscript'
will be initialized after
../pgadmin/include/frm/frmQuery.h:71: warning: 'wxTimer frmQuery::timer'
./frm/frmQuery.cpp:130: warning: when initialized here

- The following script crashes (yes, I realise it's missing a cast)

declare @i, @t;

set @i = 0;

while @i < 20
begin
set @t = 'aa' + @i;
create table @t (id serial primary key, data text);

set @i = @i + 1;
end

- The corrected script gives no feedback that it's finished, other
than re-enabling buttons. I would expect to see the appropriate
notices from the server about each table that is created, and the
status message on the status bar should change.

- The following script (with missing increment of @i) gave appropriate
errors when run the first time, but ran silently the second:

declare @i, @t;

set @i = 0;

while @i < 20
begin
set @t = 'aa' + cast(@i as string);
create table @t (id serial primary key, data text);
end

- Cancelling that script the first time round is awkward (we should
offer a cancel option on the error dialogue). Using the Stop button
seems to crash.

I'll leave it at that for now, and look forward to the next patch :-)

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com