Thread

1. parallel pg_restore - WIP patch

   Andrew Dunstan <andrew@dunslane.net> — 2008-09-25T03:02:51Z

   Attached is my WIP patch for parallel pg_restore. It's still very rough, 
   but seems to work.
   
   Anyone who can test this with highend equipment would be helping some.
   
   cheers
   
   andrew
   

2. Re: parallel pg_restore - WIP patch

   Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> — 2008-09-26T08:27:13Z

   Andrew Dunstan wrote:
   > 
   > Attached is my WIP patch for parallel pg_restore. It's still very rough, 
   > but seems to work.
   > 
   > Anyone who can test this with highend equipment would be helping some.
   
   tried playing with this(on a 22Gb compressed dump using 4 connections) 
   but it does not seem to work at all for me:
   
   pg_restore: [custom archiver] out of memory
   pg_restore: [custom archiver] could not uncompress data: invalid block type
   pg_restore: [custom archiver] out of memory
   pg_restore: [custom archiver] could not uncompress data: invalid stored 
   block lengths
   pg_restore: [custom archiver] out of memory
   pg_restore: [custom archiver] out of memory
   pg_restore: [custom archiver] could not uncompress data: invalid 
   distance too far back
   pg_restore: [custom archiver] could not uncompress data: invalid 
   distances set
   pg_restore: [custom archiver] could not uncompress data: invalid code 
   lengths set
   pg_restore: [custom archiver] could not uncompress data: incorrect data 
   check
   pg_restore: [custom archiver] could not uncompress data: invalid code 
   lengths set
   pg_restore: [custom archiver] out of memory
   pg_restore: [custom archiver] out of memory
   pg_restore: [custom archiver] out of memory
   pg_restore: [custom archiver] out of memory
   pg_restore: [custom archiver] could not uncompress data: invalid 
   literal/length code
   pg_restore: [custom archiver] could not uncompress data: invalid 
   literal/length code
   pg_restore: [custom archiver] could not uncompress data: invalid block type
   
   each pg_restore process seem to eat a few gigabytes of memory in a few 
   seconds.
   
   
   Stefan
   
   

3. Re: parallel pg_restore - WIP patch

   Russell Smith <mr-russ@pws.com.au> — 2008-09-26T10:55:17Z

   Hi,
   
   As I'm interested in this topic, I thought I'd take a look at the
   patch.  I have no capability to test it on high end hardware but did
   some basic testing on my workstation and basic review of the patch.
   
   I somehow had the impression that instead of creating a new connection
   for each restore item we would create the processes at the start and
   then send them the dumpId's they should be restoring.  That would allow
   the controller to batch dumpId's together and expect the worker to
   process them in a transaction.  But this is probably just an idea I
   created in my head.
   
   Do we know why we experience "tuple concurrently updated" errors if we
   spawn thread too fast?
   
   I completed some test restores using the pg_restore from head with the
   patch applied.  The dump was a custom dump created with pg 8.2 and
   restored to an 8.2 database.  To confirm this would work, I completed a
   restore using the standard single threaded mode.   The schema restore
   successfully.  The only errors reported involved non-existent roles.
   
   When I attempt to restore using parallel restore I get out of memory
   errors reported from _PrintData.   The code returning the error is;
   
   _PrintData(...
       while (blkLen != 0)
       {
           if (blkLen + 1 > ctx->inSize)
           {
               free(ctx->zlibIn);
               ctx->zlibIn = NULL;
               ctx->zlibIn = (char *) malloc(blkLen + 1);
               if (!ctx->zlibIn)
                   die_horribly(AH, modulename, " out of memory\n");
   
               ctx->inSize = blkLen + 1;
               in = ctx->zlibIn;
           }
   
   
   It appears from my debugging and looking at the code that in _PrintData;
       lclContext *ctx = (lclContext *) AH->formatData;
   
   the memory context is shared across all threads.  Which means that it's
   possible the memory contexts are stomping on each other.  My GDB skills
   are now up to being able to reproduce this in a gdb session as there are
   forks going on all over the place.  And if you process them in a serial
   fashion, there aren't any errors.  I'm not sure of the fix for this. 
   But in a parallel environment it doesn't seem possible to store the
   memory context in the AH.
   
   I also receive messages saying "pg_restore: [custom archiver] could not
   read from input file: end of file".  I have not investigated these
   further as my current guess is they are linked to the out of memory error.
   
   Given I ran into this error at my first testing attempt  I haven't
   evaluated much else at this point in time.  Now all this could be
   because I'm using the 8.2 archive, but it works fine in single restore
   mode.  The dump file is about 400M compressed and an entire archive
   schema was removed from the restore path with a custom restore list.
   
   Command line used;  PGPORT=5432 ./pg_restore -h /var/run/postgresql -m4
   --truncate-before-load -v -d tt2 -L tt.list
   /home/mr-russ/pg-index-test/timetable.pgdump 2> log.txt
   
   I sent the log and this email originally to the list, but I think the attachment was too large, so I've resent without any attachements.  Since my initial testing, Stefan has confirmed the problem I am having.
   
   If you have any questions, would like me to run other tests or anything,
   feel free to contact me.
   
   Regards
   
   Russell
   
   
   

4. Re: parallel pg_restore - WIP patch

   Andrew Dunstan <andrew@dunslane.net> — 2008-09-26T12:29:04Z

   
   Russell Smith wrote:
   > Hi,
   >
   > As I'm interested in this topic, I thought I'd take a look at the
   > patch.  I have no capability to test it on high end hardware but did
   > some basic testing on my workstation and basic review of the patch.
   >
   > I somehow had the impression that instead of creating a new connection
   > for each restore item we would create the processes at the start and
   > then send them the dumpId's they should be restoring.  That would allow
   > the controller to batch dumpId's together and expect the worker to
   > process them in a transaction.  But this is probably just an idea I
   > created in my head.
   >   
   
   Yes it is. To do that I would have to invent a protocol for talking to 
   the workers, etc, and there is not the slightest chance I would get that 
   done by November.
   And I don't see the virtue in processing them all in a transaction. I've 
   provided a much simpler means of avoiding WAL logging of the COPY.
   
   > Do we know why we experience "tuple concurrently updated" errors if we
   > spawn thread too fast?
   >   
   
   No. That's an open item.
   > I completed some test restores using the pg_restore from head with the
   > patch applied.  The dump was a custom dump created with pg 8.2 and
   > restored to an 8.2 database.  To confirm this would work, I completed a
   > restore using the standard single threaded mode.   The schema restore
   > successfully.  The only errors reported involved non-existent roles.
   >
   > When I attempt to restore using parallel restore I get out of memory
   > errors reported from _PrintData.   The code returning the error is;
   >
   > _PrintData(...
   >     while (blkLen != 0)
   >     {
   >         if (blkLen + 1 > ctx->inSize)
   >         {
   >             free(ctx->zlibIn);
   >             ctx->zlibIn = NULL;
   >             ctx->zlibIn = (char *) malloc(blkLen + 1);
   >             if (!ctx->zlibIn)
   >                 die_horribly(AH, modulename, " out of memory\n");
   >
   >             ctx->inSize = blkLen + 1;
   >             in = ctx->zlibIn;
   >         }
   >
   >
   > It appears from my debugging and looking at the code that in _PrintData;
   >     lclContext *ctx = (lclContext *) AH->formatData;
   >
   > the memory context is shared across all threads.  Which means that it's
   > possible the memory contexts are stomping on each other.  My GDB skills
   > are now up to being able to reproduce this in a gdb session as there are
   > forks going on all over the place.  And if you process them in a serial
   > fashion, there aren't any errors.  I'm not sure of the fix for this. 
   > But in a parallel environment it doesn't seem possible to store the
   > memory context in the AH.
   >   
   
   
   There are no threads, hence nothing is shared. fork() create s new 
   process, not a new thread, and all they share are file descriptors.
   
   
   > I also receive messages saying "pg_restore: [custom archiver] could not
   > read from input file: end of file".  I have not investigated these
   > further as my current guess is they are linked to the out of memory error.
   >
   > Given I ran into this error at my first testing attempt  I haven't
   > evaluated much else at this point in time.  Now all this could be
   > because I'm using the 8.2 archive, but it works fine in single restore
   > mode.  The dump file is about 400M compressed and an entire archive
   > schema was removed from the restore path with a custom restore list.
   >
   > Command line used;  PGPORT=5432 ./pg_restore -h /var/run/postgresql -m4
   > --truncate-before-load -v -d tt2 -L tt.list
   > /home/mr-russ/pg-index-test/timetable.pgdump 2> log.txt
   >
   > I've attached the log.txt file so you can review the errors that I saw. 
   > I have adjusted the "out of memory" error to include a number to work
   > out which one was being triggered.  So you'll see "5 out of memory" in
   > the log file, which corresponds to the code above.
   >   
   
   However, there does seem to be something odd happening with the 
   compression lib, which I will investigate. Thanks for the report.
   
   cheers
   
   andrew
   
   
   

5. Re: parallel pg_restore - WIP patch

   Andrew Dunstan <andrew@dunslane.net> — 2008-09-26T12:37:26Z

   
   Stefan Kaltenbrunner wrote:
   > Andrew Dunstan wrote:
   >>
   >> Attached is my WIP patch for parallel pg_restore. It's still very 
   >> rough, but seems to work.
   >>
   >> Anyone who can test this with highend equipment would be helping some.
   >
   > tried playing with this(on a 22Gb compressed dump using 4 connections) 
   > but it does not seem to work at all for me:
   >
   > pg_restore: [custom archiver] out of memory
   > pg_restore: [custom archiver] could not uncompress data: invalid block 
   > type
   > pg_restore: [custom archiver] out of memory
   > pg_restore: [custom archiver] could not uncompress data: invalid 
   > stored block lengths
   > pg_restore: [custom archiver] out of memory
   > pg_restore: [custom archiver] out of memory
   > pg_restore: [custom archiver] could not uncompress data: invalid 
   > distance too far back
   > pg_restore: [custom archiver] could not uncompress data: invalid 
   > distances set
   > pg_restore: [custom archiver] could not uncompress data: invalid code 
   > lengths set
   > pg_restore: [custom archiver] could not uncompress data: incorrect 
   > data check
   > pg_restore: [custom archiver] could not uncompress data: invalid code 
   > lengths set
   > pg_restore: [custom archiver] out of memory
   > pg_restore: [custom archiver] out of memory
   > pg_restore: [custom archiver] out of memory
   > pg_restore: [custom archiver] out of memory
   > pg_restore: [custom archiver] could not uncompress data: invalid 
   > literal/length code
   > pg_restore: [custom archiver] could not uncompress data: invalid 
   > literal/length code
   > pg_restore: [custom archiver] could not uncompress data: invalid block 
   > type
   >
   > each pg_restore process seem to eat a few gigabytes of memory in a few 
   > seconds.
   
   
   Ouch. Ok, Thanks for the report. I will investigate.
   
   cheers
   
   andrew
   
   
   

6. Re: parallel pg_restore - WIP patch

   Russell Smith <mr-russ@pws.com.au> — 2008-09-26T12:56:24Z

   Andrew Dunstan wrote:
   >> Do we know why we experience "tuple concurrently updated" errors if we
   >> spawn thread too fast?
   >>   
   >
   > No. That's an open item.
   
   Okay, I'll see if I can have a little more of a look into it.  No
   promises as the restore the restore isn't playing nicely.
   >
   >>
   >> the memory context is shared across all threads.  Which means that it's
   >> possible the memory contexts are stomping on each other.  My GDB skills
   >> are now up to being able to reproduce this in a gdb session as there are
   >> forks going on all over the place.  And if you process them in a serial
   >> fashion, there aren't any errors.  I'm not sure of the fix for this.
   >> But in a parallel environment it doesn't seem possible to store the
   >> memory context in the AH.
   >>   
   >
   >
   > There are no threads, hence nothing is shared. fork() create s new
   > process, not a new thread, and all they share are file descriptors.
   >
   > However, there does seem to be something odd happening with the
   > compression lib, which I will investigate. Thanks for the report.
   
   I'm sorry, I meant processes there.  I'm aware there are no threads. 
   But my feeling was that when you forked with open files you got all of
   the open file properties, including positions, and as you dupped the
   descriptor, you share all that it's pointing to with every other copy of
   the descriptor.  My brief research on that shows that in 2005 there was
   a kernel mailing list discussion on this issue. 
   http://mail.nl.linux.org/kernelnewbies/2005-09/msg00479.html was quite
   informative for me.  I again could be wrong but worth a read.  If it is
   true, then the file needs to be reopened by each child, it can't use the
   duplicated descriptor.  I haven't had a change to implementation test is
   as it's late here.  But I'd take a stab that it will solve the
   compression library problems.
   
   I hope this helps, not hinders
   
   Russell.
   
   

7. Re: parallel pg_restore - WIP patch

   Andrew Dunstan <andrew@dunslane.net> — 2008-09-26T13:25:04Z

   
   Russell Smith wrote:
   > I'm sorry, I meant processes there.  I'm aware there are no threads. 
   > But my feeling was that when you forked with open files you got all of
   > the open file properties, including positions, and as you dupped the
   > descriptor, you share all that it's pointing to with every other copy of
   > the descriptor.  My brief research on that shows that in 2005 there was
   > a kernel mailing list discussion on this issue. 
   > http://mail.nl.linux.org/kernelnewbies/2005-09/msg00479.html was quite
   > informative for me.  I again could be wrong but worth a read.  If it is
   > true, then the file needs to be reopened by each child, it can't use the
   > duplicated descriptor.  I haven't had a change to implementation test is
   > as it's late here.  But I'd take a stab that it will solve the
   > compression library problems.
   >
   > I hope this helps, not hinders
   >
   >
   >   
   
   I'm sure that's the problem. Should be fairly easily fixable, I believe.
   
   Thanks for the info.
   
   cheers
   
   andrew
   
   

8. Re: parallel pg_restore - WIP patch

   Andrew Dunstan <andrew@dunslane.net> — 2008-09-26T15:20:56Z

   
   This version of the patch should fix the "shared file descriptor" bug 
   Russell Smith noticed. It also disables the 1/2 second sleep between 
   forks, so the performance on a small db (regression) is vastly improved.
   
   cheers
   
   andrew
   
   
   
   

9. Re: parallel pg_restore - WIP patch

   Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> — 2008-09-26T21:03:51Z

   Andrew Dunstan wrote:
   > 
   > 
   > This version of the patch should fix the "shared file descriptor" bug 
   > Russell Smith noticed. It also disables the 1/2 second sleep between 
   > forks, so the performance on a small db (regression) is vastly improved.
   
   this works better but there is something fishy still - using the same 
   dump file I get a proper restore using pg_restore normally. If I however 
   use -m for a parallel one I only get parts (in this case only 243 of the 
   709 tables) of the database restored ...
   
   
   Stefan
   
   

10. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-26T21:10:44Z

    
    Stefan Kaltenbrunner wrote:
    > Andrew Dunstan wrote:
    >>
    >>
    >> This version of the patch should fix the "shared file descriptor" bug 
    >> Russell Smith noticed. It also disables the 1/2 second sleep between 
    >> forks, so the performance on a small db (regression) is vastly improved.
    >
    > this works better but there is something fishy still - using the same 
    > dump file I get a proper restore using pg_restore normally. If I 
    > however use -m for a parallel one I only get parts (in this case only 
    > 243 of the 709 tables) of the database restored ...
    >
    >
    >
    
    Yes, there are several funny things going on, including some stuff with 
    dependencies. I'll have a new patch tomorrow with luck. Thanks for testing.
    
    cheers
    
    andrew
    
    

11. Re: parallel pg_restore - WIP patch

    Joshua D. Drake <jd@commandprompt.com> — 2008-09-26T21:21:34Z

    On Fri, 26 Sep 2008 17:10:44 -0400
    Andrew Dunstan <andrew@dunslane.net> wrote:
    
    > Yes, there are several funny things going on, including some stuff
    > with dependencies. I'll have a new patch tomorrow with luck. Thanks
    > for testing.
    
    O.k. I took at look at the patch itself and although I don't understand
    all of it there were a couple of red flags to me:
    
    + if (ropt->create)
    + 		die_horribly(AH,modulename,
    + 					 "parallel restore is
    incompatible with --create\n");
    + 
    
    This seems like an odd limitation. In my mind, the schema would not be
    restored in parallel. The schema before data would restore as a single
    thread. Even the largest schemas would only take minutes (if that).
    Thus something like --create should never be a problem.
    
    I also noticed you check if we have zlib? Is it even possible to use
    the c format without it? (that would be new to me).
    
    I noticed this line:
    
    
    + 	while((next_work_item = get_next_work_item(AH)) != NULL)
    + 	{
    + 		/* XXX need to improve this test in case there is no
    table data */
    + 		/* need to test for indexes, FKs, PK, Unique, etc */
    + 		if(strcmp(next_work_item->desc,"TABLE DATA") == 0)
    + 			break;
    + 		(void) _restore_one_te(AH, next_work_item, ropt,
    false);
    + 
    + 		next_work_item->prestored = true;
    + 
    + 		_reduce_dependencies(AH,next_work_item);
    + 	}
    
    
    Intead of the TABLE DATA compare, perhaps it makes sense to back patch
    pg_dump to have a line delimiter in the TOC? That way even if there is
    no TABLE DATA there would be a delimiter that says:
    
    --- BEGIN TABLE DATA
    --- END TABLE DATA
    
    Thus if nothing is there... nothing is there?
    
    + 			/* delay just long enough betweek forks to
    give the catalog some
    + 			 * breathing space. Without this sleep I got 
    + 			 * "tuple concurrently updated" errors.
    + 			 */
    + 			pg_usleep(500000);
    + 			continue; /* in case the slots are not yet
    full */
    + 		}
    
    Could that be solved with a lock instead? Once the lock is released....
    
    Anyway... just some thoughts. I apologize if I misunderstood the patch.
    
    Sincerely,
    
    Joshua D. Drake
    
    
    
    > 
    > cheers
    > 
    > andrew
    > 
    
    
    -- 
    The PostgreSQL Company since 1997: http://www.commandprompt.com/ 
    PostgreSQL Community Conference: http://www.postgresqlconference.org/
    United States PostgreSQL Association: http://www.postgresql.us/
    
    
    
    

12. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-26T22:23:30Z

    
    Joshua Drake wrote:
    > On Fri, 26 Sep 2008 17:10:44 -0400
    > Andrew Dunstan <andrew@dunslane.net> wrote:
    >
    >   
    >> Yes, there are several funny things going on, including some stuff
    >> with dependencies. I'll have a new patch tomorrow with luck. Thanks
    >> for testing.
    >>     
    >
    > O.k. I took at look at the patch itself and although I don't understand
    > all of it there were a couple of red flags to me:
    >
    > + if (ropt->create)
    > + 		die_horribly(AH,modulename,
    > + 					 "parallel restore is
    > incompatible with --create\n");
    > + 
    >   
    > This seems like an odd limitation. In my mind, the schema would not be
    > restored in parallel. The schema before data would restore as a single
    > thread. Even the largest schemas would only take minutes (if that).
    > Thus something like --create should never be a problem.
    >   
    
    
    Originally I had everything restoring in parallel. Now I am in fact (as 
    the patch should have showed you) restoring the first part in a single 
    thread like you say. Thus I probably can relax that restriction. I will 
    look and see.
    
    > I also noticed you check if we have zlib? Is it even possible to use
    > the c format without it? (that would be new to me).
    >
    > I noticed this line:
    >
    >
    > + 	while((next_work_item = get_next_work_item(AH)) != NULL)
    > + 	{
    > + 		/* XXX need to improve this test in case there is no
    > table data */
    > + 		/* need to test for indexes, FKs, PK, Unique, etc */
    > + 		if(strcmp(next_work_item->desc,"TABLE DATA") == 0)
    > + 			break;
    > + 		(void) _restore_one_te(AH, next_work_item, ropt,
    > false);
    > + 
    > + 		next_work_item->prestored = true;
    > + 
    > + 		_reduce_dependencies(AH,next_work_item);
    > + 	}
    >
    >
    > Intead of the TABLE DATA compare, perhaps it makes sense to back patch
    > pg_dump to have a line delimiter in the TOC? That way even if there is
    > no TABLE DATA there would be a delimiter that says:
    >
    > --- BEGIN TABLE DATA
    > --- END TABLE DATA
    >
    > Thus if nothing is there... nothing is there?
    >   
    
    The TOC isn't stored as a text file. So we'll need to look by entry 
    tags. It's no big deal - there aren't a huge number.
    
    > + 			/* delay just long enough betweek forks to
    > give the catalog some
    > + 			 * breathing space. Without this sleep I got 
    > + 			 * "tuple concurrently updated" errors.
    > + 			 */
    > + 			pg_usleep(500000);
    > + 			continue; /* in case the slots are not yet
    > full */
    > + 		}
    >
    > Could that be solved with a lock instead? Once the lock is released....
    >   
    
    
    That sleep is now gone.
    
    
    > Anyway... just some thoughts. I apologize if I misunderstood the patch.
    >
    >
    >   
    
    
    No problem. Thanks for looking.
    
    cheers
    
    andrew
    
    

13. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-29T02:58:24Z

    
    Andrew Dunstan wrote:
    >
    >
    >>
    >> this works better but there is something fishy still - using the same 
    >> dump file I get a proper restore using pg_restore normally. If I 
    >> however use -m for a parallel one I only get parts (in this case only 
    >> 243 of the 709 tables) of the database restored ...
    >>
    >>
    >>
    >
    > Yes, there are several funny things going on, including some stuff 
    > with dependencies. I'll have a new patch tomorrow with luck. Thanks 
    > for testing.
    >
    >
    
    OK, in this version a whole heap of bugs are fixed, mainly those to do 
    with dependencies and saved state. I get identical row counts in the 
    source and destination now, quite reliably.
    
    cheers
    
    andrew
    

14. Re: parallel pg_restore - WIP patch

    Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> — 2008-09-29T11:55:03Z

    Andrew Dunstan wrote:
    > 
    > 
    > Andrew Dunstan wrote:
    >>
    >>
    >>>
    >>> this works better but there is something fishy still - using the same 
    >>> dump file I get a proper restore using pg_restore normally. If I 
    >>> however use -m for a parallel one I only get parts (in this case only 
    >>> 243 of the 709 tables) of the database restored ...
    >>>
    >>>
    >>>
    >>
    >> Yes, there are several funny things going on, including some stuff 
    >> with dependencies. I'll have a new patch tomorrow with luck. Thanks 
    >> for testing.
    >>
    >>
    > 
    > OK, in this version a whole heap of bugs are fixed, mainly those to do 
    > with dependencies and saved state. I get identical row counts in the 
    > source and destination now, quite reliably.
    
    this looks much better (for a restore that usually takes 180min I can 
    get down to 72min using -m 4) - however especially with higher 
    concurrency I'm sometimes running into restore failures due to deadlocks 
    happening during constraint restoration (slightly redacted):
    
    pg_restore: [archiver (db)] Error from TOC entry 7765; 2606 1460743180 
    FK CONSTRAINT fk_av_relations_av db_owner
    pg_restore: [archiver (db)] could not execute query: ERROR:  deadlock 
    detected
    DETAIL:  Process 18100 waits for AccessExclusiveLock on relation 
    1460818342 of database 1460815284; blocked by process 18103.
    Process 18103 waits for AccessExclusiveLock on relation 1460818336 of 
    database 1460815284; blocked by process 18100.
    HINT:  See server log for query details.
    
    ALTER TABLE ONLY foo
         ADD CONSTRAINT fk_av_relations_av FOREIGN KEY (vs_id) REFERENCES 
    bar ...
    
    

15. Re: parallel pg_restore - WIP patch

    Tom Lane <tgl@sss.pgh.pa.us> — 2008-09-29T12:06:23Z

    Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes:
    > pg_restore: [archiver (db)] could not execute query: ERROR:  deadlock 
    > detected
    > DETAIL:  Process 18100 waits for AccessExclusiveLock on relation 
    > 1460818342 of database 1460815284; blocked by process 18103.
    > Process 18103 waits for AccessExclusiveLock on relation 1460818336 of 
    > database 1460815284; blocked by process 18100.
    > HINT:  See server log for query details.
    
    > ALTER TABLE ONLY foo
    >      ADD CONSTRAINT fk_av_relations_av FOREIGN KEY (vs_id) REFERENCES 
    > bar ...
    
    Hmm, I'll bet the restore code doesn't realize that this can't run in
    parallel with index creation on either table ...
    
    			regards, tom lane
    
    

16. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-29T12:32:31Z

    
    Tom Lane wrote:
    > Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> writes:
    >   
    >> pg_restore: [archiver (db)] could not execute query: ERROR:  deadlock 
    >> detected
    >> DETAIL:  Process 18100 waits for AccessExclusiveLock on relation 
    >> 1460818342 of database 1460815284; blocked by process 18103.
    >> Process 18103 waits for AccessExclusiveLock on relation 1460818336 of 
    >> database 1460815284; blocked by process 18100.
    >> HINT:  See server log for query details.
    >>     
    >
    >   
    >> ALTER TABLE ONLY foo
    >>      ADD CONSTRAINT fk_av_relations_av FOREIGN KEY (vs_id) REFERENCES 
    >> bar ...
    >>     
    >
    > Hmm, I'll bet the restore code doesn't realize that this can't run in
    > parallel with index creation on either table ...
    >
    > 			
    >   
    
    Yeah. Of course, it's never needed to bother with stuff like that till now.
    
    The very simple fix is probably to run a separate parallel cycle just 
    for FKs, after the index creation.
    
    A slightly more elegant fix would probably be to add dependencies from 
    each index that might cause this to the FK constraint.
    
    I'll work on the first for now.
    
    Is there any chance that the locks we're taking here are too strong? 
    Intuitively it looks a bit like it.
    
    cheers
    
    andrew
    
    

17. Re: parallel pg_restore - WIP patch

    Tom Lane <tgl@sss.pgh.pa.us> — 2008-09-29T12:39:24Z

    Andrew Dunstan <andrew@dunslane.net> writes:
    > Tom Lane wrote:
    >> Hmm, I'll bet the restore code doesn't realize that this can't run in
    >> parallel with index creation on either table ...
    
    > Yeah. Of course, it's never needed to bother with stuff like that till now.
    
    > The very simple fix is probably to run a separate parallel cycle just 
    > for FKs, after the index creation.
    
    Um, FKs could conflict with each other too, so that by itself isn't
    gonna fix anything.
    
    			regards, tom lane
    
    

18. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-29T13:02:38Z

    
    Tom Lane wrote:
    > Andrew Dunstan <andrew@dunslane.net> writes:
    >   
    >> Tom Lane wrote:
    >>     
    >>> Hmm, I'll bet the restore code doesn't realize that this can't run in
    >>> parallel with index creation on either table ...
    >>>       
    >
    >   
    >> Yeah. Of course, it's never needed to bother with stuff like that till now.
    >>     
    >
    >   
    >> The very simple fix is probably to run a separate parallel cycle just 
    >> for FKs, after the index creation.
    >>     
    >
    > Um, FKs could conflict with each other too, so that by itself isn't
    > gonna fix anything.
    >
    > 			
    >   
    
    Good point. Looks like we'll need to make a list of "can't run in 
    parallel with" items as well as strict dependencies.
    
    cheers
    
    andrew
    
    

19. Re: parallel pg_restore - WIP patch

    Tom Lane <tgl@sss.pgh.pa.us> — 2008-09-29T13:17:12Z

    Andrew Dunstan <andrew@dunslane.net> writes:
    > Tom Lane wrote:
    >> Um, FKs could conflict with each other too, so that by itself isn't
    >> gonna fix anything.
    
    > Good point. Looks like we'll need to make a list of "can't run in 
    > parallel with" items as well as strict dependencies.
    
    Yeah, I was just thinking about that.  The current archive format
    doesn't really carry enough information for this.  I think there
    are two basic solutions we could adopt:
    
    * Extend the archive format to provide some indication that "restoring
    this object requires exclusive access to these dependencies".
    
    * Hardwire knowledge into pg_restore that certain types of objects
    require exclusive access to their dependencies.
    
    The former seems more flexible, as well as more in tune with the basic
    design assumption that pg_restore shouldn't have a lot of knowledge
    about individual archive object types.  But it would mean that you
    couldn't use parallel restore with any pre-8.4 dumps.  In the long run
    that's no big deal, but in the short run it's annoying.
    
    Another angle is that it's not clear what happens if the need for
    exclusive access changes over time.  You were just speculating about
    reducing the lock strength required for ALTER TABLE ADD FOREIGN KEY.
    I don't know if that's workable or not, but certainly reducing the
    lock strength for some types of ALTER TABLE might be in our future.
    Contrarily, we don't currently try hard to lock any non-table objects
    (schemas, functions, etc) while building dependent objects; but that's
    obviously not really right, and someday we might decide to fix it.
    So having pg_dump prepare the list of exclusive dependencies at dump
    time might be the wrong thing --- it would reflect the behavior of
    the source server version, not the target which is what matters.
    
    Thoughts?
    
    			regards, tom lane
    
    

20. Re: parallel pg_restore - WIP patch

    Dimitri Fontaine <dfontaine@hi-media.com> — 2008-09-29T13:51:40Z

    Le lundi 29 septembre 2008, Tom Lane a écrit :
    > * Extend the archive format to provide some indication that "restoring
    > this object requires exclusive access to these dependencies".
    >
    > * Hardwire knowledge into pg_restore that certain types of objects
    > require exclusive access to their dependencies.
    
    Well, it seems to me that currently the FK needs in term of existing indexes 
    and locks, and some other object lock needs, are all hardwired. Is it even 
    safe to consider having the locks needed for certain commands not be 
    hardwired?
    
    Provided I'm not all wrong here, I don't see how having something more 
    flexible at restore time than at build time is a win. The drawback is that 
    whenever you change a lock need in commands, you have to remember teaching 
    pg_restore about it too.
    
    So my vote here is in favor of hardwired knowledge of pg_restore, matching 
    target server code assumptions and needs.
    
    Regards,
    -- 
    dim
    

21. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-29T14:25:10Z

    
    Dimitri Fontaine wrote:
    > Le lundi 29 septembre 2008, Tom Lane a écrit :
    >   
    >> * Extend the archive format to provide some indication that "restoring
    >> this object requires exclusive access to these dependencies".
    >>
    >> * Hardwire knowledge into pg_restore that certain types of objects
    >> require exclusive access to their dependencies.
    >>     
    >
    > Well, it seems to me that currently the FK needs in term of existing indexes 
    > and locks, and some other object lock needs, are all hardwired. Is it even 
    > safe to consider having the locks needed for certain commands not be 
    > hardwired?
    >
    > Provided I'm not all wrong here, I don't see how having something more 
    > flexible at restore time than at build time is a win. The drawback is that 
    > whenever you change a lock need in commands, you have to remember teaching 
    > pg_restore about it too.
    >
    > So my vote here is in favor of hardwired knowledge of pg_restore, matching 
    > target server code assumptions and needs.
    >
    >   
    
    Well, I've had to use some knowledge of various item types already, and 
    I have been trying not to disturb pg_dump also, so I'm inclined to build 
    this knowledge into pg_restore.
    
    ISTM that "things that will have lock conflicts" are different and more 
    target version dependent than "things that logically depend on other 
    things", so we can still rely on pg_dump to some extent to provide the 
    latter while building the former at restore time.
    
    cheers
    
    andrew
    
    

22. Re: parallel pg_restore - WIP patch

    Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> — 2008-09-29T19:54:42Z

    Tom Lane wrote:
    > Andrew Dunstan <andrew@dunslane.net> writes:
    >> Tom Lane wrote:
    >>> Um, FKs could conflict with each other too, so that by itself isn't
    >>> gonna fix anything.
    > 
    >> Good point. Looks like we'll need to make a list of "can't run in 
    >> parallel with" items as well as strict dependencies.
    > 
    > Yeah, I was just thinking about that.  The current archive format
    > doesn't really carry enough information for this.  I think there
    > are two basic solutions we could adopt:
    > 
    > * Extend the archive format to provide some indication that "restoring
    > this object requires exclusive access to these dependencies".
    > 
    > * Hardwire knowledge into pg_restore that certain types of objects
    > require exclusive access to their dependencies.
    > 
    > The former seems more flexible, as well as more in tune with the basic
    > design assumption that pg_restore shouldn't have a lot of knowledge
    > about individual archive object types.  But it would mean that you
    > couldn't use parallel restore with any pre-8.4 dumps.  In the long run
    > that's no big deal, but in the short run it's annoying.
    
    hmm not sure how much of a problem that really is - we usually recommend 
    to use the pg_dump version of the target database anyway.
    
    
    
    
    Stefan
    
    

23. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-29T23:38:58Z

    
    Stefan Kaltenbrunner wrote:
    > Tom Lane wrote:
    >> Andrew Dunstan <andrew@dunslane.net> writes:
    >>> Tom Lane wrote:
    >>>> Um, FKs could conflict with each other too, so that by itself isn't
    >>>> gonna fix anything.
    >>
    >>> Good point. Looks like we'll need to make a list of "can't run in 
    >>> parallel with" items as well as strict dependencies.
    >>
    >> Yeah, I was just thinking about that.  The current archive format
    >> doesn't really carry enough information for this.  I think there
    >> are two basic solutions we could adopt:
    >>
    >> * Extend the archive format to provide some indication that "restoring
    >> this object requires exclusive access to these dependencies".
    >>
    >> * Hardwire knowledge into pg_restore that certain types of objects
    >> require exclusive access to their dependencies.
    >>
    >> The former seems more flexible, as well as more in tune with the basic
    >> design assumption that pg_restore shouldn't have a lot of knowledge
    >> about individual archive object types.  But it would mean that you
    >> couldn't use parallel restore with any pre-8.4 dumps.  In the long run
    >> that's no big deal, but in the short run it's annoying.
    >
    > hmm not sure how much of a problem that really is - we usually 
    > recommend to use the pg_dump version of the target database anyway.
    >
    >
    >
    >
    
    We don't really need a huge amount of hardwiring as it turns out. Here 
    is a version of the patch that tries to do what's needed in this area.
    
    cheers
    
    andrew
    

24. Re: parallel pg_restore - WIP patch

    Philip Warner <pjw@rhyme.com.au> — 2008-09-30T03:26:00Z

    >
    > + 					if (strcmp(te->desc,"CONSTRAINT") == 0 ||
    > + 						strcmp(te->desc,"FK CONSTRAINT") == 0 ||
    > + 						strcmp(te->desc,"CHECK CONSTRAINT") == 0 ||
    > + 						strcmp(te->desc,"TRIGGER") == 0 ||
    > + 						strcmp(slots[i].te->desc,"CONSTRAINT") == 0 ||
    > + 						strcmp(slots[i].te->desc,"FK CONSTRAINT") == 0 ||
    > + 						strcmp(slots[i].te->desc,"CHECK CONSTRAINT") == 0 ||
    > + 						strcmp(slots[i].te->desc,"TRIGGER") == 0)
    >   
    Really just an observation from the peanut gallery here, but every time
    pg_restore hard-codes this kind of thing, it introduces yet another
    possible side-effect bug when someone, eg, adds a new TOC type.
    
    Would it substantially decrease the benefits of the patch to skip *any*
    toc entry that shares dependencies with another? (rather than just those
    listed above).
    
    
     
    
    
    

25. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-30T03:59:23Z

    
    Philip Warner wrote:
    >> + 					if (strcmp(te->desc,"CONSTRAINT") == 0 ||
    >> + 						strcmp(te->desc,"FK CONSTRAINT") == 0 ||
    >> + 						strcmp(te->desc,"CHECK CONSTRAINT") == 0 ||
    >> + 						strcmp(te->desc,"TRIGGER") == 0 ||
    >> + 						strcmp(slots[i].te->desc,"CONSTRAINT") == 0 ||
    >> + 						strcmp(slots[i].te->desc,"FK CONSTRAINT") == 0 ||
    >> + 						strcmp(slots[i].te->desc,"CHECK CONSTRAINT") == 0 ||
    >> + 						strcmp(slots[i].te->desc,"TRIGGER") == 0)
    >>   
    >>     
    > Really just an observation from the peanut gallery here, but every time
    > pg_restore hard-codes this kind of thing, it introduces yet another
    > possible side-effect bug when someone, eg, adds a new TOC type.
    >
    > Would it substantially decrease the benefits of the patch to skip *any*
    > toc entry that shares dependencies with another? (rather than just those
    > listed above).
    >
    >
    >   
    
    Unfortunately, it quite possibly would. You would not be able to build 
    two indexes on the same table in parallel, even though they wouldn't 
    have conflicting locks.
    
    cheers
    
    andrew
    
    
    
    

26. Re: parallel pg_restore - WIP patch

    Philip Warner <pjw@rhyme.com.au> — 2008-09-30T04:13:06Z

    Andrew Dunstan wrote:
    > Unfortunately, it quite possibly would. You would not be able to build
    > two indexes on the same table in parallel, even though they wouldn't
    > have conflicting locks.
    I suppose so, but:
    
    1. By the same logic it might speed things up; it might build two
    completely separate indexes and thereby avoid (some kind of) contention.
    In any case, it would most likely do *something* else. It should only
    reduce performance if (a) it can do nothing or (b) there is a benefit in
    building multiple indexes on the same table at the same time.
    
    2. Perhaps if there are a limited number of items that share
    dependencies but which are known to be OK (ie. indexes), maybe list them
    in the inner loop as exceptions and allow them to run parallel. This
    would mean a failure to list a new TOC item type would result in worse
    performance rather than a crash.
    
    
    
    
    

27. Re: parallel pg_restore - WIP patch

    Andrew Dunstan <andrew@dunslane.net> — 2008-09-30T12:55:47Z

    
    Philip Warner wrote:
    > Andrew Dunstan wrote:
    >   
    >> Unfortunately, it quite possibly would. You would not be able to build
    >> two indexes on the same table in parallel, even though they wouldn't
    >> have conflicting locks.
    >>     
    > I suppose so, but:
    >
    > 1. By the same logic it might speed things up; it might build two
    > completely separate indexes and thereby avoid (some kind of) contention.
    > In any case, it would most likely do *something* else. It should only
    > reduce performance if (a) it can do nothing or (b) there is a benefit in
    > building multiple indexes on the same table at the same time.
    >
    > 2. Perhaps if there are a limited number of items that share
    > dependencies but which are known to be OK (ie. indexes), maybe list them
    > in the inner loop as exceptions and allow them to run parallel. This
    > would mean a failure to list a new TOC item type would result in worse
    > performance rather than a crash.
    >
    >
    >   
    
    I will look at it in due course. Right now my concern is simply to get 
    something that works that we can do some testing with. I think that's 
    what we have now (fingers crossed). Some parts of it are jury rigged.
    
    BTW, though, building indexes for the same table together is likely to 
    be a win AIUI, especially given the recent work on synchronised scans.
    
    cheers
    
    andrew
    
    

28. Re: parallel pg_restore - WIP patch

    Stefan Kaltenbrunner <stefan@kaltenbrunner.cc> — 2008-10-03T08:51:27Z

    Andrew Dunstan wrote:
    > 
    > 
    > Stefan Kaltenbrunner wrote:
    >> Tom Lane wrote:
    >>> Andrew Dunstan <andrew@dunslane.net> writes:
    >>>> Tom Lane wrote:
    >>>>> Um, FKs could conflict with each other too, so that by itself isn't
    >>>>> gonna fix anything.
    >>>
    >>>> Good point. Looks like we'll need to make a list of "can't run in 
    >>>> parallel with" items as well as strict dependencies.
    >>>
    >>> Yeah, I was just thinking about that.  The current archive format
    >>> doesn't really carry enough information for this.  I think there
    >>> are two basic solutions we could adopt:
    >>>
    >>> * Extend the archive format to provide some indication that "restoring
    >>> this object requires exclusive access to these dependencies".
    >>>
    >>> * Hardwire knowledge into pg_restore that certain types of objects
    >>> require exclusive access to their dependencies.
    >>>
    >>> The former seems more flexible, as well as more in tune with the basic
    >>> design assumption that pg_restore shouldn't have a lot of knowledge
    >>> about individual archive object types.  But it would mean that you
    >>> couldn't use parallel restore with any pre-8.4 dumps.  In the long run
    >>> that's no big deal, but in the short run it's annoying.
    >>
    >> hmm not sure how much of a problem that really is - we usually 
    >> recommend to use the pg_dump version of the target database anyway.
    >>
    >>
    >>
    >>
    > 
    > We don't really need a huge amount of hardwiring as it turns out. Here 
    > is a version of the patch that tries to do what's needed in this area.
    
    this one is much better - however I still seem to be able to create 
    deadlock scenarios with strange FK relations - ie FKs going in both 
    directions between two tables.
    
    for those interested these are the timings on my 8 core testbox for my 
    test database:
    
    single process restore: 169min
    -m2: 101min
    -m6: 64min
    -m8: 63min
    -m16: 56min
    
    
    Stefan