Re: running logical replication as the subscription owner

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.

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"?

--
With Regards,
Amit Kapila.