On Mon, Oct 24, 2011 at 14:40, Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, Oct 24, 2011 at 13:46, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.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.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/