On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapil...@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.
