Re: Changeset Extraction v7.9.1

On Tue, Mar 4, 2014 at 6:26 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-03-03 16:48:15 -0500, Robert Haas wrote:
>> OK, I've committed the 0001 patch, which is the core of this feature,
>> with a bit of minor additional hacking.
>
> Attached are the rebased patches that are remaining.
>
> Changes:
> * minor conflict due to 7558cc95d31edb
> * removal of the last XXX in the walsender patch by setting the
> timestamps in the 'd' messages correctly.
> * Some documentation wordsmithing by Craig
>
> The walsender patch currently contains the changes about feedback we
> argued about elsewhere, I guess I either need to back them out, or we
> need to argue out that minor bit.

OK, reading through the walsender patch (0002 in this series):

PLEASE stop using a comma to join two independent thoughts. Don't do
it in the comments, and definitely don't do it in error messages. I'm
referring to things like this: "invalid value for option
\"replication\", legal values are false, 0, true, 1 or database". I
know that you're not a native English speaker, and if you were
submitting a smaller amount of code I wouldn't just fix it for you,
but you do this A LOT and I've probably fixed a hundred instances of
it already and I can't cope with fixing another hundred. In code
comments, a semicolon is often an adequate substitute, but that even
with that change this won't do for an error message. For that, you
should copy the style of something done elsewhere. For example, in
this instance, perhaps look to this precedent:

rhaas=# set synchronous_commit = barfle;
ERROR: invalid value for parameter "synchronous_commit": "barfle"
HINT: Available values: local, remote_write, on, off.

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

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

+ /*
+ * XXX: For feedback purposes it would be nicer to set sentPtr to
+ * cmd->startpoint, but we use it to know where to read xlog in the main
+ * loop...
+ */

I'm not sure I understand this.

WalSndWriteData() looks kind of cut-and-pasty.

WalSndWaitForWal() is yet another slightly-modified copy of the same
darn loop. Surely we need a better way of doing this. It's
absolutely inevitable that some future hacker will not patch every
copy of this loop in some situation where that is required.

There might be more; that's all I see at the moment.

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