Обсуждение: doc - add missing documentation for "acldefault"
I couldn't find the documentation. Attached patch adds one. Probably this function should have been named pg_*. Too late. -- Fabien.
Вложения
Fabien, On 01.08.2018 15:28, Fabien COELHO wrote: > > I couldn't find the documentation. Attached patch adds one. > > Probably this function should have been named pg_*. Too late. > I think this is intentionally hidden function, like others started with acl*. postgres=# \df acl* List of fun Schema | Name | Result data type | ------------+-------------+------------------+---------------------------------- pg_catalog | aclcontains | boolean | aclitem[], aclitem pg_catalog | acldefault | aclitem[] | "char", oid pg_catalog | aclexplode | SETOF record | acl aclitem[], OUT grantor oid, O pg_catalog | aclinsert | aclitem[] | aclitem[], aclitem pg_catalog | aclitemeq | boolean | aclitem, aclitem pg_catalog | aclitemin | aclitem | cstring pg_catalog | aclitemout | cstring | aclitem pg_catalog | aclremove | aclitem[] | aclitem[], aclitem ----- Pavel Luzanov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Hello Pavel, >> I couldn't find the documentation. Attached patch adds one. >> >> Probably this function should have been named pg_*. Too late. > > I think this is intentionally hidden function, like others started with acl*. Yep, I thought of that. However, the point of having hidden and/or undocumented functions fails me: they are hard/impossible to find if you do not know they exist from the start, and if you ever find one you do not know what they do without reading the source code in detail, eg to know what to give arguments to get an answer. So I assumed that it was more lazyness and could be remedied with a doc patch for the one function I read. Maybe it would make sense to document the others as well, which are more straightforward. Also, that does not explain why they do not use a "pg_" prefix, which is already a way of saying "this is an internal-purpose function". So I would be in favor of prefixing them with pg_: as it is an undocumented feature, there would not be a compatibility break, somehow:-) > postgres=# \df acl* > List of fun > Schema | Name | Result data type | > ------------+-------------+------------------+---------------------------------- > pg_catalog | aclcontains | boolean | aclitem[], aclitem > pg_catalog | acldefault | aclitem[] | "char", oid > pg_catalog | aclexplode | SETOF record | acl aclitem[], OUT grantor > oid, O > pg_catalog | aclinsert | aclitem[] | aclitem[], aclitem > pg_catalog | aclitemeq | boolean | aclitem, aclitem > pg_catalog | aclitemin | aclitem | cstring > pg_catalog | aclitemout | cstring | aclitem > pg_catalog | aclremove | aclitem[] | aclitem[], aclitem -- Fabien.
Hello Fabien, > However, the point of having hidden and/or undocumented functions > fails me: they are hard/impossible to find if you do not know they > exist from > the start, and if you ever find one you do not know what they do > without reading the source code in detail, eg to know what to give > arguments to get an answer. At first, we must decide in which cases users will use them. And I don't see such cases. I must to know how to grant privileges, how to revoke them and how to check existing priveleges. All theese tasks documented in GRANT, REVOKE commands and system catalog descriptions. Your's patch from another thread closes the last hole - describing default privileges in various psql commands. ----- Pavel Luzanov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Wed, Aug 01, 2018 at 04:41:44PM +0300, Pavel Luzanov wrote: > postgres=# \df acl* > List of fun > Schema | Name | Result data type | > ------------+-------------+------------------+---------------------------------- > pg_catalog | aclcontains | boolean | aclitem[], aclitem > pg_catalog | acldefault | aclitem[] | "char", oid > pg_catalog | aclexplode | SETOF record | acl aclitem[], OUT grantor > oid, O > pg_catalog | aclinsert | aclitem[] | aclitem[], aclitem > pg_catalog | aclitemeq | boolean | aclitem, aclitem > pg_catalog | aclitemin | aclitem | cstring > pg_catalog | aclitemout | cstring | aclitem > pg_catalog | aclremove | aclitem[] | aclitem[], aclitem Some of them are definitely internal. For example, aclitemin and aclitemout are input and output functions respectively of the aclitem data type. I think they don't need to be documented and shouldn't have "pg_" prefix as they was created to maintenance aclitem data type. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Hello, >> However, the point of having hidden and/or undocumented functions fails me: >> they are hard/impossible to find if you do not know they exist from >> the start, and if you ever find one you do not know what they do without >> reading the source code in detail, eg to know what to give arguments to get >> an answer. > At first, we must decide in which cases users will use them. And I don't > see such cases. I must to know how to grant privileges, how to revoke > them and how to check existing priveleges. All theese tasks documented > in GRANT, REVOKE commands and system catalog descriptions. These are end-user needs. There are also other needs, such as devs. I see no reason to make the developer work harder by not providing documentation about available functions. Tom mention the "acldefault" function that I did not know existed, and I have read the doc! So I'm still favorable to documenting all functions:-) Maybe there could be a special section about special/internal functions, separate from functions which are more of interest to the end-user? But for me this is already the purpose of the "System information" sections in the documentation. Maybe there could be another sub-section about aclitem related functions in the "System information" section for these. > Your's patch from another thread closes the last hole - describing default > privileges in various psql commands. Yep. -- Fabien.
>> postgres=# \df acl*
>> List of fun
>> Schema | Name | Result data type |
>> ------------+-------------+------------------+----------------------------------
>> pg_catalog | aclcontains | boolean | aclitem[], aclitem
>> pg_catalog | acldefault | aclitem[] | "char", oid
>> pg_catalog | aclexplode | SETOF record | acl aclitem[], OUT grantor
>> oid, O
>> pg_catalog | aclinsert | aclitem[] | aclitem[], aclitem
>> pg_catalog | aclitemeq | boolean | aclitem, aclitem
>> pg_catalog | aclitemin | aclitem | cstring
>> pg_catalog | aclitemout | cstring | aclitem
>> pg_catalog | aclremove | aclitem[] | aclitem[], aclitem
>
> Some of them are definitely internal. For example, aclitemin and
> aclitemout are input and output functions respectively of the aclitem
> data type. I think they don't need to be documented and shouldn't have
> "pg_" prefix as they was created to maintenance aclitem data type.
Indeed, some are internal for managing the type itself. Maybe that
encompasses also "aclitemeq", not sure. I would see no harm in having them
documented though.
ISTM that acl{contains,default,explode,insert,remove} deserve a real doc.
--
Fabien.
Here is a version of the patch which documents briefly all aclitem-related functions, in a separate table. -- Fabien.
Вложения
On 08/03/2018 09:04 AM, Fabien COELHO wrote: > Here is a version of the patch which documents briefly all aclitem-related > functions, in a separate table. I claimed this patch for review and commit. Comments: --- * There is a comment in src/backend/utils/adt/acl.c noting that acldefault is "not documented for general use" which needs adjustment * It makes no sense to me to document purely internal functions, e.g. aclitemin/out. If we are going to do that we should do it universally, which is not true currently, and IMHO makes no sense anyway. * I do believe aclitemeq() has utility outside internal purposes. * The <indexterm> section is incomplete * Interestingly (since that is what started this thread apparently) I found myself questioning whether acldefault() is really worth documenting since the result will be misleading if the defaults have been altered via SQL. --- Attached patch addresses those items and does a bit of reordering and editorialization. If there are no objections I will commit it this way in a day or two. Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
Joe Conway <mail@joeconway.com> writes:
> * I do believe aclitemeq() has utility outside internal purposes.
Our normal policy is that we do not document functions that are meant to
be invoked through operators. The \df output saying that is sufficient:
# \df+ aclitemeq
List of functions
Schema | Name | Result data type | Argument data types | Type | Volatility | Parallel | Owner | Security |
Accessprivileges | Language | Source code | Description
------------+-----------+------------------+---------------------+------+------------+----------+----------+----------+-------------------+----------+-------------+------------------------------
pg_catalog | aclitemeq | boolean | aclitem, aclitem | func | immutable | safe | postgres | invoker |
| internal | aclitem_eq | implementation of = operator
(1 row)
I would strongly object to ignoring that policy in just one place.
Actually, it appears that most of these functions have associated
operators:
# select oid::regoperator, oprcode from pg_operator where oprright = 'aclitem'::regtype;
oid | oprcode
-----------------------+-------------
+(aclitem[],aclitem) | aclinsert
-(aclitem[],aclitem) | aclremove
@>(aclitem[],aclitem) | aclcontains
=(aclitem,aclitem) | aclitemeq
~(aclitem[],aclitem) | aclcontains
(5 rows)
So maybe what we really need is a table of operators not functions.
However, I don't object to documenting any function that has its
own pg_description string.
regards, tom lane
On 09/19/2018 10:54 AM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> * I do believe aclitemeq() has utility outside internal purposes. > > Our normal policy is that we do not document functions that are meant to > be invoked through operators. The \df output saying that is sufficient: <snip> > I would strongly object to ignoring that policy in just one place. Ok, fair enough. > Actually, it appears that most of these functions have associated > operators: > > # select oid::regoperator, oprcode from pg_operator where oprright = 'aclitem'::regtype; > oid | oprcode > -----------------------+------------- > +(aclitem[],aclitem) | aclinsert > -(aclitem[],aclitem) | aclremove > @>(aclitem[],aclitem) | aclcontains > =(aclitem,aclitem) | aclitemeq > ~(aclitem[],aclitem) | aclcontains > (5 rows) > > So maybe what we really need is a table of operators not functions. Good idea -- I will take a look at that. > However, I don't object to documenting any function that has its > own pg_description string. Ok. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
Hello, >> Our normal policy is that we do not document functions that are meant to >> be invoked through operators. The \df output saying that is sufficient: The output of \df is one thing, but I was looking at pg online documentation and hoping to find things as well. \dfS returns nearly 3000 results, it is not really pratical unless you already know what you are looking for, eg the name of the function... >> I would strongly object to ignoring that policy in just one place. > > Ok, fair enough. > >> Actually, it appears that most of these functions have associated >> operators: >> >> # select oid::regoperator, oprcode from pg_operator where oprright = 'aclitem'::regtype; >> oid | oprcode >> -----------------------+------------- >> +(aclitem[],aclitem) | aclinsert >> -(aclitem[],aclitem) | aclremove >> @>(aclitem[],aclitem) | aclcontains >> =(aclitem,aclitem) | aclitemeq >> ~(aclitem[],aclitem) | aclcontains >> (5 rows) >> >> So maybe what we really need is a table of operators not functions. > Good idea -- I will take a look at that. My initial complaint is that I did not know that there was an "acldefault" function while I was looking for that kind of thing. ISTM that this one should be kept explicitely in the doc. I'm okay with documenting operators instead of the basic undelying functions and skipping type management (in/out) functions, though. -- Fabien.
On 9/19/18, Tom Lane <tgl@sss.pgh.pa.us> wrote: > However, I don't object to documenting any function that has its > own pg_description string. Speaking of, that description string seems to have been neglected. I've attached a remedy for that. -John Naylor
Вложения
On 09/19/2018 12:30 PM, John Naylor wrote: > On 9/19/18, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> However, I don't object to documenting any function that has its >> own pg_description string. > > Speaking of, that description string seems to have been neglected. > I've attached a remedy for that. Thanks, will take care of it. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On 09/19/2018 11:18 AM, Joe Conway wrote:
> On 09/19/2018 10:54 AM, Tom Lane wrote:
>> Joe Conway <mail@joeconway.com> writes:
>>> * I do believe aclitemeq() has utility outside internal purposes.
>>
>> Our normal policy is that we do not document functions that are meant to
>> be invoked through operators. The \df output saying that is sufficient:
>
> <snip>
>
>> I would strongly object to ignoring that policy in just one place.
>
> Ok, fair enough.
>
>> Actually, it appears that most of these functions have associated
>> operators:
>>
>> # select oid::regoperator, oprcode from pg_operator where oprright = 'aclitem'::regtype;
>> oid | oprcode
>> -----------------------+-------------
>> +(aclitem[],aclitem) | aclinsert
>> -(aclitem[],aclitem) | aclremove
>> @>(aclitem[],aclitem) | aclcontains
>> =(aclitem,aclitem) | aclitemeq
>> ~(aclitem[],aclitem) | aclcontains
>> (5 rows)
>>
>> So maybe what we really need is a table of operators not functions.
>
> Good idea -- I will take a look at that.
>
>> However, I don't object to documenting any function that has its
>> own pg_description string.
Ok, so the attached version refactors/splits the group into two tables
-- operators and functions.
It drops aclinsert and aclremove entirely due to the fact that they no
longer do anything useful, to wit:
-----
Datum
aclinsert(PG_FUNCTION_ARGS)
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("aclinsert is no longer supported")));
PG_RETURN_NULL(); /* keep compiler quiet */
}
Datum
aclremove(PG_FUNCTION_ARGS)
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("aclremove is no longer supported")));
PG_RETURN_NULL(); /* keep compiler quiet */
}
-----
I also included John Naylor's patch with some minor editorialization.
Any further comments or complaints?
Thanks,
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Вложения
On 09/21/2018 01:51 PM, Joe Conway wrote: > On 09/19/2018 11:18 AM, Joe Conway wrote: >> On 09/19/2018 10:54 AM, Tom Lane wrote: >>> So maybe what we really need is a table of operators not functions. >> >> Good idea -- I will take a look at that. >> >>> However, I don't object to documenting any function that has its >>> own pg_description string. > > Ok, so the attached version refactors/splits the group into two tables > -- operators and functions. <snip> > I also included John Naylor's patch with some minor editorialization. > > Any further comments or complaints? Having seen none, committed/pushed. This did not seem worth back-patching, so I only pushed to master. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
Joe Conway <mail@joeconway.com> writes:
> Having seen none, committed/pushed. This did not seem worth
> back-patching, so I only pushed to master.
I don't see anything on gitmaster?
regards, tom lane
On 09/24/2018 10:01 AM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> Having seen none, committed/pushed. This did not seem worth >> back-patching, so I only pushed to master. > > I don't see anything on gitmaster? Hmm, yes, interesting -- I must of messed up my local git repo somehow. Will try again. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On 09/24/2018 10:09 AM, Joe Conway wrote: > On 09/24/2018 10:01 AM, Tom Lane wrote: >> Joe Conway <mail@joeconway.com> writes: >>> Having seen none, committed/pushed. This did not seem worth >>> back-patching, so I only pushed to master. >> >> I don't see anything on gitmaster? > > Hmm, yes, interesting -- I must of messed up my local git repo somehow. > Will try again. This time it seems to have worked. Sorry for the noise earlier :-/ Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development