Re: running logical replication as the subscription owner

On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > I've attached the updated patch. Please review it.
> > >
> >
> > Few comments:
> > 1.
> > + /* get the owner for ACL and RLS checks */
> > + run_as_owner = MySubscription->runasowner;
> > + checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner;
> > +
> >   /*
> >   * Check that our table sync worker has permission to insert into the
> >   * target table.
> >   */
> > - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
> > + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner,
> >
> > One thing that slightly worries me about this change is that we
> > started to check the permission for relowner before even ensuring that
> > we can switch to relowner. See checks in SwitchToUntrustedUser(). If
> > we want to first ensure that we can switch to relowner then I think we
> > should move this permission-checking code before we try to copy the
> > table.
>
> Agreed. I thought it's better to do ACL and RLS checks before creating
> the replication slot but it's not important. Rather checking them
> after switching user would make sense since we do the same in
> worker.c.
>

LGTM.

--
With Regards,
Amit Kapila.