Обсуждение: Fix output of zero privileges in psql

I wrote a patch to change psql's display of zero privileges after a user's
reported confusion with the psql output for zero vs. default privileges [1].
Admittedly, zero privileges is a rare use case [2] but I think psql should not
confuse the user in the off chance that this happens.

With this change psql now prints "(none)" for zero privileges instead of
nothing.  This affects the following meta commands:

    \db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l

Default privileges start as NULL::aclitem[] in various catalog columns but
revoking the default privileges leaves an empty aclitem array.  Using
\pset null '(null)' as a workaround to spot default privileges does not work
because the meta commands ignore this setting.

The privileges shown by \dconfig+ and \ddp as well as the column privileges
shown by \dp are not affected by this change because those privileges are reset
to NULL instead of leaving empty arrays.

Commands \des+ and \dew+ are not covered in src/test/regress because no foreign
data wrapper is available at this point to create a foreign server.

[1] https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com
[2] https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us

--
Erik

On 17/09/2023 21:31 CEST Erik Wienhold <ewie@ewie.name> wrote:

> This affects the following meta commands:
>
>     \db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l

also \des+ and \dew+

--
Erik

On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote:
> I wrote a patch to change psql's display of zero privileges after a user's
> reported confusion with the psql output for zero vs. default privileges [1].
> Admittedly, zero privileges is a rare use case [2] but I think psql should not
> confuse the user in the off chance that this happens.
>
> With this change psql now prints "(none)" for zero privileges instead of
> nothing.  This affects the following meta commands:
>
>     \db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
>
> Default privileges start as NULL::aclitem[] in various catalog columns but
> revoking the default privileges leaves an empty aclitem array.  Using
> \pset null '(null)' as a workaround to spot default privileges does not work
> because the meta commands ignore this setting.
>
> The privileges shown by \dconfig+ and \ddp as well as the column privileges
> shown by \dp are not affected by this change because those privileges are reset
> to NULL instead of leaving empty arrays.
>
> Commands \des+ and \dew+ are not covered in src/test/regress because no foreign
> data wrapper is available at this point to create a foreign server.
>
> [1] https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com
> [2] https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us

Reading that thread, I had the impression that there was more support for
honoring "\pset null" rather than unconditionally displaying "(none)".

The simple attached patch does it like that.  What do you think?

Yours,
Laurenz Albe

On 2023-10-06 22:32 +0200, Laurenz Albe write:
> On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote:
> > I wrote a patch to change psql's display of zero privileges after a user's
> > reported confusion with the psql output for zero vs. default privileges [1].
> > Admittedly, zero privileges is a rare use case [2] but I think psql should not
> > confuse the user in the off chance that this happens.
> > 
> > With this change psql now prints "(none)" for zero privileges instead of
> > nothing.  This affects the following meta commands:
> > 
> >     \db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
> > 
> > Default privileges start as NULL::aclitem[] in various catalog columns but
> > revoking the default privileges leaves an empty aclitem array.  Using
> > \pset null '(null)' as a workaround to spot default privileges does not work
> > because the meta commands ignore this setting.
> > 
> > The privileges shown by \dconfig+ and \ddp as well as the column privileges
> > shown by \dp are not affected by this change because those privileges are reset
> > to NULL instead of leaving empty arrays.
> > 
> > Commands \des+ and \dew+ are not covered in src/test/regress because no foreign
> > data wrapper is available at this point to create a foreign server.
> > 
> > [1] https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com
> > [2] https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us
> 
> Reading that thread, I had the impression that there was more support for
> honoring "\pset null" rather than unconditionally displaying "(none)".

I took Tom's response in the -general thread to mean that we could fix
\pset null also as a "nice to have" but not as a solution to the display
of zero privileges.

Only fixing \pset null has one drawback IMO because it only affects how
default privileges (more common) are printed.  The edge case of zero
privileges (less common) gets lost in a bunch of NULL output.  And I
assume most users change the default \pset null to some non-empty string
in their psqlrc (I do).

For example with your patch applied:

    create table t1 (a int);
    create table t2 (a int);
    create table t3 (a int);

    revoke all on t2 from :USER;

    \pset null <NULL>
    \dp t1|t2|t3
                                Access privileges
     Schema | Name | Type  | Access privileges | Column privileges | Policies
    --------+------+-------+-------------------+-------------------+----------
     public | t1   | table | <NULL>            |                   |
     public | t2   | table |                   |                   |
     public | t3   | table | <NULL>            |                   |
    (3 rows)

Instead of only displaying the zero privileges with my patch and default
\pset null:

    \pset null ''
    \dp t1|t2|t3
                                Access privileges
     Schema | Name | Type  | Access privileges | Column privileges | Policies
    --------+------+-------+-------------------+-------------------+----------
     public | t1   | table |                   |                   |
     public | t2   | table | (none)            |                   |
     public | t3   | table |                   |                   |
    (3 rows)

I guess if most tables have any non-default privileges then both
solutions are equally good.

> The simple attached patch does it like that.  What do you think?

LGTM.

-- 
Erik

On Sat, 2023-10-07 at 05:07 +0200, Erik Wienhold wrote:
> On 2023-10-06 22:32 +0200, Laurenz Albe write:
> > On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote:
> > > I wrote a patch to change psql's display of zero privileges after a user's
> > > reported confusion with the psql output for zero vs. default privileges [1].
> > > Admittedly, zero privileges is a rare use case [2] but I think psql should not
> > > confuse the user in the off chance that this happens.
> > >
> > > [1] https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com
> > > [2] https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us
> >
> > Reading that thread, I had the impression that there was more support for
> > honoring "\pset null" rather than unconditionally displaying "(none)".
>
> For example with your patch applied:
>
>         create table t1 (a int);
>         create table t2 (a int);
>         create table t3 (a int);
>
>         revoke all on t2 from :USER;
>
>         \pset null <NULL>
>         \dp t1|t2|t3
>                                     Access privileges
>          Schema | Name | Type  | Access privileges | Column privileges | Policies
>         --------+------+-------+-------------------+-------------------+----------
>          public | t1   | table | <NULL>            |                   |
>          public | t2   | table |                   |                   |
>          public | t3   | table | <NULL>            |                   |
>         (3 rows)
>
> Instead of only displaying the zero privileges with my patch and default
> \pset null:
>
>         \pset null ''
>         \dp t1|t2|t3
>                                     Access privileges
>          Schema | Name | Type  | Access privileges | Column privileges | Policies
>         --------+------+-------+-------------------+-------------------+----------
>          public | t1   | table |                   |                   |
>          public | t2   | table | (none)            |                   |
>          public | t3   | table |                   |                   |
>         (3 rows)
>
> I guess if most tables have any non-default privileges then both
> solutions are equally good.

It is a tough call.

For somebody who knows PostgreSQL well enough to know that default privileges are
represented by NULL values, my solution is probably more appealing.

It seems that we both had the goal of distinguishing the cases of default and
zero privileges, but for a beginner, both versions are confusing. better would
probably be

                             Access privileges
  Schema | Name | Type  | Access privileges | Column privileges | Policies
 --------+------+-------+-------------------+-------------------+----------
  public | t1   | table | default           | default           |
  public | t2   | table |                   | default           |
  public | t3   | table | default           | default           |

The disadvantage of this (and the advantage of my proposal) is that it might
confuse experienced users (and perhaps automated tools) if the output changes
too much.

> > The simple attached patch does it like that.  What do you think?
>
> LGTM.

If you are happy enough with my patch, shall we mark it as ready for committer?
Or do you want to have a stab at something like I suggested above?

Yours,
Laurenz Albe

On 2023-10-07 14:29 +0200, Laurenz Albe write:
> On Sat, 2023-10-07 at 05:07 +0200, Erik Wienhold wrote:
> > On 2023-10-06 22:32 +0200, Laurenz Albe write:
> > > On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote:
> > > > I wrote a patch to change psql's display of zero privileges after a user's
> > > > reported confusion with the psql output for zero vs. default privileges [1].
> > > > Admittedly, zero privileges is a rare use case [2] but I think psql should not
> > > > confuse the user in the off chance that this happens.
> > > > 
> > > > [1] https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com
> > > > [2] https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us
> > > 
> > > Reading that thread, I had the impression that there was more support for
> > > honoring "\pset null" rather than unconditionally displaying "(none)".
> > 
> > For example with your patch applied:
> > 
> >         create table t1 (a int);
> >         create table t2 (a int);
> >         create table t3 (a int);
> > 
> >         revoke all on t2 from :USER;
> > 
> >         \pset null <NULL>
> >         \dp t1|t2|t3
> >                                     Access privileges
> >          Schema | Name | Type  | Access privileges | Column privileges | Policies
> >         --------+------+-------+-------------------+-------------------+----------
> >          public | t1   | table | <NULL>            |                   |
> >          public | t2   | table |                   |                   |
> >          public | t3   | table | <NULL>            |                   |
> >         (3 rows)
> > 
> > Instead of only displaying the zero privileges with my patch and default
> > \pset null:
> > 
> >         \pset null ''
> >         \dp t1|t2|t3
> >                                     Access privileges
> >          Schema | Name | Type  | Access privileges | Column privileges | Policies
> >         --------+------+-------+-------------------+-------------------+----------
> >          public | t1   | table |                   |                   |
> >          public | t2   | table | (none)            |                   |
> >          public | t3   | table |                   |                   |
> >         (3 rows)
> > 
> > I guess if most tables have any non-default privileges then both
> > solutions are equally good.
> 
> It is a tough call.
> 
> For somebody who knows PostgreSQL well enough to know that default
> privileges are represented by NULL values, my solution is probably
> more appealing.
> 
> It seems that we both had the goal of distinguishing the cases of
> default and zero privileges, but for a beginner, both versions are
> confusing. better would probably be
> 
>                              Access privileges
>   Schema | Name | Type  | Access privileges | Column privileges | Policies
>  --------+------+-------+-------------------+-------------------+----------
>   public | t1   | table | default           | default           |
>   public | t2   | table |                   | default           |
>   public | t3   | table | default           | default           |

Ah yes.  The problem seems to be more with default privileges producing
no output right now.  I was just focusing on the zero privs edge case.

> The disadvantage of this (and the advantage of my proposal) is that it
> might confuse experienced users (and perhaps automated tools) if the
> output changes too much.

I agree that your patch is less invasive under default settings.  But is
the output of meta commands considered part of the interface where we
need to be cautious about not breaking clients?

I've written quite a few scripts that parse results from psql's stdout,
but always with simple queries to have control over columns and the
formatting of values.  I always expect meta command output to change
with the next release because to me they look more like a human-readable
interface, e.g. the localizable header which of course one can still
hide with --tuples-only.

> > > The simple attached patch does it like that.  What do you think?
> > 
> > LGTM.
> 
> If you are happy enough with my patch, shall we mark it as ready for
> committer?

I amended your patch to also document the effect of \pset null in this
case.  See the attached v2.

> Or do you want to have a stab at something like I suggested above?

Not right now if the user can just use \pset null 'default' with your
patch.

-- 
Erik

On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
> > If you are happy enough with my patch, shall we mark it as ready for
> > committer?
>
> I amended your patch to also document the effect of \pset null in this
> case.  See the attached v2.

+1

If you mention in ddl.sgml that you can use "\pset null" to distinguish
default from no privileges, you should mention that this only works with
psql.  Many people out there don't use psql.

Also, I'm not sure if "zero privileges" will be readily understood by
everybody.  Perhaps "no privileges at all, even for the object owner"
would be a better wording.

Perhaps it would also be good to mention this in the psql documentation.

Yours,
Laurenz Albe

On 2023-10-08 06:14 +0200, Laurenz Albe write:
> On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
> > > If you are happy enough with my patch, shall we mark it as ready for
> > > committer?
> > 
> > I amended your patch to also document the effect of \pset null in this
> > case.  See the attached v2.
> 
> +1
> 
> If you mention in ddl.sgml that you can use "\pset null" to distinguish
> default from no privileges, you should mention that this only works with
> psql.  Many people out there don't use psql.

I don't think this is necessary because that section in ddl.sgml is
already about psql and \dp.

> Also, I'm not sure if "zero privileges" will be readily understood by
> everybody.  Perhaps "no privileges at all, even for the object owner"
> would be a better wording.

Changed in v3 to "empty privileges" with an explanation that this means
"no privileges at all, even for the object owner".

> Perhaps it would also be good to mention this in the psql documentation.

Just once under  \pset null  with a reference to section 5.7?  Something
like "This is also useful for distinguishing default privileges from
empty privileges as explained in Section 5.7."

Or instead under every command affected by this change?  \dp and \ddp
already have such a reference ("The meaning of the privilege display is
explained in Section 5.7.")

I prefer the first one because it's less effort ;)  Also done in v3.

-- 
Erik

On Mon, 2023-10-09 at 03:53 +0200, Erik Wienhold wrote:
> On 2023-10-08 06:14 +0200, Laurenz Albe write:
> > On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
> > > > If you are happy enough with my patch, shall we mark it as ready for
> > > > committer?
> > >
> > > I amended your patch to also document the effect of \pset null in this
> > > case.  See the attached v2.
> >
> > +1
> >
> > If you mention in ddl.sgml that you can use "\pset null" to distinguish
> > default from no privileges, you should mention that this only works with
> > psql.  Many people out there don't use psql.
>
> I don't think this is necessary because that section in ddl.sgml is
> already about psql and \dp.

You are right.

> > Also, I'm not sure if "zero privileges" will be readily understood by
> > everybody.  Perhaps "no privileges at all, even for the object owner"
> > would be a better wording.
>
> Changed in v3 to "empty privileges" with an explanation that this means
> "no privileges at all, even for the object owner".

Looks good.

> > Perhaps it would also be good to mention this in the psql documentation.
>
> Just once under  \pset null  with a reference to section 5.7?  Something
> like "This is also useful for distinguishing default privileges from
> empty privileges as explained in Section 5.7."
>
> Or instead under every command affected by this change?  \dp and \ddp
> already have such a reference ("The meaning of the privilege display is
> explained in Section 5.7.")
>
> I prefer the first one because it's less effort ;)  Also done in v3.

I think that sufficient.

I tinkered a bit with your documentation.  For example, the suggestion to
"\pset null" seemed to be in an inappropriate place.  Tell me what you think.

Yours,
Laurenz Albe

On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote:
> For us, I would suggest the following wording:
>
> In addition to the situation of printing all acl items, the Access and Column
> privileges columns report two other situations specially.  In the rare case
> where all privileges for an object have been explicitly removed, including
> from the owner and PUBLIC, (i.e., has empty privileges) these columns will
> display NULL.  The other case is where the built-in default privileges are
> in effect, in which case these columns will display the empty string.
> (Note that by default psql will print NULL as an empty string, so in order
> to visually distinguish these two cases you will need to issue the \pset null
> meta-command and choose some other string to print for NULLs).  Built-in
> default privileges include all privileges for the owner, as well as those
> granted to PUBLIC per for relevant object types as described above.

That doesn't look like an improvement over the latest patches to me.

> The built-in default privileges are only in effect if the object has not been
> the target of a GRANT or REVOKE and also has not had its default privileges
> modified using ALTER DEFAULT PRIVILEGES. (???: if it is possible to revert
> back to the state of built-in privileges that would need to be described here.)

I don't think that we need to mention ALTER DEFAULT PRIVILEGES there.  If
the default privileges have been altered, the ACL will not be stored as
NULL in the catalogs.

> The above removes the parenthetical regarding null in the catalogs, this is
> intentional as it seems that the goal here is to use psql instead of the
> catalogs and adding its use of null being printed as the empty string just
> seems likely to add confusion.

To me, mentioning the default privileges are stored as NULLs in the catalogs
is not an invitation to view the privileges with catalog queries, but
information about implementation details that explains why default privileges
are displayed the way they are.

> We probably should add the two terms to the glossary:
>
> empty privileges: all privileges explicitly revoked from the owner and PUBLIC
> (where applicable), and none otherwise granted.
>
> built-in default privileges: owner having all privileges and no privileges
> granted or removed via ALTER DEFAULT PRIVILEGES

"Empty privileges" are too unimportant to warrant an index entry.

I can see the value of an index entry

<indexterm>
 <primary>privilege</primary>
 <secondary>default</secondary>
</indexterm>

Done in the attached v5 of the patch, even though this is not really
the business of this patch.

> > > Perhaps it would also be good to mention this in the psql documentation.
>
> We've chosen a poor default and I'd rather inform the user of specific meta-commands
> to be wary of this poor default and change it at the point they will be learning
> about the meta-commands adversely affected.
>
> That said, I'd be willing to document that these commands, because they are affected
> by empty string versus null, require a non-empty-string value for \pset null and will
> choose the string '<null>' for the duration of the meta-command's execution if the
> user's setting is incompatible.

I am not certain I understood you correctly.
Are you advocating for adding a mention of "\pset null" to every backslash command
that displays privileges?  That is excessive, in my opinion.

Yours,
Laurenz Albe

On Mon, 2023-10-09 at 09:30 -0700, David G. Johnston wrote:
> On Mon, Oct 9, 2023 at 1:29 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote:
> >
> > > The built-in default privileges are only in effect if the object has not been
> > > the target of a GRANT or REVOKE and also has not had its default privileges
> > > modified using ALTER DEFAULT PRIVILEGES. (???: if it is possible to revert
> > > back to the state of built-in privileges that would need to be described here.)
> >
> > I don't think that we need to mention ALTER DEFAULT PRIVILEGES there.  If
> > the default privileges have been altered, the ACL will not be stored as
> > NULL in the catalogs.
>
> It's already mentioned there, I just felt bringing the mention of ADP and
> grant/revoke both invalidating the built-in default privileges closer together
> would be an improvement.

Ah, sorry, I misread your comment.  You are right.  But then, the effects of
ALTER DEFAULT PRIVILEGES are discussed later in the paragraph anyway, and we don't
have to repeat that here.

> >
> > To me, mentioning the default privileges are stored as NULLs in the catalogs
> > is not an invitation to view the privileges with catalog queries, but
> > information about implementation details that explains why default privileges
> > are displayed the way they are.
>
> Calling it an implementation detail leads me to conclude the point does not
> belong in the user-facing administration documentation.

Again, you have a point there.  However, I find that detail useful, as it explains
the user-facing behavior.  Anyway, I don't think it is the job of this patch to
remove that pre-existing detail.

> > Are you advocating for adding a mention of "\pset null" to every backslash command
> > that displays privileges?  That is excessive, in my opinion.
>
> Yes, I am.  I suppose the argument that any user of those commands is expected
> to have read the ddl/privileges chapter would suffice for me though.

Thanks.  Then let's leave it like that.

> My point with the second paragraph is that we could, instead of documenting the
> caveat about null printing as empty strings is to instead issue an implicit
> "\pset null '<null>'" as part of these commands, and a "\pset null" afterward,
> conditioned upon it not already being set to a non-empty value.  IOW, the
> special-casing we do today but actually do it in a way that distinguishes the
> two cases instead of forcing them to be indistinguishable.

-1

The whole point of this patch is to make psql behave consistently with respect to
NULLs in meta-commands.  Your suggestion would subvert that idea.

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Mon, 2023-10-09 at 09:30 -0700, David G. Johnston wrote:
>> My point with the second paragraph is that we could, instead of documenting the
>> caveat about null printing as empty strings is to instead issue an implicit
>> "\pset null '<null>'" as part of these commands, and a "\pset null" afterward,
>> conditioned upon it not already being set to a non-empty value.  IOW, the
>> special-casing we do today but actually do it in a way that distinguishes the
>> two cases instead of forcing them to be indistinguishable.

> -1

> The whole point of this patch is to make psql behave consistently with respect to
> NULLs in meta-commands.  Your suggestion would subvert that idea.

Yeah.  There is a lot of attraction in having \pset null affect these
displays just like all other ones.  The fact that they act differently
now is a wart, not something we should replace with a different special
case behavior.

Also, I'm fairly concerned about not changing the default behavior here.
The fact that this behavior has stood for a couple dozen years without
many complaints indicates that there's not all that big a problem to be
solved.  I doubt that a new default behavior will be well received,
even if it's arguably better.

            regards, tom lane

On Mon, 2023-10-09 at 15:13 -0400, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > The whole point of this patch is to make psql behave consistently with respect to
> > NULLs in meta-commands.
>
> Yeah.  There is a lot of attraction in having \pset null affect these
> displays just like all other ones.  The fact that they act differently
> now is a wart, not something we should replace with a different special
> case behavior.
>
> Also, I'm fairly concerned about not changing the default behavior here.
> The fact that this behavior has stood for a couple dozen years without
> many complaints indicates that there's not all that big a problem to be
> solved.  I doubt that a new default behavior will be well received,
> even if it's arguably better.

I understand your concern.  People who have "\pset null" in their .psqlrc
might be surprised if suddenly "<null>" starts appearing in the outout
of \l.

But then the people who have "\pset null" in their .psqlrc are typically
already somewhat experienced and might have less trouble dealing with that
change (but they are more likely to bleat on the mailing list, granted).

Users with little experience won't notice any difference, so they won't
be confused by the change.

Yours,
Laurenz Albe

On 2023-10-09 09:54 +0200, Laurenz Albe write:
> 
> I tinkered a bit with your documentation.  For example, the suggestion to
> "\pset null" seemed to be in an inappropriate place.  Tell me what you think.

+1  You're right to put that sentence right after the explanation of
empty privileges.

-- 
Erik

On 2023-10-09 10:29 +0200, Laurenz Albe write:
> On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote:
> > We probably should add the two terms to the glossary:
> > 
> > empty privileges: all privileges explicitly revoked from the owner and PUBLIC
> > (where applicable), and none otherwise granted.
> > 
> > built-in default privileges: owner having all privileges and no privileges
> > granted or removed via ALTER DEFAULT PRIVILEGES
> 
> "Empty privileges" are too unimportant to warrant an index entry.
> 
> I can see the value of an index entry
> 
> <indexterm>
>  <primary>privilege</primary>
>  <secondary>default</secondary>
> </indexterm>
> 
> Done in the attached v5 of the patch, even though this is not really
> the business of this patch.

+1

-- 
Erik

On 2023-10-09 22:34 +0200, David G. Johnston write:
> On Mon, Oct 9, 2023 at 12:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Yeah.  There is a lot of attraction in having \pset null affect these
> > displays just like all other ones.  The fact that they act differently
> > now is a wart, not something we should replace with a different special
> > case behavior.
> >
> > Also, I'm fairly concerned about not changing the default behavior here.
> > The fact that this behavior has stood for a couple dozen years without
> > many complaints indicates that there's not all that big a problem to be
> > solved.  I doubt that a new default behavior will be well received,
> > even if it's arguably better.
> >
> 
> My position is that the default behavior should be changed such that the
> distinction these reports need to make between empty privileges and default
> privileges is impossible for the user to remove.  I suppose the best direct
> solution given that goal is for the query to simply do away with any
> reliance on NULL being an indicator.  Output an i18n'd "no permissions
> present" line in the rare empty permissions situation and leave the empty
> string for built-in default.

My initial patch does exactly that.

> If the consensus is to not actually fix these views to make them
> environment invariant in their accuracy then so be it.  Having them obey
> \pset null seems like a half-measure but it at least is an improvement over
> the status quo such that users are capable of altering their system to make
> the reports distinguish the two cases if they realize the need.

I agree.

-- 
Erik

On Sat, 2023-10-14 at 02:45 +0200, Erik Wienhold wrote:
> On 2023-10-09 09:54 +0200, Laurenz Albe write:
> >
> > I tinkered a bit with your documentation.  For example, the suggestion to
> > "\pset null" seemed to be in an inappropriate place.  Tell me what you think.
>
> +1  You're right to put that sentence right after the explanation of
> empty privileges.

Thanks for looking.

David, how do you feel about this?  I am wondering whether to set this patch
"ready for committer" or not.

There is Tom wondering upthread whether changing psql's behavior like that
is too much of a compatibility break or not, but I guess it is alright to
leave that final verdict to a committer.

Yours,
Laurenz Albe

On 2023-10-16 17:56 +0200, Laurenz Albe write:
> David, how do you feel about this?  I am wondering whether to set this patch
> "ready for committer" or not.
> 
> There is Tom wondering upthread whether changing psql's behavior like that
> is too much of a compatibility break or not, but I guess it is alright to
> leave that final verdict to a committer.

What's the process for the CommitFest now since we settled on your
patch?  This is my first time being involved in this, so still learning.
I'see that you've withdrawn your initial patch [1] but this thread is
still attached to my patch [2].  Should I edit my CF entry (or withdraw
it) and you reactivate yours?

[1] https://commitfest.postgresql.org/45/4603/
[2] https://commitfest.postgresql.org/45/4593/

-- 
Erik

On Mon, 2023-10-16 at 23:51 +0200, Erik Wienhold wrote:
> What's the process for the CommitFest now since we settled on your
> patch?  This is my first time being involved in this, so still learning.
> I'see that you've withdrawn your initial patch [1] but this thread is
> still attached to my patch [2].  Should I edit my CF entry (or withdraw
> it) and you reactivate yours?

I don't think it makes sense to have two competing commitfest entries,
and we should leave it on this thread.  If you are concerned about
authorship, we could both be mentioned as co-authors, if the patch ever
gets committed.

I'd still like to wait for feedback from David before I change anything.

Yours,
Laurenz Albe

On Mon, 2023-10-16 at 19:05 -0700, David G. Johnston wrote:
> Reading both threads I'm not seeing any specific rejection of the
> solution that we simply represent empty privileges as "(none)".
>
> I see an apparent consensus that if we do continue to represent it
> as NULL that the printout should respect \pset null.
>
> Tom commented in favor of (none) on the other thread with some
> commentary regarding how extremely rare it is; then turns around
> and is "fairly concerned" about changing the current blank presentation
> of its value which is going to happen for some people regardless
> of which approach is chosen.
>
> Stuart, the original user complainant, seems to favor (none)
>
> Erik seems to favors (none)
>
> I favor (none)
>
> It's unclear to me whether you Laurenz are against (none) or just
> trying to go with the group consensus that doesn't actually seem to
> be against (none).
>
> I'm in favor of iterating on v1 on this thread (haven't read it yet)
> and presenting it as the proposed solution.  If that ends up getting
> shot down we can revive v5 (still need to review as well).

Thanks for that summary.  I prefer my version (simply display NULLs
as NULLs), but I am not wedded to it.  I had the impression that Tom
would prefer that too, but is woried if the incompatibility introduced
would outweigh the small benefit (of either patch).

So it is clear that we don't have a consensus.

I don't think I want to go ahead with my version of the patch unless
there is more support for it.  I can review Erik's original code, if
that design meets with more favor.

> We should probably post on that thread that this one exists and post a link to it.

Perhaps a good idea, yes.

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Mon, 2023-10-16 at 19:05 -0700, David G. Johnston wrote:
>> Reading both threads I'm not seeing any specific rejection of the
>> solution that we simply represent empty privileges as "(none)".

> Thanks for that summary.  I prefer my version (simply display NULLs
> as NULLs), but I am not wedded to it.  I had the impression that Tom
> would prefer that too, but is woried if the incompatibility introduced
> would outweigh the small benefit (of either patch).

> So it is clear that we don't have a consensus.

FWIW, my druthers are to make the describe.c queries honor \pset null
(not only for privileges, but anywhere else they fail to) and do
nothing beyond that.  I think that'll generally reduce the surprise
factor, while anything else we might opt to do will increase it.

If that fails to garner a consensus, my second choice would be to
do that plus translate empty-but-not-null ACLs to "(none)".

            regards, tom lane

On 2023-10-17 04:05 +0200, David G. Johnston wrote:
> Erik seems to favors (none)

Yes, with a slight favor for "(none)" because it's the least disruptive
to users who change \pset null to a non-blank string.  The output of \dp
etc. would still look the same for default privileges.

But I'm also okay with respecting \pset null because it is so simple.
We will no longer hide the already documented null representation of
default privileges by allowing the user to display the ACL as it is.
And with Laurenz' patch we will also document the special case of zero
privileges and how to distinguish it.

-- 
Erik

On Fri, 2023-10-20 at 04:13 +0200, Erik Wienhold wrote:
> On 2023-10-17 04:05 +0200, David G. Johnston wrote:
> > Erik seems to favors (none)
>
> Yes, with a slight favor for "(none)" because it's the least disruptive
> to users who change \pset null to a non-blank string.  The output of \dp
> etc. would still look the same for default privileges.
>
> But I'm also okay with respecting \pset null because it is so simple.
> We will no longer hide the already documented null representation of
> default privileges by allowing the user to display the ACL as it is.
> And with Laurenz' patch we will also document the special case of zero
> privileges and how to distinguish it.

If you want to proceed with your patch, you could send a new version.

I think it could do with an added line of documentation to the
"Privileges" chapter, and I'd say that the regression tests should be
in "psql.sql" (not that it is very important).

I am not sure how to proceed. Perhaps it would indeed be better to have
two competing commitfest entries.  Both could be "ready for committer",
and the committers can decide what they prefer.

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> I am not sure how to proceed. Perhaps it would indeed be better to have
> two competing commitfest entries.  Both could be "ready for committer",
> and the committers can decide what they prefer.

As near as I can tell, doing both things (the \pset null fix and
substituting "(none)" for empty privileges) would be an acceptable
answer to everyone who has commented.  Let's proceed with both
patches, or combine them into one if there are merge conflicts.

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Fri, Oct 20, 2023 at 12:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As near as I can tell, doing both things (the \pset null fix and
>> substituting "(none)" for empty privileges) would be an acceptable
>> answer to everyone who has commented.  Let's proceed with both
>> patches, or combine them into one if there are merge conflicts.

> I'm under the impression that removing the null representation of empty
> privileges by making them (none) removes all known \d commands that output
> nulls and don't obey \pset null.

How so?  IIUC the proposal is to substitute "(none)" for empty-string
ACLs, not null ACLs.  The \pset change should be addressing an
independent case.

            regards, tom lane

On 2023-10-20 08:42 +0200, Laurenz Albe wrote:
> If you want to proceed with your patch, you could send a new version.

v2 attached.

> I think it could do with an added line of documentation to the
> "Privileges" chapter, and I'd say that the regression tests should be
> in "psql.sql" (not that it is very important).

I added some docs.  There will be merge conflicts when combining with
your v5!  Also moved the regression tests to psql.sql which is the right
place for testing meta commands.

-- 
Erik

On 2023-10-20 22:35 +0200, David G. Johnston wrote:
> Ok, I found my mis-understanding and better understand where you are all
> coming from now; I apparently had the usage of NULL flip-flopped.
> 
> Taking pg_tablespace as an example.  Its "spcacl" column produces NULL for
> default privileges and '{}'::text[] for empty privileges.
> 
> Thus, today:
> empty: array_to_string('{}'::text[], '\n') produces an empty string
> default: array_to_string(null, '\n') produces null which then was printed
> as a hard-coded empty string via forcibly changing \pset null
> 
> I was thinking the two cases were reversed.
> 
> My proposal for changing printACLColumn is thus:
> 
> case when spcacl is null then ''
>          when cardinality(spcacl) = 0 then '(none)'
>          else array_to_string(spcacl, E'\\n')
> end as "Access privileges"
> 
> In short, I don't want default privileges to start to obey \pset null when
> it never has before and is documented as displaying the empty string.  I do
> want the empty string produced by empty privileges to change to (none) so
> that it no longer is indistinguishable from our choice of presentation for
> the default privilege case.
> 
> Mechanically, we remove the existing \pset null for these metacommands and
> move it into the query via the added CASE expression in the ‎printACLColumn
> function.
> 
> This gets rid of NULL as an output value for this column and makes the
> patch regarding \pset null discussion unnecessary.  And it leaves the
> existing well-established empty string for default privileges alone (and
> changing this is what I believe Tom is against and I agree on that point).

I haven't thought off this yet.  The attached v3 of my initial patch
does that.  It also includes Laurenz' fix to no longer ignore \pset null
(minus the doc changes that suggest using \pset null to distinguish
between default and empty privileges because that's no longer needed).

-- 
Erik

On Sat, 2023-10-21 at 04:29 +0200, Erik Wienhold wrote:
> The attached v3 of my initial patch
> does that.  It also includes Laurenz' fix to no longer ignore \pset null
> (minus the doc changes that suggest using \pset null to distinguish
> between default and empty privileges because that's no longer needed).

Thanks!

I went over the patch, fixed some problems and added some more stuff from
my patch.

In particular:

  --- a/doc/src/sgml/ddl.sgml
  +++ b/doc/src/sgml/ddl.sgml
  @@ -2353,7 +2353,9 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
     <para>
      If the <quote>Access privileges</quote> column is empty for a given
      object, it means the object has default privileges (that is, its
  -   privileges entry in the relevant system catalog is null).  Default
  +   privileges entry in the relevant system catalog is null).  The column shows
  +   <literal>(none)</literal> for empty privileges (that is, no privileges at
  +   all, even for the object owner — a rare occurrence).  Default
      privileges always include all privileges for the owner, and can include
      some privileges for <literal>PUBLIC</literal> depending on the object
      type, as explained above.  The first <command>GRANT</command>

This description of empty privileges is smack in the middle of describing
default privileges.  I thought that was confusing and moved it to its
own paragraph.

  --- a/src/bin/psql/describe.c
  +++ b/src/bin/psql/describe.c
  @@ -6718,7 +6680,13 @@ static void
   printACLColumn(PQExpBuffer buf, const char *colname)
   {
      appendPQExpBuffer(buf,
  -                     "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
  +                     "CASE\n"
  +                     "  WHEN %s IS NULL THEN ''\n"
  +                     "  WHEN pg_catalog.cardinality(%s) = 0 THEN '%s'\n"
  +                     "  ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
  +                     "END AS \"%s\"",
  +                     colname,
  +                     colname, gettext_noop("(none)"),
                        colname, gettext_noop("Access privileges"));
   }

This erroneously displays NULL as empty string and subverts my changes.
I have removed the first branch of the CASE expression.

  --- a/src/test/regress/expected/psql.out
  +++ b/src/test/regress/expected/psql.out
  @@ -6663,3 +6663,97 @@ DROP ROLE regress_du_role0;
   DROP ROLE regress_du_role1;
   DROP ROLE regress_du_role2;
   DROP ROLE regress_du_admin;
  +-- Test empty privileges.
  +BEGIN;
  +WARNING:  there is already a transaction in progress

This warning is caused by a pre-existing error in the regression test, which
forgot to close the transaction.  I have added a COMMIT at the appropriate place.

  +ALTER TABLESPACE regress_tblspace OWNER TO CURRENT_USER;
  +REVOKE ALL ON TABLESPACE regress_tblspace FROM CURRENT_USER;
  +\db+ regress_tblspace
  +                                                List of tablespaces
  +       Name       |         Owner          |    Location     | Access privileges | Options |  Size   | Description
  +------------------+------------------------+-----------------+-------------------+---------+---------+-------------
  + regress_tblspace | regress_zeropriv_owner | pg_tblspc/16385 | (none)            |         | 0 bytes |
  +(1 row)

This test is not stable, since it contains the OID of the tablespace, which
is different every time.

  +ALTER DATABASE :"DBNAME" OWNER TO CURRENT_USER;
  +REVOKE ALL ON DATABASE :"DBNAME" FROM CURRENT_USER, PUBLIC;
  +\l :"DBNAME"
  +                                                        List of databases
  +    Name    |         Owner          | Encoding  | Locale Provider | Collate | Ctype | ICU Locale | ICU Rules |
Accessprivileges  

+------------+------------------------+-----------+-----------------+---------+-------+------------+-----------+-------------------
  + regression | regress_zeropriv_owner | SQL_ASCII | libc            | C       | C     |            |           |
(none)
  +(1 row)

This test is also not stable, since it depends on the locale definition
of the regression test database.  If you use "make installcheck", that could
be a different locale.

I think that these tests are not absolutely necessary, and the other tests
are sufficient.  Consequently, I took the simple road of removing them.

I also tried to improve the commit message.

Patch attached.

Yours,
Laurenz Albe

  --- a/src/bin/psql/describe.c
  +++ b/src/bin/psql/describe.c
  @@ -6718,7 +6680,13 @@ static void
   printACLColumn(PQExpBuffer buf, const char *colname)
   {
      appendPQExpBuffer(buf,
  -                     "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
  +                     "CASE\n"
  +                     "  WHEN %s IS NULL THEN ''\n"
  +                     "  WHEN pg_catalog.cardinality(%s) = 0 THEN '%s'\n"
  +                     "  ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
  +                     "END AS \"%s\"",
  +                     colname,
  +                     colname, gettext_noop("(none)"),
                        colname, gettext_noop("Access privileges"));
   }

This erroneously displays NULL as empty string and subverts my changes.
I have removed the first branch of the CASE expression.

On Mon, 2023-10-23 at 07:03 -0700, David G. Johnston wrote:
> On Monday, October 23, 2023, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> >
> >   --- a/src/bin/psql/describe.c
> >   +++ b/src/bin/psql/describe.c
> >   @@ -6718,7 +6680,13 @@ static void
> >    printACLColumn(PQExpBuffer buf, const char *colname)
> >    {
> >       appendPQExpBuffer(buf,
> >   -                     "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
> >   +                     "CASE\n"
> >   +                     "  WHEN %s IS NULL THEN ''\n"
> >   +                     "  WHEN pg_catalog.cardinality(%s) = 0 THEN '%s'\n"
> >   +                     "  ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
> >   +                     "END AS \"%s\"",
> >   +                     colname,
> >   +                     colname, gettext_noop("(none)"),
> >                         colname, gettext_noop("Access privileges"));
> >    }
> >
> > This erroneously displays NULL as empty string and subverts my changes.
> > I have removed the first branch of the CASE expression.
>
> There is no error here, the current consensus which this patch implements is to
> not change the documented “default privileges display as blank”.  We are solving
> the empty privileges are not distinguishable complaint by printing (none) for them.

Erik's latest patch included my changes to display NULL as NULL in psql,
so that "\pset null" works as expected.
But he left the above hunk from his original patch, and that hunk replaces
NULL with an empty string, so "\pset null" wouldn't work for the display
of default provoleges.

He didn't notice it because he didn't have a regression test that displays
default privileges with non-empty "\pset null".

Yours,
Laurenz Albe

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> We document default privileges print as an empty string - I do not think we
> should change the definition to "default privileges print null which can be
> controlled via \pset null", and regardless of preference doing so is not a
> bug.

Well, "if it's documented it's not a bug" is a defensible argument
for calling something not a bug, but it doesn't address the question
of whether changing it would be an improvement.  I think it would be,
and I object to your claim that we have a consensus to not do that.

The core of the problem here, IMO, is exactly that there is confusion
between whether a visibly empty string represents NULL (default
privileges) or an empty ACL (no privileges).  I believe we have agreed
that printing "(none)" or some other clearly-not-an-ACL-entry string
for the second case is an improvement, even though that's a change
in behavior.  That doesn't mean that changing the other case can't
also be an improvement.  I think it'd be less confusing all around
if this instance of NULL prints like other instances of NULL.

IOW, the current definition is "NULL privileges print as an empty
string no matter what", and I don't think that serves to reduce
confusion about whether an ACL is NULL or not.  We ought to be doing
what we can to make clear that such an ACL *is* NULL, because
otherwise people won't understand how it differs from an empty ACL.

            regards, tom lane

On Mon, 2023-10-23 at 08:35 -0700, David G. Johnston wrote:
> I tend to prefer the argument that these views are for human consumption and should
> be designed with that in mind.

True, although given the shape of ACLs, it takes a somewhat trained human to
consume the string representation.  But we won't be able to hide the fact that
default ACLs are NULL in the catalogs.  We can leave them empty, we can show
them as "(default)" or we can let the user choose with "\pset null".


> I would suggest that we make the expected presence of NULL as an input explicit:
> I would offer up:
>
> when spcacl is null then '(default)'

Noted.

> along with not translating (none) and (default) and thus making the data contents
> of these views environment independent.  But minimizing the variance of these command's
> output across systems doesn't seem like a design goal that is likely to gain consensus
> and is excessive when viewed within the framework of these being only for human consumption.

I didn't understand this completely.  You want default privileges displayed as
"(default)", but are you for or against "\pset null" to have its normal effect on
the output of backslash commands in all other cases?

Speaking of consensus, it seems to me that Tom, Erik and me are in consensus.

Yours,
Laurenz Albe

On Mon, 2023-10-23 at 08:35 -0700, David G. Johnston wrote:

> along with not translating (none) and (default) and thus making the data contents
> of these views environment independent.  But minimizing the variance of these command's
> output across systems doesn't seem like a design goal that is likely to gain consensus
> and is excessive when viewed within the framework of these being only for human consumption.

I didn't understand this completely.  You want default privileges displayed as
"(default)", but are you for or against "\pset null" to have its normal effect on
the output of backslash commands in all other cases?

On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
> > I didn't understand this completely.  You want default privileges displayed as
> > "(default)", but are you for or against "\pset null" to have its normal effect on
> > the output of backslash commands in all other cases?
>
> I haven’t inspected other cases but to my knowledge we don’t typically represent
> non-unknown things using NULL so I’m not expecting other places to have this
> representation problem.

The first example that comes to my mind is the "ICU Locale" and the "ICU Rules"
in the output of \l.  There are many others.

> I don’t think any of our meta-command outputs should modify pset null.
> Left join cases should be considered unknown, represented as NULL, and obey the
> user’s setting.

That's what I think too.  psql output should respect "\pset null".
So it looks like we agree on that.

> I do believe that we should be against exposing, like in this case, any internal
> implementation detail that encodes something (e.g., default privileges) as NULL
> in the catalogs, to the user of the psql meta-commands.
>
> I won’t argue that exposing such NULLS is wrong, just it would preferable IME
> to avoid doing so.  NULL means unknown or not applicable and default privileges
> are neither of those things.  I get why our catalogs choose such an encoding and
> agree with it, and users that find the need to consult the catalogs will need to
> learn such details.  But we should strive for them to be able to survive with
> psql meta-commands.

Sure, it would be best to hide this implementation detail from the user.
The correct way to do that would be to fake an ACL entry like "laurenz=arwdDxt/laurenz"
if there is a NULL in the catalog, but that would add a ton of special-case
code to psql, which does not look appealing at all.

So we cannot completely hide the implementation, but perhaps "(default)" would
be less confusing than a NULL value.

If everybody agrees, I can modify the patch to do that.

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
>> I do believe that we should be against exposing, like in this case, any internal
>> implementation detail that encodes something (e.g., default privileges) as NULL
>> in the catalogs, to the user of the psql meta-commands.

> Sure, it would be best to hide this implementation detail from the user.
> The correct way to do that would be to fake an ACL entry like "laurenz=arwdDxt/laurenz"
> if there is a NULL in the catalog, but that would add a ton of special-case
> code to psql, which does not look appealing at all.

For better or worse, that *is* the backend's catalog representation,
and I don't think that psql would be doing our users a service by
trying to obscure the fact.  They'd run into it anyway the moment
they look at the catalogs with anything but a \d-something command.

            regards, tom lane

On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
> > I didn't understand this completely.  You want default privileges displayed as
> > "(default)", but are you for or against "\pset null" to have its normal effect on
> > the output of backslash commands in all other cases?
>
> I haven’t inspected other cases but to my knowledge we don’t typically represent
> non-unknown things using NULL so I’m not expecting other places to have this
> representation problem.

The first example that comes to my mind is the "ICU Locale" and the "ICU Rules"
in the output of \l.  There are many others.

>
> I won’t argue that exposing such NULLS is wrong, just it would preferable IME
> to avoid doing so.  NULL means unknown or not applicable and default privileges
> are neither of those things.  I get why our catalogs choose such an encoding and
> agree with it, and users that find the need to consult the catalogs will need to
> learn such details.  But we should strive for them to be able to survive with
> psql meta-commands.

Sure, it would be best to hide this implementation detail from the user.
The correct way to do that would be to fake an ACL entry like "laurenz=arwdDxt/laurenz"
if there is a NULL in the catalog, but that would add a ton of special-case
code to psql, which does not look appealing at all.

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
>> I do believe that we should be against exposing, like in this case, any internal
>> implementation detail that encodes something (e.g., default privileges) as NULL
>> in the catalogs, to the user of the psql meta-commands.

> Sure, it would be best to hide this implementation detail from the user.
> The correct way to do that would be to fake an ACL entry like "laurenz=arwdDxt/laurenz"
> if there is a NULL in the catalog, but that would add a ton of special-case
> code to psql, which does not look appealing at all.

For better or worse, that *is* the backend's catalog representation,
and I don't think that psql would be doing our users a service by
trying to obscure the fact.  They'd run into it anyway the moment
they look at the catalogs with anything but a \d-something command.

On Mon, 2023-10-23 at 22:43 -0400, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > On Mon, 2023-10-23 at 11:37 -0700, David G. Johnston wrote:
> > > I do believe that we should be against exposing, like in this case, any internal
> > > implementation detail that encodes something (e.g., default privileges) as NULL
> > > in the catalogs, to the user of the psql meta-commands.
>
> > Sure, it would be best to hide this implementation detail from the user.
> > The correct way to do that would be to fake an ACL entry like "laurenz=arwdDxt/laurenz"
> > if there is a NULL in the catalog, but that would add a ton of special-case
> > code to psql, which does not look appealing at all.
>
> For better or worse, that *is* the backend's catalog representation,
> and I don't think that psql would be doing our users a service by
> trying to obscure the fact.  They'd run into it anyway the moment
> they look at the catalogs with anything but a \d-something command.

... for example with a client like pgAdmin, which is a frequent choice
of many PostgreSQL beginners (they display empty privileges).

Yes, it is "(default)" or NULL.  The former is friendlier for beginners,
the latter incurs less backward incompatibility.

I could live with either solution, but I am still leaning towards NULL.

I ran the regression tests with a patch that displays "(default)",
and I counted 22 failures, excluding the one added by my patch.
The tests can of course be fixed, but perhaps that serves as a measure
of the backward incompatibility.

Yours,
Laurenz Albe

On Wed, Nov 8, 2023 at 10:46 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Sat, 2023-10-21 at 04:29 +0200, Erik Wienhold wrote:
> > The attached v3 of my initial patch
> > does that.  It also includes Laurenz' fix to no longer ignore \pset null
> > (minus the doc changes that suggest using \pset null to distinguish
> > between default and empty privileges because that's no longer needed).
>
> Thanks!
>
> I went over the patch, fixed some problems and added some more stuff from
> my patch.
>
> In particular:
>
>   --- a/doc/src/sgml/ddl.sgml
>   +++ b/doc/src/sgml/ddl.sgml
>   @@ -2353,7 +2353,9 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw;
>      <para>
>       If the <quote>Access privileges</quote> column is empty for a given
>       object, it means the object has default privileges (that is, its
>   -   privileges entry in the relevant system catalog is null).  Default
>   +   privileges entry in the relevant system catalog is null).  The column shows
>   +   <literal>(none)</literal> for empty privileges (that is, no privileges at
>   +   all, even for the object owner — a rare occurrence).  Default
>       privileges always include all privileges for the owner, and can include
>       some privileges for <literal>PUBLIC</literal> depending on the object
>       type, as explained above.  The first <command>GRANT</command>
>
> This description of empty privileges is smack in the middle of describing
> default privileges.  I thought that was confusing and moved it to its
> own paragraph.
>
>   --- a/src/bin/psql/describe.c
>   +++ b/src/bin/psql/describe.c
>   @@ -6718,7 +6680,13 @@ static void
>    printACLColumn(PQExpBuffer buf, const char *colname)
>    {
>       appendPQExpBuffer(buf,
>   -                     "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
>   +                     "CASE\n"
>   +                     "  WHEN %s IS NULL THEN ''\n"
>   +                     "  WHEN pg_catalog.cardinality(%s) = 0 THEN '%s'\n"
>   +                     "  ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
>   +                     "END AS \"%s\"",
>   +                     colname,
>   +                     colname, gettext_noop("(none)"),
>                         colname, gettext_noop("Access privileges"));
>    }
>
> This erroneously displays NULL as empty string and subverts my changes.
> I have removed the first branch of the CASE expression.
>
>   --- a/src/test/regress/expected/psql.out
>   +++ b/src/test/regress/expected/psql.out
>   @@ -6663,3 +6663,97 @@ DROP ROLE regress_du_role0;
>    DROP ROLE regress_du_role1;
>    DROP ROLE regress_du_role2;
>    DROP ROLE regress_du_admin;
>   +-- Test empty privileges.
>   +BEGIN;
>   +WARNING:  there is already a transaction in progress
>
> This warning is caused by a pre-existing error in the regression test, which
> forgot to close the transaction.  I have added a COMMIT at the appropriate place.
>
>   +ALTER TABLESPACE regress_tblspace OWNER TO CURRENT_USER;
>   +REVOKE ALL ON TABLESPACE regress_tblspace FROM CURRENT_USER;
>   +\db+ regress_tblspace
>   +                                                List of tablespaces
>   +       Name       |         Owner          |    Location     | Access privileges | Options |  Size   | Description
>
+------------------+------------------------+-----------------+-------------------+---------+---------+-------------
>   + regress_tblspace | regress_zeropriv_owner | pg_tblspc/16385 | (none)            |         | 0 bytes |
>   +(1 row)
>
> This test is not stable, since it contains the OID of the tablespace, which
> is different every time.
>
>   +ALTER DATABASE :"DBNAME" OWNER TO CURRENT_USER;
>   +REVOKE ALL ON DATABASE :"DBNAME" FROM CURRENT_USER, PUBLIC;
>   +\l :"DBNAME"
>   +                                                        List of databases
>   +    Name    |         Owner          | Encoding  | Locale Provider | Collate | Ctype | ICU Locale | ICU Rules |
Accessprivileges 
>
+------------+------------------------+-----------+-----------------+---------+-------+------------+-----------+-------------------
>   + regression | regress_zeropriv_owner | SQL_ASCII | libc            | C       | C     |            |           |
(none)
>   +(1 row)
>
> This test is also not stable, since it depends on the locale definition
> of the regression test database.  If you use "make installcheck", that could
> be a different locale.
>
> I think that these tests are not absolutely necessary, and the other tests
> are sufficient.  Consequently, I took the simple road of removing them.
>
> I also tried to improve the commit message.
>
> Patch attached.

I tested the Patch for the modified changes and it is working fine.

Thanks and regards,
Shubham Khanna.

On Wed, 2023-11-08 at 10:56 +0530, Shubham Khanna wrote:
> I tested the Patch for the modified changes and it is working fine.

Thanks for the review!

I wonder how to proceed with this patch.  The main disagreement is
whether default privileges should be displayed as NULL (less invasive,
but more confusing for beginners) or "(default)" (more invasive,
but nicer for beginners).

David is for "(default)", Tom and me are for NULL, and I guess Erik
would also prefer "(default)", since that was how his original
patch did it, IIRC.  I think I could live with both solutions.

Kind of a stalemate.  Who wants to tip the scales?

Yours,
Laurenz Albe

On 2023-11-08 13:23 +0100, Laurenz Albe wrote:
> I wonder how to proceed with this patch.  The main disagreement is
> whether default privileges should be displayed as NULL (less invasive,
> but more confusing for beginners) or "(default)" (more invasive,
> but nicer for beginners).

Are there any reports from beginners being confused about default
privileges being NULL or being displayed as a blank string in psql?
This is usually resolved with a pointer to the docs if it comes up in
discussions or the user makes the mental leap and checks the docs
himself.  Both patches add some details to the docs to explain psql's
output.

> David is for "(default)", Tom and me are for NULL, and I guess Erik
> would also prefer "(default)", since that was how his original
> patch did it, IIRC.  I think I could live with both solutions.
>
> Kind of a stalemate.  Who wants to tip the scales?

Yes I had a slight preference for my patch but I'd go with yours (\pset
null) now.  I followed the discussion after my last mail but had nothing
more to add that wasn't already said.  Tom then wrote that NULL is the
catalog's representation for the default privileges and obscuring that
fact in psql is not doing any service to the users.  This convinced me
because users may have to deal with aclitem[] being NULL anyway at some
point if they need to check privileges in more detail.  So it makes
absolutely sense that psql is transparent about that.

-- 
Erik

On Thu, 2023-11-09 at 03:40 +0100, Erik Wienhold wrote:
> On 2023-11-08 13:23 +0100, Laurenz Albe wrote:
> > I wonder how to proceed with this patch.  The main disagreement is
> > whether default privileges should be displayed as NULL (less invasive,
> > but more confusing for beginners) or "(default)" (more invasive,
> > but nicer for beginners).
>
> Are there any reports from beginners being confused about default
> privileges being NULL or being displayed as a blank string in psql?
> This is usually resolved with a pointer to the docs if it comes up in
> discussions or the user makes the mental leap and checks the docs
> himself.  Both patches add some details to the docs to explain psql's
> output.

Right.

> > David is for "(default)", Tom and me are for NULL, and I guess Erik
> > would also prefer "(default)", since that was how his original
> > patch did it, IIRC.  I think I could live with both solutions.
> >
> > Kind of a stalemate.  Who wants to tip the scales?
>
> Yes I had a slight preference for my patch but I'd go with yours (\pset
> null) now.  I followed the discussion after my last mail but had nothing
> more to add that wasn't already said.  Tom then wrote that NULL is the
> catalog's representation for the default privileges and obscuring that
> fact in psql is not doing any service to the users.  This convinced me
> because users may have to deal with aclitem[] being NULL anyway at some
> point if they need to check privileges in more detail.  So it makes
> absolutely sense that psql is transparent about that.

Thanks for the feedback.  I'll set the patch to "ready for committer" then.

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Thanks for the feedback.  I'll set the patch to "ready for committer" then.

So, just to clarify, we're settling on your v4 from [1]?

            regards, tom lane

[1] https://www.postgresql.org/message-id/d799f996f422231a99655f1223667d6d887e4c95.camel@cybertec.at

On 2023-11-09 20:19 +0100, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > Thanks for the feedback.  I'll set the patch to "ready for committer" then.
> 
> So, just to clarify, we're settling on your v4 from [1]?
> 
> [1] https://www.postgresql.org/message-id/d799f996f422231a99655f1223667d6d887e4c95.camel@cybertec.at

Yes from my side.

-- 
Erik

On Mon, 2023-11-13 at 11:27 +0100, Erik Wienhold wrote:
> On 2023-11-09 20:19 +0100, Tom Lane wrote:
> > Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > > Thanks for the feedback.  I'll set the patch to "ready for committer" then.
> >
> > So, just to clarify, we're settling on your v4 from [1]?
> >
> > [1] https://www.postgresql.org/message-id/d799f996f422231a99655f1223667d6d887e4c95.camel@cybertec.at
>
> Yes from my side.

+1

Yours,
Laurenz Albe

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Mon, Nov 13, 2023 at 12:36 PM Laurenz Albe <laurenz.albe@cybertec.at>
> wrote:
>> On Mon, 2023-11-13 at 11:27 +0100, Erik Wienhold wrote:
>>> On 2023-11-09 20:19 +0100, Tom Lane wrote:
>>>> So, just to clarify, we're settling on your v4 from [1]?

>>> Yes from my side.

>> +1

> +0.5 for the reasons already stated; but I get and accept the argument for
> NULL.

Patch pushed with minor adjustments, mainly rewriting some comments.

One notable change is that I dropped the newline whitespace printed
by printACLColumn.  That was contrary to the policy expressed in the
function's comment, and IMO it made -E output look worse not better.
The problem is that the calling code determines the indentation
that this targetlist item should have, and we don't want to outdent
from that.  I think it's better to make it one line, even though
that will run a bit over 80 columns.

I also got rid of the use of a created superuser in the test case.
The test seems pretty duplicative to me anyway, so let's just not
test the object types that need superuser.

> I will reiterate my preference for writing an explicit IS NULL branch in
> the case expression instead of relying upon the strict-ness of
> array_to_string.

Meh.  We were relying on that already, and it wasn't a problem.
I might have done it, except that it'd have made the one line
even longer and harder to read (and slower to execute, probably).

            regards, tom lane

On Mon, 2023-11-13 at 15:49 -0500, Tom Lane wrote:
> Patch pushed with minor adjustments, mainly rewriting some comments.

Thank you!

Yours,
Laurenz Albe

On 2023-11-13 21:49 +0100, Tom Lane wrote:
> Patch pushed with minor adjustments, mainly rewriting some comments.

Thanks a lot!

-- 
Erik