Обсуждение: parallel restore vs. windows
OK, after quite some trying I have hit a brick wall. I have been unable to get parallel restore to work with Windows threading. No doubt I am missing something, but I really don't know what. Unless someone can tell me what I am doing wrong, I have these possibilities: * run parallel steps on Windows in separate processes rather than threads, similar to what we do in the server, or * disable parallel restore on Windows for now. Time is unfortunately running very short. Latest attempt is attached. cheers andrew
Вложения
Andrew Dunstan wrote: > > OK, after quite some trying I have hit a brick wall. I have been unable > to get parallel restore to work with Windows threading. No doubt I am > missing something, but I really don't know what. Unless someone can tell > me what I am doing wrong, I have these possibilities: > > * run parallel steps on Windows in separate processes rather than > threads, similar to what we do in the server, or > * disable parallel restore on Windows for now. > > > Time is unfortunately running very short. > > Latest attempt is attached. > > We use _beginthread. I don't remember exactely how it broke, but it did. Try using the below instead of CreateThread. // NOTE: if you don't need the returned handle, close it or // leaks will occur. Closing it doesn't kill the thread. HANDLE h = (HANDLE)_beginthreadex(NULL, 0, thread_start, arg, 0, NULL); if(h) CloseHandle(h); From MSDN: "A thread in an executable that calls the C run-time library (CRT) should use the _beginthread and _endthread functions for thread management rather than CreateThread and ExitThread;" -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > Andrew Dunstan wrote: >> >> OK, after quite some trying I have hit a brick wall. I have been >> unable to get parallel restore to work with Windows threading. No >> doubt I am missing something, but I really don't know what. Unless >> someone can tell me what I am doing wrong, I have these possibilities: >> >> * run parallel steps on Windows in separate processes rather than >> threads, similar to what we do in the server, or >> * disable parallel restore on Windows for now. >> >> >> Time is unfortunately running very short. >> >> Latest attempt is attached. >> >> > > We use _beginthread. I don't remember exactely how it broke, but it > did. Try using the below instead of CreateThread. > > // NOTE: if you don't need the returned handle, close it or > // leaks will occur. Closing it doesn't kill the thread. > HANDLE h = (HANDLE)_beginthreadex(NULL, 0, thread_start, arg, 0, NULL); This didn't give me any more joy, unfortunately. But you're right, I should be using it. > if(h) > CloseHandle(h); Umm, even if I wait on the handle using waitForMultipleObjects() ? > > From MSDN: > "A thread in an executable that calls the C run-time library (CRT) > should use the _beginthread and _endthread functions for thread > management rather than CreateThread and ExitThread;" I am terminating the thread by returning from the thread function. I understand this is the recommended way. cheers andrew
>> HANDLE h = (HANDLE)_beginthreadex(NULL, 0, thread_start, arg, 0, NULL); > > This didn't give me any more joy, unfortunately. But you're right, I > should be using it. > Are these threads sharing memory, intentionally or by mistake? >> if(h) >> CloseHandle(h); > > Umm, even if I wait on the handle using waitForMultipleObjects() ? > I was only trying to demonstrate that the value returned by _beginthread can be managed/closed just like any other win32 HANDLE. > I am terminating the thread by returning from the thread function. I> understand this is the recommended way. I didn't see a CloseHandle on ret_child anywhere. The HANDLE still exists after the thread exists, you still have to call CloseHandle. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > >>> HANDLE h = (HANDLE)_beginthreadex(NULL, 0, thread_start, arg, 0, NULL); >> >> This didn't give me any more joy, unfortunately. But you're right, I >> should be using it. >> > > Are these threads sharing memory, intentionally or by mistake? Things they write, and things they read but might not be stable, are not supposed to be shared. If they are it's a mistake. > >>> if(h) >>> CloseHandle(h); >> >> Umm, even if I wait on the handle using waitForMultipleObjects() ? >> > > I was only trying to demonstrate that the value returned by > _beginthread can be managed/closed just like any other win32 HANDLE. > > > I am terminating the thread by returning from the thread function. I > > understand this is the recommended way. > > I didn't see a CloseHandle on ret_child anywhere. The HANDLE still > exists after the thread exists, you still have to call CloseHandle. OK. I'll put that in after handling the return. thanks andrew
Andrew Dunstan wrote: > > > Andrew Chernow wrote: >> >>>> HANDLE h = (HANDLE)_beginthreadex(NULL, 0, thread_start, arg, 0, NULL); >>> >>> This didn't give me any more joy, unfortunately. But you're right, I >>> should be using it. >>> >> >> Are these threads sharing memory, intentionally or by mistake? > > > Things they write, and things they read but might not be stable, are not > supposed to be shared. If they are it's a mistake. > Looks like the ArchiveHandle variable 'AH' and the TocEntry 'next_work_item' are not being deep copied at line 315 of your patch, where you prepare the RestoreArgs struct for the thread. Every thread is accessing and possibly updating the members of these structs that need to be deep copied. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
>>> >>> Are these threads sharing memory, intentionally or by mistake? >> >> >> Things they write, and things they read but might not be stable, are >> not supposed to be shared. If they are it's a mistake. >> > > Looks like the ArchiveHandle variable 'AH' and the TocEntry > 'next_work_item' are not being deep copied at line 315 of your patch, > where you prepare the RestoreArgs struct for the thread. Every thread > is accessing and possibly updating the members of these structs that > need to be deep copied. > Forgot something, the prestore function leaks the RestoreArgs struct. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: > > Looks like the ArchiveHandle variable 'AH' and the TocEntry > 'next_work_item' are not being deep copied at line 315 of your patch, > where you prepare the RestoreArgs struct for the thread. Every thread > is accessing and possibly updating the members of these structs that > need to be deep copied. > Each thread deals with a different TocEntry, which no other thread deals with, so there should be no need to clone it, I believe. Parts of AH need deep cloning, notably the formatData member, which is done in _ReopenArchive(). I am aware that there are some minor memory leaks, which I will remedy. cheers andrew
> > Parts of AH need deep cloning, notably the formatData member, which is > done in _ReopenArchive(). > Is it okay to clone this from within the thread? The reopen() appears to mess with AH->FH, which mutltiple threads are calling fclose on. The second thread is going to fail and the first fclose() will close the main threads handle. + #ifndef WIN32 + if (fclose(AH->FH) != 0) + die_horribly(AH, modulename, "could not close archive file: %s\n", + strerror(errno)); + #else How are things failing? Core dump, maybe you are seeing the above error? The non-windows path is safe from this because a) it never does an fclose and b) its a fork and has its own copy of the FH. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: >> >> Parts of AH need deep cloning, notably the formatData member, which >> is done in _ReopenArchive(). >> > > Is it okay to clone this from within the thread? I don't see why not. > > The reopen() appears to mess with AH->FH, which mutltiple threads are > calling fclose on. The second thread is going to fail and the first > fclose() will close the main threads handle. > > + #ifndef WIN32 > + if (fclose(AH->FH) != 0) > + die_horribly(AH, modulename, "could not close archive file: > %s\n", > + strerror(errno)); > + #else > > How are things failing? Core dump, maybe you are seeing the above > error? The non-windows path is safe from this because a) it never > does an fclose and b) its a fork and has its own copy of the FH. No, as this fragment shows, fclose() is NOT called on Windows. The program dies with a nasty dialog box when restoring a dump of the regression database after the second COPY thread disconnects. cheers andrew
Andrew Dunstan wrote: > > No, as this fragment shows, fclose() is NOT called on Windows. > Oooppps. I'm the village idiot today. > The program dies with a nasty dialog box when restoring a dump of the > regression database after the second COPY thread disconnects. > I'll poke around but apparently I need food :) -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
>>> >>> Parts of AH need deep cloning, notably the formatData member, which >>> is done in _ReopenArchive(). >>> >> >> Is it okay to clone this from within the thread? > > I don't see why not. > Because another thread may be modifying the memory you are trying to clone. If no one modifies the formatData struct, then why is it being deep copied to begin with. > > The program dies with a nasty dialog box when restoring a dump of the > regression database after the second COPY thread disconnects. Sounds like the friendly and helpful GPF Dialog (General Protection Fault). This is a core dump which strongly suggests your threads are trampling over one another. Its possible that a couple threads get fired off but upon the first thread completion, something !(deep_copied) is freed/modified ... bang-bang your dead :o I tried to find this, but haven't yet. Maybe do a full deep copy in the main thread and comment out any in-thread deep copying. I wonder if that would work with no other changes. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Andrew Chernow wrote: >>>> >>>> Parts of AH need deep cloning, notably the formatData member, which >>>> is done in _ReopenArchive(). >>>> >>> >>> Is it okay to clone this from within the thread? >> >> I don't see why not. >> > > Because another thread may be modifying the memory you are trying to > clone. If no one modifies the formatData struct, then why is it being > deep copied to begin with. > >> >> The program dies with a nasty dialog box when restoring a dump of the >> regression database after the second COPY thread disconnects. > > Sounds like the friendly and helpful GPF Dialog (General Protection > Fault). This is a core dump which strongly suggests your threads are > trampling over one another. Its possible that a couple threads get > fired off but upon the first thread completion, something > !(deep_copied) is freed/modified ... bang-bang your dead :o I tried > to find this, but haven't yet. > > Maybe do a full deep copy in the main thread and comment out any > in-thread deep copying. I wonder if that would work with no other > changes. I'll try. It's unfortunately not as simple as it sounds, because of the way the abstractions are arranged. I can't count the number of times I have had to stop and try to clear my head while working on this code. Thanks for the suggestion. cheers andrew
Andrew Dunstan wrote: > > > Andrew Chernow wrote: >>>>> >>>>> Parts of AH need deep cloning, notably the formatData member, which >>>>> is done in _ReopenArchive(). >>>>> >>>> >>>> Is it okay to clone this from within the thread? >>> >>> I don't see why not. >>> >> >> Because another thread may be modifying the memory you are trying to >> clone. If no one modifies the formatData struct, then why is it being >> deep copied to begin with. >> >>> >>> The program dies with a nasty dialog box when restoring a dump of the >>> regression database after the second COPY thread disconnects. >> >> Sounds like the friendly and helpful GPF Dialog (General Protection >> Fault). This is a core dump which strongly suggests your threads are >> trampling over one another. Its possible that a couple threads get >> fired off but upon the first thread completion, something >> !(deep_copied) is freed/modified ... bang-bang your dead :o I tried >> to find this, but haven't yet. >> >> Maybe do a full deep copy in the main thread and comment out any >> in-thread deep copying. I wonder if that would work with no other >> changes. > > I'll try. It's unfortunately not as simple as it sounds, because of the > way the abstractions are arranged. I can't count the number of times I > have had to stop and try to clear my head while working on this code. That's what killed me when I tried to review that stuff and figure it out. Does that indicate that the abstractions are bad and should be changed, or just that there's no reasonably way to make the abstractions both make sense for the internal API itself *and* for being threadsafe? //Magnus
Magnus Hagander <magnus@hagander.net> writes: > Andrew Dunstan wrote: >> I'll try. It's unfortunately not as simple as it sounds, because of the >> way the abstractions are arranged. I can't count the number of times I >> have had to stop and try to clear my head while working on this code. > That's what killed me when I tried to review that stuff and figure it out. > Does that indicate that the abstractions are bad and should be changed, > or just that there's no reasonably way to make the abstractions both > make sense for the internal API itself *and* for being threadsafe? I think pretty much everybody except Philip Warner has found the stuff around the TOC data structure and the "archiver" API to be confusing. I'm not immediately sure about a better design though, at least not if you don't want to duplicate a lot of code between the plain pg_dump and the pg_dump/pg_restore cases. I don't see that this has much of anything to do with thread safety, however --- it's just a matter of too many layers of indirection IMHO. regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Andrew Dunstan wrote: >>> I'll try. It's unfortunately not as simple as it sounds, because of the >>> way the abstractions are arranged. I can't count the number of times I >>> have had to stop and try to clear my head while working on this code. > >> That's what killed me when I tried to review that stuff and figure it out. > >> Does that indicate that the abstractions are bad and should be changed, >> or just that there's no reasonably way to make the abstractions both >> make sense for the internal API itself *and* for being threadsafe? > > I think pretty much everybody except Philip Warner has found the stuff > around the TOC data structure and the "archiver" API to be confusing. > I'm not immediately sure about a better design though, at least not if > you don't want to duplicate a lot of code between the plain pg_dump and > the pg_dump/pg_restore cases. > > I don't see that this has much of anything to do with thread safety, > however --- it's just a matter of too many layers of indirection IMHO. It doesn't - but it makes it harder to find the issue I think :-( If it was reasonably easy, an API redesign might help that. But I haven't looked at all at the possibility of doing so, so I won't comment on if it's likely to be doable. //Magnus
Magnus Hagander wrote: > Tom Lane wrote: >> Magnus Hagander <magnus@hagander.net> writes: >>> Andrew Dunstan wrote: >>>> I'll try. It's unfortunately not as simple as it sounds, because of the >>>> way the abstractions are arranged. I can't count the number of times I >>>> have had to stop and try to clear my head while working on this code. >>> That's what killed me when I tried to review that stuff and figure it out. >>> Does that indicate that the abstractions are bad and should be changed, >>> or just that there's no reasonably way to make the abstractions both >>> make sense for the internal API itself *and* for being threadsafe? >> I think pretty much everybody except Philip Warner has found the stuff >> around the TOC data structure and the "archiver" API to be confusing. >> I'm not immediately sure about a better design though, at least not if >> you don't want to duplicate a lot of code between the plain pg_dump and >> the pg_dump/pg_restore cases. >> >> I don't see that this has much of anything to do with thread safety, >> however --- it's just a matter of too many layers of indirection IMHO. > > It doesn't - but it makes it harder to find the issue I think :-( If it > was reasonably easy, an API redesign might help that. But I haven't > looked at all at the possibility of doing so, so I won't comment on if > it's likely to be doable. > > //Magnus > > If it previously worked without threads, than in theory a deep copy of the thread_arg should fix the core dump; especially if the non-windows fork() method works with this patch. Maybe you can get away with only copying some of the members (trial-n-error), I don't think they are all being used in this context. Nothing should be copied from within thethread itself. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/
Tom Lane wrote: > I think pretty much everybody except Philip Warner has found the stuff > around the TOC data structure and the "archiver" API to be confusing. > I'm not immediately sure about a better design though, at least not if > you don't want to duplicate a lot of code between the plain pg_dump and > the pg_dump/pg_restore cases. > Here was I thinking it was more or less self-documenting and clear ;-). But, yes, it is complex, and I can still see no way to reduce the complexity. I should have some old notes on the code and am happy to expand them as much as necessary. If people want to nominate key areas of confusion, I will concentrate on those first. In terms of the current discussion, I am not sure I can help greatly; writing cross-platform thread code is non-trivial. One minor point: I noticed in early versions of the code that a global AH had been created -- it occurs to me that this could be problem.
Philip Warner wrote: > Tom Lane wrote: > >> I think pretty much everybody except Philip Warner has found the stuff >> around the TOC data structure and the "archiver" API to be confusing. >> I'm not immediately sure about a better design though, at least not if >> you don't want to duplicate a lot of code between the plain pg_dump and >> the pg_dump/pg_restore cases. >> >> > > Here was I thinking it was more or less self-documenting and clear ;-). > But, yes, it is complex, and I can still see no way to reduce the > complexity. I should have some old notes on the code and am happy to > expand them as much as necessary. > > If people want to nominate key areas of confusion, I will concentrate on > those first. > > In terms of the current discussion, I am not sure I can help greatly; > writing cross-platform thread code is non-trivial. One minor point: I > noticed in early versions of the code that a global AH had been created > -- it occurs to me that this could be problem. > > No, it's not. It's not used in any thread except the main thread. cheers andrew
Andrew Chernow wrote: > > If it previously worked without threads, than in theory a deep copy of > the thread_arg should fix the core dump; especially if the non-windows > fork() method works with this patch. Maybe you can get away with only > copying some of the members (trial-n-error), I don't think they are > all being used in this context. Nothing should be copied from within > the thread itself. > I did this, but it turned out that the problem was a logic error that I found once I had managed to get a working debugger. However, the Windows thread code should now be more robust, so thanks to Andrew and Magnus for the suggestions. This version completes properly on Windows with the regression database. Left to do: . improve error checking . memory leak cleanup . code cleanup . docs I hope to have this done shortly. cheers andrew
Вложения
Andrew Dunstan <andrew@dunslane.net> wrote: > I did this, but it turned out that the problem was a logic error that I > found once I had managed to get a working debugger. However, the Windows > thread code should now be more robust, so thanks to Andrew and Magnus > for the suggestions. Hello, I tested parallel restore on Windows. I have some random comments about it: * Two compiler warnings. pg_backup_custom.c: In function `_PrintTocData': pg_backup_custom.c:437: warning: unused variable `ctx' pg_backup_custom.c: In function `_ReopenArchive': pg_backup_custom.c:849: warning: unused variable `ctx' * No description about new options in pg_restore --help. There are no help messages about multi-thread (-m) and truncate-before-load options. * multi-thread option is ignored if --data-only is on. Is it an intended behavior? Even if so, we'd better to have warning messages here. * Threads, forked processes and connections are disposed per entry. I think it's a designed behavior, but there might be room for improvement. The present implementation is slower when there are many small objects. If we can specialize in thread-based implementation, thread pooling and connections pooling are typically used in the context. -- it might be a ToDo item in 8.5. ---- I have no idea about performance because I don't have multi-core machine for windows. Parallel restore seems to be slower than serial restore on single-cpu machine. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
ITAGAKI Takahiro wrote: > Andrew Dunstan <andrew@dunslane.net> wrote: > > >> I did this, but it turned out that the problem was a logic error that I >> found once I had managed to get a working debugger. However, the Windows >> thread code should now be more robust, so thanks to Andrew and Magnus >> for the suggestions. >> > > Hello, I tested parallel restore on Windows. > I have some random comments about it: > Thanks for this > * Two compiler warnings. > pg_backup_custom.c: In function `_PrintTocData': > pg_backup_custom.c:437: warning: unused variable `ctx' > pg_backup_custom.c: In function `_ReopenArchive': > pg_backup_custom.c:849: warning: unused variable `ctx' > Will be fixed in code cleanup > * No description about new options in pg_restore --help. > There are no help messages about multi-thread (-m) and > truncate-before-load options. > Will fix > * multi-thread option is ignored if --data-only is on. > Is it an intended behavior? Even if so, we'd better to have > warning messages here. > Not intended, unless my memory is fading. I will check. > * Threads, forked processes and connections are disposed per entry. > I think it's a designed behavior, but there might be room for > improvement. The present implementation is slower when there > are many small objects. If we can specialize in thread-based > implementation, thread pooling and connections pooling are > typically used in the context. -- it might be a ToDo item in 8.5. > Yes. I only got threading working at all just a few days ago. I think your suggestion is a good one, and we should probably converge on a threaded implementation and then look at using pooling. However, as you say that would be work for the 8.5 timeframe. > ---- > I have no idea about performance because I don't have multi-core > machine for windows. Parallel restore seems to be slower than > serial restore on single-cpu machine. > Not surprising. There is extra connection, worker setup/breakdown, dependency housekeeping and context switching involved. However, I'd be surprised if the overhead were huge. cheers andrew
On Sun, Dec 14, 2008 at 12:13 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > > This version completes properly on Windows with the regression database. > actually, this one doesn't apply cleanly on head -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Jaime Casanova wrote: > On Sun, Dec 14, 2008 at 12:13 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > >> This version completes properly on Windows with the regression database. >> >> > > actually, this one doesn't apply cleanly on head > I will have a new patch a day or two after Christmas, which I hope will be very close to being fully done. cheers andrew