Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
At Tue, 07 Jun 2016 15:38:04 -0400, Tom Lane wrote in <24181.1465328...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > At Mon, 06 Jun 2016 11:12:14 -0400, Tom Lane wrote in > > <17504.1465225...@sss.pgh.pa.us> > >> Uh, what? PQcancel is very carefully coded so that it's safe to use > >> in a signal handler. If it's doing mallocs someplace, that would be > >> surprising. > > > PQcancel is disigned to run in a signal handler on *Linux*, but > > the discussion here is that the equivalent of send/recv and the > > similars on Windows can be blocked by the TerminateThread'ed > > thread via heap lock. > > What's your evidence for that? I'd expect those to be kernel calls, > or whatever the equivalent concept is on Windows. If they are not, > and in particular if they do malloc's, I think we have got problems > in many other contexts besides parallel dump. Well, I found that I misunderstood your following sentence. > Your original suggestion to use write_msg would end up going > through fprintf, which might well use malloc internally. (It's > possible that Windows' version of write() could too, I suppose, > but that's probably as low-level as we are going to get.) I took the sentence enclosed by parentheses as that write() or other syscalls may blocked by heap-lock on Windows. But it should mean that "We have no way than to regard Windows' version of write() as a kernel call, that is, heap-lock safe". It seems quite natural and I totally agree to the judgment. Consequently, I agree to just use write(), not write_msg() and consider the combination of PQcancel and TerminateThread safe on Windows. Sorry for my confusion. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > At Mon, 06 Jun 2016 11:12:14 -0400, Tom Lane wrote in > <17504.1465225...@sss.pgh.pa.us> >> Uh, what? PQcancel is very carefully coded so that it's safe to use >> in a signal handler. If it's doing mallocs someplace, that would be >> surprising. > PQcancel is disigned to run in a signal handler on *Linux*, but > the discussion here is that the equivalent of send/recv and the > similars on Windows can be blocked by the TerminateThread'ed > thread via heap lock. What's your evidence for that? I'd expect those to be kernel calls, or whatever the equivalent concept is on Windows. If they are not, and in particular if they do malloc's, I think we have got problems in many other contexts besides parallel dump. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
At Mon, 06 Jun 2016 11:12:14 -0400, Tom Lane wrote in <17504.1465225...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > At Fri, 03 Jun 2016 09:44:30 -0400, Tom Lane wrote in > > <11515.1464961...@sss.pgh.pa.us> > >> I think that this one: > >>> If the target thread is allocating memory from the heap, the heap > >>> lock will not be released. > >> is potentially a hazard, which is why I made sure to use write_stderr > >> later on in the console interrupt handler. Your original suggestion > >> to use write_msg would end up going through fprintf, which might well > >> use malloc internally. (It's possible that Windows' version of write() > >> could too, I suppose, but that's probably as low-level as we are > >> going to get.) > > > I have to admit that I forgot about the possible malloc's, and > > PQcancel() can be blocked from the same reason. > > Uh, what? PQcancel is very carefully coded so that it's safe to use > in a signal handler. If it's doing mallocs someplace, that would be > surprising. PQcancel is disigned to run in a signal handler on *Linux*, but the discussion here is that the equivalent of send/recv and the similars on Windows can be blocked by the TerminateThread'ed thread via heap lock. > > If the issue to be settled here is the unwanted error messages, > > we could set a flag to instruct write_msg to sit silent. Anyway > > the workers should have been dead that time so discarding any > > error messages don't matter. > > What do you think about this? > > This is really ugly and I'm unconvinced that it fixes anything. > write_msg is hardly the only place in a worker thread that might > be doing malloc's; moreover, preventing workers from entering it > after we start a shutdown does nothing for workers that might be > in it already. Yeah, it's ugly. But if we assume write() can be blocked, the similar system calls used within PQcancel can be blocked, too. If we don't assume so, using write instead of write_msg would do. The problem I think is we don't have (or they don't offer?) enough knowlegde about the inside of Windows APIs. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > At Fri, 03 Jun 2016 09:44:30 -0400, Tom Lane wrote in > <11515.1464961...@sss.pgh.pa.us> >> I think that this one: >>> If the target thread is allocating memory from the heap, the heap >>> lock will not be released. >> is potentially a hazard, which is why I made sure to use write_stderr >> later on in the console interrupt handler. Your original suggestion >> to use write_msg would end up going through fprintf, which might well >> use malloc internally. (It's possible that Windows' version of write() >> could too, I suppose, but that's probably as low-level as we are >> going to get.) > I have to admit that I forgot about the possible malloc's, and > PQcancel() can be blocked from the same reason. Uh, what? PQcancel is very carefully coded so that it's safe to use in a signal handler. If it's doing mallocs someplace, that would be surprising. > If the issue to be settled here is the unwanted error messages, > we could set a flag to instruct write_msg to sit silent. Anyway > the workers should have been dead that time so discarding any > error messages don't matter. > What do you think about this? This is really ugly and I'm unconvinced that it fixes anything. write_msg is hardly the only place in a worker thread that might be doing malloc's; moreover, preventing workers from entering it after we start a shutdown does nothing for workers that might be in it already. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
How about silencing the workers on termination? # Build on Windows (with VC?) is very time consuming... At Fri, 03 Jun 2016 09:44:30 -0400, Tom Lane wrote in <11515.1464961...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > For sure, any of the "dangers" of TerminateThread don't matter > > for this case. > > I think that this one: > > >> If the target thread is allocating memory from the heap, the heap > >> lock will not be released. > > is potentially a hazard, which is why I made sure to use write_stderr > later on in the console interrupt handler. Your original suggestion > to use write_msg would end up going through fprintf, which might well > use malloc internally. (It's possible that Windows' version of write() > could too, I suppose, but that's probably as low-level as we are > going to get.) I have to admit that I forgot about the possible malloc's, and PQcancel() can be blocked from the same reason. What is worse, even if we managed to avoid this particular disaster, the MSDN page says the problems are *for example*. So if we don't allow any possible unknown blocking on ctrl-C termination and want to forcibly terminate workers, we might have to use process instead of thread for worker entity. (This might reduce the difference related to WIN32..) We also might be able to cause immediate process termination for the second Ctrl-C but this seems to lead to other issues. If the issue to be settled here is the unwanted error messages, we could set a flag to instruct write_msg to sit silent. Anyway the workers should have been dead that time so discarding any error messages don't matter. What do you think about this? Timeout for select still seems to be needless. regards, -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index fb48a02..b74a396 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -147,7 +147,11 @@ static DWORD tls_index; /* globally visible variables (needed by exit_nicely) */ bool parallel_init_done = false; -DWORD mainThreadId; +bool is_terminating = false; +static DWORD mainThreadId; +static bool handler_active = false; +static DWORD handlerThreadId; + #endif /* WIN32 */ static const char *modulename = gettext_noop("parallel archiver"); @@ -180,9 +184,26 @@ static char *readMessageFromPipe(int fd); /* - * Shutdown callback to clean up socket access + * Return true if I am the main thread + * + * This function regards the console handler thread as the main thread. We + * need a separate boolean for validity of the handlerThreadId since an + * invalid value for thread id is not defined. */ #ifdef WIN32 +bool +am_main_thread(void) +{ + DWORD current_thread_id = GetCurrentThreadId(); + + return (mainThreadId == current_thread_id || + (handler_active && + handlerThreadId == current_thread_id)); +} + +/* + * Shutdown callback to clean up socket access + */ static void shutdown_parallel_dump_utils(int code, void *unused) { @@ -190,7 +211,7 @@ shutdown_parallel_dump_utils(int code, void *unused) if (mainThreadId == GetCurrentThreadId()) WSACleanup(); } -#endif +#endif /* ifdef WIN32 */ /* * Initialize parallel dump support --- should be called early in process @@ -390,6 +411,8 @@ ShutdownWorkersHard(ParallelState *pstate) * On Windows, send query cancels directly to the workers' backends. Use * a critical section to ensure worker threads don't change state. */ + is_terminating = true; /* Silence workers */ + EnterCriticalSection(&signal_info_lock); for (i = 0; i < pstate->numWorkers; i++) { @@ -602,6 +625,15 @@ consoleHandler(DWORD dwCtrlType) if (dwCtrlType == CTRL_C_EVENT || dwCtrlType == CTRL_BREAK_EVENT) { + /* + * We don't forcibly terminate workes. Instead, these + * variables make them silent ever after. This handler thread + * is regarded as the main thread. + */ + is_terminating = true; + handlerThreadId = GetCurrentThreadId(); + handler_active = true; + /* Critical section prevents changing data we look at here */ EnterCriticalSection(&signal_info_lock); @@ -642,6 +674,8 @@ consoleHandler(DWORD dwCtrlType) write_msg(NULL, "terminated by user\n"); } + handler_active = false; + /* Always return FALSE to allow signal handling to continue */ return FALSE; } diff --git a/src/bin/pg_dump/parallel.h b/src/bin/pg_dump/parallel.h index 21739ca..59fba33 100644 --- a/src/bin/pg_dump/parallel.h +++ b/src/bin/pg_dump/parallel.h @@ -64,11 +64,11 @@ typedef struct ParallelState #ifdef WIN32 extern bool parallel_init_done; -extern DWORD mainThreadId; +extern bool is_terminating; #endif extern void init_parallel_dump_utils(void); - +extern bool am_main_thread(void); extern int GetIdleWorker(ParallelState *pstate); extern bool IsEveryWorkerIdle(ParallelState *pstate); extern void ListenToWorkers(ArchiveHandle *AH, ParallelState *pstate, bool do_wait); diff --git a/src/bin/pg
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > For sure, any of the "dangers" of TerminateThread don't matter > for this case. I think that this one: >> If the target thread is allocating memory from the heap, the heap >> lock will not be released. is potentially a hazard, which is why I made sure to use write_stderr later on in the console interrupt handler. Your original suggestion to use write_msg would end up going through fprintf, which might well use malloc internally. (It's possible that Windows' version of write() could too, I suppose, but that's probably as low-level as we are going to get.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
At Thu, 2 Jun 2016 12:17:11 -0400, Alvaro Herrera wrote in <20160602161711.GA239156@alvherre.pgsql> > Tom Lane wrote: > > Kyotaro HORIGUCHI writes: > > > Apart from the invalid snapshot problem, I looked the patch > > > previously mentioned mainly for Windows. > > > > Thanks for looking! > > > > > Even though the threads started by beginthread cannot be > > > terminated cleanly from outside, but the whole process will soon > > > terminate anyway, so we could use TreminateThread. This seems > > > working. (Attached patch) > > > > Seems reasonable to me; I was unhappy about the lack of any direct > > equivalent to the child SIGTERMs that the Unix code does. For sure, any of the "dangers" of TerminateThread don't matter for this case. https://msdn.microsoft.com/en-us/em-us/library/windows/desktop/ms686717(v=vs.85).aspx > If the target thread owns a critical section, the critical > section will not be released. > > If the target thread is allocating memory from the heap, the heap > lock will not be released. > > If the target thread is executing certain kernel32 calls when it > is terminated, the kernel32 state for the thread's process could > be inconsistent. > > If the target thread is manipulating the global state of a shared > DLL, the state of the DLL could be destroyed, affecting other > users of the DLL. > Given this testing, it's clear that the timeout on select() is useless; > we could get rid of it in vacuumdb.c too. I'll post a patch later. Agreed. Command pipes may be in blocking mode for the case. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Tom Lane wrote: > Kyotaro HORIGUCHI writes: > > Apart from the invalid snapshot problem, I looked the patch > > previously mentioned mainly for Windows. > > Thanks for looking! > > > Even though the threads started by beginthread cannot be > > terminated cleanly from outside, but the whole process will soon > > terminate anyway, so we could use TreminateThread. This seems > > working. (Attached patch) > > Seems reasonable to me; I was unhappy about the lack of any direct > equivalent to the child SIGTERMs that the Unix code does. Given this testing, it's clear that the timeout on select() is useless; we could get rid of it in vacuumdb.c too. I'll post a patch later. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > Apart from the invalid snapshot problem, I looked the patch > previously mentioned mainly for Windows. Thanks for looking! > Even though the threads started by beginthread cannot be > terminated cleanly from outside, but the whole process will soon > terminate anyway, so we could use TreminateThread. This seems > working. (Attached patch) Seems reasonable to me; I was unhappy about the lack of any direct equivalent to the child SIGTERMs that the Unix code does. > Is there any reason for the name "ourAH" not to be "myAH"? Don't much care, I'll change it. > setup_cancel_handler looks somewhat bizarre. It eventually works > only for the main process/thread and does nothing for workers. It > is enough to be run once before forking in ParalleBackupStart and > that makes handler_set unnecessary. No, because we also want it to work in non-parallel cases. As coded, we'll establish the handler whenever set_archive_cancel_info is first called, which will be in the first ConnectDatabase() call. It would be possible to do that someplace else maybe, but it would require more code changes than just attaching it to set_archive_cancel_info. > In EndDBCopyMode, the result of PQgetResult is abandoned. This > can leak memory and such usage is not seen elsewhere in the > source tree. Shouldn't we hold the result and PQclear it? (Mainly > as a convention, not for benefit.) There should never be any non-null result, so that seems like a waste of code to me. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Apart from the invalid snapshot problem, I looked the patch previously mentioned mainly for Windows. At Tue, 31 May 2016 12:29:50 -0400, Tom Lane wrote in <7445.1464712...@sss.pgh.pa.us> > In the patch I posted yesterday, I reversed the order of those two > steps, which should fix this problem in most scenarios: > https://www.postgresql.org/message-id/7005.1464657...@sss.pgh.pa.us Entering ctrl-C while parallel pg_dump is running, on my console I saw a repetition of the following error message, which is not seen on Linux thanks to forcible termination of worker processes in sigTermHandler(). > pg_dump: Dumping the contents of table "t" failed: PQgetResult() failed. > pg_dump: Error message from server: ERROR: canceling statement due to user > request > pg_dump: The command was: COPY public.t (a) TO stdout; We could provide a global flag (wantAbort?) that inform the canceling of queries but it needs adding checks for it everywhere. Even though the threads started by beginthread cannot be terminated cleanly from outside, but the whole process will soon terminate anyway, so we could use TreminateThread. This seems working. (Attached patch) We might be allowed to do exit() in colsoleHandler(). Other questions follow. Is there any reason for the name "ourAH" not to be "myAH"? setup_cancel_handler looks somewhat bizarre. It eventually works only for the main process/thread and does nothing for workers. It is enough to be run once before forking in ParalleBackupStart and that makes handler_set unnecessary. In EndDBCopyMode, the result of PQgetResult is abandoned. This can leak memory and such usage is not seen elsewhere in the source tree. Shouldn't we hold the result and PQclear it? (Mainly as a convention, not for benefit.) regards, -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index cf97d9c..fb48a02 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -618,6 +618,12 @@ consoleHandler(DWORD dwCtrlType) { ArchiveHandle *AH = signal_info.pstate->parallelSlot[i].args->AH; +/* + * Using TerminateThread here may leave some resources leaked + * but whole this process will soon die. + */ +TerminateThread(signal_info.pstate->parallelSlot[i].hThread, 0); + if (AH != NULL && AH->connCancel != NULL) (void) PQcancel(AH->connCancel, errbuf, sizeof(errbuf)); } @@ -632,6 +638,8 @@ consoleHandler(DWORD dwCtrlType) errbuf, sizeof(errbuf)); LeaveCriticalSection(&signal_info_lock); + + write_msg(NULL, "terminated by user\n"); } /* Always return FALSE to allow signal handling to continue */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
At Tue, 31 May 2016 12:29:50 -0400, Tom Lane wrote in <7445.1464712...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > At Fri, 27 May 2016 13:20:20 -0400, Tom Lane wrote in > > <14603.1464369...@sss.pgh.pa.us> > >> Kyotaro HORIGUCHI writes: > >>> By the way, the reason of the "invalid snapshot identifier" is > >>> that some worker threads try to use it after the connection on > >>> the first worker closed. > > >> ... BTW, I don't quite see what the issue is there. > > > The master session died from lack of libz and the failure of > > compressLevel's propagation already fixed. Some of the children > > that started transactions after the master's death will get the > > error. > > I don't think I believe that theory, because it would require the master > to not notice the lack of libz before it launches worker processes, but > instead while the workers are working. The master actually *didn't* notice the lack of libz until it launces worker processes before cae2bb1. So the current master don't suffer the problem, but it is not desirable that sudden death from any reason of a child causes this kind of behavior. > But AFAICS, while there are worker > processes open, the master does nothing except wait for workers and > dispatch new jobs to them; it does no database work of its own. So the > libz-isn't-there error has to have occurred in one of the workers. Yes, the firstly-commanded worker dies from that then the master disconencts its connection owning the snapshot before terminating any other workers. It occurs with the current master (9ee56df) minus cae2bb1, having --without-zlib at configure. > > If we want prevent it perfectly, one solution could be that > > non-master children explicitly wait the master to arrive at the > > "safe" state before starting their transactions. But I suppose it > > is not needed here. > > Actually, I believe the problem is in archive_close_connection, around > line 295 in HEAD: once the master realizes that one child has failed, > it first closes its own database connection and only second tries to kill > the remaining children. So there's a race condition wherein remaining > children have time to see the missing-snapshot error. Agreed. > In the patch I posted yesterday, I reversed the order of those two > steps, which should fix this problem in most scenarios: > https://www.postgresql.org/message-id/7005.1464657...@sss.pgh.pa.us Yeah, just transposing DisconnectDatabase and ShutdownWorkersHard in archive_close_connection fixed the problem. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > At Fri, 27 May 2016 13:20:20 -0400, Tom Lane wrote in > <14603.1464369...@sss.pgh.pa.us> >> Kyotaro HORIGUCHI writes: >>> By the way, the reason of the "invalid snapshot identifier" is >>> that some worker threads try to use it after the connection on >>> the first worker closed. >> ... BTW, I don't quite see what the issue is there. > The master session died from lack of libz and the failure of > compressLevel's propagation already fixed. Some of the children > that started transactions after the master's death will get the > error. I don't think I believe that theory, because it would require the master to not notice the lack of libz before it launches worker processes, but instead while the workers are working. But AFAICS, while there are worker processes open, the master does nothing except wait for workers and dispatch new jobs to them; it does no database work of its own. So the libz-isn't-there error has to have occurred in one of the workers. > If we want prevent it perfectly, one solution could be that > non-master children explicitly wait the master to arrive at the > "safe" state before starting their transactions. But I suppose it > is not needed here. Actually, I believe the problem is in archive_close_connection, around line 295 in HEAD: once the master realizes that one child has failed, it first closes its own database connection and only second tries to kill the remaining children. So there's a race condition wherein remaining children have time to see the missing-snapshot error. In the patch I posted yesterday, I reversed the order of those two steps, which should fix this problem in most scenarios: https://www.postgresql.org/message-id/7005.1464657...@sss.pgh.pa.us regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Hello, At Fri, 27 May 2016 13:20:20 -0400, Tom Lane wrote in <14603.1464369...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > By the way, the reason of the "invalid snapshot identifier" is > > that some worker threads try to use it after the connection on > > the first worker closed. > > ... BTW, I don't quite see what the issue is there. The snapshot is > exported from the master session, so errors in worker sessions should not > cause such failures in other workers. And I don't see any such failure > when setting up a scenario that will cause a worker to fail on Linux. > The "invalid snapshot identifier" bleats would make sense if you had > gotten a server-side error (and transaction abort) in the master session, > but I don't see any evidence that that happened in that example. Might be > worth seeing if that's reproducible. The master session died from lack of libz and the failure of compressLevel's propagation already fixed. Some of the children that started transactions after the master's death will get the error. Similary, sudden close of the session of the master child at very early in its transaction could cause the same symptom but it seems not likely if master surely arrives at command-waiting, or "safe", state. If we want prevent it perfectly, one solution could be that non-master children explicitly wait the master to arrive at the "safe" state before starting their transactions. But I suppose it is not needed here. Does this make sense? regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Amit Kapila writes: > On Sat, May 28, 2016 at 5:06 AM, Tom Lane wrote: >> It looks like the vacuumdb.c version of this code actually is tied >> into an interrupt handler, but whoever copied it for parallel.c just >> ripped out the CancelRequested checks, making the looping behavior >> pretty useless. > It seems to me that CancelRequested checks were introduced in vacuumdb.c as > part of commit a1792320 and select_loop for parallel.c version exists from > commit 9e257a18 which got committed earlier. Huh, interesting. I wonder how parallel.c's select_loop got to be like that then? The git history offers no clue: it is essentially the same as HEAD as far back as the initial commit of parallel.c. It certainly looks like someone intended to introduce a cancel check and never did, or had one and took it out without simplifying the rest of the logic. Anyway, AFAICS the time limit on select() wait is completely useless in the code as it stands; but we'll likely want to add a cancel check there, so ripping it out wouldn't be a good plan. I'll change the added comment. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
On Sat, May 28, 2016 at 5:06 AM, Tom Lane wrote: > > Alvaro Herrera writes: > > Regarding this: > >> + * XXX this certainly looks useless, why not just wait indefinitely? > > > There's another select_loop() in vacuumdb.c suggesting that the timeout > > is used to check for cancel requests; as I understood while working on > > the vacuumdb changes, select() is not interrupted in that case. I > > suppose that means it's necessary here also. But on the other hand it's > > quite possible that the original developer just copied what was in > > pg_dump and that it's not actually needed; if that's the case, perhaps > > it's better to rip it out from both places. > > Ah, interesting. That ties into something else I was wondering about, > which is how we could get useful control-C cancellation on Windows. > It looks like the vacuumdb.c version of this code actually is tied > into an interrupt handler, but whoever copied it for parallel.c just > ripped out the CancelRequested checks, making the looping behavior > pretty useless. > It seems to me that CancelRequested checks were introduced in vacuumdb.c as part of commit a1792320 and select_loop for parallel.c version exists from commit 9e257a18 which got committed earlier. I think control-C handling for Windows in parallel.c is missing or if there is some way to deal with it, clearly it is not same as what we do in vacuumdb.c. > For pg_restore (parallel or not) it would be useful if the program > didn't just fall over at control-C but actually sent cancel requests > to the backend(s). It's not such a problem if we're transferring > data, but if we're waiting for some slow operation like CREATE INDEX, > the current behavior isn't very good. On the Unix side we have some > SIGINT infrastructure there already, but I don't see any for Windows. > > So now I'm thinking we should leave that alone, with the expectation > that we'll be putting CancelRequested checks back in at some point. > > > https://www.postgresql.org/message-id/20150122174601.gb1...@alvh.no-ip.org > > Hmm, did the patch you're discussing there get committed? > Yes, it was committed - a1792320 With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Alvaro Herrera writes: > Regarding this: >> + * XXX this certainly looks useless, why not just wait >> indefinitely? > There's another select_loop() in vacuumdb.c suggesting that the timeout > is used to check for cancel requests; as I understood while working on > the vacuumdb changes, select() is not interrupted in that case. I > suppose that means it's necessary here also. But on the other hand it's > quite possible that the original developer just copied what was in > pg_dump and that it's not actually needed; if that's the case, perhaps > it's better to rip it out from both places. Ah, interesting. That ties into something else I was wondering about, which is how we could get useful control-C cancellation on Windows. It looks like the vacuumdb.c version of this code actually is tied into an interrupt handler, but whoever copied it for parallel.c just ripped out the CancelRequested checks, making the looping behavior pretty useless. For pg_restore (parallel or not) it would be useful if the program didn't just fall over at control-C but actually sent cancel requests to the backend(s). It's not such a problem if we're transferring data, but if we're waiting for some slow operation like CREATE INDEX, the current behavior isn't very good. On the Unix side we have some SIGINT infrastructure there already, but I don't see any for Windows. So now I'm thinking we should leave that alone, with the expectation that we'll be putting CancelRequested checks back in at some point. > https://www.postgresql.org/message-id/20150122174601.gb1...@alvh.no-ip.org Hmm, did the patch you're discussing there get committed? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Regarding this: > *** select_loop(int maxFd, fd_set *workerset > *** 1092,1104 > --- 1150,1160 > fd_set saveSet = *workerset; > > #ifdef WIN32 > for (;;) > { > /* >* sleep a quarter of a second before checking if we should > terminate. > + * XXX this certainly looks useless, why not just wait > indefinitely? >*/ > struct timeval tv = {0, 25}; > There's another select_loop() in vacuumdb.c suggesting that the timeout is used to check for cancel requests; as I understood while working on the vacuumdb changes, select() is not interrupted in that case. I suppose that means it's necessary here also. But on the other hand it's quite possible that the original developer just copied what was in pg_dump and that it's not actually needed; if that's the case, perhaps it's better to rip it out from both places. https://www.postgresql.org/message-id/20150122174601.gb1...@alvh.no-ip.org -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
I wrote: >> BTW, after having spent the afternoon staring at it, I will assert with >> confidence that pg_dump/parallel.c is an embarrassment to the project. > I've done a bit of work on a cosmetic cleanup patch, but don't have > anything to show yet. Attached is the threatened cosmetic cleanup of parallel.c. As I went through it, I found quite a few things not to like, but in this patch I've mostly resisted the temptation to fix them immediately, and have just tried to document what's there more accurately. Aside from a major amount of comment-rewriting and a very small amount of cosmetic code adjustment (mostly moving code for more clarity), this patch changes these things: * Rename SetupWorker() to RunWorker() to reflect what it actually does, and remove its unused "worker" argument. * Rename lockTableNoWait() to lockTableForWorker() for clarity, and move the test for BLOBS into it instead of having an Assert that the caller checked that. * Don't bother with getThreadLocalPQExpBuffer() at all in non-Windows builds; it was identical to getLocalPQExpBuffer() anyway, except for being misleadingly named. * Remove some completely-redundant or otherwise ill-considered Asserts. * Fix incorrect "Assert(msgsize <= bufsize)" --- should be < bufsize. * Fix missing socket close in one error exit from pgpipe(). This isn't too exciting at present since we'll just curl up and die if it fails, but might as well get it right. I have some other, less-cosmetic, things I want to do here, but first does anyone care to review this? regards, tom lane diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index c656ba5..1a52fae 100644 *** a/src/bin/pg_dump/parallel.c --- b/src/bin/pg_dump/parallel.c *** *** 2,21 * * parallel.c * ! * Parallel support for the pg_dump archiver * * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * The author is not responsible for loss or damages that may - * result from its use. - * * IDENTIFICATION * src/bin/pg_dump/parallel.c * *- */ #include "postgres_fe.h" #include "parallel.h" --- 2,62 * * parallel.c * ! * Parallel support for pg_dump and pg_restore * * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION * src/bin/pg_dump/parallel.c * *- */ + /* + * Parallel operation works like this: + * + * The original, master process calls ParallelBackupStart(), which forks off + * the desired number of worker processes, which each enter WaitForCommands(). + * + * The master process dispatches an individual work item to one of the worker + * processes in DispatchJobForTocEntry(). That calls + * AH->MasterStartParallelItemPtr, a routine of the output format. This + * function's arguments are the parents archive handle AH (containing the full + * catalog information), the TocEntry that the worker should work on and a + * T_Action value indicating whether this is a backup or a restore task. The + * function simply converts the TocEntry assignment into a command string that + * is then sent over to the worker process. In the simplest case that would be + * something like "DUMP 1234", with 1234 being the TocEntry id. + * + * The worker process receives and decodes the command and passes it to the + * routine pointed to by AH->WorkerJobDumpPtr or AH->WorkerJobRestorePtr, + * which are routines of the current archive format. That routine performs + * the required action (dump or restore) and returns a malloc'd status string. + * The status string is passed back to the master where it is interpreted by + * AH->MasterEndParallelItemPtr, another format-specific routine. That + * function can update state or catalog information on the master's side, + * depending on the reply from the worker process. In the end it returns a + * status code, which is 0 for successful execution. + * + * Remember that we have forked off the workers only after we have read in + * the catalog. That's why our worker processes can also access the catalog + * information. (In the Windows case, the workers are threads in the same + * process. To avoid problems, they work with cloned copies of the Archive + * data structure; see init_spawned_worker_win32().) + * + * In the master process, the workerStatus field for each worker has one of + * the following values: + * WRKR_IDLE: it's waiting for a command + * WRKR_WORKING: it's been sent a command + * WRKR_FINISHED: it's returned a result + * WRKR_TERMINATED: process ended + * The FINISHED state indicates that the worker is idle, but we'
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > By the way, the reason of the "invalid snapshot identifier" is > that some worker threads try to use it after the connection on > the first worker closed. ... BTW, I don't quite see what the issue is there. The snapshot is exported from the master session, so errors in worker sessions should not cause such failures in other workers. And I don't see any such failure when setting up a scenario that will cause a worker to fail on Linux. The "invalid snapshot identifier" bleats would make sense if you had gotten a server-side error (and transaction abort) in the master session, but I don't see any evidence that that happened in that example. Might be worth seeing if that's reproducible. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
At Thu, 26 May 2016 10:53:47 -0400, Tom Lane wrote in <5237.1464274...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > >> Next, I got the following behavior for the following command, > >> then freeze. Maybe stopping at the same point with the next > >> paragraph but I'm not sure. The same thing occurs this patch on > >> top of the current master but doesn't on Linux. > > > [ need to close command sockets in ShutdownWorkersHard ] > > Hah. I had made that same change in the cosmetic-cleanups patch I was > working on ... but I assumed it wasn't a live bug or we'd have heard > about it. > > Pushed along with the comment improvements I'd made to that function. Thank you! regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
At Thu, 26 May 2016 12:15:29 -0400, Tom Lane wrote in <8273.1464279...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > At Wed, 25 May 2016 10:11:28 -0400, Tom Lane wrote in > > <24577.1464185...@sss.pgh.pa.us> > >> The only case > >> that is certain to work is switches before non-switch arguments, and so > >> we should not give any documentation examples in the other order. On a > >> platform using GNU getopt(), getopt() will rearrange the argv[] array to > >> make such cases work, but non-GNU getopt() doesn't do that (and I don't > >> really trust the GNU version not to get confused, either). > > > Yeah, I knew it after reading port/getopt_long.c. But the error > > message seems saying nothing about that (at least to me). And > > those accumstomed to the GNU getopt's behavior will fail to get > > the point of the message. The documentation also doesn't > > explicitly saying about the restriction; it is only implicitly > > shown in synopsis. How about something like the following > > message? (The patch attached looks too dependent on the specific > > behavior of our getopt_long.c, but doing it in getopt_long.c also > > seems to be wrong..) > > It's not a bad idea to try to improve our response to this situation, > but I think this patch needs more work. I wonder in particular why > you're not basing the variant error message on the first unprocessed > argument starting with '-', rather than looking at the word before it. Sorry, it just comes from my carelessness. It is correct to do what you wrote as above. And it is also the cause of my obscure error message. > Another thought is that the message is fairly unhelpful because it does > not show where in the arguments things went wrong. Maybe something > more like > > if (argv[optind][0] == '-') > fprintf(stderr, _("%s: misplaced option \"%s\": options must > appear before non-option arguments\n"), > progname, argv[optind]); > else > // existing message > > In any case, if we wanted to do something about this scenario, we should > do it consistently across all our programs, not just pg_dump. I count > 25 appearances of that "too many command-line arguments" message... Although I suppose no one has ever complained before about that, and the same thing will occur on the other new programs even if the programs are fixed now.. I'll consider more on it for some time.. Thank you for the suggestion. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > At Wed, 25 May 2016 10:11:28 -0400, Tom Lane wrote in > <24577.1464185...@sss.pgh.pa.us> >> The only case >> that is certain to work is switches before non-switch arguments, and so >> we should not give any documentation examples in the other order. On a >> platform using GNU getopt(), getopt() will rearrange the argv[] array to >> make such cases work, but non-GNU getopt() doesn't do that (and I don't >> really trust the GNU version not to get confused, either). > Yeah, I knew it after reading port/getopt_long.c. But the error > message seems saying nothing about that (at least to me). And > those accumstomed to the GNU getopt's behavior will fail to get > the point of the message. The documentation also doesn't > explicitly saying about the restriction; it is only implicitly > shown in synopsis. How about something like the following > message? (The patch attached looks too dependent on the specific > behavior of our getopt_long.c, but doing it in getopt_long.c also > seems to be wrong..) It's not a bad idea to try to improve our response to this situation, but I think this patch needs more work. I wonder in particular why you're not basing the variant error message on the first unprocessed argument starting with '-', rather than looking at the word before it. Another thought is that the message is fairly unhelpful because it does not show where in the arguments things went wrong. Maybe something more like if (argv[optind][0] == '-') fprintf(stderr, _("%s: misplaced option \"%s\": options must appear before non-option arguments\n"), progname, argv[optind]); else // existing message In any case, if we wanted to do something about this scenario, we should do it consistently across all our programs, not just pg_dump. I count 25 appearances of that "too many command-line arguments" message... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: >> Next, I got the following behavior for the following command, >> then freeze. Maybe stopping at the same point with the next >> paragraph but I'm not sure. The same thing occurs this patch on >> top of the current master but doesn't on Linux. > [ need to close command sockets in ShutdownWorkersHard ] Hah. I had made that same change in the cosmetic-cleanups patch I was working on ... but I assumed it wasn't a live bug or we'd have heard about it. Pushed along with the comment improvements I'd made to that function. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
> Sounds reasonable. I look into this further. I looked into that and found one problem in the patch. > Next, I got the following behavior for the following command, > then freeze. Maybe stopping at the same point with the next > paragraph but I'm not sure. The same thing occurs this patch on > top of the current master but doesn't on Linux. This occurs in the following steps. 1. One of the workers dies from some reason. (InitCompressorZlib immediately goes into exit_horribly in this case) 2. The main thread detects in ListenToWorkers that the worker is dead. 3. ListenToWorkers calls exit_horribly then exit_nicely 4. exit_nicely calls archive_close_connection as a callback then the callback calls ShutdownWorkersHard 5. ShutdownWorkersHard should close the write side of the pipe but the patch skips it for WIN32. So, the attached patch on top the patch fixes that, that is, pg_dump returns to command prompt even for the case. By the way, the reason of the "invalid snapshot identifier" is that some worker threads try to use it after the connection on the first worker closed. Some of the workers succesfully did before the connection closing and remained listening to their master to inhibit termination of the entire process. regards, -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index f650d3f..6c08426 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -308,7 +308,6 @@ checkAborting(ArchiveHandle *AH) static void ShutdownWorkersHard(ParallelState *pstate) { -#ifndef WIN32 int i; /* @@ -318,6 +317,7 @@ ShutdownWorkersHard(ParallelState *pstate) for (i = 0; i < pstate->numWorkers; i++) closesocket(pstate->parallelSlot[i].pipeWrite); +#ifndef WIN32 for (i = 0; i < pstate->numWorkers; i++) kill(pstate->parallelSlot[i].pid, SIGTERM); #else -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
At Wed, 25 May 2016 10:11:28 -0400, Tom Lane wrote in <24577.1464185...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > I tried it on Windows 7/64 but first of all, I'm surprised to see > > that the following command line gets an error but it would be > > fine because it is consistent with what is written in the manual. > > > | >pg_dump postgres://horiguti:hoge@localhost/postgres --jobs=9 -Fd -f > > testdump > > | pg_dump: too many command-line arguments (first is "--jobs=9") > > | Try "pg_dump --help" for more information. > > Where do you see an example like that? We should fix it. Sorry for the confusing sentence. The command line came from your first mail starting this thread:p And "consistent with manual" means that the command line results in an error (even only on Windows) since it is difrerent from the document. No such example has been seen in the documentation AFAICS. https://www.postgresql.org/message-id/2458.1450894...@sss.pgh.pa.us https://www.postgresql.org/docs/9.6/static/app-pgdump.html > The only case > that is certain to work is switches before non-switch arguments, and so > we should not give any documentation examples in the other order. On a > platform using GNU getopt(), getopt() will rearrange the argv[] array to > make such cases work, but non-GNU getopt() doesn't do that (and I don't > really trust the GNU version not to get confused, either). Yeah, I knew it after reading port/getopt_long.c. But the error message seems saying nothing about that (at least to me). And those accumstomed to the GNU getopt's behavior will fail to get the point of the message. The documentation also doesn't explicitly saying about the restriction; it is only implicitly shown in synopsis. How about something like the following message? (The patch attached looks too dependent on the specific behavior of our getopt_long.c, but doing it in getopt_long.c also seems to be wrong..) | >pg_dump hoge -f | pg_dump: non-option arguments should not precede options. > > I might be wrong with something, but pg_dump seems to have some > > issues in thread handling. > > Wouldn't surprise me ... there's a lot of code in there depending on > static variables, and we've only tried to thread-proof a few. Still, > I think that's a separate issue from what this patch is addressing. Sounds reasonable. I look into this further. I see no other functional problem in this patch. regards, -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 1267afb..52e9094 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -546,9 +546,13 @@ main(int argc, char **argv) /* Complain if any arguments remain */ if (optind < argc) { - fprintf(stderr, _("%s: too many command-line arguments (first is \"%s\")\n"), -progname, argv[optind]); - fprintf(stderr, _("Try \"%s --help\" for more information.\n"), + if (optind > 0 && argv[optind - 1][0] != '-') + fprintf(stderr, _("%s: non-option arguments should not precede options.\n"), + progname); + else + fprintf(stderr, _("%s: too many command-line arguments (first is \"%s\")\n"), + progname, argv[optind]); + fprintf(stderr, _("Non-Try \"%s --help\" for more information.\n"), progname); exit_nicely(1); } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > I tried it on Windows 7/64 but first of all, I'm surprised to see > that the following command line gets an error but it would be > fine because it is consistent with what is written in the manual. > | >pg_dump postgres://horiguti:hoge@localhost/postgres --jobs=9 -Fd -f > testdump > | pg_dump: too many command-line arguments (first is "--jobs=9") > | Try "pg_dump --help" for more information. Where do you see an example like that? We should fix it. The only case that is certain to work is switches before non-switch arguments, and so we should not give any documentation examples in the other order. On a platform using GNU getopt(), getopt() will rearrange the argv[] array to make such cases work, but non-GNU getopt() doesn't do that (and I don't really trust the GNU version not to get confused, either). > I might be wrong with something, but pg_dump seems to have some > issues in thread handling. Wouldn't surprise me ... there's a lot of code in there depending on static variables, and we've only tried to thread-proof a few. Still, I think that's a separate issue from what this patch is addressing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
At Wed, 25 May 2016 00:15:57 -0400, Tom Lane wrote in <3149.1464149...@sss.pgh.pa.us> > Kyotaro HORIGUCHI writes: > > Shouldn't archive_close_connection have an assertion (si->AHX != > > NULL) instead of checking "if (si->AHX)" in each path? > > I thought about that but didn't see any really good argument for it. > It'd be making that function dependent on the current behavior of > unrelated code, when it doesn't need to be. It's also fine with me. I tried it on Windows 7/64 but first of all, I'm surprised to see that the following command line gets an error but it would be fine because it is consistent with what is written in the manual. | >pg_dump postgres://horiguti:hoge@localhost/postgres --jobs=9 -Fd -f testdump | pg_dump: too many command-line arguments (first is "--jobs=9") | Try "pg_dump --help" for more information. Next, I got the following behavior for the following command, then freeze. Maybe stopping at the same point with the next paragraph but I'm not sure. The same thing occurs this patch on top of the current master but doesn't on Linux. | >pg_dump -d postgres --jobs=9 -Fd -f testdump | Password: | pg_dump: [archiver] WARNING: requested compression not available in this installation -- archive will be uncompressed | pg_dump: [compress_io] not built with zlib support | pg_dump: [parallel archiver] a worker process died unexpectedly | pg_dump: [archiver (db)] query failed: ERROR: invalid snapshot identifier: "2299-1" | pg_dump: [archiver (db)] query was: SET TRANSACTION SNAPSHOT '2299-1' | pg_dump: [archiver (db)] query failed: ERROR: invalid snapshot identifier: "2299-1" | pg_dump: [archiver (db)] query was: SET TRANSACTION SNAPSHOT '2299-1' | pg_dump: [archiver (db)] query failed: ERROR: invalid snapshot identifier: "2299-1" | pg_dump: [archiver (db)] query was: SET TRANSACTION SNAPSHOT '2299-1' | pg_dump: [archiver (db)] query failed: ERROR: invalid snapshot identifier: "2299-1" | pg_dump: [archiver (db)] query was: SET TRANSACTION SNAPSHOT '2299-1' | pg_dump: [archiver (db)] query failed: ERROR: invalid snapshot identifier: "2299-1" | pg_dump: [archiver (db)] query was: SET TRANSACTION SNAPSHOT '2299-1' | pg_dump: [archiver (db)] query failed: ERROR: invalid snapshot identifier: "2299-1" | pg_dump: [archiver (db)] query was: SET TRANSACTION SNAPSHOT '2299-1' Third, I'm not sure on this detail, but pg_dump shows the following message then freeze until Ctrl-C. I think that I forgot to set password to the user for the time. It doesn't seem to occur for this patch on top of the current master. | >pg_dump --jobs=9 -Fd -f testdump "postgres://horiguti:hoge@localhost/postgres" | pg_dump: [archiver] WARNING: requested compression not available in this installation -- archive will be uncompressed | pg_dump: [compress_io] not built with zlib support | pg_dump: [parallel archiver] a worker process died unexpectedly | ^C | > The main thread was stopping at WaitForMultiplObjects() around parallel.c:361(@master + this patch) but I haven't investigated it any more. Finally, I got the following expected result, which seems sane. | >pg_dump --jobs=9 -Fd -f testdump "postgres://horiguti:hoge@localhost/postgres" | pg_dump: [archiver] WARNING: requested compression not available in this installation -- archive will be uncompressed | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied | pg_dump: [parallel archiver] a worker process died unexpectedly | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied | pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth:no password supplied I might be wrong with something, but pg_dump seems to have some issues in thread handling. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Kyotaro HORIGUCHI writes: > Shouldn't archive_close_connection have an assertion (si->AHX != > NULL) instead of checking "if (si->AHX)" in each path? I thought about that but didn't see any really good argument for it. It'd be making that function dependent on the current behavior of unrelated code, when it doesn't need to be. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
Hello, # Note for convenience for others: The commit fixing the original # issue is 1aa41e3eae3746e05d0e23286ac740a9a6cee7df. At Mon, 23 May 2016 13:40:30 -0400, Tom Lane wrote in <1336.1464025...@sss.pgh.pa.us> > I wrote: > >> ... the pg_dump process has crashed with a SIGPIPE without printing > >> any message whatsoever, and without coming anywhere near finishing the > >> dump. > > > Attached is a proposed patch for this. It reverts exit_horribly() to > > what it used to be pre-9.3, ie just print the message on stderr and die. > > The master now notices child failure by observing EOF on the status pipe. > > While that works automatically on Unix, we have to explicitly close the > > child side of the pipe on Windows (could someone check that that works?). > > I also fixed the parent side to ignore SIGPIPE earlier, so that it won't > > just die if it was in the midst of sending to the child. > > Now that we're all back from PGCon ... does anyone wish to review this? > Or have an objection to treating it as a bug fix and patching all branches > back to 9.3? FWIW, it seems to me a bug which should be fixed to back branches. I tried this only on Linux (I'll try it on Windows afterward) and got something like this. pg_dump: [archiver (db)] pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth: no password supplied connection to database "postgres" failed: fe_sendauth: no password supplied pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth: no password supplied pg_dump: [parallel archiver] a worker process died unexpectedly pg_dump: [archiver (db)] connection to database "postgres" failed: fe_sendauth: no password supplied Although the error messages from multiple children are cluttered, it would be inevitable and better than vanishing. It seems hard to distinguish the meaning among the module names enclosed by '[]' but it is another issue. In archive_close_connection, the following change means that si->AHX could be NULL there, as the existing code for non-parallel does. But it seems to be set once for si(=shutdown_info)'s lifetime, before reaching there, to a valid value. - DisconnectDatabase(si->AHX); + if (si->AHX) + DisconnectDatabase(si->AHX); Shouldn't archive_close_connection have an assertion (si->AHX != NULL) instead of checking "if (si->AHX)" in each path? > > BTW, after having spent the afternoon staring at it, I will assert with > > confidence that pg_dump/parallel.c is an embarrassment to the project. > > I've done a bit of work on a cosmetic cleanup patch, but don't have > anything to show yet. > > regards, tom lane regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
I wrote: >> ... the pg_dump process has crashed with a SIGPIPE without printing >> any message whatsoever, and without coming anywhere near finishing the >> dump. > Attached is a proposed patch for this. It reverts exit_horribly() to > what it used to be pre-9.3, ie just print the message on stderr and die. > The master now notices child failure by observing EOF on the status pipe. > While that works automatically on Unix, we have to explicitly close the > child side of the pipe on Windows (could someone check that that works?). > I also fixed the parent side to ignore SIGPIPE earlier, so that it won't > just die if it was in the midst of sending to the child. Now that we're all back from PGCon ... does anyone wish to review this? Or have an objection to treating it as a bug fix and patching all branches back to 9.3? > BTW, after having spent the afternoon staring at it, I will assert with > confidence that pg_dump/parallel.c is an embarrassment to the project. I've done a bit of work on a cosmetic cleanup patch, but don't have anything to show yet. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
[ getting back to a complaint I made in December ] I wrote: > ... the pg_dump process has crashed with a SIGPIPE without printing > any message whatsoever, and without coming anywhere near finishing the > dump. > A bit of investigation says that this is because somebody had the bright > idea that worker processes could report fatal errors back to the master > process instead of just printing them to stderr. So when the workers > fail to establish connections (because of the password problem cited in > #13727), they don't tell me about that. Oh no, they send those errors > back up to the pipe to the parent, and then die silently. Meanwhile, > the parent is trying to send them commands, and since it doesn't protect > itself against SIGPIPE on the command pipes, it crashes without ever > reporting anything. If you aren't paying close attention, you wouldn't > even realize you didn't get a completed dump. Attached is a proposed patch for this. It reverts exit_horribly() to what it used to be pre-9.3, ie just print the message on stderr and die. The master now notices child failure by observing EOF on the status pipe. While that works automatically on Unix, we have to explicitly close the child side of the pipe on Windows (could someone check that that works?). I also fixed the parent side to ignore SIGPIPE earlier, so that it won't just die if it was in the midst of sending to the child. BTW, after having spent the afternoon staring at it, I will assert with confidence that pg_dump/parallel.c is an embarrassment to the project. It is chock-full of misleading, out-of-date, and/or outright wrong comments, useless or even wrong Asserts, ill-designed APIs, code that works quite differently in Unix and Windows cases without a shred of commentary about the fact, etc etc. I have mostly resisted the temptation to do cosmetic cleanup in the attached, but this code really needs some editorial attention. regards, tom lane diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index 9167294..f650d3f 100644 *** a/src/bin/pg_dump/parallel.c --- b/src/bin/pg_dump/parallel.c *** static ShutdownInformation shutdown_info *** 77,84 static const char *modulename = gettext_noop("parallel archiver"); static ParallelSlot *GetMyPSlot(ParallelState *pstate); - static void parallel_msg_master(ParallelSlot *slot, const char *modulename, - const char *fmt, va_list ap) pg_attribute_printf(3, 0); static void archive_close_connection(int code, void *arg); static void ShutdownWorkersHard(ParallelState *pstate); static void WaitForTerminatingWorkers(ParallelState *pstate); --- 77,82 *** GetMyPSlot(ParallelState *pstate) *** 163,227 } /* - * Fail and die, with a message to stderr. Parameters as for write_msg. - * - * This is defined in parallel.c, because in parallel mode, things are more - * complicated. If the worker process does exit_horribly(), we forward its - * last words to the master process. The master process then does - * exit_horribly() with this error message itself and prints it normally. - * After printing the message, exit_horribly() on the master will shut down - * the remaining worker processes. - */ - void - exit_horribly(const char *modulename, const char *fmt,...) - { - va_list ap; - ParallelState *pstate = shutdown_info.pstate; - ParallelSlot *slot; - - va_start(ap, fmt); - - if (pstate == NULL) - { - /* Not in parallel mode, just write to stderr */ - vwrite_msg(modulename, fmt, ap); - } - else - { - slot = GetMyPSlot(pstate); - - if (!slot) - /* We're the parent, just write the message out */ - vwrite_msg(modulename, fmt, ap); - else - /* If we're a worker process, send the msg to the master process */ - parallel_msg_master(slot, modulename, fmt, ap); - } - - va_end(ap); - - exit_nicely(1); - } - - /* Sends the error message from the worker to the master process */ - static void - parallel_msg_master(ParallelSlot *slot, const char *modulename, - const char *fmt, va_list ap) - { - char buf[512]; - int pipefd[2]; - - pipefd[PIPE_READ] = slot->pipeRevRead; - pipefd[PIPE_WRITE] = slot->pipeRevWrite; - - strcpy(buf, "ERROR "); - vsnprintf(buf + strlen("ERROR "), - sizeof(buf) - strlen("ERROR "), fmt, ap); - - sendMessageToMaster(pipefd, buf); - } - - /* * A thread-local version of getLocalPQExpBuffer(). * * Non-reentrant but reduces memory leakage. (On Windows the memory leakage --- 161,166 *** getThreadLocalPQExpBuffer(void) *** 271,277 /* * pg_dump and pg_restore register the Archive pointer for the exit handler ! * (called from exit_horribly). This function mainly exists so that we can * keep shutdown_info in file scope only. */ void --- 210,216 /* * pg_dump and pg_restore register the Archive pointer for the exit handler ! * (called from exit_nicely). This function mainly exi
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
On 12/23/2015 10:16 AM, Tom Lane wrote: Depending on timing, this scheme might accidentally fail to fail, but it seems fragile as can be. I would bet that it's prone to deadlocks, quite aside from the SIGPIPE problem. Considering how amazingly ugly the underlying code is (exit_horribly is in parallel.c now? Really?), I want to rip it out entirely, not try to band-aid it by suppressing SIGPIPE --- though likely we need to do that too. Thoughts? This is something that should work, period. It should be a showcase for the code we ship because it shows how serious we take data integrity (backups/restore etc...). +1 for turning it into something irrefutably as close to perfect as humans can produce. Sincerely, JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Centered full stack support, consulting and development. Announcing "I'm offended" is basically telling the world you can't control your own emotions, so everyone else should do it for you. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers