Hi,
a few comments about the last version of the patch:
1) LogicalDecodeMessageCB
Do we actually need the 'transactional' parameter here? I mean, having
the 'txn' should be enough, as
transactional = (txt != NULL)
Of course, having a simple flag is more convenient.
2) pg_logical_emit_message_bytea / pg_logical_emit_message_text
The comment before _bytea is wrong - it's just a copy'n'paste from the
preceding function (pg_logical_slot_peek_binary_changes). _text has no
comment at all, but it's true it's just a simple _bytea wrapper.
The main issue here however is that the functions are not defined as
strict, but ignore the possibility that the parameters might be NULL. So
for example this crashes the backend
SELECT pg_logical_emit_message(NULL::boolean, NULL::text, NULL::text);
3) ReorderBufferQueueMessage
No comment. Not a big deal I guess, the method is simple enough, but why
to break the rule when all the other methods around have at least a
short one?
4) ReorderBufferChange
The new struct in the 'union' would probably deserve at least a short
comment explaining the purpose (just like the other structs around).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services