Re: running logical replication as the subscription owner

On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, May 23, 2023 at 8:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > Thank you for updating the patch! Here are review comments:
> > > >
> > > > +       /*
> > > > +        * Make sure that the copy command runs as the table owner, unless
> > > > +        * the user has opted out of that behaviour.
> > > > +        */
> > > > +       run_as_owner = MySubscription->runasowner;
> > > > +       if (!run_as_owner)
> > > > +               SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
> > > > +
> > > >         /* Now do the initial data copy */
> > > >         PushActiveSnapshot(GetTransactionSnapshot());
> > > >
> > > > I think we should switch users before the acl check in
> > > > LogicalRepSyncTableStart().
> > > >
> > >
> > > Agreed, we should check acl with the user that is going to perform
> > > operations on the target table. BTW, is it okay to perform an
> > > operation on the system table with the changed user as that would be
> > > possible with your suggestion (see replorigin_create())?
> >
> > Do you see any problem in particular?
> >
> > As per the documentation, pg_replication_origin_create() is only
> > allowed to the superuser by default, but in CreateSubscription() a
> > non-superuser (who has pg_create_subscription privilege) can call
> > replorigin_create().
>
> Nothing in particular but it seems a bit odd to perform operations on
> catalog tables with some other user table owners when that was not the
> actual intent of this option.
>
> > OTOH, we don't necessarily need to switch to the
> > table owner user for checking ACL and RLS. We can just pass either
> > table owner OID or subscription owner OID to pg_class_aclcheck() and
> > check_enable_rls() without actually switching the user.
> >
>
> I think that would be better.

Agreed.

I've attached the updated patch. Please review it.

Regards,

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