Re: Proposal: Generic WAL logical messages

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