Re: Reset the output buffer after sending from WalSndWriteData

On 2/21/25 07:17, Masahiko Sawada wrote:
> On Thu, Feb 20, 2025 at 12:50 PM Markus Wanner
> <markus.wanner@enterprisedb.com> wrote:
>>
>> Hi,
>>
>> I recently stumbled over an issue with an unintentional re-transmission.
>> While this clearly was our fault in the output plugin code, I think the
>> walsender's exposed API could easily be hardened to prevent the bad
>> consequence from this mistake.
>>
>> Does anything speak against the attached one line patch?
> 
> According to the documentation[1], OutputPluginPrepareWrite() has to
> be called before OutputPluginWrite().

Sure, that's exactly what we forgot. My complaint is that it's a mistake 
that's unnecessarily hard to spot and the API could be improved. There 
is no good reason to keep the sent data in the ctx->out buffer. But 
clearly, there's some danger to it.

(Note that it wasn't quite as simple as you may think, though. With an 
error involved in the send/recv loop of the walsender invoked from 
OutputPluginWrite. The error handling code in PG_CATCH then attempting 
to flush the queue with OutputPluginWrite.)

Arguably, one could even say that replacing the call to resetStringInfo 
in WalSndPrepareWrite with an Assert(ctx->out->len == 0) would catch a 
variant of improper use. And another Assert in WalSndWriteData to check 
OutputPluginPrepareWrite had properly been invoked before can't hurt, 
either.

Best Regards

Markus