Re: Proposal: Generic WAL logical messages

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