Re: Fix for REFRESH MATERIALIZED VIEW ownership error message

Hi,

I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as a
non-superuser or table owner yields the following message:

test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah

The error message should say "...owner of materialized view..."

The attached patch corrects this by setting the "relkind" for the
REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheck
returns the appropriate error message. The updated patch can be tested as such:

CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR: must be owner of materialized view b

I'm happy to generate the backpatches for it but wanted to receive feedback
first.

Thanks,

Jonathan

[1] https://www.postgresql.org/message-id/55498B5B-0155-4B0E-9B97-23167F8CB380%40excoventures.com <https://www.postgresql.org/message-id/[email protected]>

On 2018-Aug-17, Jonathan S. Katz wrote:

> Hi,
>
> I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as a
> non-superuser or table owner yields the following message:
>
> test=> REFRESH MATERIALIZED VIEW blah;
> ERROR: must be owner of relation blah
>
> The error message should say "...owner of materialized view..."
>
> The attached patch corrects this by setting the "relkind" for the
> REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheck
> returns the appropriate error message. The updated patch can be tested as such:
>
> CREATE ROLE bar LOGIN;
> CREATE TABLE a (x int);
> CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
> \c - bar
> REFRESH MATERIALIZED VIEW b;
> ERROR: must be owner of materialized view b
>
> I'm happy to generate the backpatches for it but wanted to receive feedback
> first.

Maybe add your test to some regress/ file?

As it is cosmetic, my inclination would be not to backpatch it.
However, I don't quite see how this patch can possibly fix the problem,
since the new struct member is not used anywhere AFAICT.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Dave Cramer

davec(at)postgresintl(dot)com
www.postgresintl.com

On Fri, 17 Aug 2018 at 18:30, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> On 2018-Aug-17, Jonathan S. Katz wrote:
>
> > Hi,
> >
> > I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW
> as a
> > non-superuser or table owner yields the following message:
> >
> > test=> REFRESH MATERIALIZED VIEW blah;
> > ERROR: must be owner of relation blah
> >
> > The error message should say "...owner of materialized view..."
> >
> > The attached patch corrects this by setting the "relkind" for the
> > REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the
> aclcheck
> > returns the appropriate error message. The updated patch can be tested
> as such:
> >
> > CREATE ROLE bar LOGIN;
> > CREATE TABLE a (x int);
> > CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
> > \c - bar
> > REFRESH MATERIALIZED VIEW b;
> > ERROR: must be owner of materialized view b
> >
> > I'm happy to generate the backpatches for it but wanted to receive
> feedback
> > first.
>
> Maybe add your test to some regress/ file?
>
+1

>
> As it is cosmetic, my inclination would be not to backpatch it.
> However, I don't quite see how this patch can possibly fix the problem,
> since the new struct member is not used anywhere AFAICT.
>
> The only place this is used is in aclcheck_error
case OBJECT_MATVIEW:
msg = gettext_noop("permission denied for materialized view %s");
break;

Dave

On 2018-Aug-17, Dave Cramer wrote:

> The only place this is used is in aclcheck_error
> case OBJECT_MATVIEW:
> msg = gettext_noop("permission denied for materialized view %s");
> break;

Yes, but do we pass RefreshMatViewStmt->relkind to that routine? I
don't see that we do. Maybe I misread the code.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, 17 Aug 2018 at 19:35, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> On 2018-Aug-17, Dave Cramer wrote:
>
> > The only place this is used is in aclcheck_error
> > case OBJECT_MATVIEW:
> > msg = gettext_noop("permission denied for materialized view %s");
> > break;
>
> Yes, but do we pass RefreshMatViewStmt->relkind to that routine? I
> don't see that we do. Maybe I misread the code.
>

Actually the code path that gets executed is:

case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;

as I have the patch applied and now see:

\c - test
You are now connected to database "test" as user "test".
test=> refresh materialized view b;
ERROR: must be owner of materialized view b

Dave Cramer

> On Aug 17, 2018, at 6:30 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> On 2018-Aug-17, Jonathan S. Katz wrote:
>
>> Hi,
>>
>> I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as a
>> non-superuser or table owner yields the following message:
>>
>> test=> REFRESH MATERIALIZED VIEW blah;
>> ERROR: must be owner of relation blah
>>
>> The error message should say "...owner of materialized view..."
>>
>> The attached patch corrects this by setting the "relkind" for the
>> REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheck
>> returns the appropriate error message. The updated patch can be tested as such:
>>
>> CREATE ROLE bar LOGIN;
>> CREATE TABLE a (x int);
>> CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
>> \c - bar
>> REFRESH MATERIALIZED VIEW b;
>> ERROR: must be owner of materialized view b
>>
>> I'm happy to generate the backpatches for it but wanted to receive feedback
>> first.
>
> Maybe add your test to some regress/ file?

Done. Please see attached.

> As it is cosmetic, my inclination would be not to backpatch it.

It’s cosmetic, but it’s a cosmetic bug: it incorrectly tells the user that they
must be the owner of the “relational” when in reality it’s the materialized view.

Thanks,

Jonathan

On Saturday, August 18, 2018, Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
>
> It’s cosmetic, but it’s a cosmetic bug: it incorrectly tells the user that
> they
> must be the owner of the “relational” when in reality it’s the
> materialized view.
>

Materialized views are a type of relation so it is not wrong, just one of
many instances where we generalize to "relation" based in implementation
details ins team of being explicit about which type of relation is being
affected.

David J.

> On Aug 18, 2018, at 5:26 PM, David G. Johnston <david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
> On Saturday, August 18, 2018, Jonathan S. Katz <jkatz(at)postgresql(dot)org <mailto:jkatz(at)postgresql(dot)org>> wrote:
> It’s cosmetic, but it’s a cosmetic bug: it incorrectly tells the user that they
> must be the owner of the “relational” when in reality it’s the materialized view.
>
> Materialized views are a type of relation so it is not wrong, just one of many instances where we generalize to "relation" based in implementation details ins team of being explicit about which type of relation is being affected.

Sure, that’s technically correct, but it’s still confusing to a user,
particularly in this cased since the error comes from running
"REFRESH MATERIALIZED VIEW" which is only applied to materialized views.

For instance, if you try running the command on a table:

CREATE TABLE a (x int);
REFRESH MATERIALIZED VIEW a;
ERROR: "a" is not a materialized view

which is what you would and should expect.

Jonathan

On Sat, 18 Aug 2018 at 17:30, Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:

>
> On Aug 18, 2018, at 5:26 PM, David G. Johnston <david(dot)g(dot)johnston(at)gmail(dot)com>
> wrote:
>
> On Saturday, August 18, 2018, Jonathan S. Katz <jkatz(at)postgresql(dot)org>
> wrote:
>>
>> It’s cosmetic, but it’s a cosmetic bug: it incorrectly tells the user
>> that they
>> must be the owner of the “relational” when in reality it’s the
>> materialized view.
>>
>
> Materialized views are a type of relation so it is not wrong, just one of
> many instances where we generalize to "relation" based in implementation
> details ins team of being explicit about which type of relation is being
> affected.
>
>
So why bother having the error message in the code at all then ? Clearly it
was the intent of the author to use this language, unfortunately there was
no test to prove that it works.

This is a simple fix why push back ? Additionally it clarifies exactly what
the problem is for the user as Jonathan points out.

Dave

Dave Cramer <pg(at)fastcrypt(dot)com> writes:
> This is a simple fix why push back ?

What was being pushed back on, I think, was the claim that this needed to
be back-patched. I'd be inclined not to, since (a) the message is not
wrong, only less specific than it could be, and (b) people tend to get
annoyed by unnecessary behavior changes in released branches.

There might be an argument for putting it into v11, but not further back.

regards, tom lane

Greetings,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Dave Cramer <pg(at)fastcrypt(dot)com> writes:
> > This is a simple fix why push back ?
>
> What was being pushed back on, I think, was the claim that this needed to
> be back-patched. I'd be inclined not to, since (a) the message is not
> wrong, only less specific than it could be, and (b) people tend to get
> annoyed by unnecessary behavior changes in released branches.
>
> There might be an argument for putting it into v11, but not further back.

I tend to agree with this, and we don't need to add more work for
translators either, which I'm guessing this would. For v11 and HEAD, as
long as it doesn't make the code ugly, I do think it'd be good to be
more specific.

Thanks!

Stephen

> On Aug 18, 2018, at 5:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Dave Cramer <pg(at)fastcrypt(dot)com> writes:
>> This is a simple fix why push back ?
>
> What was being pushed back on, I think, was the claim that this needed to
> be back-patched. I'd be inclined not to, since (a) the message is not
> wrong, only less specific than it could be,

I know I’m being pedantic on this one, but “technically” not wrong, it’s still
incomplete to a user. But...

> and (b) people tend to get
> annoyed by unnecessary behavior changes in released branches.

I will agree with this - thinking about it, people have have coded their apps
to work with the existing message and may have logic around handling it.

I don’t know how likely that is, but I’m willing to err on the side of caution.

> There might be an argument for putting it into v11, but not further back.

I’m fine with that.

Thanks,

Jonathan

On Sat, 18 Aug 2018 at 17:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Dave Cramer <pg(at)fastcrypt(dot)com> writes:
> > This is a simple fix why push back ?
>
> What was being pushed back on, I think, was the claim that this needed to
> be back-patched. I'd be inclined not to, since (a) the message is not
> wrong, only less specific than it could be, and (b) people tend to get
> annoyed by unnecessary behavior changes in released branches.
>

I was referring to:

"Materialized views are a type of relation so it is not wrong, just one of
many instances where we generalize to "relation" based in implementation
details ins team of being explicit about which type of relation is being
affected."

As being push back.

I don't have an opinion on back patching this.

Dave Cramer

davec(at)postgresintl(dot)com
www.postgresintl.com

On Saturday, August 18, 2018, Dave Cramer <pg(at)fastcrypt(dot)com> wrote:

>
> I was referring to:
>
> "Materialized views are a type of relation so it is not wrong, just one
> of many instances where we generalize to "relation" based in implementation
> details ins team of being explicit about which type of relation is being
> affected."
>
> As being push back.
>
> I don't have an opinion on back patching this.
>
>
I was arguing against back patching on the basis of defining this as a
bug. It's not wrong nor severe enough to warrant the side effects others
have noted.

David J.

On Sat, Aug 18, 2018 at 03:38:47PM -0700, David G. Johnston wrote:
> On Saturday, August 18, 2018, Dave Cramer <pg(at)fastcrypt(dot)com> wrote:
>> I was referring to:
>>
>> "Materialized views are a type of relation so it is not wrong, just one
>> of many instances where we generalize to "relation" based in implementation
>> details ins team of being explicit about which type of relation is being
>> affected."
>>
>> As being push back.
>>
>> I don't have an opinion on back patching this.
>
> I was arguing against back patching on the basis of defining this as a
> bug. It's not wrong nor severe enough to warrant the side effects others
> have noted.

I am not so sure about v11 as it is very close to release, surely we can
do something for HEAD as that's cosmetic. Anyway, if something is
proposed, could a patch be posted? The only patch I am seeing on this
thread refers to improvements for error messages of procedures.
--
Michael

> On Aug 18, 2018, at 8:45 PM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
>> On Sat, Aug 18, 2018 at 03:38:47PM -0700, David G. Johnston wrote:
>>> On Saturday, August 18, 2018, Dave Cramer <pg(at)fastcrypt(dot)com> wrote:
>>> I was referring to:
>>>
>>> "Materialized views are a type of relation so it is not wrong, just one
>>> of many instances where we generalize to "relation" based in implementation
>>> details ins team of being explicit about which type of relation is being
>>> affected."
>>>
>>> As being push back.
>>>
>>> I don't have an opinion on back patching this.
>>
>> I was arguing against back patching on the basis of defining this as a
>> bug. It's not wrong nor severe enough to warrant the side effects others
>> have noted.
>
> I am not so sure about v11 as it is very close to release, surely we can
> do something for HEAD as that's cosmetic. Anyway, if something is
> proposed, could a patch be posted? The only patch I am seeing on this
> thread refers to improvements for error messages of procedures.

Oops, too much multitasking. I will attach the correct patch when I get home.

Jonathan

> On Aug 18, 2018, at 8:52 PM, Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
>
>>
>> On Aug 18, 2018, at 8:45 PM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>>
>> I am not so sure about v11 as it is very close to release, surely we can
>> do something for HEAD as that's cosmetic. Anyway, if something is
>> proposed, could a patch be posted? The only patch I am seeing on this
>> thread refers to improvements for error messages of procedures.
>
> Oops, too much multitasking. I will attach the correct patch when I get home.

Here is the correct patch, sorry about that. This includes aforementioned
tests.

Jonathan

On 2018-Aug-18, Jonathan S. Katz wrote:

>
> > On Aug 18, 2018, at 8:52 PM, Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
> >
> >>
> >> On Aug 18, 2018, at 8:45 PM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >>
> >> I am not so sure about v11 as it is very close to release, surely we can
> >> do something for HEAD as that's cosmetic. Anyway, if something is
> >> proposed, could a patch be posted? The only patch I am seeing on this
> >> thread refers to improvements for error messages of procedures.
> >
> > Oops, too much multitasking. I will attach the correct patch when I get home.
>
> Here is the correct patch, sorry about that. This includes aforementioned
> tests.

I ran the test without the code change, and it passes. I don't think
it's testing what you think it is testing.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On Aug 18, 2018, at 11:59 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> On 2018-Aug-18, Jonathan S. Katz wrote:
>
>>
>>> On Aug 18, 2018, at 8:52 PM, Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
>>>
>>>>
>>>> On Aug 18, 2018, at 8:45 PM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>>>>
>>>> I am not so sure about v11 as it is very close to release, surely we can
>>>> do something for HEAD as that's cosmetic. Anyway, if something is
>>>> proposed, could a patch be posted? The only patch I am seeing on this
>>>> thread refers to improvements for error messages of procedures.
>>>
>>> Oops, too much multitasking. I will attach the correct patch when I get home.
>>
>> Here is the correct patch, sorry about that. This includes aforementioned
>> tests.
>
> I ran the test without the code change, and it passes. I don't think
> it's testing what you think it is testing.

So I ran the tests against 10.5 unpatched and it failed as expected. I then
ran it against HEAD unpatched and it passed.

Digging into it, it appears the issue was resolved in this commit[1] for 11
and beyond. As the plan is not to backpatch, I’ll withdraw my patch.

Thanks for the help,

Jonathan

https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=8b9e9644dc6a9bd4b7a97950e6212f63880cf18b <https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=8b9e9644dc6a9bd4b7a97950e6212f63880cf18b>