On 09/09/16 06:33, Peter Eisentraut wrote:
Review of 0003-Define-logical-replication-protocol-and-output-plugi.patch:
(This is still based on the Aug 31 patch set, but at quick glance I
didn't see any significant changes in the Sep 8 set.)
Yep.
The start_replication option pg_version option is not documented and
not used in any later patch. We can probably do without it and just
rely on the protocol version.
That's leftover from binary type data transfer which is not part of this
initial approach as it adds a lot of complications to both protocol and
apply side. So yes can do without.
In pgoutput_startup(), you check opt->output_type. But it is not set
anywhere. Actually, the startup callback is supposed to set it
itself.
Leftover from pglogical which actually supports both output types.
In init_rel_sync_cache(), the way hash_flags is set seems kind of
weird. I think that variable could be removed and the flags put
directly into the hash_create() call.
Eh, yes no idea how that came to be.
pgoutput_config.c seems over-engineered, e.g., converting cstring to
Datum and back. Just do normal DefElem list parsing in pgoutput.c.
That's not pretty either, but at least it's a common coding pattern.
Yes now that we have only couple of options I agree.
In the protocol documentation, explain the meaning of int64 as a
commit timestamp.
You mean that it's milliseconds since postgres epoch?
On the actual protocol messages:
Why do strings have a length byte? That is not how other strings in
the protocol work. As a minor side-effect, this would limit for
example column names to 255 characters.
Because I originally sent them without the null termination but I guess
they don't really need it anymore. (the 255 char limit is not really
important in practice given the column length is limited to 64
characters anyway)
The message structure doesn't match the documentation in some ways.
For example Attributes and TupleData are not separate messages but are
contained in Relation and Insert/Update/Delete messages. So the
documentation needs to be structured a bit differently.
In the Attributes message (or actually Relation message), we don't
need the 'A' and 'C' bytes.
Hmm okay will look into it. I guess if we remove the 'A' then rest of
the Attribute message neatly merges into the Relation message. The more
interesting part will be the TupleData as it's common part of other
messages.
I'm not sure that pgoutput should concern itself with the client
encoding. The client encoding should already be set by the initial
FE/BE protocol handshake. I haven't checked that further yet, so it
might already work, or it should be made to work that way, or I might
be way off.
Yes, I think you are right, that was there mostly for same reason as the
pg_version.
Slight abuse of pqformat functions. We're not composing messages
using pq_beginmessage()/pq_endmessage(), and we're not using
pq_getmsgend() when reading. The "proper" way to do this is probably
to define a custom set of PQcommMethods. (low priority)
If we change that, I'd probably rather go with direct use of StringInfo
functions.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
