On Thu, Aug 6, 2026 at 12:03 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> On Wed, Aug 5, 2026 at 11:56 AM Masahiko Sawada <sawada.mshk@gmail.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?
Agreed.
>
> 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?
IIUC non-transactional messages would have the same commit timestamp
as the previously applied transaction, which is wrong to me.
>
> 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.
>
begin_replication_step() switches the memory context to
ApplyMessageContext. Given logicalrep_read_message() palloc's for
messages, it should be called after begin_replication_step(). Fixed it.
I've attached the updated patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
