Обсуждение: Tab Completion for CREATE DATABASE ... TEMPLATE ...
We use "CREATE DATABASE foo TEMPLATE foo_bk" to restore development databases to a known snapshot (ex: prior to testing DB migrations). Currently psql only autocompletes "foo_bk" if it's marked as a template database. It's mildly inconvenient to have to type out the entire database name (as they're not marked as templates).
The CREATE DATABASE command allows a super user to use any database as the template, and a non-super user (with CREATEDB privilege) to use any database of which it's the owner.
The attached patch updates the psql "CREATE DATABASE ... TEMPLATE <tab>" completion to match what the command actually allows.
Вложения
Hello Sehrope Sarkuni,
I have reviewed the patch.
It is very simple (only an SQL query in the "psql" application changed).
It applies at the top of master.
It implements completion database names ("<X>") for commands like
"CREATE DATABASE ... TEMPLATE <X>".
According to the documentation since 9.2 till devel a database can be
used as a template if it has a "datistemplate" mark or by superusers
or by their owners.
Previous implementation checked only "datistemplate" mark.
Tested manually in versions 9.2 and 10devel, I hope it can be
back-patched to all supported versions.
No documentation needed.
Mark it as "Ready for committer".
P.S.: While I was reviewing I simplified SQL query: improved version
only 2 seqscans instead of 3 seqscans with an inner loop in an
original one.
Please find a file "tab-complete-create-database-improved.patch" attached.
--
Best regards,
Vitaly Burovoy
Вложения
On Sat, Sep 10, 2016 at 12:26 AM, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote: > According to the documentation since 9.2 till devel a database can be > used as a template if it has a "datistemplate" mark or by superusers > or by their owners. Hm. I wonder whether the following failure of "bob" to use the specified template is intended or a bug. It seems to make sense to sort that out before looking at the related tab completion. test=# checkpoint; CHECKPOINT test=# create role fred with createdb; CREATE ROLE test=# create user bob; CREATE ROLE test=# grant fred to bob; GRANT ROLE test=# alter database postgres owner to fred; ALTER DATABASE test=# set role fred; SET test=> create database db1 template postgres; CREATE DATABASE test=> reset role; RESET test=# set role bob; SET test=> create database db2 template postgres; ERROR: permission denied to create database Opinions on whether this is a bug or correct behavior? -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@gmail.com> writes:
> test=# create role fred with createdb;
> CREATE ROLE
> test=# create user bob;
> CREATE ROLE
> test=# grant fred to bob;
> GRANT ROLE
> test=# alter database postgres owner to fred;
> ALTER DATABASE
> test=# set role fred;
> SET
> test=> create database db1 template postgres;
> CREATE DATABASE
> test=> reset role;
> RESET
> test=# set role bob;
> SET
> test=> create database db2 template postgres;
> ERROR: permission denied to create database
> Opinions on whether this is a bug or correct behavior?
It's operating as designed, anyway. Role properties such as CREATEDB
are not grantable privileges and thus can't be inherited via GRANT.
There's been some muttering about changing that; but most people don't
seem to think that letting superuserness in particular be inherited
would be a good thing, so it hasn't gone anywhere.
regards, tom lane
On Sat, Sep 10, 2016 at 12:26 AM, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote: > Tested manually in versions 9.2 and 10devel, I hope it can be > back-patched to all supported versions. We don't normally back-patch tab completion work unless it is fixing a crash or removing displayed entries which make no sense. This is in line with the overall project policy of only back-patching bug fixes, not enhancements. At this point I have only applied it to the master branch, and won't go back farther without a clear consensus to do so. It's not that this particular change is all that scary, but it's a step down a slippery slope that should not be taken without everyone being on the same page about why this should be an exception and the next thing should not. > Mark it as "Ready for committer". > > P.S.: While I was reviewing I simplified SQL query: improved version > only 2 seqscans instead of 3 seqscans with an inner loop in an > original one. > Please find a file "tab-complete-create-database-improved.patch" attached. I was able to find cases during test which were not handled correctly with either version, so I tweaked the query a little. Also, for future reference, please try to use white-space that matches surrounding code -- it make scanning through code less "jarring". Thanks all for the patch and the reviews! -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 9/11/16, Kevin Grittner <kgrittn@gmail.com> wrote: > On Sat, Sep 10, 2016 at 12:26 AM, Vitaly Burovoy >> Mark it as "Ready for committer". >> >> P.S.: While I was reviewing I simplified SQL query: improved version >> only 2 seqscans instead of 3 seqscans with an inner loop in an >> original one. >> Please find a file "tab-complete-create-database-improved.patch" >> attached. > > I was able to find cases during test which were not handled > correctly with either version, so I tweaked the query a little. Hmm. Which one? Attempt to "SET ROLE <grouprole>"? Unfortunately, I after reading your letter I realized that I missed a case (it is not working even with your version): postgres=# CREATE GROUP g1; CREATE ROLE postgres=# CREATE GROUP g2; CREATE ROLE postgres=# GRANT g2 TO g1; GRANT ROLE postgres=# CREATE USER u1 CREATEDB; CREATE ROLE postgres=# GRANT g1 TO u1; GRANT ROLE postgres=# CREATE DATABASE db_tpl; CREATE DATABASE postgres=# ALTER DATABASE db_tpl OWNER TO g2; ALTER DATABASE postgres=# SET ROLE g1; SET postgres=> CREATE DATABASE db1 TEMPLATE db -- shows nothing postgres=> CREATE DATABASE db1 TEMPLATE db^C -- shows nothing postgres=> RESET ROLE; RESET postgres=# SET ROLE u1; SET postgres=> CREATE DATABASE db1 TEMPLATE db -- shows nothing postgres=> CREATE DATABASE db1 TEMPLATE db^C -- shows nothing postgres=> CREATE DATABASE db1 TEMPLATE db_tpl; CREATE DATABASE It seems if a user has the CREATEDB privilege and he is a member of a group (role) which owns a database, he can use the database as a template. But to support it recursive queries should be used. Is it useless or should be fixed? > Also, for future reference, please try to use white-space that > matches surrounding code -- it make scanning through code less > "jarring". I tried to. Should "FROM" be at the same row as sub-"SELECT" is placed? Or there should be something else (I'm now only about white-space formatting)? Surrounding code looks similar enough for me... =( > Thanks all for the patch and the reviews! Thank you! > -- > Kevin Grittner > EDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company -- Best regards, Vitaly Burovoy
Vitaly Burovoy <vitaly.burovoy@gmail.com> writes:
> On 9/11/16, Kevin Grittner <kgrittn@gmail.com> wrote:
>> I was able to find cases during test which were not handled
>> correctly with either version, so I tweaked the query a little.
> Hmm. Which one? Attempt to "SET ROLE <grouprole>"?
> Unfortunately, I after reading your letter I realized that I missed a
> case (it is not working even with your version):
I wasn't aware that this patch was doing anything nontrivial ...
After looking at it I think it's basically uninformed about how to test
for ownership. An explicit join against pg_roles is almost never the
right way for SQL queries to do that. Lose the join and write it more
like this:
+"SELECT pg_catalog.quote_ident(d.datname) "\
+" FROM pg_catalog.pg_database d "\
+" WHERE substring(pg_catalog.quote_ident(d.datname),1,%d)='%s' "\
+" AND (d.datistemplate OR pg_catalog.pg_has_role(d.datdba, 'USAGE'))"
See the information_schema views for precedent.
regards, tom lane
On 9/11/16, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Vitaly Burovoy <vitaly.burovoy@gmail.com> writes: >> On 9/11/16, Kevin Grittner <kgrittn@gmail.com> wrote: >>> I was able to find cases during test which were not handled >>> correctly with either version, so I tweaked the query a little. > >> Hmm. Which one? Attempt to "SET ROLE <grouprole>"? >> Unfortunately, I after reading your letter I realized that I missed a >> case (it is not working even with your version): > > I wasn't aware that this patch was doing anything nontrivial ... > > After looking at it I think it's basically uninformed about how to test > for ownership. An explicit join against pg_roles is almost never the > right way for SQL queries to do that. Lose the join and write it more > like this: > > +"SELECT pg_catalog.quote_ident(d.datname) "\ > +" FROM pg_catalog.pg_database d "\ > +" WHERE substring(pg_catalog.quote_ident(d.datname),1,%d)='%s' "\ > +" AND (d.datistemplate OR pg_catalog.pg_has_role(d.datdba, 'USAGE'))" > > See the information_schema views for precedent. > > regards, tom lane Wow! I have not pay enough attention to a description of "pg_has_role". Your version works for all my tests. Thank you. -- Best regards, Vitaly Burovoy
On Sun, Sep 11, 2016 at 8:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wasn't aware that this patch was doing anything nontrivial ...
Well, it is not doing anything other than showing candidate
templates for tab completion beyond those flagged as template
databases.
> After looking at it I think it's basically uninformed about how to test
> for ownership. An explicit join against pg_roles is almost never the
> right way for SQL queries to do that. Lose the join and write it more
> like this:
>
> +"SELECT pg_catalog.quote_ident(d.datname) "\
> +" FROM pg_catalog.pg_database d "\
> +" WHERE substring(pg_catalog.quote_ident(d.datname),1,%d)='%s' "\
> +" AND (d.datistemplate OR pg_catalog.pg_has_role(d.datdba, 'USAGE'))"
But that gives incorrect results for the case I asked about earlier
on the thread, while the query I pushed gives correct results:
test=# \du fred List of rolesRole name | Attributes | Member of
-----------+-------------------------+-----------fred | Create DB, Cannot login | {}
test=# \du bob List of rolesRole name | Attributes | Member of
-----------+------------+-----------bob | | {fred}
test=# \l List of databases Name | Owner | Encoding | Collate | Ctype |
Access
privileges
------------+---------+----------+-------------+-------------+---------------------db1 | fred | UTF8 |
en_US.UTF-8| en_US.UTF-8 |db2 | bob | UTF8 | en_US.UTF-8 | en_US.UTF-8 |postgres | fred | UTF8
|en_US.UTF-8 | en_US.UTF-8 |regression | kgrittn | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
=Tc/kgrittn + | | | | |
kgrittn=CTc/kgrittntemplate0 | kgrittn | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
=c/kgrittn + | | | | |
kgrittn=CTc/kgrittntemplate1 | kgrittn | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
=c/kgrittn + | | | | |
kgrittn=CTc/kgrittntest | kgrittn | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
(7 rows)
test=# set role bob;
SET
test=> SELECT pg_catalog.quote_ident(d.datname) FROM pg_catalog.pg_database dWHERE (d.datistemplate OR
pg_catalog.pg_has_role(d.datdba,'USAGE'));quote_ident
-------------template1template0postgresdb1db2
(5 rows)
test=> SELECT pg_catalog.quote_ident(datname) FROM pg_catalog.pg_database d JOIN pg_catalog.pg_roles r ON rolname =
CURRENT_USERWHERE(datistemplate OR rolsuper OR datdba = r.oid);quote_ident
-------------template1template0db2
(3 rows)
There is absolutely no question that the query you suggest is
wrong; the only case I had to think about very hard after
establishing that CREATEDB was not inherited was when bob owned a
database but did not have CREATEDB permission. It seemed to me
least astonishing to show such a database, because that seemed
parallel to, for example, tab completion for table names on CREATE
TABLE AS. We show a database object in the tab completion when it
would be available except for absence of a permission on the user.
That seems sane to me, because the attempt to actually execute with
that object gives a potentially useful error message. Anyway, I
tend to like symmetry in these things -- it could also be
considered sane not to show t2 on tab completion fro bob above, but
then we should probably add more security tests to other tab
completion queries, like CREATE TABLE AS ... SELECT ... FROM.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 9/12/16, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Sun, Sep 11, 2016 at 8:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> I wasn't aware that this patch was doing anything nontrivial ...
>
> Well, it is not doing anything other than showing candidate
> templates for tab completion beyond those flagged as template
> databases.
>
>> After looking at it I think it's basically uninformed about how to test
>> for ownership. An explicit join against pg_roles is almost never the
>> right way for SQL queries to do that. Lose the join and write it more
>> like this:
>>
>> +"SELECT pg_catalog.quote_ident(d.datname) "\
>> +" FROM pg_catalog.pg_database d "\
>> +" WHERE substring(pg_catalog.quote_ident(d.datname),1,%d)='%s' "\
>> +" AND (d.datistemplate OR pg_catalog.pg_has_role(d.datdba, 'USAGE'))"
>
> But that gives incorrect results for the case I asked about earlier
> on the thread, while the query I pushed gives correct results:
>
> test=# \du fred
> List of roles
> Role name | Attributes | Member of
> -----------+-------------------------+-----------
> fred | Create DB, Cannot login | {}
>
> test=# \du bob
> List of roles
> Role name | Attributes | Member of
> -----------+------------+-----------
> bob | | {fred}
>
> test=# \l
> List of databases
> Name | Owner | Encoding | Collate | Ctype | Access
> privileges
> ------------+---------+----------+-------------+-------------+---------------------
> db1 | fred | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
> db2 | bob | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
> postgres | fred | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
> regression | kgrittn | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
> =Tc/kgrittn +
> | | | | |
> kgrittn=CTc/kgrittn
> template0 | kgrittn | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
> =c/kgrittn +
> | | | | |
> kgrittn=CTc/kgrittn
> template1 | kgrittn | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
> =c/kgrittn +
> | | | | |
> kgrittn=CTc/kgrittn
> test | kgrittn | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
> (7 rows)
>
> test=# set role bob;
> SET
> test=> SELECT pg_catalog.quote_ident(d.datname)
> FROM pg_catalog.pg_database d
> WHERE (d.datistemplate OR pg_catalog.pg_has_role(d.datdba, 'USAGE'));
> quote_ident
> -------------
> template1
> template0
> postgres
> db1
> db2
> (5 rows)
>
> test=> SELECT pg_catalog.quote_ident(datname)
> FROM pg_catalog.pg_database d
> JOIN pg_catalog.pg_roles r ON rolname = CURRENT_USER
> WHERE (datistemplate OR rolsuper OR datdba = r.oid);
> quote_ident
> -------------
> template1
> template0
> db2
> (3 rows)
>
> There is absolutely no question that the query you suggest is
> wrong; the only case I had to think about very hard after
> establishing that CREATEDB was not inherited was when bob owned a
> database but did not have CREATEDB permission. It seemed to me
> least astonishing to show such a database, because that seemed
> parallel to, for example, tab completion for table names on CREATE
> TABLE AS. We show a database object in the tab completion when it
> would be available except for absence of a permission on the user.
> That seems sane to me, because the attempt to actually execute with
> that object gives a potentially useful error message. Anyway, I
> tend to like symmetry in these things -- it could also be
> considered sane not to show t2 on tab completion fro bob above, but
> then we should probably add more security tests to other tab
> completion queries, like CREATE TABLE AS ... SELECT ... FROM.
>
> --
> Kevin Grittner
> EDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
While I was checking Tom's version, I thought that way, but finally I
realized there is no matter what tab completion shows because user
without the CREATEDB privileges can create no database at all:
postgres=# \du fred List of rolesRole name | Attributes | Member of
-----------+-------------------------+-----------fred | Create DB, Cannot login | {}
postgres=# \du bob List of rolesRole name | Attributes | Member of
-----------+------------+-----------bob | | {fred}
postgres=# \l db* List of databasesName | Owner | Encoding | Collate | Ctype |
Accessprivileges
------+-------+----------+-------------+-------------+-------------------db1 | fred | UTF8 | en_US.UTF-8 |
en_US.UTF-8|db2 | bob | UTF8 | en_US.UTF-8 | en_US.UTF-8 |
(2 rows)
postgres=# set role bob;
SET
postgres=> CREATE DATABASE ss TEMPLATE db -- shows both
db1 db2
postgres=> CREATE DATABASE ss TEMPLATE db2;
ERROR: permission denied to create database
postgres=>
So a check for the CREATEDB privilege should be done at the point
whether to show CREATE DATABASE or not.
But if a user has privileges, Tom's version works fine.
--
Best regards,
Vitaly Burovoy
Kevin Grittner <kgrittn@gmail.com> writes:
> But that gives incorrect results for the case I asked about earlier
> on the thread, while the query I pushed gives correct results:
AFAICS, my query gives correct results for that case. bob is permitted
to copy fred's databases db1 and postgres, or would be if he had createdb
privileges. The query you committed is flat wrong, because it doesn't
account for database ownership being granted via role membership.
> There is absolutely no question that the query you suggest is
> wrong;
Please provide some evidence of that.
> the only case I had to think about very hard after
> establishing that CREATEDB was not inherited was when bob owned a
> database but did not have CREATEDB permission.
I agree that we should not look at createdb here. If we did, the
effect would be that for a user without createdb, psql would refuse
to offer any completions, which seems more confusing than helpful.
(If it were the tab completion's code to enforce that, which it isn't,
surely it should have complained much earlier in the command.)
regards, tom lane
On Mon, Sep 12, 2016 at 7:55 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kevin Grittner <kgrittn@gmail.com> writes: >> But that gives incorrect results for the case I asked about earlier >> on the thread, while the query I pushed gives correct results: > > AFAICS, my query gives correct results for that case. bob is permitted > to copy fred's databases db1 and postgres, or would be if he had createdb > privileges. The query you committed is flat wrong, because it doesn't > account for database ownership being granted via role membership. Ah, there was a flaw in my testing script. It appeared from my (flawed) testing that the inherited ownership wasn't enough to allow use as a template. With the script fixed I see that it does. Sorry for the noise. Will fix. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Sep 12, 2016 at 8:14 AM, Kevin Grittner <kgrittn@gmail.com> wrote: > On Mon, Sep 12, 2016 at 7:55 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> The query you committed is flat wrong, because it doesn't >> account for database ownership being granted via role membership. > > Ah, there was a flaw in my testing script. It appeared from my > (flawed) testing that the inherited ownership wasn't enough to > allow use as a template. With the script fixed I see that it does. > > Sorry for the noise. Will fix. Done. Thanks guys! -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company