Re: Changeset Extraction v7.9.1

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?

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

> +       if (am_cascading_walsender && !RecoveryInProgress())
> +       {
> +               ereport(LOG,
> +                               (errmsg("terminating walsender process
> to force cascaded standby to update timeline and reconnect")));
> +               walsender_ready_to_stop = true;
> +       }
> 
> Does this apply to logical replication?  Seems like it could at least
> have a comment.

I think it does make sense to force a disconnect in this case to
simplify code, but you're right, both a comment and some TLC for the
message are in order.


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

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services