Re: Rework LogicalOutputPluginWriterUpdateProgress

Hi,

Replying on the new thread. Original message at
https://www.postgresql.org/message-id/CAA4eK1%2BH2m95HhzfpRkwv2-GtFwtbcVp7837X49%2Bvs0RXX3dBA%40mail.gmail.com

On 2023-02-09 15:54:19 +0530, Amit Kapila wrote:
> One thing to note about the changes we are discussing here is that
> some of the plugins like wal2json already call
> OutputPluginUpdateProgress in their commit callback. They may need to
> update it accordingly.

It was a fundamental mistake to add OutputPluginUpdateProgress(). I don't like
causing unnecessary breakage, but this seems necessary.

> One difference I see with the patch is that I think we will end up
> sending keepalive for empty prepared transactions even though we don't
> skip sending begin/prepare messages for those.

With the proposed approach we reliably know whether a callback wrote
something, so we can tune the behaviour here fairly easily.

Likely WalSndUpdateProgress() should not do anything if
did_write && !finished_xact.

> The reason why we don't skip sending prepare for empty 2PC xacts is that if
> the WALSender restarts after the PREPARE of a transaction and before the
> COMMIT PREPARED of the same transaction then we won't be able to figure out
> if we have skipped sending BEGIN/PREPARE of a transaction.

It's probably not a good idea to skip sending 2PC state changes anyway, at
least when used for replication, rather than CDC type use cases.

But I again think that that's not something the core system can assume.

I'm sad that we went so far down a pretty obviously bad rabbit hole. Adding
incrementally more of the progress calls to pgoutput, and knowing that
wal2json also added some, should have run some pretty large alarm bells.

> To skip sending prepare for empty xacts, we previously thought of some ideas
> like (a) At commit-prepare time have a check on the subscriber-side to know
> whether there is a corresponding prepare for it before actually doing
> commit-prepare but that sounded costly. (b) somehow persist the information
> whether the PREPARE for a xact is already sent and then use that information
> for commit prepared but again that also didn't sound like a good idea.

I don't think it's worth optimizing this. However, the explanation for why
we're not skipping empty prepared xacts needs to be added to
pgoutput_prepare_txn() etc.

Greetings,

Andres Freund