Re: running logical replication as the subscription owner

On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

>
> 2. In the commit message, the link for discussion
> "https://postgr.es/m/CAA4eK1KfZcRq7hUqQ7WknP+u=08+6MevVm+2W5RrAb+DTxrdww@mail.gmail.com"
> is slightly misleading. Can we instead use
> "https://www.postgresql.org/message-id/CAA4eK1L%3DqzRHPEn%2BqeMoKQGFBzqGoLBzt_ov0A89iFFiut%2BppA%40mail.gmail.com"?

Agreed.

I've attached the updated patch.

Regards,

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