Re: Add a hook for handling logical decoding messages on subscribers. - Mailing list pgsql-hackers

On Thu, Aug 6, 2026 at 10:42 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> On Thu, Aug 6, 2026 at 9:59 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > > 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.
>
> Having replorigin_xact_clear there looks fine to me. The next
> transaction commit would anyway set the origin LSN and timestamp.
>
> > > 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.
>
> Right. I verified other places and wherever the read does a palloc, it
> is wrapped within begin and end replication step.
>
> > I've attached the updated patch.
>
> Thanks. The v4 patch looks good to me. pgindent and tests are happy. I
> have no further comments. I marked the CF entry RfC
> (https://commitfest.postgresql.org/patch/7092/). FWIW, the CF bot
> complains with "needs rebase":
> https://cfbot.cputube.org/patch_7092.log.

Thanks for reviewing the patch!

While I also think the patch is in good shape, I'd like to raise a
security risk this feature might introduce, particularly around
pg_logical_emit_message():

EXECUTE on that function is granted to PUBLIC, so any role that can
connect to the publisher database can emit a logical decoding message.
With this patch the apply worker hands the message to a handler that
runs with the privileges of the subscription owner, and unlike
insert/update/delete there is no table owner to switch to. One use
case I have in mind is DDL replication. If an extension implements it
on top of this hook and the subscription is owned by a superuser, any
role on the publisher can choose what the handler is given and have it
executed with superuser privileges on the subscriber. So extensions
should carefully consider this case. I think the same applies to other
extensions that might use this feature. The real problem is that there
is no reliable way for the subscriber to tell whether a message came
from the source it expects.

A practical solution is to revoke EXECUTE on pg_logical_emit_message()
from PUBLIC on the publisher and grant it to a role created for that
purpose. I think that covers most cases I did consider having the
server record the emitting role in the message so that the subscriber
could check it, but I'm not sure this feature alone justifies it. So
my current thought is to document these risks and add nothing special
for these cases.

I've added the documentation changes and rebased the patch. Any ideas
and feedback is very welcome.

Regards,

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