Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix psql \?'s entries for \dp and \z.

  2. psql: Add support for \dpS and \zS.

  3. Add grantable MAINTAIN privilege and pg_maintain role.

  1. add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-06T19:36:06Z

    Hi hackers,

As discussed elsewhere [0], \dp doesn't show privileges on system objects,
and this behavior is not mentioned in the docs.  I've attached a small
patch that adds support for the S modifier (i.e., \dpS) and the adjusts the
docs.

Thoughts?

[0] https://postgr.es/m/a2382acd-e465-85b2-9d8e-f9ed1a5a66e9%40postgrespro.ru

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  2. Re: add \dpS to psql

    Pavel Luzanov <p.luzanov@postgrespro.ru> — 2022-12-07T07:48:49Z

    On 06.12.2022 22:36, Nathan Bossart wrote:

> As discussed elsewhere [0], \dp doesn't show privileges on system objects,
> and this behavior is not mentioned in the docs.  I've attached a small
> patch that adds support for the S modifier (i.e., \dpS) and the adjusts the
> docs.
>
> Thoughts?
>
> [0] https://postgr.es/m/a2382acd-e465-85b2-9d8e-f9ed1a5a66e9%40postgrespro.ru

A few words in support of this patch, since I was the initiator of the 
discussion.

Before VACUUM, ANALYZE privileges, there was no such question.
Why check privileges on system catalog objects? But now it doesn't.

It is now possible to grant privileges on system tables,
so it should be possible to see privileges with psql commands.
However, the \dp command does not support the S modifier, which is 
inconsistent.

Furthermore. The VACUUM privilege allows you to also execute VACUUM FULL.
VACUUM and VACUUM FULL are commands with similar names, but work 
completely differently.
It may be worth clarifying on this page: 
https://www.postgresql.org/docs/devel/ddl-priv.html

Something like: Allows VACUUM on a relation, including VACUUM FULL.

But that's not all.

There is a very similar command to VACUUM FULL with a different name - 
CLUSTER.
The VACUUM privilege does not apply to the CLUSTER command. This is 
probably correct.
However, the documentation for the CLUSTER command does not say
who can perform this command. I think it would be correct to add a sentence
to the Notes section 
(https://www.postgresql.org/docs/devel/sql-cluster.html)
similar to the one in the VACUUM documentation:

"To cluster a table, one must ordinarily be the table's owner or a 
superuser."

Ready to participate, if it seems reasonable.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

  3. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-07T22:41:05Z

    On Wed, Dec 07, 2022 at 10:48:49AM +0300, Pavel Luzanov wrote:
> There is a very similar command to VACUUM FULL with a different name -
> CLUSTER.
> The VACUUM privilege does not apply to the CLUSTER command. This is probably
> correct.
> However, the documentation for the CLUSTER command does not say
> who can perform this command. I think it would be correct to add a sentence
> to the Notes section
> (https://www.postgresql.org/docs/devel/sql-cluster.html)
> similar to the one in the VACUUM documentation:
> 
> "To cluster a table, one must ordinarily be the table's owner or a
> superuser."

I created a new thread for this:

	https://postgr.es/m/20221207223924.GA4182184%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  4. Re: add \dpS to psql

    Justin Pryzby <pryzby@telsasoft.com> — 2022-12-08T02:39:30Z

    On Wed, Dec 07, 2022 at 10:48:49AM +0300, Pavel Luzanov wrote:
> Furthermore. The VACUUM privilege allows you to also execute VACUUM FULL.
> VACUUM and VACUUM FULL are commands with similar names, but work completely
> differently.
> It may be worth clarifying on this page:
> https://www.postgresql.org/docs/devel/ddl-priv.html
> 
> Something like: Allows VACUUM on a relation, including VACUUM FULL.

Since (as you said) they work completely differently, I think it'd be
more useful if vacuum_full were a separate privilege, rather than being
included in vacuum.  And cluster could be allowed whenever vacuum_full
is allowed.

> There is a very similar command to VACUUM FULL with a different name -
> CLUSTER.  The VACUUM privilege does not apply to the CLUSTER command.
> This is probably correct.

I think if vacuum privilege allows vacuum full, then it ought to also
allow cluster.  But I suggest that it'd be even better if it doesn't
allow either, and there was a separate privilege for those.

Disclaimer: I have not been following these threads.

-- 
Justin

  5. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-08T04:19:00Z

    On Wed, Dec 07, 2022 at 08:39:30PM -0600, Justin Pryzby wrote:
> I think if vacuum privilege allows vacuum full, then it ought to also
> allow cluster.  But I suggest that it'd be even better if it doesn't
> allow either, and there was a separate privilege for those.
> 
> Disclaimer: I have not been following these threads.

I haven't formed an opinion on whether VACUUM FULL should get its own bit,
but FWIW І just finished writing the first draft of a patch set to add bits
for CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX.  I plan to post that
tomorrow.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  6. Re: add \dpS to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-08T04:25:32Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> I haven't formed an opinion on whether VACUUM FULL should get its own bit,
> but FWIW І just finished writing the first draft of a patch set to add bits
> for CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX.  I plan to post that
> tomorrow.

The fact that we just doubled the number of available bits doesn't
mean we should immediately strive to use them up.  Perhaps it'd
be better to subsume these retail privileges under some generic
"maintenance action" privilege?

			regards, tom lane

  7. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-08T04:43:14Z

    On Wed, Dec 07, 2022 at 11:25:32PM -0500, Tom Lane wrote:
> The fact that we just doubled the number of available bits doesn't
> mean we should immediately strive to use them up.  Perhaps it'd
> be better to subsume these retail privileges under some generic
> "maintenance action" privilege?

That's fine with me, but I wouldn't be surprised if there's disagreement on
how to group the commands.  I certainly don't want to use up the rest of
the bits right away, but there might not be too many more existing
privileges after these three that deserve them.  Perhaps I should take
inventory and offer a plan for all the remaining privileges.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  8. Re: add \dpS to psql

    Isaac Morland <isaac.morland@gmail.com> — 2022-12-08T04:48:20Z

    On Wed, 7 Dec 2022 at 23:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Nathan Bossart <nathandbossart@gmail.com> writes:
> > I haven't formed an opinion on whether VACUUM FULL should get its own
> bit,
> > but FWIW І just finished writing the first draft of a patch set to add
> bits
> > for CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX.  I plan to post that
> > tomorrow.
>
> The fact that we just doubled the number of available bits doesn't
> mean we should immediately strive to use them up.  Perhaps it'd
> be better to subsume these retail privileges under some generic
> "maintenance action" privilege?
>

That was my original suggestion:

https://www.postgresql.org/message-id/CAMsGm5c4DycKBYZCypfV02s-SC8GwF%2BKeTt%3D%3DvbWrFn%2Bdz%3DKeg%40mail.gmail.com

In that message I review the history of permission bit growth. A bit later
in the discussion, I did some investigation into the history of demand for
new permission bits and I proposed calling the new privilege MAINTAIN:

https://www.postgresql.org/message-id/CAMsGm5d%3D2gi4kyKONUJyYFwen%3DbsWm4hz_KxLXkEhMmg5WSWTA%40mail.gmail.com

For what it's worth, I wouldn't bother changing the format of the
permission bits to expand the pool of available bits. My previous analysis
shows that there is no vast hidden demand for new privilege bits. If we
implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH, CLUSTER,
and REINDEX, we will cover everything that I can find that has seriously
discussed on this list, and still leave 3 unused bits for future expansion.
There is even justification for stopping after this expansion: if it is
done, then schema changes (DDL) will only be able to be done by owner; data
changes (insert, update, delete, as well as triggering of automatic data
maintenance actions) will be able to be done by anybody who is granted
permission.

My guess is that if we ever do expand the privilege bit system, it should
be in a way that removes the limit entirely, replacing a bit map model with
something more like a table with one row for each individual grant, with a
field indicating which grant is involved. But that is a hypothetical future.

  9. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-08T05:07:54Z

    On Wed, Dec 07, 2022 at 11:48:20PM -0500, Isaac Morland wrote:
> For what it's worth, I wouldn't bother changing the format of the
> permission bits to expand the pool of available bits.

7b37823 expanded AclMode to 64 bits, so we now have room for 16 additional
privileges (after the addition of VACUUM and ANALYZE in b5d6382).

> My previous analysis
> shows that there is no vast hidden demand for new privilege bits. If we
> implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH, CLUSTER,
> and REINDEX, we will cover everything that I can find that has seriously
> discussed on this list, and still leave 3 unused bits for future expansion.

If we added CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX as individual
privilege bits, we'd still have 13 remaining for future use.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  10. Re: add \dpS to psql

    Isaac Morland <isaac.morland@gmail.com> — 2022-12-08T05:12:05Z

    On Thu, 8 Dec 2022 at 00:07, Nathan Bossart <nathandbossart@gmail.com>
wrote:

> On Wed, Dec 07, 2022 at 11:48:20PM -0500, Isaac Morland wrote:
> > For what it's worth, I wouldn't bother changing the format of the
> > permission bits to expand the pool of available bits.
>
> 7b37823 expanded AclMode to 64 bits, so we now have room for 16 additional
> privileges (after the addition of VACUUM and ANALYZE in b5d6382).
>
> > My previous analysis
> > shows that there is no vast hidden demand for new privilege bits. If we
> > implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH,
> CLUSTER,
> > and REINDEX, we will cover everything that I can find that has seriously
> > discussed on this list, and still leave 3 unused bits for future
> expansion.
>
> If we added CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX as individual
> privilege bits, we'd still have 13 remaining for future use.
>

I was a bit imprecise. I was comparing to the state before the recent
changes - so 12 bits used out of 16, with MAINTAIN being the 13th bit. I
think in my mind it's still approximately 2019 on some level.

  11. Re: add \dpS to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-08T05:15:23Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Dec 07, 2022 at 11:48:20PM -0500, Isaac Morland wrote:
>> My previous analysis
>> shows that there is no vast hidden demand for new privilege bits. If we
>> implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH, CLUSTER,
>> and REINDEX, we will cover everything that I can find that has seriously
>> discussed on this list, and still leave 3 unused bits for future expansion.

> If we added CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX as individual
> privilege bits, we'd still have 13 remaining for future use.

I think the appropriate question is not "have we still got bits left?".
It should be more like "under what plausible scenario would it be useful
to grant somebody CLUSTER but not VACUUM privileges on a table?".

I'm really thinking that MAINTAIN is the right level of granularity
here.  Or maybe it's worth segregating exclusive-lock from
not-exclusive-lock maintenance.  But I really fail to see how it's
useful to distinguish CLUSTER from REINDEX, say.

			regards, tom lane

  12. Re: add \dpS to psql

    Pavel Luzanov <p.luzanov@postgrespro.ru> — 2022-12-08T11:03:43Z

    On 08.12.2022 07:48, Isaac Morland wrote:
> If we implement MAINTAIN to control access to VACUUM, ANALYZE, 
> REFRESH, CLUSTER, and REINDEX, we will cover everything that I can 
> find that has seriously discussed on this list

I like this approach with MAINTAIN privilege. I'm trying to find any 
disadvantages ... and I can't.

For the complete picture, I tried to see what other actions with the 
table could *potentially* be considered as maintenance.
Here is the list:

- create|alter|drop on extended statistics objects
- alter table|index alter column set statistics
- alter table|index [re]set (storage_parameters)
- alter table|index set tablespace
- alter table alter column set storage|compression
- any actions with the TOAST table that can be performed separately from 
the main table

I have to admit that the discussion has moved away from the $subject.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

  13. Re: add \dpS to psql

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-12-08T11:35:28Z

    On 2022-Dec-08, Pavel Luzanov wrote:

> For the complete picture, I tried to see what other actions with the table
> could *potentially* be considered as maintenance.
> Here is the list:
> 
> - create|alter|drop on extended statistics objects
> - alter table|index alter column set statistics
> - alter table|index [re]set (storage_parameters)
> - alter table|index set tablespace
> - alter table alter column set storage|compression
> - any actions with the TOAST table that can be performed separately from the
> main table

Well, I can't see that any of these is valuable to grant separately from
the table's owner.  The maintenance ones are the ones that are
interesting to run from a database-owner perspective, but these ones do
not seem to need that treatment.

If you're extremely generous you could think that ALTER .. SET STORAGE
would be reasonable to be run by the db-owner.  However, that's not
something you do on an ongoing basis -- you just do it once -- so it
seems pointless.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  14. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-08T17:15:03Z

    On Thu, Dec 08, 2022 at 12:15:23AM -0500, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> On Wed, Dec 07, 2022 at 11:48:20PM -0500, Isaac Morland wrote:
>>> My previous analysis
>>> shows that there is no vast hidden demand for new privilege bits. If we
>>> implement MAINTAIN to control access to VACUUM, ANALYZE, REFRESH, CLUSTER,
>>> and REINDEX, we will cover everything that I can find that has seriously
>>> discussed on this list, and still leave 3 unused bits for future expansion.
> 
>> If we added CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX as individual
>> privilege bits, we'd still have 13 remaining for future use.
> 
> I think the appropriate question is not "have we still got bits left?".
> It should be more like "under what plausible scenario would it be useful
> to grant somebody CLUSTER but not VACUUM privileges on a table?".
> 
> I'm really thinking that MAINTAIN is the right level of granularity
> here.  Or maybe it's worth segregating exclusive-lock from
> not-exclusive-lock maintenance.  But I really fail to see how it's
> useful to distinguish CLUSTER from REINDEX, say.

The main idea behind this work is breaking out privileges into more
granular pieces.  If I want to create a role that only runs VACUUM on some
tables on the weekend, why ѕhould I have to also give it the ability to
ANALYZE, REFRESH, CLUSTER, and REINDEX?  IMHO we should really let the user
decide what set of privileges makes sense for their use-case.  I'm unsure
the grouping all these privileges together serves much purpose besides
preserving ACL bits.

The other reason I'm hesitant to group the privileges together is because I
suspect it will be difficult to reach agreement on how to do so, as
evidenced by past discussion [0].  That being said, I'm open to it if we
find a way that folks are happy with.  For example, separating
exclusive-lock and non-exclusive-lock maintenance actions seems like a
reasonable idea (which perhaps is an argument for moving VACUUM FULL out of
the VACUUM privilege).

[0] https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  15. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-08T18:39:46Z

    I've created a new thread for making CLUSTER, REFRESH MATERIALIZED VIEW,
and REINDEX grantable:

	https://postgr.es/m/20221208183707.GA55474%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  16. Re: add \dpS to psql

    Michael Paquier <michael@paquier.xyz> — 2022-12-09T04:36:03Z

    On Thu, Dec 08, 2022 at 09:15:03AM -0800, Nathan Bossart wrote:
> The main idea behind this work is breaking out privileges into more
> granular pieces.  If I want to create a role that only runs VACUUM on some
> tables on the weekend, why ѕhould I have to also give it the ability to
> ANALYZE, REFRESH, CLUSTER, and REINDEX?  IMHO we should really let the user
> decide what set of privileges makes sense for their use-case.  I'm unsure
> the grouping all these privileges together serves much purpose besides
> preserving ACL bits.

Hmm.  I'd like to think that we should keep a frugal mind here.  More
bits are now available, but it does not strike me as a good idea to
force their usage more than necessary, so grouping all these no-quite
DDL commands into the same bag does not sound that bad to me.
--
Michael

  17. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-09T18:40:55Z

    On Fri, Dec 09, 2022 at 01:36:03PM +0900, Michael Paquier wrote:
> On Thu, Dec 08, 2022 at 09:15:03AM -0800, Nathan Bossart wrote:
>> The main idea behind this work is breaking out privileges into more
>> granular pieces.  If I want to create a role that only runs VACUUM on some
>> tables on the weekend, why ѕhould I have to also give it the ability to
>> ANALYZE, REFRESH, CLUSTER, and REINDEX?  IMHO we should really let the user
>> decide what set of privileges makes sense for their use-case.  I'm unsure
>> the grouping all these privileges together serves much purpose besides
>> preserving ACL bits.
> 
> Hmm.  I'd like to think that we should keep a frugal mind here.  More
> bits are now available, but it does not strike me as a good idea to
> force their usage more than necessary, so grouping all these no-quite
> DDL commands into the same bag does not sound that bad to me.

Okay, it seems I am outnumbered.  I will work on updating the patch to add
an ACL_MAINTAIN bit and a pg_maintain_all_tables predefined role.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  18. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-09T18:44:11Z

    On Fri, Dec 09, 2022 at 10:40:55AM -0800, Nathan Bossart wrote:
> On Fri, Dec 09, 2022 at 01:36:03PM +0900, Michael Paquier wrote:
>> On Thu, Dec 08, 2022 at 09:15:03AM -0800, Nathan Bossart wrote:
>>> The main idea behind this work is breaking out privileges into more
>>> granular pieces.  If I want to create a role that only runs VACUUM on some
>>> tables on the weekend, why ѕhould I have to also give it the ability to
>>> ANALYZE, REFRESH, CLUSTER, and REINDEX?  IMHO we should really let the user
>>> decide what set of privileges makes sense for their use-case.  I'm unsure
>>> the grouping all these privileges together serves much purpose besides
>>> preserving ACL bits.
>> 
>> Hmm.  I'd like to think that we should keep a frugal mind here.  More
>> bits are now available, but it does not strike me as a good idea to
>> force their usage more than necessary, so grouping all these no-quite
>> DDL commands into the same bag does not sound that bad to me.
> 
> Okay, it seems I am outnumbered.  I will work on updating the patch to add
> an ACL_MAINTAIN bit and a pg_maintain_all_tables predefined role.

Any thoughts on $SUBJECT?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  19. Re: add \dpS to psql

    Andrew Dunstan <andrew@dunslane.net> — 2022-12-12T12:01:01Z

    On 2022-12-09 Fr 13:44, Nathan Bossart wrote:
> On Fri, Dec 09, 2022 at 10:40:55AM -0800, Nathan Bossart wrote:
>> On Fri, Dec 09, 2022 at 01:36:03PM +0900, Michael Paquier wrote:
>>> On Thu, Dec 08, 2022 at 09:15:03AM -0800, Nathan Bossart wrote:
>>>> The main idea behind this work is breaking out privileges into more
>>>> granular pieces.  If I want to create a role that only runs VACUUM on some
>>>> tables on the weekend, why ѕhould I have to also give it the ability to
>>>> ANALYZE, REFRESH, CLUSTER, and REINDEX?  IMHO we should really let the user
>>>> decide what set of privileges makes sense for their use-case.  I'm unsure
>>>> the grouping all these privileges together serves much purpose besides
>>>> preserving ACL bits.
>>> Hmm.  I'd like to think that we should keep a frugal mind here.  More
>>> bits are now available, but it does not strike me as a good idea to
>>> force their usage more than necessary, so grouping all these no-quite
>>> DDL commands into the same bag does not sound that bad to me.
>> Okay, it seems I am outnumbered.  I will work on updating the patch to add
>> an ACL_MAINTAIN bit and a pg_maintain_all_tables predefined role.
> Any thoughts on $SUBJECT?


Yeah, the discussion got way off into the weeds here. I think the
original proposal seems reasonable. Please add it to the next CF if you
haven't already.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  20. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-12T17:18:22Z

    On Mon, Dec 12, 2022 at 07:01:01AM -0500, Andrew Dunstan wrote:
> On 2022-12-09 Fr 13:44, Nathan Bossart wrote:
>> Any thoughts on $SUBJECT?
> 
> Yeah, the discussion got way off into the weeds here. I think the
> original proposal seems reasonable. Please add it to the next CF if you
> haven't already.

Here it is: https://commitfest.postgresql.org/41/4043/

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  21. Re: add \dpS to psql

    Maxim Orlov <orlovmg@gmail.com> — 2022-12-28T11:46:23Z

    > Here it is: https://commitfest.postgresql.org/41/4043/
>

Hi!

The patch applies with no problem, implements what it declared, CF bot is
happy.
Without patch \dpS shows 0 rows, after applying system objects are shown.
Consider this patch useful, hope it will be committed soon.

-- 
Best regards,
Maxim Orlov.

  22. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-28T21:26:12Z

    On Wed, Dec 28, 2022 at 02:46:23PM +0300, Maxim Orlov wrote:
> The patch applies with no problem, implements what it declared, CF bot is
> happy.
> Without patch \dpS shows 0 rows, after applying system objects are shown.
> Consider this patch useful, hope it will be committed soon.

Thanks for reviewing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  23. Re: add \dpS to psql

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-01-06T18:52:33Z

    On Wed, 28 Dec 2022 at 21:26, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Wed, Dec 28, 2022 at 02:46:23PM +0300, Maxim Orlov wrote:
> > The patch applies with no problem, implements what it declared, CF bot is
> > happy.
> > Without patch \dpS shows 0 rows, after applying system objects are shown.
> > Consider this patch useful, hope it will be committed soon.
>
> Thanks for reviewing.
>

Looking this over this, I have a couple of comments:

Firstly, I think it should allow \zS in the same fashion as \dpS,
since \z is an alias for \dp, so the 2 should be kept in sync.

Secondly, I don't think the following is the right SQL clause to use
in the absence of "S":

    if (!showSystem && !pattern)
        appendPQExpBufferStr(&buf, "AND n.nspname !~ '^pg_'\n");

I know that's the condition it used before, but the problem with using
that now is that it will cause temporary relations to be excluded
unless the "S" modifier is used, which goes against the expectation
that "S" just causes system relations to be included. Also, it fails
to exclude information_schema relations, if that happens to be on the
user's search_path.

So I think we should use the same SQL clauses as every other psql
command that supports "S", namely:

    if (!showSystem && !pattern)
        appendPQExpBufferStr(&buf, "      AND n.nspname <> 'pg_catalog'\n"
                             "      AND n.nspname <> 'information_schema'\n");

Updated patch attached.

Regards,
Dean

  24. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2023-01-07T00:36:51Z

    On Fri, Jan 06, 2023 at 06:52:33PM +0000, Dean Rasheed wrote:
> Looking this over this, I have a couple of comments:

Thanks for reviewing.

> Firstly, I think it should allow \zS in the same fashion as \dpS,
> since \z is an alias for \dp, so the 2 should be kept in sync.

That seems reasonable to me.

> Secondly, I don't think the following is the right SQL clause to use
> in the absence of "S":
> 
>     if (!showSystem && !pattern)
>         appendPQExpBufferStr(&buf, "AND n.nspname !~ '^pg_'\n");
> 
> I know that's the condition it used before, but the problem with using
> that now is that it will cause temporary relations to be excluded
> unless the "S" modifier is used, which goes against the expectation
> that "S" just causes system relations to be included. Also, it fails
> to exclude information_schema relations, if that happens to be on the
> user's search_path.
> 
> So I think we should use the same SQL clauses as every other psql
> command that supports "S", namely:
> 
>     if (!showSystem && !pattern)
>         appendPQExpBufferStr(&buf, "      AND n.nspname <> 'pg_catalog'\n"
>                              "      AND n.nspname <> 'information_schema'\n");

Good catch.  I should have noticed this.  The deleted comment mentions that
the system/temp tables normally aren't very interesting from a permissions
perspective, so perhaps there is an argument for always excluding temp
tables without a pattern.  After all, \dp always excludes indexes and TOAST
tables.  However, it looks like \dt includes temp tables, and I didn't see
any other meta-commands that excluded them.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  25. Re: add \dpS to psql

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-01-07T11:18:59Z

    On Sat, 7 Jan 2023 at 00:36, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Fri, Jan 06, 2023 at 06:52:33PM +0000, Dean Rasheed wrote:
> >
> > So I think we should use the same SQL clauses as every other psql
> > command that supports "S", namely:
> >
> >     if (!showSystem && !pattern)
> >         appendPQExpBufferStr(&buf, "      AND n.nspname <> 'pg_catalog'\n"
> >                              "      AND n.nspname <> 'information_schema'\n");
>
> Good catch.  I should have noticed this.  The deleted comment mentions that
> the system/temp tables normally aren't very interesting from a permissions
> perspective, so perhaps there is an argument for always excluding temp
> tables without a pattern.  After all, \dp always excludes indexes and TOAST
> tables.  However, it looks like \dt includes temp tables, and I didn't see
> any other meta-commands that excluded them.
>

It might be true that temp tables aren't usually interesting from a
permissions point of view, but it's not hard to imagine situations
where interesting things do happen. It's also probably the case that
most users won't have many temp tables, so I don't think including
them by default will be particularly intrusive.

Also, from a user perspective, I think it would be something of a POLA
violation for \dp[S] and \dt[S] to include different sets of tables,
though I appreciate that we do that now. There's nothing in the docs
to indicate that that's the case.

Anyway, I've pushed the v2 patch as-is. If anyone feels strongly
enough that we should change its behaviour for temp tables, then we
can still discuss that.

Regards,
Dean

  26. Re: add \dpS to psql

    Nathan Bossart <nathandbossart@gmail.com> — 2023-01-09T17:45:39Z

    On Sat, Jan 07, 2023 at 11:18:59AM +0000, Dean Rasheed wrote:
> It might be true that temp tables aren't usually interesting from a
> permissions point of view, but it's not hard to imagine situations
> where interesting things do happen. It's also probably the case that
> most users won't have many temp tables, so I don't think including
> them by default will be particularly intrusive.
> 
> Also, from a user perspective, I think it would be something of a POLA
> violation for \dp[S] and \dt[S] to include different sets of tables,
> though I appreciate that we do that now. There's nothing in the docs
> to indicate that that's the case.

Agreed.

> Anyway, I've pushed the v2 patch as-is. If anyone feels strongly
> enough that we should change its behaviour for temp tables, then we
> can still discuss that.

Thanks!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  27. RE: add \dpS to psq [16beta1]

    Shinoda, Noriyoshi <noriyoshi.shinoda@hpe.com> — 2023-06-29T02:11:43Z

    Hi,
Thank you for developing a good feature.
I found while testing PostgreSQL 16 Beta 1 that the output of the \? metacommand did not include \dS, \dpS. 
The attached patch changes the output of the \? meta command to:

Current output
psql=# \? 
  \z      [PATTERN]      same as \dp
  \dp     [PATTERN]      list table, view, and sequence access privileges

Patched output
psql=# \?
  \dp[S]  [PATTERN]      list table, view, and sequence access privileges
  \z[S]   [PATTERN]      same as \dp

Regards,
Noriyoshi Shinoda

-----Original Message-----
From: Nathan Bossart <nathandbossart@gmail.com> 
Sent: Tuesday, January 10, 2023 2:46 AM
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Maxim Orlov <orlovmg@gmail.com>; Andrew Dunstan <andrew@dunslane.net>; Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Isaac Morland <isaac.morland@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; Pavel Luzanov <p.luzanov@postgrespro.ru>; pgsql-hackers@postgresql.org
Subject: Re: add \dpS to psql

On Sat, Jan 07, 2023 at 11:18:59AM +0000, Dean Rasheed wrote:
> It might be true that temp tables aren't usually interesting from a 
> permissions point of view, but it's not hard to imagine situations 
> where interesting things do happen. It's also probably the case that 
> most users won't have many temp tables, so I don't think including 
> them by default will be particularly intrusive.
> 
> Also, from a user perspective, I think it would be something of a POLA 
> violation for \dp[S] and \dt[S] to include different sets of tables, 
> though I appreciate that we do that now. There's nothing in the docs 
> to indicate that that's the case.

Agreed.

> Anyway, I've pushed the v2 patch as-is. If anyone feels strongly 
> enough that we should change its behaviour for temp tables, then we 
> can still discuss that.

Thanks!

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  28. Re: add \dpS to psq [16beta1]

    Nathan Bossart <nathandbossart@gmail.com> — 2023-06-29T04:38:58Z

    On Thu, Jun 29, 2023 at 02:11:43AM +0000, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
> I found while testing PostgreSQL 16 Beta 1 that the output of the \? metacommand did not include \dS, \dpS. 
> The attached patch changes the output of the \? meta command to:

Thanks for the report!  I've committed your patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com