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?
> s/IDENFITY_SYSTEM/IDENTIFY_SYSTEM/ (two occurrences)
Oops.
> 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.
> 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.
> synchronous_standby_names='*' is prone to such confusion in general, but it
> seems that it's particularly surprising if a running pg_basebackup lets a
> commit in synchronous replication to proceed. Maybe we just need a warning
> in the docs. I think we should advise that synchronous_standby_names='*' is
> dangerous in general, and cite this as one reason for that.
Hmm. i think this is common enough that we want to make sure we avoid
it in code.
Could we pass a parameter from the client indicating to the master
that it refuses to be a sync slave? An optional keyword to the
START_REPLICATION command, perhaps?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/