Re: Updated version of pg_receivexlog

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?

> 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/