Thread: parallel restore
Latest patch is attached. Changed as discussed to issue TRUNCATE ... ONLY when talking to servers >= 8.4 instead of plain TRUNCATE. cheers andrew
Attachment
Andrew Dunstan <andrew@dunslane.net> writes: > Latest patch is attached. Starting to look at this now. One thing that is bothering me is that if the connection parameters are such as to cause prompts for passwords, it's going to be broken beyond usability (multiple threads all trying to read the terminal at once). Is there anything we can do about that? If not, we've at least got to warn people to avoid it in the manual. Also, how does this interact with single_txn mode? I suspect that's just not very sane at all and we should forbid the combination. regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Latest patch is attached. >> > > Starting to look at this now. Excellent! > One thing that is bothering me is that > if the connection parameters are such as to cause prompts for passwords, > it's going to be broken beyond usability (multiple threads all trying > to read the terminal at once). Is there anything we can do about that? > If not, we've at least got to warn people to avoid it in the manual. > I thought I had put in changes to cache the password, so you shouldn't get multiple prompts. That's one reason that we make sure we connect in the main thread before we ever fork/spawn children. > Also, how does this interact with single_txn mode? I suspect that's > just not very sane at all and we should forbid the combination. > Yes. I thought I had done that too, will check. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > Tom Lane wrote: >> One thing that is bothering me is that >> if the connection parameters are such as to cause prompts for passwords, >> it's going to be broken beyond usability (multiple threads all trying >> to read the terminal at once). Is there anything we can do about that? > I thought I had put in changes to cache the password, so you shouldn't > get multiple prompts. Ah, you can tell I hadn't gotten to the bottom of the patch yet ;-). Still, that's not a 100% solution because of the cases where we use reconnections to change user IDs --- the required password would (usually) vary. It might be sufficient to forbid that case with parallel restore, though; I think it's mostly a legacy thing anyway. >> Also, how does this interact with single_txn mode? > Yes. I thought I had done that too, will check. Yeah, found that too. regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> One thing that is bothering me is that >>> if the connection parameters are such as to cause prompts for passwords, >>> it's going to be broken beyond usability (multiple threads all trying >>> to read the terminal at once). Is there anything we can do about that? >>> > > >> I thought I had put in changes to cache the password, so you shouldn't >> get multiple prompts. >> > > Ah, you can tell I hadn't gotten to the bottom of the patch yet ;-). > Still, that's not a 100% solution because of the cases where we use > reconnections to change user IDs --- the required password would > (usually) vary. It might be sufficient to forbid that case with > parallel restore, though; I think it's mostly a legacy thing anyway. > > I didn't know such a thing even existed. What causes it to happen? I agree it should be forbidden. cheers andrew
Okay, another question --- there are two places in pg_backup_custom.c where the patch #ifdef's out hasSeek tests on WIN32. Why is that? If checkSeek() is wrong on Windows, wouldn't it be better to fix it? regards, tom lane
Tom Lane wrote: > Okay, another question --- there are two places in pg_backup_custom.c > where the patch #ifdef's out hasSeek tests on WIN32. Why is that? > If checkSeek() is wrong on Windows, wouldn't it be better to fix it? > > > Oh, dear. That's a hangover from before that got fixed earlier this month. checkSeek() should now work. I will make sure it does and let you know. cheers andrew
Andrew Dunstan wrote: > > > Tom Lane wrote: >> Okay, another question --- there are two places in pg_backup_custom.c >> where the patch #ifdef's out hasSeek tests on WIN32. Why is that? >> If checkSeek() is wrong on Windows, wouldn't it be better to fix it? >> > > Oh, dear. That's a hangover from before that got fixed earlier this > month. checkSeek() should now work. I will make sure it does and let > you know. > > Here is a new patch. Changes: * above #ifdefs removed * fixed declaration of DeClone() * brought up to date with CVS. successfully tested on Windows. cheers andrew
Attachment
Andrew Dunstan wrote: >> Still, that's not a 100% solution because of the cases where we use >> reconnections to change user IDs --- the required password would >> (usually) vary. It might be sufficient to forbid that case with >> parallel restore, though; I think it's mostly a legacy thing anyway. > > I didn't know such a thing even existed. What causes it to happen? I > agree it should be forbidden. It was the only way to switch users before we had SET SESSION AUTHORIZATION and SET ROLE and such. But the pg_restore man page says that -R/--no-reconnect is obsolete, so I'm not sure what the current behavior really is.
Peter Eisentraut <peter_e@gmx.net> writes: > Andrew Dunstan wrote: >> I didn't know such a thing even existed. What causes it to happen? I >> agree it should be forbidden. > It was the only way to switch users before we had SET SESSION > AUTHORIZATION and SET ROLE and such. But the pg_restore man page says > that -R/--no-reconnect is obsolete, so I'm not sure what the current > behavior really is. Yeah, I think I was remembering ancient history. AFAICT we now never do a reconnect with anything but the originally specified username. I thought for a bit about stripping out the apparent flexibility to use other names, and making these low-level functions just consult ropt->username for themselves. But we might regret that someday. What's probably better is to have them notice whether the argument is ropt->username, and only attempt to cache the password if so. I'm almost done reviewing the patch, and will send along an updated version shortly. regards, tom lane
I wrote: > I'm almost done reviewing the patch, and will send along an updated > version shortly. And here 'tis. I didn't commit because I have no way to test whether I broke the Windows code path. Please test, and commit if OK. There is an unfinished TODO item here: we really ought to make it work for tar-format archives. That's probably not hugely difficult, but I didn't look into it, and don't think we should hold up applying the existing patch for it. regards, tom lane
Attachment
Tom Lane wrote: > I wrote: > >> I'm almost done reviewing the patch, and will send along an updated >> version shortly. >> > > And here 'tis. Many many thanks. Your edits look very sensible, as always. > I didn't commit because I have no way to test whether > I broke the Windows code path. Please test, and commit if OK. > Will do. > There is an unfinished TODO item here: we really ought to make it work > for tar-format archives. That's probably not hugely difficult, but > I didn't look into it, and don't think we should hold up applying the > existing patch for it. > > > Right. Were you thinking this should be done for 8.4? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > Tom Lane wrote: >> There is an unfinished TODO item here: we really ought to make it work >> for tar-format archives. That's probably not hugely difficult, but >> I didn't look into it, and don't think we should hold up applying the >> existing patch for it. > Right. Were you thinking this should be done for 8.4? If you have time to look into it, sure. Otherwise we should just put it on the TODO list. regards, tom lane
Tom Lane wrote: > And here 'tis. I didn't commit because I have no way to test whether > I broke the Windows code path. Please test, and commit if OK. > > Tested and committed. Thanks to the people who reviewed and tested this - it was quite a difficult piece of work, much more difficult than I originally expected. cheers andrew
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> There is an unfinished TODO item here: we really ought to make it work >>> for tar-format archives. That's probably not hugely difficult, but >>> I didn't look into it, and don't think we should hold up applying the >>> existing patch for it. >>> > > >> Right. Were you thinking this should be done for 8.4? >> > > If you have time to look into it, sure. Otherwise we should just put it > on the TODO list. > > > I've had a look at this. If our tar code supported out of order restoration(using fseeko) I'd be done. But it doesn't, and I won't get that done for 8.4, if at all. I'm not sure what would be involved in making it work. cheers andrew
I wrote: > > > Tom Lane wrote: >> Andrew Dunstan <andrew@dunslane.net> writes: >> >>> Tom Lane wrote: >>> >>>> There is an unfinished TODO item here: we really ought to make it work >>>> for tar-format archives. That's probably not hugely difficult, but >>>> I didn't look into it, and don't think we should hold up applying the >>>> existing patch for it. >>>> >> >> >>> Right. Were you thinking this should be done for 8.4? >>> >> >> If you have time to look into it, sure. Otherwise we should just put it >> on the TODO list. >> >> >> > > I've had a look at this. If our tar code supported out of order > restoration(using fseeko) I'd be done. But it doesn't, and I won't get > that done for 8.4, if at all. I'm not sure what would be involved in > making it work. > > OK, I've spent some more time on this. pg_dump when writing a custom format file writes out the header and table of contents and then the data members, keeping track of where each one starts. If the output is seekable (as it usually is) it then rewrites the table of contents, this time including the data member offsets. Parallel restore requires that this offset info be available, and if the pg_dump output file was not seekable by pg_dump (e.g. if it was a pipe) then it will be unsuitable for use with parallel restore, which will fail. In the case of tar output, pg_dump doesn't make any effort to keep the offset info at all, so parallel restore is not currently suitable for use with tar output, regardless of whether or not the pg_dump output was seekable. I think we could cure both of these cases by having pg_dump write out a second copy of the table of contents, including data member offsets, at the end of the archive. Or it might just be a table of <data-member-id, offset> pairs if we're worried about space. In the latter case we'd need to go back and fix up the TOC, but that would be fairly simple. Either way I think we'd need to bump the archive version number so we'd know when to expect this. Once we have that the custom format code should fail on this no matter how the dump was made, and parallel restore should work with tar format once we add code to it to seek for data members. I think all of this can wait to 8.5, except that we should possibly document a bit more completely the current limitations on parallel restore. (I was initially tempted to say we'd need compression of individual data members in tar format to do this sanely, but since the offsets-at-the-end suggestion should work even when pg_dump is outputting to a pipe, we'd still be able to send the output through gzip and so get a conventional .tgz file.) cheers andrew
I wrote: > > > Once we have that the custom format code should fail on this no matter > how the dump was made, and parallel restore should work with tar > format once we add code to it to seek for data members. > > s/should fail/should not fail/ :-) cheers andrew
Andrew Dunstan wrote: > > > Tom Lane wrote: > > Andrew Dunstan <andrew@dunslane.net> writes: > > > >> Tom Lane wrote: > >> > >>> There is an unfinished TODO item here: we really ought to make it work > >>> for tar-format archives. That's probably not hugely difficult, but > >>> I didn't look into it, and don't think we should hold up applying the > >>> existing patch for it. > >>> > > > > > >> Right. Were you thinking this should be done for 8.4? > >> > > > > If you have time to look into it, sure. Otherwise we should just put it > > on the TODO list. > > > > > > > > I've had a look at this. If our tar code supported out of order > restoration(using fseeko) I'd be done. But it doesn't, and I won't get > that done for 8.4, if at all. I'm not sure what would be involved in > making it work. Added to TODO: Allow parallel restore of tar dumps * http://archives.postgresql.org/pgsql-hackers/2009-02/msg01154.php -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +