Re: Updated version of pg_receivexlog

On Tue, Oct 25, 2011 at 12:37, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Mon, Oct 24, 2011 at 14:40, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> On Mon, Oct 24, 2011 at 13:46, Heikki Linnakangas
>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>>> +               /*
>>>> +                * Looks like an xlog file. Parse it's position.
>>>
>>> s/it's/its/
>>>
>>>> +                */
>>>> +               if (sscanf(dirent->d_name, "%08X%08X%08X", &tli, &log,
>>>> &seg) != 3)
>>>> +               {
>>>> +                       fprintf(stderr, _("%s: could not parse xlog
>>>> filename \"%s\"\n"),
>>>> +                                       progname, dirent->d_name);
>>>> +                       disconnect_and_exit(1);
>>>> +               }
>>>> +               log *= XLOG_SEG_SIZE;
>>>
>>> That multiplication by XLOG_SEG_SIZE could overflow, if logid is very high.
>>> It seems completely unnecessary, anyway,
>>
>> How do you mean completely unnecessary? We'd have to change the points
>> that use it to divide by XLOG_SEG_SIZE otherwise, no? That might be a
>> way to get around the overflow, but I'm not sure that's what you mean?
>
> Talked to Heikki on IM about this one, turns out we were both wrong.
> It's needed, but there was a bug hiding under it, due to (once again)
> mixing up segments and offsets. Has been fixed now.
>
>>> In pg_basebackup, it would be a good sanity check to check that the systemid
>>> returned by IDENTIFY_SYSTEM in the main connection and the WAL-streaming
>>> connection match. Just to be sure that some connection pooler didn't hijack
>>> one of the connections and point to a different server. And better check
>>> timelineid too while you're at it.
>>
>> That's a good idea. Will fix.
>
> Added to the new version of the patch.
>
>
>>> How does this interact with synchronous replication? If a base backup that
>>> streams WAL is in progress, and you have synchronous_standby_names set to
>>> '*', I believe the in-progress backup will count as a standby for that
>>> purpose. That might give a false sense of security.
>>
>> Ah yes. Did not think of that. Yes, it will have this problem.
>
> Actually, thinking more, per other mail, it won't. Because it will
> never report that the data is synced to disk, so it will not be
> considered for sync standby.
>
> This is something we might consider in the future (it could be a
> reasonable scenario where you had this), but not in the first version.
>
> Updated version of the patch attached.

I've applied this version with a few more minor changes that Heikki found.

His comment about .partial files still applies, and I intend to
address this in a follow-up commit, along with some further
documentation enhancements.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/