Re: CATUPDATE confusion?

All,

While reviewing the supporting documentation and functions for role
attributes I noticed that there seems to be some confusion (at least for
me) with CATUPDATE.

This was prompted by the following comment from Peter about
'has_rolcatupdate' not having a superuser check.

http://www.postgresql.org/message-id/54590BBF.1080008@gmx.net

So, where I find this confusing is that the documentation supports this
functionality and the check keeps superuser separate from CATUPDATE...
however... comments and implementation in user.c state/do the opposite and
couple them together.

Documentation:
http://www.postgresql.org/docs/9.4/static/catalog-pg-authid.html - "Role
can update system catalogs directly. (Even a superuser cannot do this
unless this column is true)"

src/backend/commands/user.c

/* superuser gets catupdate right by default */
new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper);

and...

/*
* issuper/createrole/catupdate/etc
*
* XXX It's rather unclear how to handle catupdate. It's probably best to
* keep it equal to the superuser status, otherwise you could end up with
* a situation where no existing superuser can alter the catalogs,
* including pg_authid!
*/
if (issuper >= 0)
{
new_record[Anum_pg_authid_rolsuper - 1] = BoolGetDatum(issuper > 0);
new_record_repl[Anum_pg_authid_rolsuper - 1] = true;

new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper > 0);
new_record_repl[Anum_pg_authid_rolcatupdate - 1] = true;
}

Perhaps this is not an issue but it seemed odd to me, especially after
giving Peter's comment more thought. So, I suppose the question is whether
or not a superuser check should be added to 'has_rolcatupdate' or not? I
believe I understand the reasoning for coupling the two at role
creation/alter time, however, is there really a case where a superuser
wouldn't be allowed to bypass this check? Based on the comments, there
seems like there is potentially enough concern to allow it. And if it is
allowed, couldn't CATUPDATE then be treated like every other attribute and
the coupling with superuser removed? Thoughts?

Thanks,
Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:
> So, where I find this confusing is that the documentation supports this
> functionality and the check keeps superuser separate from CATUPDATE...
> however... comments and implementation in user.c state/do the opposite and
> couple them together.
>
> Documentation:
> http://www.postgresql.org/docs/9.4/static/catalog-pg-authid.html - "Role
> can update system catalogs directly. (Even a superuser cannot do this
> unless this column is true)"
>
> src/backend/commands/user.c
>
> /* superuser gets catupdate right by default */
> new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper);
>
> and...
>
> /*
> * issuper/createrole/catupdate/etc
> *
> * XXX It's rather unclear how to handle catupdate. It's probably best to
> * keep it equal to the superuser status, otherwise you could end up with
> * a situation where no existing superuser can alter the catalogs,
> * including pg_authid!
> */
> if (issuper >= 0)
> {
> new_record[Anum_pg_authid_rolsuper - 1] = BoolGetDatum(issuper > 0);
> new_record_repl[Anum_pg_authid_rolsuper - 1] = true;
>
> new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper > 0);
> new_record_repl[Anum_pg_authid_rolcatupdate - 1] = true;
> }

That documentation is correct as far it goes. It does neglect to mention
that, as you have discovered, any CREATE ROLE or ALTER ROLE [NO]SUPERUSER will
change rolcatupdate to match.

> Perhaps this is not an issue but it seemed odd to me, especially after
> giving Peter's comment more thought. So, I suppose the question is whether
> or not a superuser check should be added to 'has_rolcatupdate' or not? I

I would not. PostgreSQL has had this feature since day one (original name
"usecatupd"). It has two use cases, (1) giving effect to non-superuser
catalog grants and (2) preventing a narrow class of superuser mistakes. We
wouldn't add such a thing today, but one can safely ignore its existence.
Making has_rolcatupdate() approve superusers unconditionally would exclude use
case (2). Neither use case is valuable, but if I had to pick, (2) is more
valuable than (1).

> believe I understand the reasoning for coupling the two at role
> creation/alter time, however, is there really a case where a superuser
> wouldn't be allowed to bypass this check? Based on the comments, there
> seems like there is potentially enough concern to allow it. And if it is
> allowed, couldn't CATUPDATE then be treated like every other attribute and
> the coupling with superuser removed? Thoughts?

A superuser always has _some_ way to bypass the check, if nothing else by
loading a C-language function to update pg_authid. The user.c code you quoted
ensures that, if the DBA manages to remove rolcatupdate from every user, a
simple CREATE ROLE or ALTER ROLE can fix things.

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:
> > Perhaps this is not an issue but it seemed odd to me, especially after
> > giving Peter's comment more thought. So, I suppose the question is whether
> > or not a superuser check should be added to 'has_rolcatupdate' or not? I
>
> I would not. PostgreSQL has had this feature since day one (original name
> "usecatupd"). It has two use cases, (1) giving effect to non-superuser
> catalog grants and (2) preventing a narrow class of superuser mistakes. We
> wouldn't add such a thing today, but one can safely ignore its existence.
> Making has_rolcatupdate() approve superusers unconditionally would exclude use
> case (2). Neither use case is valuable, but if I had to pick, (2) is more
> valuable than (1).

What's confusing about this is that use-case (2) appears to have been
the intent of the option, but because it's tied directly to superuser
and isn't an independently controllable option, the only way change it
is to do an 'update pg_authid ...'. Even when set that way, it isn't
visible that it's been set through \du, you have to query the catalog
directly.

I wonder if the option had originally been intended to be independent
and *not* given to superusers by default, but because of the concern
about dealing with corrupt systems, etc, it was never actually done.
I'm fine with the line of thinking that says we can't deny superusers
this power because of corruption/debugging concerns, but if that's the
case, then there is no use-case (2) and we should just remove the option
entirely.

I don't buy into use-case (1) above being at all reasonable. With it,
one can trivially make themselves a superuser, and in many ways the
catupdate capability is *more* dangerous than superuser- if you have
catupdate but not superuser, you'd be tempted to hack at the catalog to
make the changes you want instead of using the regular ALTER TABLE, etc,
commands.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Noah Misch (noah(at)leadboat(dot)com) wrote:
>> On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:
>>> Perhaps this is not an issue but it seemed odd to me, especially after
>>> giving Peter's comment more thought. So, I suppose the question is whether
>>> or not a superuser check should be added to 'has_rolcatupdate' or not? I

>> I would not. PostgreSQL has had this feature since day one (original name
>> "usecatupd"). It has two use cases, (1) giving effect to non-superuser
>> catalog grants and (2) preventing a narrow class of superuser mistakes. We
>> wouldn't add such a thing today, but one can safely ignore its existence.
>> Making has_rolcatupdate() approve superusers unconditionally would exclude use
>> case (2). Neither use case is valuable, but if I had to pick, (2) is more
>> valuable than (1).

> What's confusing about this is that use-case (2) appears to have been
> the intent of the option,

Yes. Noah's claim that anybody intended (1) seems to be mere historical
revisionism, especially since as you note CATUPDATE could be trivially
parlayed into superuser if someone were to grant it to a non-superuser.

> but because it's tied directly to superuser
> and isn't an independently controllable option, the only way change it
> is to do an 'update pg_authid ...'. Even when set that way, it isn't
> visible that it's been set through \du, you have to query the catalog
> directly.

This is not hard to understand either: the option dates from long before
there was any concerted effort to invent bespoke SQL commands for every
interesting catalog manipulation. If CATUPDATE had any significant use,
we'd have extended ALTER USER (and \du, and pg_dump ...) at some point
to make it more easily controllable. But since it's of such marginal
usefulness, nobody ever bothered; and I doubt we should bother now.

In short, I agree with Noah's conclusion (do nothing) though not his
reasoning.

Plan C (remove CATUPDATE altogether) also has some merit. But adding a
superuser override there would be entirely pointless.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Plan C (remove CATUPDATE altogether) also has some merit. But adding a
> superuser override there would be entirely pointless.

This is be my recommendation. I do not see the point of carrying the
option around just to confuse new users of pg_authid and reviewers of
the code.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Plan C (remove CATUPDATE altogether) also has some merit. But adding a
>> superuser override there would be entirely pointless.

> This is be my recommendation. I do not see the point of carrying the
> option around just to confuse new users of pg_authid and reviewers of
> the code.

Yeah ... if no one's found it interesting in the 20 years since the
code left Berkeley, it's unlikely that interest will emerge in the
future.

regards, tom lane

On Sat, Dec 27, 2014 at 06:26:02PM -0500, Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >> Plan C (remove CATUPDATE altogether) also has some merit. But adding a
> >> superuser override there would be entirely pointless.
>
> > This is be my recommendation. I do not see the point of carrying the
> > option around just to confuse new users of pg_authid and reviewers of
> > the code.
>
> Yeah ... if no one's found it interesting in the 20 years since the
> code left Berkeley, it's unlikely that interest will emerge in the
> future.

No objection here.

All,

On Sat, Dec 27, 2014 at 6:31 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:

> On Sat, Dec 27, 2014 at 06:26:02PM -0500, Tom Lane wrote:
> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> > >> Plan C (remove CATUPDATE altogether) also has some merit. But adding
> a
> > >> superuser override there would be entirely pointless.
> >
> > > This is be my recommendation. I do not see the point of carrying the
> > > option around just to confuse new users of pg_authid and reviewers of
> > > the code.
> >
> > Yeah ... if no one's found it interesting in the 20 years since the
> > code left Berkeley, it's unlikely that interest will emerge in the
> > future.
>
> No objection here.
>

Given this discussion, I have attached a patch that removes CATUPDATE for
review/discussion.

One of the interesting behaviors (or perhaps not) is how 'pg_class_aclmask'
handles an invalid role id when checking permissions against 'rolsuper'
instead of 'rolcatupdate'. This is demonstrated by the
'has_table_privilege' regression test expected results. In summary,
'has_rolcatupdate' raises an error when an invalid OID is provided,
however, as documented in the source 'superuser_arg' does not, simply
returning false. Therefore, when checking for superuser-ness of an
non-existent role, the result will be false and not an error. Perhaps this
is OK, but my concern would be on the expected behavior around items like
'has_table_privilege'. My natural thought would be that we would want to
preserve that specific functionality, though short of adding a
'has_rolsuper' function that will raise an appropriate error, I'm uncertain
of an approach. Thoughts?

Thanks,
Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

Adam,

* Adam Brightwell (adam(dot)brightwell(at)crunchydatasolutions(dot)com) wrote:
> Given this discussion, I have attached a patch that removes CATUPDATE for
> review/discussion.

Thanks!

> One of the interesting behaviors (or perhaps not) is how 'pg_class_aclmask'
> handles an invalid role id when checking permissions against 'rolsuper'
> instead of 'rolcatupdate'. This is demonstrated by the
> 'has_table_privilege' regression test expected results. In summary,
> 'has_rolcatupdate' raises an error when an invalid OID is provided,
> however, as documented in the source 'superuser_arg' does not, simply
> returning false. Therefore, when checking for superuser-ness of an
> non-existent role, the result will be false and not an error. Perhaps this
> is OK, but my concern would be on the expected behavior around items like
> 'has_table_privilege'. My natural thought would be that we would want to
> preserve that specific functionality, though short of adding a
> 'has_rolsuper' function that will raise an appropriate error, I'm uncertain
> of an approach. Thoughts?

This role oid check is only getting called in this case because it's
pg_authid which is getting checked (because it's a system catalog table)
and it's then falling into this one case. I don't think we need to
preserve that.

If you do the same query with that bogus OID but a normal table, you get
the same 'f' result that the regression test gives with this change.
We're not back-patching this and I seriously doubt anyone is relying on
this special response for system catalog tables.

So, unless anyone wants to object, I'll continue to look over this patch
for eventual application against master. If there are objections, it'd
be great if they could be voiced sooner rather than later.

Thanks!

Stephen

>
> * Adam Brightwell (adam(dot)brightwell(at)crunchydatasolutions(dot)com) wrote:
> > Given this discussion, I have attached a patch that removes CATUPDATE for
> > review/discussion.
>
> Thanks!

I've added this patch (up-thread) to the next CommitFest (2015-02).

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On 12/29/14 7:16 PM, Adam Brightwell wrote:
> Given this discussion, I have attached a patch that removes CATUPDATE
> for review/discussion.
>
> One of the interesting behaviors (or perhaps not) is how
> 'pg_class_aclmask' handles an invalid role id when checking permissions
> against 'rolsuper' instead of 'rolcatupdate'.

I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

Peter,

Thanks for the review and feedback.

> One of the interesting behaviors (or perhaps not) is how
> > 'pg_class_aclmask' handles an invalid role id when checking permissions
> > against 'rolsuper' instead of 'rolcatupdate'.
>
> I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.
>

Ah yes, that's a good point. I have updated and attached a new version of
the patch.

Thanks,
Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On Fri, Feb 20, 2015 at 8:08 AM, Adam Brightwell <
adam(dot)brightwell(at)crunchydatasolutions(dot)com> wrote:

> Thanks for the review and feedback.
>
> > One of the interesting behaviors (or perhaps not) is how
>> > 'pg_class_aclmask' handles an invalid role id when checking permissions
>> > against 'rolsuper' instead of 'rolcatupdate'.
>>
>> I'd get rid of that whole check, not just replace rolcatupdate by
>> rolsuper.
>>
>
> Ah yes, that's a good point. I have updated and attached a new version of
> the patch.
>

I just had a look at this patch and it works as intended, simply removing
catupdate and all its code paths. As everybody on this thread seems to
agree about the removal of rolcatupdate, I think that we could let a
committer have a look at it.
Thoughts?
--
Michael

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On 12/29/14 7:16 PM, Adam Brightwell wrote:
> > Given this discussion, I have attached a patch that removes CATUPDATE
> > for review/discussion.
> >
> > One of the interesting behaviors (or perhaps not) is how
> > 'pg_class_aclmask' handles an invalid role id when checking permissions
> > against 'rolsuper' instead of 'rolcatupdate'.
>
> I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

Err, wouldn't this make it possible to grant normal users the ability to
modify system catalogs? I realize that they wouldn't have that
initially, but I'm not sure we want the superuser to be able to grant
that to non-superusers..

I'm fine with making it "if system table and not superuser, error".

Thanks!

Stephen

On 2/25/15 3:39 PM, Stephen Frost wrote:
>> I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.
>
> Err, wouldn't this make it possible to grant normal users the ability to
> modify system catalogs? I realize that they wouldn't have that
> initially, but I'm not sure we want the superuser to be able to grant
> that to non-superusers..

Why not? I thought we are trying to get rid of special superuser behavior.

After all, superusers can also make the other user a superuser to bypass
this issue.

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On 2/25/15 3:39 PM, Stephen Frost wrote:
> >> I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.
> >
> > Err, wouldn't this make it possible to grant normal users the ability to
> > modify system catalogs? I realize that they wouldn't have that
> > initially, but I'm not sure we want the superuser to be able to grant
> > that to non-superusers..
>
> Why not? I thought we are trying to get rid of special superuser behavior.

Agreed, but I'd also like to get rid of any reason, beyond emergency
cases, for people to modify the catalog directly. There's a few places
which we aren't yet doing that, but I'd rather fix those cases than
encourage people to give out rights to modify them and end up making
things like:

"UPDATE pg_database SET datallowconn = false where datname = 'xyz';"

an accepted interface.

> After all, superusers can also make the other user a superuser to bypass
> this issue.

Sure, but that gives us the option to write off whatever happens next as
not our fault.

Thanks,

Stephen

On 2/25/15 10:05 PM, Stephen Frost wrote:
> * Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
>> On 2/25/15 3:39 PM, Stephen Frost wrote:
>>>> I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.
>>>
>>> Err, wouldn't this make it possible to grant normal users the ability to
>>> modify system catalogs? I realize that they wouldn't have that
>>> initially, but I'm not sure we want the superuser to be able to grant
>>> that to non-superusers..
>>
>> Why not? I thought we are trying to get rid of special superuser behavior.
>
> Agreed, but I'd also like to get rid of any reason, beyond emergency
> cases, for people to modify the catalog directly. There's a few places
> which we aren't yet doing that, but I'd rather fix those cases than
> encourage people to give out rights to modify them and end up making
> things like:
>
> "UPDATE pg_database SET datallowconn = false where datname = 'xyz';"
>
> an accepted interface.

I'm not sure those things are related.

Getting rid of the *reasons* for updating catalogs directly might be
worthwhile, but I don't see why we need to install (or maintain) a
special invisible permission trap for it. We have a permission system,
and it works pretty well.

The Unix kernels don't have special traps for someone to modify
/etc/shadow or similar directly. That file has appropriate permissions,
and that's it. I think that works pretty well.

Peter,

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On 2/25/15 10:05 PM, Stephen Frost wrote:
> > Agreed, but I'd also like to get rid of any reason, beyond emergency
> > cases, for people to modify the catalog directly. There's a few places
> > which we aren't yet doing that, but I'd rather fix those cases than
> > encourage people to give out rights to modify them and end up making
> > things like:
> >
> > "UPDATE pg_database SET datallowconn = false where datname = 'xyz';"
> >
> > an accepted interface.
>
> I'm not sure those things are related.

Eh, I feel they are, but either way.

> Getting rid of the *reasons* for updating catalogs directly might be
> worthwhile, but I don't see why we need to install (or maintain) a
> special invisible permission trap for it. We have a permission system,
> and it works pretty well.

We have this "invisible permission trap" known as checking if you're a
superuser all over the place. I'm not against revisiting those cases
and considering using the GRANT permission system to manage access, but
that's certainly a larger project to work on. What I'm referring to
here are all the functions that check if you're a superuser, instead of
just revoking EXECUTE from public and letting the user manage the
permission.

> The Unix kernels don't have special traps for someone to modify
> /etc/shadow or similar directly. That file has appropriate permissions,
> and that's it. I think that works pretty well.

This isn't really /etc/shadow though, this is more like direct access to
the filesystem through the device node- and you'll note that Linux
certainly has got an independent set of permissions for that called the
capabilities system. That's because messing with those pieces can crash
the kernel. You're not going to crash the kernel if you goof up
/etc/shadow.

Thanks!

Stephen

On 2/28/15 6:32 PM, Stephen Frost wrote:
> This isn't really /etc/shadow though, this is more like direct access to
> the filesystem through the device node- and you'll note that Linux
> certainly has got an independent set of permissions for that called the
> capabilities system. That's because messing with those pieces can crash
> the kernel. You're not going to crash the kernel if you goof up
> /etc/shadow.

I think this characterization is incorrect. The Linux capability system
does not exist because the actions are scary or can crash the kernel.
Capabilities exist because they are not attached to file system objects
and can therefore not be represented using the usual permission system.

Note that one can write directly to raw devices or the kernel memory
through various /dev and /proc files. No "capability" protects against
that. It's only the file permissions, possibly in combination with
mount options.

Any other opinions?

The options are:

0) Leave as is.

1) Remove catupdate and replace with superuser checks.

2) Remove catupdate and rely on regular table permissions on catalogs.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Any other opinions?
> The options are:
> 0) Leave as is.
> 1) Remove catupdate and replace with superuser checks.
> 2) Remove catupdate and rely on regular table permissions on catalogs.

I think I vote for (1). I do not like (2) because of the argument I made
upthread that write access on system catalogs is far more dangerous than
a naive DBA might think. (0) has some attraction but really CATUPDATE
is one more concept than we need.

On the other hand, if Stephen is going to push forward with his plan to
subdivide superuserness, we might have the equivalent of CATUPDATE right
back again. (But at least it would be properly documented and
supported...)

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > Any other opinions?
> > The options are:
> > 0) Leave as is.
> > 1) Remove catupdate and replace with superuser checks.
> > 2) Remove catupdate and rely on regular table permissions on catalogs.
>
> I think I vote for (1). I do not like (2) because of the argument I made
> upthread that write access on system catalogs is far more dangerous than
> a naive DBA might think. (0) has some attraction but really CATUPDATE
> is one more concept than we need.

I agree with #1 on this.

> On the other hand, if Stephen is going to push forward with his plan to
> subdivide superuserness, we might have the equivalent of CATUPDATE right
> back again. (But at least it would be properly documented and
> supported...)

The subdivision of superuserness is intended only for operations which
can't be used to directly give the user superuser access back and
therefore I don't think we'd ever put back CATUPDATE or an equivilant.

I'd much rather reduce the need for direct catalog modifications by
adding additional syntax for those operations which can't be done
without modifying the catalog directly and, where it makes sense to, add
a way to control access to those operations.

For example, changing a database to not accept connections seems like
something the database owner should be allowed to do. Perhaps that'd be
interesting to allow users other than the owner to do, perhaps it
doesn't, but that would be an independent question to address.

Thanks!

Stephen

All,

Thanks for all the feedback and review.

> I think I vote for (1). I do not like (2) because of the argument I made
> > upthread that write access on system catalogs is far more dangerous than
> > a naive DBA might think. (0) has some attraction but really CATUPDATE
> > is one more concept than we need.
>
> I agree with #1 on this.
>

#1 makes sense to me as well. The current version of the patch takes this
approach. Also, I have verified against 'master' as of c6ee39b.

Thanks,
Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On 12/29/14 7:16 PM, Adam Brightwell wrote:
> Given this discussion, I have attached a patch that removes CATUPDATE
> for review/discussion.

committed this version

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 12/29/14 7:16 PM, Adam Brightwell wrote:
>> Given this discussion, I have attached a patch that removes CATUPDATE
>> for review/discussion.

> committed this version

Hmm .. I'm not sure that summarily removing usecatupd from those three
system views was well thought out. pg_shadow, especially, has no reason
to live at all except for backwards compatibility, and clients might well
expect that column to still be there. I wonder if we'd not be better off
to keep the column in the views but have it read from rolsuper.

regards, tom lane

On 3/7/15 12:31 AM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> On 12/29/14 7:16 PM, Adam Brightwell wrote:
>>> Given this discussion, I have attached a patch that removes CATUPDATE
>>> for review/discussion.
>
>> committed this version
>
> Hmm .. I'm not sure that summarily removing usecatupd from those three
> system views was well thought out. pg_shadow, especially, has no reason
> to live at all except for backwards compatibility, and clients might well
> expect that column to still be there. I wonder if we'd not be better off
> to keep the column in the views but have it read from rolsuper.

I doubt anyone is reading the column. And if they are, they should stop.

pg_shadow and pg_user have been kept around because it is plausible that
a lot of tools want to have a list of users, and requiring all of them
to change to pg_authid at once was deemed too onerous at the time. I
don't think this requires us to keep all the details the same forever.

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On 3/7/15 12:31 AM, Tom Lane wrote:
> > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> >> On 12/29/14 7:16 PM, Adam Brightwell wrote:
> >>> Given this discussion, I have attached a patch that removes CATUPDATE
> >>> for review/discussion.
> >
> >> committed this version
> >
> > Hmm .. I'm not sure that summarily removing usecatupd from those three
> > system views was well thought out. pg_shadow, especially, has no reason
> > to live at all except for backwards compatibility, and clients might well
> > expect that column to still be there. I wonder if we'd not be better off
> > to keep the column in the views but have it read from rolsuper.
>
> I doubt anyone is reading the column. And if they are, they should stop.

Certainly pgAdmin and similar tools are. From that standpoint though, I
agree that they should be modified to no longer read it, as the whole
point of those kinds of tools is to allow users to modify those
attributes and having CATUPDATE in the view would surely be confusing as
the user wouldn't be able to modify it.

> pg_shadow and pg_user have been kept around because it is plausible that
> a lot of tools want to have a list of users, and requiring all of them
> to change to pg_authid at once was deemed too onerous at the time. I
> don't think this requires us to keep all the details the same forever.

pg_shadow, pg_user and pg_group were added when role support was added,
specifically for backwards compatibility. I don't believe there was
ever discussion about keeping them because filtering pg_roles based on
rolcanlogin was too onerous. That said, we already decided recently
that we wanted to keep them updated to match the actual attributes
available (note that the replication role attribute modified those
views) and I think that makes sense on the removal side as well as the
new-attribute side.

I continue to feel that we really should officially deprecate those
views as I don't think they're actually all that useful any more and
maintaining them ends up bringing up all these questions, discussion,
and ends up being largely busy-work if no one really uses them.

Thanks,

Stephen

All,

> pg_shadow, pg_user and pg_group were added when role support was added,
> specifically for backwards compatibility. I don't believe there was
> ever discussion about keeping them because filtering pg_roles based on
> rolcanlogin was too onerous. That said, we already decided recently
> that we wanted to keep them updated to match the actual attributes
> available (note that the replication role attribute modified those
> views) and I think that makes sense on the removal side as well as the
> new-attribute side.
>
> I continue to feel that we really should officially deprecate those
> views as I don't think they're actually all that useful any more and
> maintaining them ends up bringing up all these questions, discussion,
> and ends up being largely busy-work if no one really uses them.
>

Deprecation would certainly seem like an appropriate path for 'usecatupd'
(and the mentioned views). Though, is there a 'formal' deprecation
policy/process that the community follows? I certainly understand and
support giving client/dependent tools the time and opportunity to update
accordingly. Therefore, I think having them read from 'rolsuper' for the
time being would be ok, provided that they are actually removed at the next
possible opportunity. Otherwise, I'd probably lean towards just removing
them now and getting it over with.

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On Sat, Mar 7, 2015 at 6:40 PM, Adam Brightwell
<adam(dot)brightwell(at)crunchydatasolutions(dot)com> wrote:
>> pg_shadow, pg_user and pg_group were added when role support was added,
>> specifically for backwards compatibility. I don't believe there was
>> ever discussion about keeping them because filtering pg_roles based on
>> rolcanlogin was too onerous. That said, we already decided recently
>> that we wanted to keep them updated to match the actual attributes
>> available (note that the replication role attribute modified those
>> views) and I think that makes sense on the removal side as well as the
>> new-attribute side.
>>
>> I continue to feel that we really should officially deprecate those
>> views as I don't think they're actually all that useful any more and
>> maintaining them ends up bringing up all these questions, discussion,
>> and ends up being largely busy-work if no one really uses them.
>
> Deprecation would certainly seem like an appropriate path for 'usecatupd'
> (and the mentioned views). Though, is there a 'formal' deprecation
> policy/process that the community follows? I certainly understand and
> support giving client/dependent tools the time and opportunity to update
> accordingly. Therefore, I think having them read from 'rolsuper' for the
> time being would be ok, provided that they are actually removed at the next
> possible opportunity. Otherwise, I'd probably lean towards just removing
> them now and getting it over with.

Unless some popular tool like pgAdmin is using these views, I'd say we
should just nuke them outright. If it breaks somebody's installation,
then they can always recreate the view on their particular system.
That seems like an adequate workaround to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Sat, Mar 7, 2015 at 6:40 PM, Adam Brightwell
> <adam(dot)brightwell(at)crunchydatasolutions(dot)com> wrote:
> >> pg_shadow, pg_user and pg_group were added when role support was added,
> >> specifically for backwards compatibility. I don't believe there was
> >> ever discussion about keeping them because filtering pg_roles based on
> >> rolcanlogin was too onerous. That said, we already decided recently
> >> that we wanted to keep them updated to match the actual attributes
> >> available (note that the replication role attribute modified those
> >> views) and I think that makes sense on the removal side as well as the
> >> new-attribute side.
> >>
> >> I continue to feel that we really should officially deprecate those
> >> views as I don't think they're actually all that useful any more and
> >> maintaining them ends up bringing up all these questions, discussion,
> >> and ends up being largely busy-work if no one really uses them.
> >
> > Deprecation would certainly seem like an appropriate path for 'usecatupd'
> > (and the mentioned views). Though, is there a 'formal' deprecation
> > policy/process that the community follows? I certainly understand and
> > support giving client/dependent tools the time and opportunity to update
> > accordingly. Therefore, I think having them read from 'rolsuper' for the
> > time being would be ok, provided that they are actually removed at the next
> > possible opportunity. Otherwise, I'd probably lean towards just removing
> > them now and getting it over with.
>
> Unless some popular tool like pgAdmin is using these views, I'd say we
> should just nuke them outright. If it breaks somebody's installation,
> then they can always recreate the view on their particular system.
> That seems like an adequate workaround to me.

As near as I can tell, pgAdmin3 does still use pg_user (though I don't
think it uses pg_group or pg_shadow except when connected to an ancient
server) in some cases. Where it is used, based on my quick review at
least, it looks like it'd be pretty easy to fix.

The rolcatupdate usage appears to all be associated with pg_authid
though, and the changes required to remove the places where it shows up
doesn't look particularly bad either. There are no references to
usecatupdate. Where there are references to 'use*', they'd have to also
be updated with the change to pg_user, naturally.

Looking at phppgadmin, there are quite a few more uses of pg_user there,
along with references to pg_group and even pg_shadow (for 8.0 and
earlier backends). Amusingly, the only place 'catupdate' is referenced
there is in the Polish language file. Updating phppgadmin to not
reference pg_user or the other views looks like it'd be a bit more work,
but not a huge effort either.

Basically, in my view at least, these programs are likely to continue to
use these backwards compatibility views until we either formally
deprecate them or (more likely) until we actually remove them and things
break. As such, I'm not sure if this information really helps us make a
decision here.

Thanks!

Stephen

On Thu, Mar 12, 2015 at 10:36 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> As near as I can tell, pgAdmin3 does still use pg_user (though I don't
> think it uses pg_group or pg_shadow except when connected to an ancient
> server) in some cases. Where it is used, based on my quick review at
> least, it looks like it'd be pretty easy to fix.
>
> The rolcatupdate usage appears to all be associated with pg_authid
> though, and the changes required to remove the places where it shows up
> doesn't look particularly bad either. There are no references to
> usecatupdate. Where there are references to 'use*', they'd have to also
> be updated with the change to pg_user, naturally.
>
> Looking at phppgadmin, there are quite a few more uses of pg_user there,
> along with references to pg_group and even pg_shadow (for 8.0 and
> earlier backends). Amusingly, the only place 'catupdate' is referenced
> there is in the Polish language file. Updating phppgadmin to not
> reference pg_user or the other views looks like it'd be a bit more work,
> but not a huge effort either.
>
> Basically, in my view at least, these programs are likely to continue to
> use these backwards compatibility views until we either formally
> deprecate them or (more likely) until we actually remove them and things
> break. As such, I'm not sure if this information really helps us make a
> decision here.

After poking at this a bit, I am guessing the reason they still use
pg_user is that regular users don't have permission to access
pg_authid directly. We don't want to make it impossible for pgAdmin
to work for non-superusers.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Mar 12, 2015 at 10:36 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Basically, in my view at least, these programs are likely to continue to
> > use these backwards compatibility views until we either formally
> > deprecate them or (more likely) until we actually remove them and things
> > break. As such, I'm not sure if this information really helps us make a
> > decision here.
>
> After poking at this a bit, I am guessing the reason they still use
> pg_user is that regular users don't have permission to access
> pg_authid directly. We don't want to make it impossible for pgAdmin
> to work for non-superusers.

I should have been more specific. I don't believe they've moved to
using pg_roles completely (which was created specifically to address the
issue that regular users can't select from pg_authid). Both of the
tools being discussed use pg_roles also (and, in fact, I believe they
have to for certain fields which pg_user doesn't include..
rolcreaterole being a pretty big one, but also rolconnlimit and
rolinherit, which wouldn't make sense in pg_user anyway..). I agree
that they can't simply move to pg_authid today since only superusers
have access to that table today. Of course, with column-level
privileges, we could potentially change that too..

In any case, looking at this again, it seems clear that we've not been
keeping pg_user up to date and no one seems to care. I don't think it
makes sense to go back and try to add "useconnlimit", "usecanlogin",
"usecreateuser" for 9.5 when pg_user was only ever intended for
backwards compatibility and clearly hasn't been getting the love and
attention it would deserve if it was really useful.

As such, I'm coming down on the side of simply removing pg_user,
pg_shadow, and pg_group for 9.5. Having a half-maintained mish-mash of
things from pg_authid making their way into pg_user/pg_shadow (which
suffers all the same problems of missing important fields) isn't doing
anyone any favors, and pg_group is an inefficient way of getting at the
information that's in pg_auth_members which implies that groups are
somehow different from roles, which hasn't been the case in 10 years.

Thanks!

Stephen

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> I should have been more specific. I don't believe they've moved to
> using pg_roles completely (which was created specifically to address the
> issue that regular users can't select from pg_authid).

Err, forgot to finish that thought, sorry. Let's try again:

I should have been more specific. I don't believe they've moved to
using pg_roles completely (which was created specifically to address the
issue that regular users can't select from pg_authid) simply because
they haven't had reason to.

Thanks,

Stephen

On Fri, Mar 13, 2015 at 9:52 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
>> I should have been more specific. I don't believe they've moved to
>> using pg_roles completely (which was created specifically to address the
>> issue that regular users can't select from pg_authid).
>
> Err, forgot to finish that thought, sorry. Let's try again:
>
> I should have been more specific. I don't believe they've moved to
> using pg_roles completely (which was created specifically to address the
> issue that regular users can't select from pg_authid) simply because
> they haven't had reason to.

That's another way of saying "removing pg_user would be creating extra
work for tool authors that otherwise wouldn't need to be done".

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Fri, Mar 13, 2015 at 9:52 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> >> I should have been more specific. I don't believe they've moved to
> >> using pg_roles completely (which was created specifically to address the
> >> issue that regular users can't select from pg_authid).
> >
> > Err, forgot to finish that thought, sorry. Let's try again:
> >
> > I should have been more specific. I don't believe they've moved to
> > using pg_roles completely (which was created specifically to address the
> > issue that regular users can't select from pg_authid) simply because
> > they haven't had reason to.
>
> That's another way of saying "removing pg_user would be creating extra
> work for tool authors that otherwise wouldn't need to be done".

Sure, if we never deprecate anything then tool authors would never need
to change their existing code. I don't think that's actually a viable
solution though; the reason we're discussing the removal of these
particular views is that they aren't really being maintained and, when
they are, they're making work for us. That's certainly a trade-off to
consider, of course, but in this case I'm coming down on the side of
dropping support and our own maintenance costs associated with these
views in favor of asking the tooling community to complete the migration
to the new views which have been around for the past 10 years.

Thanks!

Stephen

All,

Sure, if we never deprecate anything then tool authors would never need
> to change their existing code. I don't think that's actually a viable
> solution though; the reason we're discussing the removal of these
> particular views is that they aren't really being maintained and, when
> they are, they're making work for us. That's certainly a trade-off to
> consider, of course, but in this case I'm coming down on the side of
> dropping support and our own maintenance costs associated with these
> views in favor of asking the tooling community to complete the migration
> to the new views which have been around for the past 10 years.
>

Perhaps this is naive or has been attempted in the past without success,
but would it be possible to maintain a list of deprecated features? I
noticed the following wiki page, (though it hasn't been updated recently)
that I think could be used for this purpose.

https://wiki.postgresql.org/wiki/Deprecated_Features

Using this page as a model, having an "official deprecation list" that does
the following might be very useful:

* Lists feature that is deprecated.
* Reason it was deprecated.
* What to use instead, perhaps with example.
* Version the feature will be removed.

Or perhaps such a list could be included as part of the official
documentation? In either case, if it is well known that such a list is
available/exists then tool developers, etc. should have adequate time,
opportunity and information to make the appropriate changes to their
products with a "minimal" impact.

Thoughts?

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com