Hi, On Tue, Aug 4, 2026 at 1:26 PM Masahiko Sawada <[email protected]> wrote: > > > Nice call-out about the more-than-once delivery and the idempotency > > requirement. It's true for logical replication in general, but would.... > > The patch already mentions in the doc: > > + <para> > + Received messages are only acted upon if an extension installed on > + the subscriber has registered a handler for them; otherwise they > are > + received and discarded. A message may be passed to the handler more > + than once, for example after an apply worker restart, so a handler > + must be prepared to process the same message repeatedly. > + </para> > > Does it work for you?
WFM. I missed that, sorry for the noise! > > 4/ > > + msg_data->prefix = pstrdup(pq_getmsgstring(in)); > > + msg = palloc(len + 1); > > > > I understand this isn't a memory leak, because ApplyMessageContext is > > reset after every message is handled at the end of the apply loop in > > LogicalRepApplyLoop(). But if the intention were to hand the caller > > the message in long-lived memory, this palloc in ApplyMessageContext > > doesn't achieve that either, since it gets reset after every message > > anyway. So why not hand the hook the message directly from the > > received StringInfo input buffer and let hook implementors copy it > > into whatever context they need? > > > > The point is just that the palloc here doesn't give us anything (if > > I'm not missing something), so I'm wondering whether we can avoid it. > > It's just to make the message handling consistent with the rest of > proto.c does (see logicalrep_read_tuple()). I don't think it would be > common for the hook to keep the received message in a long-lived > memory context, so the current API seems fine to me. Ah, I see it. Also, it's better to send a null-terminated string to the hooks than sending everything across from the received input buffer. > > 6/ > > This is fine, but I personally like the idea Amit posted above: a > > simple DDL replication example on the publisher and subscriber. Why > > not have that instead? I'm aware it would mean more LoC for the test > > extension, but that's fine IMO, since I see DDL replication as a good > > use case to demonstrate in the test module. > > While query-shipping style DDL replication can be implemented using > this hook, it would not be a great solution to me as neither a test > module nor a contrib module. For a test module, it would need too much > things to be implemented as a test module and people would not use it > as it's not shipped in the package. As for implementing it as a > contrib module, I think the module would not be integrated with the > built-in logical replication well. For example, the extension cannot > respect the table filter and publication options like > publish_via_partition_root etc. Also, if we support DDL replication in > the built-in logical replication, the contrib module would no longer > be necessary but it would be hard to remove it as people might be > using it. Agreed to keep it simple the way you have it in the v2 patch. > > 7/ > > Is it safe to commit the transaction that the non-transactional > > message was emitted from, given that transaction may have already done > > some work? > > I think that non-transactional messages are never interleaved with the > remote transaction. We emit a non-transactional message as soon as we > decode it (see ReorderBufferQueueMessage()) instead of buffering it. > > > I haven't verified this in depth, but going by intuition: > > > > BEGIN; > > INSERT ... > > UPDATE ... > > DELETE ... > > emit non-transactional message --> apply worker commits here > > UPDATE ... > > DELETE ... > > COMMIT; > > > > Is something like this safe? > > In this case, the non-transaction message is sent first and the > subscriber handles it as a single separate transaction, and then the > transaction without the message is sent to the subscriber. I think it > works fine. Thanks for pointing me to the code. I understand it now. So, there can never be a case where a non-transactional message is part of an in-progress transaction on the apply worker, even in streaming mode, because logical replication can't interleave transactions for apply. If so, can we assert this or enhance the comment in apply_handle_message() a bit? -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
