Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Sun, Jan 29, 2017 at 3:33 AM, Fabrízio de Royes Mello wrote: > On Sat, Jan 28, 2017 at 4:26 PM, Fabrízio de Royes Mello > wrote: >> On Fri, Jan 27, 2017 at 8:53 PM, Peter Eisentraut >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Sat, Jan 28, 2017 at 4:26 PM, Fabrízio de Royes Mello < fabriziome...@gmail.com> wrote: > > > > On Fri, Jan 27, 2017 at 8:53 PM, Peter Eisentraut < peter.eisentr...@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? > I did what you suggested making CURRENT_DATABASE reserved but I got the following error during the bootstrap: The files belonging to this database system will be owned by user "fabrizio". This user must also own the server process. The database cluster will be initialized with locale "en_US.UTF-8". The default database encoding has accordingly been set to "UTF8". The default text search configuration will be set to "english". Data page checksums are disabled. creating directory /tmp/master5432 ... ok creating subdirectories ... ok selecting default max_connections ... 100 selecting default shared_buffers ... 128MB selecting dynamic shared memory implementation ... posix creating configuration files ... ok running bootstrap script ... ok performing post-bootstrap initialization ... 2017-01-28 16:19:07.994 BRST [18252] FATAL: syntax error at or near "current_database" at character 120 2017-01-28 16:19:07.994 BRST [18252] STATEMENT: /* * 5.2 * INFORMATION_SCHEMA_CATALOG_NAME view */ CREATE VIEW information_schema_catalog_name AS SELECT CAST(current_database() AS sql_identifier) AS catalog_name; child process exited with exit code 1 initdb: removing data directory "/tmp/master5432" pg_ctl: directory "/tmp/master5432" does not exist psql: could not connect to server: No such file or directory Is the server running locally and accepting connections on Unix domain socket "/tmp/.s.PGSQL.5432"? psql: could not connect to server: No such file or directory Is the server running locally and accepting connections on Unix domain socket "/tmp/.s.PGSQL.5432"? If I use CURRENT_CATALOG instead this error doesn't occurs of course... -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog: http://fabriziomello.github.io >> Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello >> Github: http://github.com/fabriziomello
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Fri, Jan 27, 2017 at 8:53 PM, Peter Eisentraut < peter.eisentr...@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? Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog: http://fabriziomello.github.io >> Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello >> Github: http://github.com/fabriziomello
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Mon, Jan 9, 2017 at 6:14 PM, Peter Eisentraut < peter.eisentr...@2ndquadrant.com> wrote: > > On 1/9/17 1:34 PM, Robert Haas wrote: > > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut > > 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. > Ok, but doing in that way the syntax would be: COMMENT ON DATABASE CURRENT_DATABASE IS 'comment'; Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog: http://fabriziomello.github.io >> Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello >> Github: http://github.com/fabriziomello
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Mon, Jan 9, 2017 at 3:14 PM, Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
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 > > > 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 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 + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On 1/9/17 1:34 PM, Robert Haas wrote: > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
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 > > 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Mon, Jan 9, 2017 at 01:34:03PM -0500, Robert Haas wrote: > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut > 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 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 + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Tue, Jan 3, 2017 at 10:09 PM, Robert Haas wrote: > On Tue, Jan 3, 2017 at 12:06 AM, Ashutosh Bapat > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Tue, Jan 3, 2017 at 9:18 PM, Peter Eisentraut 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.gc10...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Tue, Jan 3, 2017 at 6:40 PM, Fabrízio de Royes Mello wrote: > Hi Ashutosh, > > First of all thanks for your review. > > > On Tue, Jan 3, 2017 at 3:06 AM, Ashutosh Bapat > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
On Tue, Jan 3, 2017 at 12:06 AM, Ashutosh Bapat 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). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
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.gc10...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE
Hi Ashutosh, First of all thanks for your review. On Tue, Jan 3, 2017 at 3:06 AM, Ashutosh Bapat < ashutosh.ba...@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, ...). Should we need also add test cases for this kind of objects? > 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??? [1] https://www.postgresql.org/message-id/20150429170406.GI4369%40alvh.no-ip.org -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL >> Timbira: http://www.timbira.com.br >> Blog: http://fabriziomello.github.io >> Linkedin: http://br.linkedin.com/in/fabriziomello >> Twitter: http://twitter.com/fabriziomello >> Github: http://github.com/fabriziomello
Re: [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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Add support to COMMENT ON CURRENT DATABASE
Hi all,
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.gc10...@momjian.us
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml
index c1cf587..51b39ab 100644
--- a/doc/src/sgml/ref/comment.sgml
+++ b/doc/src/sgml/ref/comment.sgml
@@ -31,6 +31,7 @@ COMMENT ON
CONSTRAINT constraint_name ON table_name |
CONSTRAINT constraint_name ON DOMAIN domain_name |
CONVERSION object_name |
+ CURRENT DATABASE |
DATABASE object_name |
DOMAIN object_name |
EXTENSION object_name |
@@ -96,6 +97,11 @@ COMMENT ON
+ The CURRENT DATABASE means the comment will be applied to the database
+ where the command is executed.
+
+
+
Comments can be viewed using psql's
\d family of commands.
Other user interfaces to retrieve comments can be built atop
@@ -307,6 +313,7 @@ COMMENT ON COLUMN my_table.my_column IS 'Employee ID number';
COMMENT ON CONVERSION my_conv IS 'Conversion to UTF8';
COMMENT ON CONSTRAINT bar_col_cons ON bar IS 'Constrains column col';
COMMENT ON CONSTRAINT dom_col_constr ON DOMAIN dom IS 'Constrains col of domain';
+COMMENT ON CURRENT DATABASE IS 'Current Database Comment';
COMMENT ON DATABASE my_database IS 'Development Database';
COMMENT ON DOMAIN my_domain IS 'Email Address Domain';
COMMENT ON EXTENSION hstore IS 'implements the hstore data type';
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
@@ -1053,9 +1056,12 @@ get_object_address_unqualified(ObjectType objtype,
/*
* The types of names handled by this function are not permitted to be
- * schema-qualified or catalog-qualified.
+ * schema-qualified or catalog-qualified.
+ *
+ * If "CURRENT DATABASE" is used the target object name is NIL so we
+ * don't need to do this check.
*/
- if (list_length(qualname) != 1)
+ if (list_length(qualname) > 1)
{
const char *msg;
@@ -1101,7 +1107,10 @@ get_object_address_unqualified(ObjectType objtype,
}
/* Format is valid, extract the actual name. */
- name = strVal(linitial(qualname));
+ if (list_length(qualname) > 0)
+ name = strVal(linitial(qualname));
+ else
+ name = NULL;
/* Translate name to OID. */
switch (objtype)
@@ -1113,7 +1122,10 @@ get_object_address_unqualified(ObjectType objtype,
break;
case OBJECT_DATABASE:
address.classId = DatabaseRelationId;
- address.objectId = get_database_oid(name, missing_ok);
+ if (name != NULL)
+address.objectId = get_database_oid(name, missing_ok);
+ else
+address.objectId = MyDatabaseId;
address.objectSubId = 0;
break;
case OBJECT_EXTENSION:
@@ -1951,7 +1963,7 @@ pg_get_object_address(PG_FUNCTION_ARGS)
else
{
name = textarray_to_strvaluelist(namearr);
- if (list_length(name) < 1)
+ if (list_length(name) < 1 && type != OBJECT_DATABASE)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("name list length must be at least %d", 1)));
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 08cf5b7..b87aa5a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -6101,7 +6101,7 @@ opt_restart_seqs:
* the object associated with the comment. The form of the statement is:
*
* COMMENT ON [ [ ACCESS METHOD | CONVERSION | COLLATION |
- * DATABASE | DOMAIN |
+ * DATABASE | CURRENT DATABASE | DOMAIN |
* EXTENSION | EVENT TRIGGER | FOREIGN DATA WRAPPER |
* FOREIGN TABLE | INDEX | [PROCEDURAL] LANGUAGE |
* MATERIALIZED VIEW | POLICY | ROLE | SCHEMA | SEQUENCE |
@@ -6135,6 +6135,15 @@ CommentStmt:
n->comment = $6;
$$ = (Node *) n;
}
+ | COMMENT ON CURRENT_P DATABASE IS comment_text
+{
+ CommentStmt *n = makeNode(CommentStmt);
+ n->objtype = OBJECT_DATABASE;
+ n->objname = NIL;
+ n->objargs = NIL;
+ n->comment = $6;
+ $$ = (Node *) n;
+}
| COMMENT ON TYPE_P Typename IS comment_text
{
CommentStmt *n = makeNode(CommentStmt);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e5545b3..2e4746a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2679,10 +2679,20 @@ dumpDatabase(Archive *fout)
resetPQExpBuffer(dbQry)
