On Thu, Jun 8, 2023 at 7:29 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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.
Thanks, pushed. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
