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


Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to