Re: Changeset Extraction v7.9.1

On Wed, Mar 5, 2014 at 3:04 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> Hi,
>
> On 2014-03-05 13:49:23 -0500, Robert Haas wrote:
>> PLEASE stop using a comma to join two independent thoughts.
>
> Ok. I'll try.
>
> Is this a personal preference, or a general rule? There seems to be a
> fair amount of comments in pg doing so?

http://en.wikipedia.org/wiki/Comma_splice

>> This patch still treats "allow a walsender to connect to a database"
>> as a separate feature from "allow logical replication".  I'm not
>> convinced that's a good idea. What you're proposing to do is allow
>> replication=database in addition to replication=true and
>> replication=false.  But how about instead allowing
>> replication=physical and replication=logical?  "physical" can just be
>> a synonym for "true" and the database name can be ignored as it is
>> today.  "logical" can pay attention the database name.  I'm not
>> totally wedded to that exact design, but basically, I'm not
>> comfortable with allowing a physical WAL sender to connect to a
>> database in advance of a concrete need.  We might want to leave some
>> room to go there later if we think it's a likely direction, but
>> allowing people to do it in advance of any functional advantage just
>> seems like a recipe for bugs.  Practically nobody will run that way so
>> breakage won't be timely detected.  (And no, I don't know exactly what
>> will break.)
>
> I am only mildly against doing so, so you certainly can nudge me in that
> direction.
> Would you want to refuse using existing commands in logical mode?  It's not
> unrealistic to first want to perform a basebackup and then establish a
> logical slot to replay from there on. It's probably not too bad to force
> separate connections there, but it seems like a somewhwat pointless
> exercise to me?

Hmm, that's an interesting point.  I didn't consider the case of a
base backup followed by replication, on the same connection.  That
might be sufficient justification for doing it the way you have it.

>> WalSndWriteData() looks kind of cut-and-pasty.
>
> You mean from the WalSndLoop? Yea. I tried to reduce it by introducing
> WalSndCheckTimeOut() but I think at the very least
> WalSndComputeTimeOut() is in order.
>
> I very much dislike having the three different event loops, but it's
> pretty much forced by the design of the xlogreader. "My" xlogreader
> version didn't block when it neeeded to wait for WAL but just returned
> "need input/output", but with the eventually committed version you're
> pretty much forced to block inside the read_page callback.
>
> I don't really have a idea how we could sensibly unify them atm.

WalSndLoop(void (*gutsfn)())?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company