On Mon, May 22, 2023 at 10:36 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, May 17, 2023 at 10:10 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > >
> > > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > > >
> > > > > If nobody else is working on this, I can come up with a patch to fix this
> > > > >
> > > >
> > > > Attaching a patch which attempts to fix this.
> > > >
> > >
> > > Thank you for the patch! I think we might want to have tests for it.
> > >
> > I have updated the patch with a test case as well.
>
> Thank you for updating the patch! Here are review comments:
>
> + /*
> + * Make sure that the copy command runs as the table owner, unless
> + * the user has opted out of that behaviour.
> + */
> + run_as_owner = MySubscription->runasowner;
> + if (!run_as_owner)
> + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
> +
> /* Now do the initial data copy */
> PushActiveSnapshot(GetTransactionSnapshot());
>
> I think we should switch users before the acl check in
> LogicalRepSyncTableStart().
>
> ---
> +# Create a trigger on table alice.unpartitioned that writes
> +# to a table that regress_alice does not have permission.
> +$node_subscriber->safe_psql(
> + 'postgres', qq(
> +SET SESSION AUTHORIZATION regress_alice;
> +CREATE OR REPLACE FUNCTION alice.alice_audit()
> +RETURNS trigger AS
> +\$\$
> + BEGIN
> + insert into public.admin_audit values(2);
> + RETURN NEW;
> + END;
> +\$\$
> +LANGUAGE 'plpgsql';
> +CREATE TRIGGER ALICE_TRIGGER AFTER INSERT ON alice.unpartitioned FOR EACH ROW
> +EXECUTE PROCEDURE alice.alice_audit();
> +ALTER TABLE alice.unpartitioned ENABLE ALWAYS TRIGGER ALICE_TRIGGER;
> +));
>
> While this approach works, I'm not sure we really need a trigger for
> this test. I've attached a patch for discussion that doesn't use
> triggers for the regression tests. We create a new subscription owned
> by a user who doesn't have the permission to the target table. The
> test passes only if run_as_owner = true works.
>
this is better, thanks. Since you are testing run_as_owner = false behaviour
during table copy phase, you might as well add a test case that it
correctly behaves during insert replication as well.
regards,
Ajin Cherian
Fujitsu Australia