Обсуждение: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

The patch has white space error
git apply /mnt/hgfs/tmp/comment_on_current_database_v1.patch
/mnt/hgfs/tmp/comment_on_current_database_v1.patch:52: trailing whitespace.    * schema-qualified or
catalog-qualified.
warning: 1 line adds whitespace errors.

The patch compiles clean, regression is clean. I tested auto
completion of current database, as well pg_dumpall output for comments
on multiple databases. Those are working fine. Do we want to add a
testcase for testing the SQL functionality as well as pg_dumpall
functionality?

Instead of changing get_object_address_unqualified(),
get_object_address_unqualified() and pg_get_object_address(), should
we just stick get_database_name(MyDatabaseId) as object name in
gram.y? That would be much cleaner than special handling of NIL list.
It looks dubious to set that list as NIL when the target is some
object. If we do that, we will spend extra cycles in finding database
id which is ready available as MyDatabaseId, but the code will be
cleaner. Another possibility is to use objnames to store the database
name and objargs to store the database id. That means we overload
objargs, which doesn't looks good either.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 12/30/16 9:28 PM, Fabrízio de Royes Mello wrote:
> The attached patch is reworked from a previous one [1] to better deal
> with get_object_address and pg_get_object_address.
> 
> Regards,
> 
> [1] https://www.postgresql.org/message-id/20150317171836.GC10492@momjian.us

The syntax we have used for the user/role case is ALTER USER
CURRENT_USER, not ALTER CURRENT USER, so this should be done in the same
way for databases.  Eventually, we'll also want ALTER DATABASE
current_database, and probably not ALTER CURRENT DATABASE, etc.

It's also curious that we don't support COMMENT on whatever CURRENT_USER
yet.  It looks like the previous patch to allow ALTER USER CURRENT_USER
etc. was not even applied to pg_dump.

I think this patch is a good direction to move in, and it seems everyone
else agreed, but I'm looking for a bit more of a holistic solution than
one command at a time here and there.  Define a goal such as, allow
restoring into a differently-named database, and then see what needs to
happen to achieve that.

The implementation looks generally OK, but I'm not sure why you need
this part:

diff --git a/src/backend/catalog/objectaddress.c
b/src/backend/catalog/objectaddress.c
index bb4b080..71bffae 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -621,6 +621,9 @@ static const struct object_type_map    {        "database", OBJECT_DATABASE    },
+    {
+        "current database", OBJECT_DATABASE
+    },    /* OCLASS_TBLSPACE */    {        "tablespace", OBJECT_TABLESPACE


-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 3, 2017 at 12:06 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> Instead of changing get_object_address_unqualified(),
> get_object_address_unqualified() and pg_get_object_address(), should
> we just stick get_database_name(MyDatabaseId) as object name in
> gram.y?

No.  Note this comment at the top of gram.y:
*        In general, nothing in this file should initiate database accesses*        nor depend on changeable state
(suchas SET variables).  If you do*        database accesses, your code will fail when we have aborted the*
currenttransaction and are just parsing commands to find the next*        ROLLBACK or COMMIT.  If you make use of SET
variables,then you*        will do the wrong thing in multi-query strings like this:*                      SET
constraint_exclusionTO off; SELECT * FROM foo;*        because the entire string is parsed by gram.y before the SET
gets*       executed.  Anything that depends on the database or changeable state*        should be handled during parse
analysisso that it happens at the*        right time not the wrong time. 

I grant you that MyDatabaseId can't (currently, anyway) change during
the lifetime of a single backend, but it still seems like a bad idea
to make gram.y depend on that.  If nothing else, it's problematic if
we want to deparse the DDL statement (as Fabrízio also points out).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 3, 2017 at 6:40 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> Hi Ashutosh,
>
> First of all thanks for your review.
>
>
> On Tue, Jan 3, 2017 at 3:06 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>> The patch has white space error
>> git apply /mnt/hgfs/tmp/comment_on_current_database_v1.patch
>> /mnt/hgfs/tmp/comment_on_current_database_v1.patch:52: trailing
>> whitespace.
>>      * schema-qualified or catalog-qualified.
>> warning: 1 line adds whitespace errors.
>>
>
> I'll fix.
>
>
>> The patch compiles clean, regression is clean. I tested auto
>> completion of current database, as well pg_dumpall output for comments
>> on multiple databases. Those are working fine. Do we want to add a
>> testcase for testing the SQL functionality as well as pg_dumpall
>> functionality?
>>
>
> While looking into the src/test/regress/sql files I didn't find any test
> cases for shared objects (databases, roles, tablespaces, procedural
> languages, ...).


Right. There's comment.sql but it's about comments in language and not
comments on database objects. It looks like the COMMENT ON for various
objects is tested in test files for those objects and there isn't any
tests testing databases e.g. ALTER DATABASE in sql directory.

> Should we need also add test cases for this kind of
> objects?
>
Possibly, we don't have those testcases, because those will affect
existing objects when run through "make installcheck". But current
database is always going to be "regression" for these tests. That
database is dropped when installcheck starts. So, we can add a
testcase for it. But I am not sure where should we add it. Creating a
new file just for COMMENT ON DATABASE doesn't look good.

>
>> Instead of changing get_object_address_unqualified(),
>> get_object_address_unqualified() and pg_get_object_address(), should
>> we just stick get_database_name(MyDatabaseId) as object name in
>> gram.y? That would be much cleaner than special handling of NIL list.
>> It looks dubious to set that list as NIL when the target is some
>> object. If we do that, we will spend extra cycles in finding database
>> id which is ready available as MyDatabaseId, but the code will be
>> cleaner. Another possibility is to use objnames to store the database
>> name and objargs to store the database id. That means we overload
>> objargs, which doesn't looks good either.
>>
>
> In the previous thread Alvaro point me out about a possible DDL deparse
> inconsistency [1] and because this reason we decide to think better and
> rework this patch.
>
> I confess I'm not too happy with this code yet, and thinking better maybe we
> should create a new object type called OBJECT_CURRENT_DATABASE to handle it
> different than OBJECT_DATABASE. Opinions???
>

Please read my reply to Robert's mail.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Tue, Jan 3, 2017 at 9:18 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 12/30/16 9:28 PM, Fabrízio de Royes Mello wrote:
>> The attached patch is reworked from a previous one [1] to better deal
>> with get_object_address and pg_get_object_address.
>>
>> Regards,
>>
>> [1] https://www.postgresql.org/message-id/20150317171836.GC10492@momjian.us
>
> The syntax we have used for the user/role case is ALTER USER
> CURRENT_USER, not ALTER CURRENT USER, so this should be done in the same
> way for databases.  Eventually, we'll also want ALTER DATABASE
> current_database, and probably not ALTER CURRENT DATABASE, etc.

We don't allow a user to be created as CURRENT_USER, but we allow a
database to be created with name CURRENT_DATABASE.
postgres=# create user current_user;
ERROR:  CURRENT_USER cannot be used as a role name here
LINE 1: create user current_user;                   ^
postgres=# create database current_database;
CREATE DATABASE

We will need to make CURRENT_DATABASE a reserved keyword. But I like
this idea more than COMMENT ON CURRENT DATABASE.


--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Tue, Jan 3, 2017 at 10:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jan 3, 2017 at 12:06 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> Instead of changing get_object_address_unqualified(),
>> get_object_address_unqualified() and pg_get_object_address(), should
>> we just stick get_database_name(MyDatabaseId) as object name in
>> gram.y?
>
> No.  Note this comment at the top of gram.y:
>
>  *        In general, nothing in this file should initiate database accesses
>  *        nor depend on changeable state (such as SET variables).  If you do
>  *        database accesses, your code will fail when we have aborted the
>  *        current transaction and are just parsing commands to find the next
>  *        ROLLBACK or COMMIT.  If you make use of SET variables, then you
>  *        will do the wrong thing in multi-query strings like this:
>  *                      SET constraint_exclusion TO off; SELECT * FROM foo;
>  *        because the entire string is parsed by gram.y before the SET gets
>  *        executed.  Anything that depends on the database or changeable state
>  *        should be handled during parse analysis so that it happens at the
>  *        right time not the wrong time.
>
> I grant you that MyDatabaseId can't (currently, anyway) change during
> the lifetime of a single backend, but it still seems like a bad idea
> to make gram.y depend on that.  If nothing else, it's problematic if
> we want to deparse the DDL statement (as Fabrízio also points out).
>

Thanks for pointing that out.

I think that handling NIL list in get_object_address_unqualified(),
get_object_address_unqualified() and pg_get_object_address() doesn't
really look good. Intuitively having a NIL list indicates no object
being specified, hence those checks.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> this idea more than COMMENT ON CURRENT DATABASE.

We already have the reserved key word CURRENT_CATALOG, which is the
standard spelling.  But I wouldn't be bothered if we made
CURRENT_DATABASE somewhat reserved as well.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
>> We will need to make CURRENT_DATABASE a reserved keyword. But I like
>> this idea more than COMMENT ON CURRENT DATABASE.
>
> We already have the reserved key word CURRENT_CATALOG, which is the
> standard spelling.  But I wouldn't be bothered if we made
> CURRENT_DATABASE somewhat reserved as well.

Maybe I'm just lacking in imagination, but what's the argument against
spelling it CURRENT DATABASE?  AFAICS, that doesn't require reserving
anything new at all, and it also looks more SQL-ish to me.  SQL
generally tries to emulate English, and I don't normally
speak_hyphenated_words.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan  9, 2017 at 01:34:03PM -0500, Robert Haas wrote:
> On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
> > On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> >> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> >> this idea more than COMMENT ON CURRENT DATABASE.
> >
> > We already have the reserved key word CURRENT_CATALOG, which is the
> > standard spelling.  But I wouldn't be bothered if we made
> > CURRENT_DATABASE somewhat reserved as well.
> 
> Maybe I'm just lacking in imagination, but what's the argument against
> spelling it CURRENT DATABASE?  AFAICS, that doesn't require reserving
> anything new at all, and it also looks more SQL-ish to me.  SQL
> generally tries to emulate English, and I don't normally
> speak_hyphenated_words.

I assume it is to match our use of CURRENT_USER as having special
meaning.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Bruce Momjian wrote:
> On Mon, Jan  9, 2017 at 01:34:03PM -0500, Robert Haas wrote:
> > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com> wrote:
> > > On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> > >> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> > >> this idea more than COMMENT ON CURRENT DATABASE.
> > >
> > > We already have the reserved key word CURRENT_CATALOG, which is the
> > > standard spelling.  But I wouldn't be bothered if we made
> > > CURRENT_DATABASE somewhat reserved as well.
> > 
> > Maybe I'm just lacking in imagination, but what's the argument against
> > spelling it CURRENT DATABASE?  AFAICS, that doesn't require reserving
> > anything new at all, and it also looks more SQL-ish to me.  SQL
> > generally tries to emulate English, and I don't normally
> > speak_hyphenated_words.
> 
> I assume it is to match our use of CURRENT_USER as having special
> meaning.

CURRENT_USER is a standards-mandated keyword, but CURRENT_DATABASE is
not.  The closest thing SQL has is CURRENT_CATALOG, which is the string
that identifies the "current default catalog".  This would lead us to   COMMENT ON DATABASE CURRENT_CATALOG

Do we want that spelling?  It looks ugly to me.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jan  9, 2017 at 04:52:46PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Mon, Jan  9, 2017 at 01:34:03PM -0500, Robert Haas wrote:
> > > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
> > > <peter.eisentraut@2ndquadrant.com> wrote:
> > > > On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> > > >> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> > > >> this idea more than COMMENT ON CURRENT DATABASE.
> > > >
> > > > We already have the reserved key word CURRENT_CATALOG, which is the
> > > > standard spelling.  But I wouldn't be bothered if we made
> > > > CURRENT_DATABASE somewhat reserved as well.
> > > 
> > > Maybe I'm just lacking in imagination, but what's the argument against
> > > spelling it CURRENT DATABASE?  AFAICS, that doesn't require reserving
> > > anything new at all, and it also looks more SQL-ish to me.  SQL
> > > generally tries to emulate English, and I don't normally
> > > speak_hyphenated_words.
> > 
> > I assume it is to match our use of CURRENT_USER as having special
> > meaning.
> 
> CURRENT_USER is a standards-mandated keyword, but CURRENT_DATABASE is
> not.  The closest thing SQL has is CURRENT_CATALOG, which is the string
> that identifies the "current default catalog".  This would lead us to
>     COMMENT ON DATABASE CURRENT_CATALOG
> 
> Do we want that spelling?  It looks ugly to me.

Agreed.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 1/9/17 1:34 PM, Robert Haas wrote:
> On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
>>> We will need to make CURRENT_DATABASE a reserved keyword. But I like
>>> this idea more than COMMENT ON CURRENT DATABASE.
>>
>> We already have the reserved key word CURRENT_CATALOG, which is the
>> standard spelling.  But I wouldn't be bothered if we made
>> CURRENT_DATABASE somewhat reserved as well.
> 
> Maybe I'm just lacking in imagination, but what's the argument against
> spelling it CURRENT DATABASE?

To achieve consistent support for specifying the current database, we
would need to change the grammar for every command involving databases.
And it would also set a precedent for similar commands, such as current
user/role.  Plus support in psql, pg_dump, etc. would get more complicated.

Instead, it would be simpler to define a grammar symbol like

database_name: ColId | CURRENT_DATABASE

and make a small analogous change in objectaddress.c and you're done.

Compare rolespec in gram.y.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/9/17 2:52 PM, Alvaro Herrera wrote:
> CURRENT_USER is a standards-mandated keyword, but CURRENT_DATABASE is
> not.  The closest thing SQL has is CURRENT_CATALOG, which is the string
> that identifies the "current default catalog".  This would lead us to
>     COMMENT ON DATABASE CURRENT_CATALOG
> 
> Do we want that spelling?  It looks ugly to me.

Hence my suggestion earlier to make CURRENT_DATABASE reserved.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jan 9, 2017 at 3:14 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> To achieve consistent support for specifying the current database, we
> would need to change the grammar for every command involving databases.

I wouldn't have thought there would be all that many of those, though.

> And it would also set a precedent for similar commands, such as current
> user/role.

True.  Maybe it's a GOOD precedent, though.

> Plus support in psql, pg_dump, etc. would get more complicated.

I'm not totally convinced...

> Instead, it would be simpler to define a grammar symbol like
>
> database_name: ColId | CURRENT_DATABASE
>
> and make a small analogous change in objectaddress.c and you're done.
>
> Compare rolespec in gram.y.

...but that's certainly an existing precedent for your proposal.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 1/26/17 1:20 PM, Fabrízio de Royes Mello wrote:
> Ok, but doing in that way the syntax would be:
> 
> COMMENT ON DATABASE CURRENT_DATABASE IS 'comment';

Yes, that's right.  We also have ALTER USER CURRENT_USER already.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sun, Jan 29, 2017 at 3:33 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> On Sat, Jan 28, 2017 at 4:26 PM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
>> On Fri, Jan 27, 2017 at 8:53 PM, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>> >
>> > On 1/26/17 1:20 PM, Fabrízio de Royes Mello wrote:
>> > > Ok, but doing in that way the syntax would be:
>> > >
>> > > COMMENT ON DATABASE CURRENT_DATABASE IS 'comment';
>> >
>> > Yes, that's right.  We also have ALTER USER CURRENT_USER already.
>> >
>>
>> Ok, but if we make CURRENT_DATABASE reserved wouldn't it lead us to a
>> conflict with current_database() function?

Patch marked as returned with feedback as no new version has been
provided for the last 2 weeks and a half. Please feel free to update
if that's not adapted. The patch was waiting on author's input for
some time by the way..
--
Michael

On 1/28/17 1:33 PM, Fabrízio de Royes Mello wrote:
> I did what you suggested making CURRENT_DATABASE reserved but I got the
> following error during the bootstrap:

current_database is also used as a function name, so you need to do some
parser work to get it working in all the right ways.  Hard to tell
without a patch to look at.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services