Re: replication protocol documentation inconsistencies - Mailing list pgsql-hackers

On Wed, May 28, 2014 at 6:51 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-05-21 07:29:53 -0400, Peter Eisentraut wrote:
>> Looking at
>> http://www.postgresql.org/docs/devel/static/protocol-replication.html
>> under START_REPLICATION it goes
>>
>> """
>> The payload of each CopyData message from server to the client contains
>> a message of one of the following formats:
>>
>> If a slot's name is provided via slotname, it will be updated as
>> replication progresses so that the server knows which WAL segments - and
>> if hot_standby_feedback is on which transactions - are still needed by
>> the standby.
>>
>> XLogData (B)
>> ...
>>
>> Primary keepalive message (B)
>> ...
>> """
>>
>> That second paragraph was inserted recently and doesn't make sense
>> there.  It should be moved somewhere else.
>
> Hm. I am not sure why it doesn't make sense there? It's about the SLOT
> $slotname parameter to START_REPLICATION?

I think it would probably read better if we added that into the first
paragraph about START_REPLICATION, instead of having it down at the
end.  i.e. "Instructs server to start streaming WAL, starting at WAL
position XXX/XXX. If TIMELINE option is specified, streaming starts on
timeline tli; otherwise, the server's current timeline is selected.
The server can reply with an error, e.g. if the requested section of
WAL has already been recycled.  On success, server responds with a
CopyBothResponse message, and then starts to stream WAL to the
frontend.  If a slot's name is provided via slotname, it will be
updated as replication progresses so that the server knows which WAL
segments - and if hot_standby_feedback is on which transactions - are
still needed by the standby."

This bit here:

"The payload of each CopyData message from server to the client
contains a message of one of the following formats:"

...is followed by a colon and needs to immediately proceed the list to
which it refers.

>> More generally, it is weird that the message formats are described
>> there, even though the rest of the protocol documentation only mentions
>> the messages by name and then describes the formats later
>> (http://www.postgresql.org/docs/devel/static/protocol-message-types.html
>> and
>> http://www.postgresql.org/docs/devel/static/protocol-message-formats.html).
>>  For example, the meaning of the "(B)" code isn't described until two
>> sections later.
>
> I am not sure that makes sense. These messages cannot be sent as
> toplevel messages - they're just describing the contents of the CopyBoth
> stream after START_REPLICATION has begun. It seems wierd to add these
> 'subprotocol' messages to the toplevel protocol description.

I see your point, but I think Peter has a good point, too.  It would
be weird to document this mini-protocol mixed in with the main
protocol, and it's so short that adding a separate section for it
hardly makes sense, but it's still strange to have the mini-protocol
being documented before we've explained our conventions for how we
document wire protocol messages.  Forward references are best avoided,
especially implicit ones.

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