Hi,
thanks for review.
On 17/03/16 13:36, Tomas Vondra wrote:
> 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)
>
Agreed. Same goes for the ReoderBufferChange struct btw, only
transactional messages go there so no point in marking them as such.
>
>
> 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.
>
Heh, blind.
> 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);
>
Good point.
>
> 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?
>
Yeah I sometimes am not sure if there is a point to put comment to tiny
straightforward functions that do more or less same as the one above.
But it's public API so probably better to have 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).
>
Okay.
Updated version attached.
(BTW please try to CC author of the patch when reviewing)
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services