On 08.12.21 01:23, Tomas Vondra wrote:
>> The argument "create" for fill_seq_with_data() is always true (and
>> patch 0002 removes it). So I'm not sure if it's needed. If it is, it
>> should be documented somewhere.
>>
>
> Good point. I think it could be removed, but IIRC it'll be needed when
> adding sequence decoding to the built-in replication, and that patch is
> meant to be just an implementation of the API, without changing WAL.
>
> OTOH I don't see it in the last version of that patch (in ResetSequence2
> call) so maybe it's not really needed. I've kept it for now, but I'll
> investigate.
Ok, please check. If it is needed, perhaps then we need a way for
test_decoding() to simulate it, for testing. But perhaps it's not needed.
>> The order of the new fields sequence_cb and stream_sequence_cb is a
>> bit inconsistent compared to truncate_cb and stream_truncate_cb.
>> Maybe take another look to make the order more uniform.
>>
>
> You mean in OutputPluginCallbacks? I'd actually argue it's the truncate
> callbacks that are inconsistent - in the regular section truncate_cb is
> right before commit_cb, while in the streaming section it's at the end.
Ok, that makes sense. Then leave yours.
When the question about fill_seq_with_data() is resolved, I have no more
comments on this part.