Thread: Copy/Paste table(s) functions - git context patch

Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

Again, patch for Copy/Paste table(s) functions.

Done the following:
1. Copy table/schema in the same schema
2. Thread implementation for copy table(s) operations

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sat, 2011-07-16 at 15:45 +0200, Vladimir Kokovic wrote:
> Hi,
>
> Again, patch for Copy/Paste table(s) functions.
>
> Done the following:
> 1. Copy table/schema in the same schema
> 2. Thread implementation for copy table(s) operations
>

There's a really big bug (as in "segfault bug"). Try to copy a big table
(I did it with a 5,000,000 rows, 248MB table), and while doing the copy,
try to create a schema in the same database (it could have the same
effect with other objects, but I didn't try that). It just crashes. The
schema is available, but the new table isn't.

I wonder why you need a frmPasteObject at all. For the new thread? well,
anyway, if you have a frm*, it should display something. And I think it
would be great to actually show a list of objects that will be copied
and show the progress (as in "this table done, this table done, etc.").

Now that we can copy on the same schema, it shouldn't ask for an
extension, but for the complete name.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
I do not know what happened, but I am quite sure that it is not a
frmPasteObject.cpp!

1. Copy operations are in a new thread (the GUI is not blocked)
2. Copy operations do not use the GUI objects
3, Source objects are on a new connection (does not affect the status
of the browser)
4. Target objects are on a new connection (does not affect the status
of the browser)

On 7/17/11, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sat, 2011-07-16 at 15:45 +0200, Vladimir Kokovic wrote:
>> Hi,
>>
>> Again, patch for Copy/Paste table(s) functions.
>>
>> Done the following:
>> 1. Copy table/schema in the same schema
>> 2. Thread implementation for copy table(s) operations
>>
>
> There's a really big bug (as in "segfault bug"). Try to copy a big table
> (I did it with a 5,000,000 rows, 248MB table), and while doing the copy,
> try to create a schema in the same database (it could have the same
> effect with other objects, but I didn't try that). It just crashes. The
> schema is available, but the new table isn't.
>
> I wonder why you need a frmPasteObject at all. For the new thread? well,
> anyway, if you have a frm*, it should display something. And I think it
> would be great to actually show a list of objects that will be copied
> and show the progress (as in "this table done, this table done, etc.").
>
> Now that we can copy on the same schema, it shouldn't ask for an
> extension, but for the complete name.
>
>
> --
> Guillaume
>   http://blog.guillaume.lelarge.info
>   http://www.dalibo.com
>
>

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sun, 2011-07-17 at 20:41 +0200, Vladimir Kokovic wrote:
> I do not know what happened, but I am quite sure that it is not a
> frmPasteObject.cpp!
>

OK, I did more testing, and the schema stuff is actually a bug I
introduced earlier. And while doing this, I found another one. Grest
evening :-/

> 1. Copy operations are in a new thread (the GUI is not blocked)

Right.

> 2. Copy operations do not use the GUI objects

Right.

> 3, Source objects are on a new connection (does not affect the status
> of the browser)
> 4. Target objects are on a new connection (does not affect the status
> of the browser)
>

Yes, that's easy to figure out. You just need to have the server status
window, and you find two new connections, even when it doesn't need them
(when the object is copied on the same database).


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sun, 2011-07-17 at 21:44 +0200, Guillaume Lelarge wrote:
> On Sun, 2011-07-17 at 20:41 +0200, Vladimir Kokovic wrote:
> > I do not know what happened, but I am quite sure that it is not a
> > frmPasteObject.cpp!
> >
>
> OK, I did more testing, and the schema stuff is actually a bug I
> introduced earlier. And while doing this, I found another one. Grest
> evening :-/
>

s/Grest/Great/

Both fixed.

My other comments still apply.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
OK

> Now that we can copy on the same schema, it shouldn't ask for an
> extension, but for the complete name.

I do not know how to do when the table has objects that each has a name!

CREATE TABLE gk_vrsta_naloga_vk --NAME1
(
  id bigint NOT NULL DEFAULT
nextval('"''id_gk_vrsta_naloga''"'::regclass), --NAME2
  sifra character varying NOT NULL DEFAULT ''::character varying,
  CONSTRAINT gk_vrsta_naloga_vk_pkey PRIMARY KEY (id), --NAME3
  CONSTRAINT unique_gk_vrsta_naloga_vk1 UNIQUE (sifra, id), --NAME4
  CONSTRAINT unique_gk_vrsta_naloga_vk2 UNIQUE (id, sifra), --NAME5
  ... --NAMEn
)
WITH (
  OIDS=FALSE


Suffix is much better solution ...


On 7/17/11, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sun, 2011-07-17 at 21:44 +0200, Guillaume Lelarge wrote:
>> On Sun, 2011-07-17 at 20:41 +0200, Vladimir Kokovic wrote:
>> > I do not know what happened, but I am quite sure that it is not a
>> > frmPasteObject.cpp!
>> >
>>
>> OK, I did more testing, and the schema stuff is actually a bug I
>> introduced earlier. And while doing this, I found another one. Grest
>> evening :-/
>>
>
> s/Grest/Great/
>
> Both fixed.
>
> My other comments still apply.
>
>
> --
> Guillaume
>   http://blog.guillaume.lelarge.info
>   http://www.dalibo.com
>
>

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sun, 2011-07-17 at 23:17 +0200, Vladimir Kokovic wrote:
> OK
>
> > Now that we can copy on the same schema, it shouldn't ask for an
> > extension, but for the complete name.
>
> I do not know how to do when the table has objects that each has a name!
>
> CREATE TABLE gk_vrsta_naloga_vk --NAME1
> (
>   id bigint NOT NULL DEFAULT
> nextval('"''id_gk_vrsta_naloga''"'::regclass), --NAME2
>   sifra character varying NOT NULL DEFAULT ''::character varying,
>   CONSTRAINT gk_vrsta_naloga_vk_pkey PRIMARY KEY (id), --NAME3
>   CONSTRAINT unique_gk_vrsta_naloga_vk1 UNIQUE (sifra, id), --NAME4
>   CONSTRAINT unique_gk_vrsta_naloga_vk2 UNIQUE (id, sifra), --NAME5
>   ... --NAMEn
> )
> WITH (
>   OIDS=FALSE
>
>
> Suffix is much better solution ...
>

Good point.

I tried a few other things and it seems pretty solid. One of my tests
got a segfault: I copy all tables (4) in a schema, create a schema,
paste the tables in the schema. Segfault directly.

So I tried with a really small schema: only one object, a table,
declared this way: "CREATE TABLE t1 (c1 bigint);". With the previous
steps, I got a crash.

The threading is a great idea but it makes things a lot harder.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

I can not reproduce!
For me it works successfully.

1. create schema vk1
2. CREATE TABLE vk1.t1 (c1 bigint);
3. create schema vk2
4. copy schema tables vk1
5. paste into vk2 -- OK
6. paste into vk1 -- OK

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia


On 7/19/11, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sun, 2011-07-17 at 23:17 +0200, Vladimir Kokovic wrote:
>> OK
>>
>> > Now that we can copy on the same schema, it shouldn't ask for an
>> > extension, but for the complete name.
>>
>> I do not know how to do when the table has objects that each has a name!
>>
>> CREATE TABLE gk_vrsta_naloga_vk --NAME1
>> (
>>   id bigint NOT NULL DEFAULT
>> nextval('"''id_gk_vrsta_naloga''"'::regclass), --NAME2
>>   sifra character varying NOT NULL DEFAULT ''::character varying,
>>   CONSTRAINT gk_vrsta_naloga_vk_pkey PRIMARY KEY (id), --NAME3
>>   CONSTRAINT unique_gk_vrsta_naloga_vk1 UNIQUE (sifra, id), --NAME4
>>   CONSTRAINT unique_gk_vrsta_naloga_vk2 UNIQUE (id, sifra), --NAME5
>>   ... --NAMEn
>> )
>> WITH (
>>   OIDS=FALSE
>>
>>
>> Suffix is much better solution ...
>>
>
> Good point.
>
> I tried a few other things and it seems pretty solid. One of my tests
> got a segfault: I copy all tables (4) in a schema, create a schema,
> paste the tables in the schema. Segfault directly.
>
> So I tried with a really small schema: only one object, a table,
> declared this way: "CREATE TABLE t1 (c1 bigint);". With the previous
> steps, I got a crash.
>
> The threading is a great idea but it makes things a lot harder.
>
>
> --
> Guillaume
>   http://blog.guillaume.lelarge.info
>   http://www.dalibo.com
>
>

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Wed, 2011-07-20 at 06:11 +0200, Vladimir Kokovic wrote:
> Hi,
>
> I can not reproduce!
> For me it works successfully.
>
> 1. create schema vk1
> 2. CREATE TABLE vk1.t1 (c1 bigint);
> 3. create schema vk2
> 4. copy schema tables vk1
> 5. paste into vk2 -- OK
> 6. paste into vk1 -- OK
>

Because you didn't follow the right order:
1. create schema vk1
2. create table vk1.t1 (c1 bigint)
3. copy schema vk1
4. create schema vk2
5. paste into vk2 .. crashes

It also crashes if I copy only one table instead of a complete schema.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

Another try to make final version.

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

Please, ignore the last patch - Thu, Jul 21, 2011 at 5:19 AM !

I will send new one today.

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

On 7/21/11, Vladimir Kokovic <vladimir.kokovic@gmail.com> wrote:
> Hi,
>
> Another try to make final version.
>
> Best regards,
> Vladimir Kokovic, DP senior, Belgrade, Serbia
>

Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

This time, the patch is probably Release Candidate 1 !

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Fri, 2011-07-22 at 06:19 +0200, Vladimir Kokovic wrote:
> Hi,
>
> This time, the patch is probably Release Candidate 1 !
>

I'm afraid that won't be the last. It still crashes a lot. I still have
the previous issue when copying more than one table (with one table, it
works).

It doesn't like tables with no columns. Actually, it seems to be more a
pgadmin's bug than your patch's bug.

And I still have random crashes when I click around while a copy is in
progress.

Sorry that I haven't better news.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Fri, 2011-07-22 at 21:53 +0200, Guillaume Lelarge wrote:
> On Fri, 2011-07-22 at 06:19 +0200, Vladimir Kokovic wrote:
> > Hi,
> >
> > This time, the patch is probably Release Candidate 1 !
> >
>
> I'm afraid that won't be the last. It still crashes a lot. I still have
> the previous issue when copying more than one table (with one table, it
> works).
>
> It doesn't like tables with no columns. Actually, it seems to be more a
> pgadmin's bug than your patch's bug.
>

It's a pgadmin's bug. I'll apply a fix soon.

> And I still have random crashes when I click around while a copy is in
> progress.
>
> Sorry that I haven't better news.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Fri, 2011-07-22 at 21:54 +0200, Guillaume Lelarge wrote:
> On Fri, 2011-07-22 at 21:53 +0200, Guillaume Lelarge wrote:
> > On Fri, 2011-07-22 at 06:19 +0200, Vladimir Kokovic wrote:
> > > Hi,
> > >
> > > This time, the patch is probably Release Candidate 1 !
> > >
> >
> > I'm afraid that won't be the last. It still crashes a lot. I still have
> > the previous issue when copying more than one table (with one table, it
> > works).
> >
> > It doesn't like tables with no columns. Actually, it seems to be more a
> > pgadmin's bug than your patch's bug.
> >
>
> It's a pgadmin's bug. I'll apply a fix soon.
>

Fixed.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

This patch is probably Release Candidate 2!

Fixed copy / paste of tables that do not have columns.

Everything functions as provided except for one detail that I have no
explanation or solution.

Functionality:
1. copy / paste a table - OK
2. copy / paste a table schema without activity in the browser during
the operation - OK
3. copy / paste a table schema with some actions (add, drop) in the
browser during the operation, after the refresh target scheme(after
the end of pasteObjectFactory::StartDialog), something strange happens
with the GUI, as seen in gdb-bt.log). Please note that the thread that
is used to copy a scheme tables does not call any GUI function as seen
from the thread-calls.txt. valgrind.log shows no anomalies in the work
of pgadmin. I need help in this case because I have no idea what to
do!

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sun, 2011-07-24 at 16:00 +0200, Vladimir Kokovic wrote:
> Hi,
>
> This patch is probably Release Candidate 2!
>
> Fixed copy / paste of tables that do not have columns.
>
> Everything functions as provided except for one detail that I have no
> explanation or solution.
>
> Functionality:
> 1. copy / paste a table - OK
> 2. copy / paste a table schema without activity in the browser during
> the operation - OK
> 3. copy / paste a table schema with some actions (add, drop) in the
> browser during the operation, after the refresh target scheme(after
> the end of pasteObjectFactory::StartDialog), something strange happens
> with the GUI, as seen in gdb-bt.log). Please note that the thread that
> is used to copy a scheme tables does not call any GUI function as seen
> from the thread-calls.txt. valgrind.log shows no anomalies in the work
> of pgadmin. I need help in this case because I have no idea what to
> do!
>

I won't have the time to work on this before my holidays. I'll work on
this when I return, unless someone beats me to it.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

This patch is finally correct!

Instead wxTheApp->Yield wxPostEvent was applied.


 Best regards,
 Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

Everything is the same as the patch from Sun, Jul 28, 2011 at 7:15 PM
in addition to being added a simple toolbar tool which tooltip shows
the progress

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

Added a thread termination control in case of main frame OnClose.

Ignore previous two patches !

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Mon, 2011-08-01 at 06:01 +0200, Vladimir Kokovic wrote:
> Hi,
>
> Added a thread termination control in case of main frame OnClose.
>
> Ignore previous two patches !
>

Tried it. Don't like the big button, I would get rid of it.

It behaves much better. I still don't get why you have a frmXXX if you
don't show anything. Maybe a little UI would be good. Little but better
than displaying messages in the status bar of frmMain.

Still got one crash though. Let's say I copy a huge table or a schema
with many tables or some big tables. I paste them on a schema. While the
operation is ongoing (which is hard to know unless you keep a constant
eye on the statusbar), you drop the connection to the database. It
simply crash a few moment later. This shouldn't happen. If the thread
can continue, it shouldn't crash pgAdmin3. If the thread crashes too,
the whole operation should be rolled back (which isn't what happens
right now, you still have the first few tables before disconnection).


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sat, 2011-08-06 at 15:33 +0200, Guillaume Lelarge wrote:
> On Mon, 2011-08-01 at 06:01 +0200, Vladimir Kokovic wrote:
> > Hi,
> >
> > Added a thread termination control in case of main frame OnClose.
> >
> > Ignore previous two patches !
> >
>
> Tried it. Don't like the big button, I would get rid of it.
>
> It behaves much better. I still don't get why you have a frmXXX if you
> don't show anything. Maybe a little UI would be good. Little but better
> than displaying messages in the status bar of frmMain.
>
> Still got one crash though. Let's say I copy a huge table or a schema
> with many tables or some big tables. I paste them on a schema. While the
> operation is ongoing (which is hard to know unless you keep a constant
> eye on the statusbar), you drop the connection to the database. It
> simply crash a few moment later. This shouldn't happen. If the thread
> can continue, it shouldn't crash pgAdmin3. If the thread crashes too,
> the whole operation should be rolled back (which isn't what happens
> right now, you still have the first few tables before disconnection).
>

Another one. Copy a list of tables, paste them. While doind so, stop
postgres. Result is an error message showing that it cannot copy the
table. Click OK. Restart postgres. Stop pgadmin. Shows a big message:
"Paste table(s) is active! Do you really want to quit?". AFAICT, it
shouldn't be active anymore since there was a disconnection. Unless it
tries for each table and the fact that I restarted postgres made him
happy and resume its work? which I cannot tell because of no UI showing
me current status.

Bottom line is: we really need to have a simple UI showing the list of
tables to paste, which status they are in, what is the connection
status, etc. It should not stop the user to work with the browser.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

I will fix disconnection problem.

Tooltip on the toolbar paste icon shows progress.

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia


On 8/6/11, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sat, 2011-08-06 at 15:33 +0200, Guillaume Lelarge wrote:
>> On Mon, 2011-08-01 at 06:01 +0200, Vladimir Kokovic wrote:
>> > Hi,
>> >
>> > Added a thread termination control in case of main frame OnClose.
>> >
>> > Ignore previous two patches !
>> >
>>
>> Tried it. Don't like the big button, I would get rid of it.
>>
>> It behaves much better. I still don't get why you have a frmXXX if you
>> don't show anything. Maybe a little UI would be good. Little but better
>> than displaying messages in the status bar of frmMain.
>>
>> Still got one crash though. Let's say I copy a huge table or a schema
>> with many tables or some big tables. I paste them on a schema. While the
>> operation is ongoing (which is hard to know unless you keep a constant
>> eye on the statusbar), you drop the connection to the database. It
>> simply crash a few moment later. This shouldn't happen. If the thread
>> can continue, it shouldn't crash pgAdmin3. If the thread crashes too,
>> the whole operation should be rolled back (which isn't what happens
>> right now, you still have the first few tables before disconnection).
>>
>
> Another one. Copy a list of tables, paste them. While doind so, stop
> postgres. Result is an error message showing that it cannot copy the
> table. Click OK. Restart postgres. Stop pgadmin. Shows a big message:
> "Paste table(s) is active! Do you really want to quit?". AFAICT, it
> shouldn't be active anymore since there was a disconnection. Unless it
> tries for each table and the fact that I restarted postgres made him
> happy and resume its work? which I cannot tell because of no UI showing
> me current status.
>
> Bottom line is: we really need to have a simple UI showing the list of
> tables to paste, which status they are in, what is the connection
> status, etc. It should not stop the user to work with the browser.
>
>
> --
> Guillaume
>   http://blog.guillaume.lelarge.info
>   http://www.dalibo.com
>
>

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sat, 2011-08-06 at 16:30 +0200, Vladimir Kokovic wrote:
> Hi,
>
> I will fix disconnection problem.
>

Thanks.

> Tooltip on the toolbar paste icon shows progress.
>

Tooltip isn't a better UI than the status bar. And even that one is not
a good one for this matter.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Dave Page
Date:
If there's a frmXXX that doesn't implement a separate tool (like the query tool or server status monitor), then that must be moved & renamed before commit.

On Saturday, August 6, 2011, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Mon, 2011-08-01 at 06:01 +0200, Vladimir Kokovic wrote:
>> Hi,
>>
>> Added a thread termination control in case of main frame OnClose.
>>
>> Ignore previous two patches !
>>
>
> Tried it. Don't like the big button, I would get rid of it.
>
> It behaves much better. I still don't get why you have a frmXXX if you
> don't show anything. Maybe a little UI would be good. Little but better
> than displaying messages in the status bar of frmMain.
>
> Still got one crash though. Let's say I copy a huge table or a schema
> with many tables or some big tables. I paste them on a schema. While the
> operation is ongoing (which is hard to know unless you keep a constant
> eye on the statusbar), you drop the connection to the database. It
> simply crash a few moment later. This shouldn't happen. If the thread
> can continue, it shouldn't crash pgAdmin3. If the thread crashes too,
> the whole operation should be rolled back (which isn't what happens
> right now, you still have the first few tables before disconnection).
>
>
> --
> Guillaume
>  http://blog.guillaume.lelarge.info
>  http://www.dalibo.com
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sat, 2011-08-06 at 16:28 +0100, Dave Page wrote:
> If there's a frmXXX that doesn't implement a separate tool (like the query
> tool or server status monitor), then that must be moved & renamed before
> commit.
>

I would prefer the tool to show something, but yeah, you're right, if it
doesn't show something, it must be moved and renamed.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Dave Page
Date:
Just showing something would almost certainly make it a dialog. I can't see frmXXX being correct here however it's spun.

On Saturday, August 6, 2011, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sat, 2011-08-06 at 16:28 +0100, Dave Page wrote:
>> If there's a frmXXX that doesn't implement a separate tool (like the query
>> tool or server status monitor), then that must be moved & renamed before
>> commit.
>>
>
> I would prefer the tool to show something, but yeah, you're right, if it
> doesn't show something, it must be moved and renamed.
>
>
> --
> Guillaume
>  http://blog.guillaume.lelarge.info
>  http://www.dalibo.com
>
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

I changed the names of files, so files do not start now with the 'frm' !

In this version, disconnect from the DB server has been fixed.

Additionally, I started a new form, (frmCopyTables.xrc) for the
selection from the list of objects, but I have a problem with the XRC
file, so I need help with the XRC file.

Please, Guillaume see what's wrong with the XRC file.

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Mon, 2011-08-08 at 06:24 +0200, Vladimir Kokovic wrote:
> Hi,
>
> I changed the names of files, so files do not start now with the 'frm' !
>
> In this version, disconnect from the DB server has been fixed.
>

Cannot check that because I have a segfault each time I paste one or
more tables in a schema.

> Additionally, I started a new form, (frmCopyTables.xrc) for the
> selection from the list of objects, but I have a problem with the XRC
> file, so I need help with the XRC file.
>

First, please, let's get the first patch done. Let's iron it so that I
can commit it before adding new problems/bugs with new features.

> Please, Guillaume see what's wrong with the XRC file.
>

Would be simpler if you could tell me what your issue is.

I suppose it has to do with the fact that the ctlCheckTreeView doesn't
grow to fill the space in the dialog. First, growablecols, and
growablerows should be 0, and not 1. At 1, it make the cell in the 2nd
row, 2nd column to grow as much as possible. Too bad there isn't a cell
there. At 0, it make the cell in the 1st row, 1st column to grow AMAP.
Second, you should get rid of the wxPanel. It doesn't grow. With all
these fixes, you should obtain the file attached. And this time, the
ctlCheckTreeView grows with the window.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

After a while, here's a new patch for Copy/Paste table(s) functions.

The functions that were implemented:
1. Copying a table
2. Copying all tables of a schema
3. Copy one or more tables based on selection list
4. Paste in the same or a different schema with the possibility to
change the name of any
    duplicate table name

Intensively tested on several versions of PostgreSQL servers.

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

Here is the 'drag and drop' !

Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Tue, 2011-08-23 at 22:32 +0200, Vladimir Kokovic wrote:
> Hi,
>
> Here is the 'drag and drop' !
>

Don't get me wrong on this, this is great to add drag and drop support.
But if you keep fixing bugs *and* adding features on this patch, we will
never be able to commit it.

What we need now is a "not moving" patch. A patch you only fix bugs, and
don't add interesting, but probably buggy features. And I guess that
adding drag and drop just added more bugs to it.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Tue, 2011-08-23 at 22:55 +0200, Guillaume Lelarge wrote:
> On Tue, 2011-08-23 at 22:32 +0200, Vladimir Kokovic wrote:
> > Hi,
> >
> > Here is the 'drag and drop' !
> >
>
> Don't get me wrong on this, this is great to add drag and drop support.
> But if you keep fixing bugs *and* adding features on this patch, we will
> never be able to commit it.
>
> What we need now is a "not moving" patch. A patch you only fix bugs, and
> don't add interesting, but probably buggy features. And I guess that
> adding drag and drop just added more bugs to it.
>

And I won't know tonight because it doesn't apply cleanly on master.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

 After a while, here's a new patch for Copy/Paste table(s) functions.

 Best regards,
 Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sun, 2011-09-18 at 15:48 +0200, Vladimir Kokovic wrote:
> Hi,
>
>  After a while, here's a new patch for Copy/Paste table(s) functions.
>

Sorry for not replying sooner. This quick mail to tell you that I'll
work on it ASAP. Perhaps tomorrow. Anyway, I didn't forget your patch,
I'm just fighting to find some time to review it (and actually, if
someone else is interested in reviewing it, it would be fine with me).


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Sat, 2011-09-24 at 17:43 +0200, Guillaume Lelarge wrote:
> On Sun, 2011-09-18 at 15:48 +0200, Vladimir Kokovic wrote:
> > Hi,
> >
> >  After a while, here's a new patch for Copy/Paste table(s) functions.
> >
>
> Sorry for not replying sooner. This quick mail to tell you that I'll
> work on it ASAP. Perhaps tomorrow. Anyway, I didn't forget your patch,
> I'm just fighting to find some time to review it (and actually, if
> someone else is interested in reviewing it, it would be fine with me).
>

OK, took some time tonight to review it.

* When I copy a table, and then paste it, I can't paste it once more.
  Why is that?
* If I paste a table and there is an error (for example, the table
  already exists), I can't paste it once more.
* If I paste a table, and click on the dialog asking me if I'm sure I
  want to copy the table here, I can't paste it once more.
* If I paste a table to the same schema I copied it from, and if I click
  cancel on the dialog asking me for a suffix, it keeps asking me for a
  suffix (wether I click OK or cancel, same behaviour)
* If I paste a table to the another schema I copied it from, and that
  the same table name exists in that schema, it asks me for a suffix. If
  I click cancel, it still tries to paste the table (it shouldn't, I
  clicked cancel).
* If I paste a table to the another schema I copied it from, and that
  the same table name exists in that schema, it asks me for a suffix. If
  I delete the suffix by default and click OK, it still tries to paste
  the table (it shouldn't, it's obvious it would fail).
* If I paste a table and the table already exists, it asks me for a
  suffix. If I put ".whatever", it fails because the table name isn't
  quoted (I have a CREATE TABLE foo.bar.whatever, instead of CREATE
  TABLE foo."bar.whatever")
* By the way, when it fails to create the table, it shouldn't try to
  insert into it right after.
* With drag and drop, the cursor changes too late... and you should
  change the cursor, it looks non standard).

* I don't like at all the "Copy tables from the list..." dialog. It
  shouldn't display tables from every servers.
  There are unexplained colors (green, blue, and red).
  Checkboxes are not usable on servers and databases.
  Unless you explain why, I think you should get rid of it, not fix it.

On the code itself:

* ctlCheckTreeView.cpp is a widget, that shouldn't have any behaviour
  specific to one window. At least, not in the way this is done now.
* To check if a table exists, you look into pg_tables. You should better
  browse the treeview.
* BTW, you already have the function to do it (findTable)
* Messages shouldn't be in uppercase (COPIED FROM...).

I also don't understand this code in pgColumn.cpp:
@@ -327,7 +328,8 @@ wxString pgColumn::GetDefinition()

     if ((sql == wxT("integer") || sql == wxT("bigint") ||
             sql == wxT("pg_catalog.integer") || sql ==
wxT("pg_catalog.bigint"))
-            && (GetDefault() == seqDefault1 || GetDefault() ==
seqDefault2))
+            && ((sequence9 && !GetSerialSequence().IsEmpty()) ||
+            (!sequence9 && (GetDefault() == seqDefault1 || GetDefault() ==
seqDefault2))))
     {
         if (sql.Right(6) == wxT("bigint"))
             sql = wxT("bigserial");

Another issue: I launch a copy and disconnect during copy. After I
reconnect, it crashes. But the table is there with its 6 million rows.
And sometimes I don't need to reconnect: it crashes during the copy.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

I finally found the time to make a new version !

> * When I copy a table, and then paste it, I can not paste it once more.
> Why is that?
Fixed

> * If I paste a table and there is an error (for example, the table
> Already exists), I can not paste it once more.
Fixed

> * If I paste a table, and click on the dialog asking me if I'm sure I
> Want to copy the table here, I can not paste it once more.
Fixed

> * If I paste a table to the same schema I copied it from, and if I click
> Cancel on the dialog asking me for a suffix, it keeps asking me for a
> Suffix (wether I click OK or cancel, the behavior)
Fixed

> * If I paste a table to another schema and the copied it from, and that
> The same table name exists in that schema, it asks me for a suffix. If
> I click cancel, it still tries to paste the table (it should not, I
> Clicked cancel).
Fixed

> * If I paste a table to another schema and the copied it from, and that
> The same table name exists in that schema, it asks me for a suffix. If
> I delete the suffix by default and click OK, it still tries to paste
> The table (it should, it's obvious it would fail).
> * If I paste a table and the table already exists, it asks me for a
> Suffix. If I put ". Whatever", it fails because the table name is not
> Quoted (I have a foo.bar.whatever CREATE TABLE, CREATE instead of
> TABLE foo. "Bar.whatever")
Fixed

> * By the way, when it fails to create the table, it should not try to
> Insert into it right after.
Yes

> * With drag and drop, the cursor changes too late ... and you should
> Change the cursor, it looks non-standard).
I do not know how to change the treectrl DND UI !

> * I do not like at all the "Copy tables from the list ..." dialog. It
> Should not display tables from every servers.
This was the main reason why I started CopyPasteTables !
I am DBA for multiple servers in different locations of a company.

> There are unexplained colors (green, blue, and red).
> Checkboxes are not usable on servers and databases.
> Unless you explain why, I think you should get rid of it, not fix it.
Now, only schema and tables are colored.

> On the code itself:
> * CtlCheckTreeView.cpp is a widget, that should not have any behavior
> Specific to one window. At least, not in the way this is done now.
I think the changes that were made do not affect the initial design
and functionality.

> * To check if a table exists, you look into pg_tables. You should better
> Browse the treeview.
> * BTW, you already have the function to do it (findTable)
Yes, but I need the function for a server which is not connected
(without using a browser refresh).

> * Messages should be in uppercase (Copied FROM ...).
Fixed

> I also do not understand this code in pgColumn.cpp:
> @ @ @ @ -327.7 +328.8 PgColumn wxString:: GetDefinition ()

> If ((sql == wxT ("integer") | | sql == wxT ("BIGINT") | |
> Sql == wxT ("pg_catalog.integer") | | sql ==
> WxT ("pg_catalog.bigint"))
> - & & (GetDefault () == seqDefault1 | | GetDefault () ==
> SeqDefault2))
> + & & ((& & Sequence9! GetSerialSequence (). IsEmpty ()) | |
> + (! Sequence9 & & (GetDefault () == seqDefault1 | | GetDefault () ==
> SeqDefault2))))
> {
> If (sql.Right (6) == wxT ("BIGINT"))
> Sql = wxT ("bigserial");
Without this, serial fields are not displayed as serial type for versions 9.x

> Another issue: I launch a copy and disconnect during copy. After I
> Reconnect, it crashes. But the table is there with its 6 million rows.
> And sometimes I do not need to reconnect: it crashes during the copy.
Fixed


Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment

Re: Copy/Paste table(s) functions - git context patch

From
Guillaume Lelarge
Date:
On Thu, 2011-10-13 at 06:22 +0200, Vladimir Kokovic wrote:
> Hi,
>
> I finally found the time to make a new version !
>
> > * When I copy a table, and then paste it, I can not paste it once more.
> > Why is that?
> Fixed
>

OK.

> > * If I paste a table and there is an error (for example, the table
> > Already exists), I can not paste it once more.
> Fixed
>

OK.

> > * If I paste a table, and click on the dialog asking me if I'm sure I
> > Want to copy the table here, I can not paste it once more.
> Fixed
>
> > * If I paste a table to the same schema I copied it from, and if I click
> > Cancel on the dialog asking me for a suffix, it keeps asking me for a
> > Suffix (wether I click OK or cancel, the behavior)
> Fixed
>

It still does. I copy table t1 in schema public, and paste it in schema
s1. If a table named t1 already exists in schema s1, I can click OK or
Cancel, if there's nothing in the textbox, it keeps asking me about a
prefix.

> > * If I paste a table to another schema and the copied it from, and that
> > The same table name exists in that schema, it asks me for a suffix. If
> > I click cancel, it still tries to paste the table (it should not, I
> > Clicked cancel).
> Fixed
>

Nope, I still have the issue.

> > * If I paste a table to another schema and the copied it from, and that
> > The same table name exists in that schema, it asks me for a suffix. If
> > I delete the suffix by default and click OK, it still tries to paste
> > The table (it should, it's obvious it would fail).

Still fails.

> > * If I paste a table and the table already exists, it asks me for a
> > Suffix. If I put ". Whatever", it fails because the table name is not
> > Quoted (I have a foo.bar.whatever CREATE TABLE, CREATE instead of
> > TABLE foo. "Bar.whatever")
> Fixed
>

OK.

> > * By the way, when it fails to create the table, it should not try to
> > Insert into it right after.
> Yes
>

OK.

> > * With drag and drop, the cursor changes too late ... and you should
> > Change the cursor, it looks non-standard).
> I do not know how to change the treectrl DND UI !
>
> > * I do not like at all the "Copy tables from the list ..." dialog. It
> > Should not display tables from every servers.
> This was the main reason why I started CopyPasteTables !
> I am DBA for multiple servers in different locations of a company.
>

Well, I don't pretend we shouldn't offer the possibility to copy a
table, and paste it into another database. You can already do this with
the "Copy a table" item. I don't think we should keep the dialog. All
actions done from a right click happens in one database, and I don't
think we should change this behaviour.

I also don't think we should keep the icon in the toolbar.

> > There are unexplained colors (green, blue, and red).
> > Checkboxes are not usable on servers and databases.
> > Unless you explain why, I think you should get rid of it, not fix it.
> Now, only schema and tables are colored.
>

That's not an explanation. Why do you use colors?

> > On the code itself:
> > * CtlCheckTreeView.cpp is a widget, that should not have any behavior
> > Specific to one window. At least, not in the way this is done now.
> I think the changes that were made do not affect the initial design
> and functionality.
>

Yeah, but that's not the point. Your patch adds code in it that is
specific to your dialog (for example, "frmCopyTables *frm =
(frmCopyTables *)copytables;"). I don't think this is a good design.
Maybe someone will need something like the new code in this file in the
future, and it should be able to do it without having to fuss with a
frmCopyTable object.

> > * To check if a table exists, you look into pg_tables. You should better
> > Browse the treeview.
> > * BTW, you already have the function to do it (findTable)
> Yes, but I need the function for a server which is not connected
> (without using a browser refresh).
>

I don't feel comfortable with this.

If I click in a database in the copy window, it opens the connection to
this database in the browser. So, if the user deconnects in the browser,
the copy window still shows the connection as alive, but we cannot see
any tables. It's really weird. It will confuse users.

> > * Messages should be in uppercase (Copied FROM ...).
> Fixed
>
> > I also do not understand this code in pgColumn.cpp:
> > @ @ @ @ -327.7 +328.8 PgColumn wxString:: GetDefinition ()
>
> > If ((sql == wxT ("integer") | | sql == wxT ("BIGINT") | |
> > Sql == wxT ("pg_catalog.integer") | | sql ==
> > WxT ("pg_catalog.bigint"))
> > - & & (GetDefault () == seqDefault1 | | GetDefault () ==
> > SeqDefault2))
> > + & & ((& & Sequence9! GetSerialSequence (). IsEmpty ()) | |
> > + (! Sequence9 & & (GetDefault () == seqDefault1 | | GetDefault () ==
> > SeqDefault2))))
> > {
> > If (sql.Right (6) == wxT ("BIGINT"))
> > Sql = wxT ("bigserial");
> Without this, serial fields are not displayed as serial type for versions 9.x
>

That doesn't explain the code. Or IOW, I still don't understand the
code.

> > Another issue: I launch a copy and disconnect during copy. After I
> > Reconnect, it crashes. But the table is there with its 6 million rows.
> > And sometimes I do not need to reconnect: it crashes during the copy.
> Fixed
>

I still have crashes from time to time while doing a copy.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com


Re: Copy/Paste table(s) functions - git context patch

From
Vladimir Kokovic
Date:
Hi,

> > * If I paste a table to the same schema I copied it from, and if I click
> > Cancel on the dialog asking me for a suffix, it keeps asking me for a
> > Suffix (wether I click OK or cancel, the behavior)

'Cancel' now means the termination of the current paste operation.

---
> > * If I paste a table to another schema and the copied it from, and that
> > The same table name exists in that schema, it asks me for a suffix. If
> > I delete the suffix by default and click OK, it still tries to paste
> > The table (it should, it's obvious it would fail).

'Cancel' now means the termination of the current paste operation.

---
> > There are unexplained colors (green, blue, and red).
> > Checkboxes are not usable on servers and databases.
> > Unless you explain why, I think you should get rid of it, not fix it.
> Now, only schema and tables are colored.
>

It is no longer colored.

---
> > I also do not understand this code in pgColumn.cpp:
> > @ @ @ @ -327.7 +328.8 PgColumn wxString:: GetDefinition ()
>
> > if ((sql == wxT("integer") || sql == wxT("bigint") ||
> >             sql == wxT("pg_catalog.integer") || sql == wxT("pg_catalog.bigint"))
> >             && ((sequence9 && !GetSerialSequence().IsEmpty()) ||
> >             (!sequence9 && (GetDefault() == seqDefault1 || GetDefault() == seqDefault2))))
> >     {
> >         if (sql.Right(6) == wxT("bigint"))
> >             sql = wxT("bigserial");
> >         else
> >             sql = wxT("serial");
> >     }

Some of my tables have the following definition:

uniqueid integer DEFAULT nextval(('ps_nalog_uniqueid_seq'::text)::regclass),

For this case seqDefault1 and seqDefault2 not match !

---
> > I still have crashes from time to time while doing a copy.

Please send me a 'gdb backtrace'.


Best regards,
Vladimir Kokovic, DP senior, Belgrade, Serbia

Attachment