On Thu, Mar 31, 2022, at 9:24 AM, Masahiko Sawada wrote:
The patch basically looks good to me. But the only concern to me is
that once we get the patch committed, we will have to call
update_progress() at all paths in callbacks that process changes.
Which seems poor maintainability.
I didn't like the current fix for the same reason. We need a robust feedback
system for logical replication. We had this discussion in the "skip empty
transactions" thread [1].
On the other hand, possible another solution would be to add a new
callback that is called e.g., every 1000 changes so that walsender
does its job such as timeout handling while processing the decoded
data in reorderbuffer.c. The callback is set only if the walsender
does logical decoding, otherwise NULL. With this idea, other plugins
will also be able to benefit without changes. But I’m not really sure
it’s a good design, and adding a new callback introduces complexity.
No new callback is required.
In the current code, each output plugin callback is responsible to call
OutputPluginUpdateProgress. It is up to the output plugin author to add calls
to this function. The lack of a call in a callback might cause issues like what
was described in the initial message.
The functions CreateInitDecodingContext and CreateDecodingContext receives the
update_progress function as a parameter. These functions are called in 2
places: (a) streaming replication protocol (CREATE_REPLICATION_SLOT) and (b)
SQL logical decoding functions (pg_logical_*_changes). Case (a) uses
WalSndUpdateProgress as a progress function. Case (b) does not have one because
it is not required -- local decoding/communication. There is no custom update
progress routine for each output plugin which leads me to the question:
couldn't we encapsulate the update progress call into the callback functions?
If so, we could have an output plugin parameter to inform which callbacks we
would like to call the update progress routine. This would simplify the code,
make it less error prone and wouldn't impose a burden on maintainability.
