Re: Add a hook for handling logical decoding messages on subscribers.

Hi,

On Wed, Aug 5, 2026 at 11:56 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've addressed all comments I got so far unless I'm missing anything,
> and attached the updated patch. Feedback is very welcome.

Thanks for sending the v3 patch. It looks good to me as-is. I verified
the patch with different cases such as emitting a bunch of ~900MB
messages to see the memory growth, enabling/disabling the subscription
option, pg_dump with the option enabled/disabled, regression tests,
and pgindent.

Here are some comments (may not need any code changes, feel free to ignore):

1/
+ if (options->proto.logical.messages &&
+ PQserverVersion(conn->streamConn) >= 140000)
+ appendStringInfo(&cmd, ", messages 'on'");

+ <term><literal>message</literal> (<type>boolean</type>)</term>

The publisher option is "messages". Should we have the subscription
option also use the same plural form to keep it consistent, and for
the reason that we receive a stream of messages, not just one?

2/
+ /*
+ * The message doesn't belong to any remote transaction, so there is
+ * no remote commit LSN nor timestamp to record. Clear the state left
+ * over by the previously applied transaction so that this commit
+ * doesn't inherit it.
+ */
+ replorigin_xact_clear(false);

Why is this a problem if we let the non-transactional message inherit it?

3/
+ logicalrep_read_message(s, &msg);
+
+ begin_replication_step();
+
+ (*LogicalRepMessageHandle_hook) (&msg);
+
+ end_replication_step();

Wrapping the hook with begin and end replication step is nice. This
lets the hook see the correct command ID, snapshot, and memory
context. There are callers that do the begin first and read message
next (insert), but it seems okay this way because read message doesn't
do any catalog or table accesses, so it should be fine.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com