Subject: Re: pg_logical_emit_message() misses a XLogFlush()

From: Andres Freund <[email-masked]>
Date: Tue, 15 Aug 2023 17:33:33 -0700

Hi,

On 2023-08-16 06:58:56 +0900, Michael Paquier wrote:
> On Tue, Aug 15, 2023 at 11:37:32AM +0200, Tomas Vondra wrote:
> > Shouldn't the flush be done only for non-transactional messages? The
> > transactional case will be flushed by regular commit flush.
> 
> Indeed, that would be better.  I am sending an updated patch.
> 
> I'd like to backpatch that, would there be any objections to that?

Yes, I object. This would completely cripple the performance of some uses of
logical messages - a slowdown of several orders of magnitude. It's not clear
to me that flushing would be the right behaviour if it weren't released, but
it certainly doesn't seem right to make such a change in a minor release.

Greetings,

Andres Freund

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 16 Aug 2023 03:20:53 +0200

> it certainly doesn't seem right to make such a change in a minor release.

So are you objecting to adding the flush in general, or just to the
backpatching part?

IMHO we either guarantee durability of non-transactional messages, in
which case this would be a clear bug - and I'd say a fairly serious one.
I'm curious what the workload that'd see order of magnitude of slowdown
does with logical messages, but even if such workload exists, would it
really be enough to fix any other durability bug?

Or perhaps we don't want to guarantee durability for such messages, in
which case we don't need to fix it at all (even in master).

The docs are not very clear on what to expect, unfortunately. It says
that non-transactional messages are "written immediately" which I could
interpret in either way.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Michael Paquier <[email-masked]>
Date: Wed, 16 Aug 2023 12:37:21 +0900

> really be enough to fix any other durability bug?

Yes, I also think that this is a pretty serious issue to not ensure
durability in the non-transactional case if you have solutions
designed around that.

> which case we don't need to fix it at all (even in master).

I mean, just look at the first message of the thread I am mentioning
at the top of this thread: it lists three cases where something like
pg_logical_emit_message() was wanted.  And, it looks like a serious
issue to me for the first two ones at least (out-of-order messages and
inter-node communication), because an application may want to send
something from node1 to node2, and node1 may just forget about it
entirely if it crashes, finishing WAL redo at an earlier point than
the record inserted.

> that non-transactional messages are "written immediately" which I could

Agreed.  The original thread never mentions "flush", "sync" or
"durab".

I won't fight much if there are objections to backpatching, but that's
not really wise (no idea how much EDB's close flavor of BDR relies on
that).
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Tue, 15 Aug 2023 21:13:36 -0700

Hi,

> So are you objecting to adding the flush in general, or just to the

Both, I think. I don't object to adding a way to trigger flushing, but I think
it needs to be optional.

> IMHO we either guarantee durability of non-transactional messages, in which
> case this would be a clear bug - and I'd say a fairly serious one.  I'm
> curious what the workload that'd see order of magnitude of slowdown does
> with logical messages I've used it, but even if such workload exists, would
> it really be enough to fix any other durability bug?

Not sure what you mean with the last sentence?

I've e.g. used non-transactional messages for:

- A non-transactional queuing system. Where sometimes one would dump a portion
  of tables into messages, with something like
  SELECT pg_logical_emit_message(false, 'app:<task>', to_json(r)) FROM r;
  Obviously flushing after every row would be bad.

  This is useful when you need to coordinate with other systems in a
  non-transactional way. E.g. letting other parts of the system know that
  files on disk (or in S3 or ...) were created/deleted, since a database
  rollback wouldn't unlink/revive the files.

- Audit logging, when you want to log in a way that isn't undone by rolling
  back transaction - just flushing every pg_logical_emit_message() would
  increase the WAL flush rate many times, because instead of once per
  transaction, you'd now flush once per modified row. It'd basically make it
  impractical to use for such things.

- Optimistic locking. Emitting things that need to be locked on logical
  replicas, to be able to commit on the primary. A pre-commit hook would wait
  for the WAL to be replayed sufficiently - but only once per transaction, not
  once per object.

> which case we don't need to fix it at all (even in master).

Well, I can see adding an option to flush, or perhaps a separate function to
flush, to master.

> that non-transactional messages are "written immediately" which I could

Yea, the docs certainly should be improved, regardless what we end up with.

Greetings,

Andres Freund

---

From: Tomas Vondra <[email-masked]>
Date: Wed, 16 Aug 2023 12:01:01 +0200

> Not sure what you mean with the last sentence?

Sorry, I meant to write "enough not to fix any other durability bug".

That is, either we must not lose messages, in which case it's a bug and
we should fix that. Or it's acceptable for the intended use cases, in
which case there's nothing to fix.

To me losing messages seems like a bad thing, but if the users are aware
of it and are fine with it ... I'm simply arguing that if we conclude
this is a durability bug, we should not leave it unfixed because it
might have performance impact.

>   for the WAL to be replayed sufficiently - but only once per transaction, not

How come the possibility of losing messages is not an issue for these
use cases? I mean, surely auditors would not like that, and I guess
forgetting locks might be bad too.

regards

---

From: Michael Paquier <[email-masked]>
Date: Thu, 17 Aug 2023 10:15:35 +0900

> might have performance impact.

I've been doing some digging here, and the original bdr repo posted at
[1] has a concept similar to LogLogicalMessage() called
LogStandbyMessage().  *All* the non-transactional code paths enforce
an XLogFlush() after *each* message logged.  So the original
expectation seems pretty clear to me: flushes were wanted.

[1]: https://github.com/2ndQuadrant/bdr

> forgetting locks might be bad too.

+1.  Now I can also get why one may not want to flush every individual
messages if you care only about a queue to be flushed after generating
a series of them, so after sleeping on it I'm OK with the last patch I
posted where one can just choose what he wants.  The default, though,
may be better if flush is true compared to false (the patch has kept
flush at false)..

I am not sure where this is leading yet, so I have registered a CF
entry to keep track of that:
https://commitfest.postgresql.org/44/4505/
--
Michael