Thread: Reset the output buffer after sending from WalSndWriteData

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?

Best Regards

Markus

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(). When it comes to walsender
codes, we reset the ctx->out buffer in WalSndPrepareWrite() called via
OutputPluginPrepareWrite(). Could you share the case where you faced
the unintentional re-transmission error?

Regards,

[1] https://www.postgresql.org/docs/devel/logicaldecoding-output-plugin.html#LOGICALDECODING-OUTPUT-PLUGIN-OUTPUT

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

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