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

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;

[1]
https://www.postgresql.org/message-id/d99c688994ab3a998afe26e61fe4f69f%40oss.nttdata.com

--
Thanks,

--
Atsushi Torikoshi
Seconded from NTT DATA CORPORATION to SRA OSS K.K.