Re: Standby trying "restore_command" before local WAL

On 8/8/18 11:45 AM, Stephen Frost wrote:
> 
> * Tomas Vondra (tomas.vondra@2ndquadrant.com) wrote:
>> On 08/08/2018 04:08 PM, David Steele wrote:
>>> On 8/7/18 12:05 PM, Stephen Frost wrote:
>>>>> All I'm saying is that (assuming my understanding of RestoreArchivedFile is
>>>>> correct) we can't just do that in the current restore_command. We do need a
>>>>> way to ask the archive for some metadata/checksums, and restore_command is
>>>>> too late.
>>>>
>>>> Yeah, I see what you mean there.  An alternative might be to *not*
>>>> unlink the file, in case the restore command wants to check if it's
>>>> valid and matches what's in the archive, and instead restore the
>>>> requested file somewhere else and then perform an unlink/mv after
>>>> the restore command has been run.
>>> I don't see any cases (in master, at least) where RestoredArchivedFile()
>>> would unlink an existing WAL segment.  If you look at the call sites
>>> they all pass "RECOVERYHISTORY" or "RECOVERYXLOG"  to the recovername
>>> parameter.
>>>
>>> RestoreArchivedFile() does overwrite the existing file after we decide
>>> that we prefer the restored version over the one in pg_wal, but that
>>> happens after restore_command returns.
>>>
>>> So as far as I can see, it could be possible for the restore_command to
>>> do something clever here.
>>
>> Oh, I see - I really was reading RestoredArchivedFile() wrong ...
>>
>> Yes, it seems possible to do this in restore_command. Currently it'd require
>> constructing the local segment path (replacing RECOVERYXLOG with the actual
>> segment filename), but that's solvable by adding a new pattern similar to
>> %p.
> 
> Nice, but I don't know that there's any need to add another %-flag,
> really.  David, do you think we'd really need that?

%p is already available for restore_command, so the path from %p and the
segment name from %f would do the trick.

>> I'm not sure if packing all this into a single restore_command is a good
>> idea, because I'm pretty sure it'll lead to abhorrently monstrous bash
>> commands jammed into the restore_command. But then again, we kinda already
>> have that, and all archival systems already provide simple and correct
>> commands doing the right thing. And extending that would not be a big deal.
> 
> Running a bash snippet as restore_command is a bad idea already, imv.

Agreed.  Archiving is more complicated than backup in many ways and a
good implementation is far from trivial.

> The real PG backup tools already have much more complicated restore
> commands written in C, Perl, or Python, and could definitely implement
> this.  There's still a bit of a question as to if it'd really be worth
> it, but perhaps it would be in certain cases.

I can see cases where it *might* be worth it, but several backup tools
support prefetch and/or parallelism which should be able to keep
Postgres fed with WAL unless there is very high latency to the repo.
I'm not sure the small performance boost (if any) would be enough to
justify the extra code paths and tests.

That said, if there is anything we can do in core to make these tools
simpler or more performant I'm all for it.  For instance, it might be
good to have return code (as suggested upstream) that indicates to
Postgres that the segment in pg_wal is a good choice.   Some use cases
might benefit enormously even if it doesn't make sense as a general case.

Regards,
-- 
-David
david@pgmasters.net