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

On Mon, Nov 2, 2026 at 7:06 AM torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> wrote:
>
> On 2026-10-31 07:34, Masahiko Sawada wrote:
> > On Fri, Oct 16, 2026 at 3:16 PM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >>
> >> Hi,
> >>
> >> On Fri, Oct 16, 2026 at 2:45 PM Masahiko Sawada
> >> <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >> >
> >> > > > 4. Based on above two points, I think we can fill the hole with something like:
> >> >
> >> > > > CREATE SUBSCRIPTION ... WITH (messages = true, message_handler_role =
> >> > > > 'msg_handler')
> >> >
> >> > > > and then run the hook under SwitchToUntrustedUser(that_role). That
> >> >
> >> > > I like this idea more than all the other approaches.
> >> >
> >> > My concern is that since the handler is a C function, this doesn't
> >> > help much as a security boundary. The handler can get back to the
> >> > subscription owner by calling SetUserIdAndSecContext() with
> >> > MySubscription->owner or even switch to the superuser. That function
> >> > does no permission check, and since the security context is its second
> >> > argument, the same call clears SECURITY_RESTRICTED_OPERATION as well.
> >> > As I mentioned in the reply to Amit, the option would help in cases
> >> > where the handler doesn't switch to the target role, or forgets to,
> >> > and keeps working as the subscription owner unexpectedly. But I'm not
> >> > sure we need this option just for that case.
> >>
> >> Agreed. In a way, we already trust every hook as far as switching to
> >> whatever role with SetUserIdAndSecContext() is concerned. So
> >> documenting the risk clearly here works for me.
> >>
> >> > > > I think we should have a similar ERROR for parallel apply worker.
> >> >
> >> > > Also, error out for the sequence sync worker?
> >> >
> >> > I agree to add a check for parallel apply workers, but I don't think
> >> > we need it for the sequence sync workers as they neither enter the
> >> > apply loop nor receive logical replication changes in the first place.
> >>
> >> Right, we don't need to error out for seq sync workers. However, do we
> >> need to error out for the table sync worker, since we don't expect a
> >> logical message there?
> >
> > It seems to me worth adding a check for tablesync workers since both
> > the apply workers and the tablesync workers call
> > walrcv_startstreaming() and it would prevent mistakes by future
> > changes.
> >
> > I've attached the updated patch.
>
> Thanks for the proposal and patch!
>
> As a precaution, I checked whether this patch could cause data loss
> similar to the issue I previously reported with Debezium [1], where
> logical messages report their end LSN while ordinary changes report
> their start LSN.
>
> Based on my code review and some tests, I haven't found a similar
> issue with this patch.
>
> The issue occurred with transactional messages.
> Debezium tracks processing progress at the event level, including
> individual DML operations and logical messages. In PostgreSQL's
> built-in logical replication, however, the handler's changes are
> committed as part of the replicated transaction, and replication
> progress is tracked at the transaction boundary. Therefore, the
> difference between start and end LSNs does not appear to cause
> the same problem.
>
> I also wondered whether there could be a timing window in which
> consecutive non-transactional logical messages might lead to data
> loss, so I checked this case as well. It seems that the following
> condition in logicalmsg_decode() handles this case correctly,
> so I don't see a problem here either.
>
> else if (!message->transactional &&
> (SnapBuildCurrentState(builder) != SNAPBUILD_CONSISTENT ||
> SnapBuildXactNeedsSkip(builder, buf->origptr)))
> return;

Thank you for reviewing and testing the patch!

For a non-transactional message, the apply worker commits the
handler's work as its own transaction and uses the message LSN as the
flush position. So this patch does rely on the message reporting its
end LSN in that case. If it reported the start LSN instead, the
publisher's confirmed_flush would point at the message record itself,
and the message would be decoded and sent again after a restart even
though the subscriber already committed it and sent an ack.

So I think using origptr for transactional messages and endptr for
non-transactional ones, as we discussed in that thread, is the right
thing for this patch too. I'll reply there.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com