Thread: Tab Completion for CREATE DATABASE ... TEMPLATE ...

Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Sehrope Sarkuni
Date:
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.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

Attachment

Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Vitaly Burovoy
Date:
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

Attachment

Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Kevin Grittner
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Tom Lane
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Kevin Grittner
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Vitaly Burovoy
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Tom Lane
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Vitaly Burovoy
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Kevin Grittner
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Vitaly Burovoy
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Tom Lane
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Kevin Grittner
Date:
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



Re: [REVIEW] Tab Completion for CREATE DATABASE ... TEMPLATE ...

From
Kevin Grittner
Date:
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