Обсуждение: 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

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

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

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. +