Thread: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

On Mon, Jun 5, 2017 at 10:09 AM, Jing Wang <jingwangian@gmail.com> wrote:
> By using the patch the CURRENT_DATABASE as a keyword can be used in the
> following SQL commands:
>
>     1. COMMENT ON DATABASE CURRENT_DATABASE is ...
>     2. ALTER DATABASE CURRENT_DATABASE OWNER to ...
>     3. ALTER DATABASE CURRENT_DATABASE SET parameter ...
>     4. ALTER DATABASE CURRENT_DATABASE RESET parameter ...
>     5. SELECT CURRENT_DATABASE

You should add that to the next commit fest:
https://commitfest.postgresql.org/14/
Note that it may take a couple of months until you get some review, as
the focus now is to stabilize Postgres 10.
-- 
Michael

On Tue, Sep 12, 2017 at 1:11 PM, Jing Wang <jingwangian@gmail.com> wrote:
> Please find the rebased patch based on latest version in the attached file.

Hi Jing

It looks like you created dbname.sql and dbname.out files for a
regression test but forgot to "git add" them to your branch before you
created the patch, so "make check" fails with the patch applied:

test identity                 ... ok
test event_trigger            ... ok
test stats                    ... ok
test dbname                   ... /bin/sh: 1: cannot open
/home/travis/build/postgresql-cfbot/postgresql/src/test/regress/sql/dbname.sql:
No such file

+ printf("target = %s\n",target);

Looks like a stray debugging message?

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

A few general comments.

While this patch applies, I am still seeing some whitespace errors:
 comment_on_current_database_no_pgdump_v4.1.patch:488: trailing whitespace.             ColId
comment_on_current_database_no_pgdump_v4.1.patch:490:trailing whitespace.             | CURRENT_DATABASE
comment_on_current_database_no_pgdump_v4.1.patch:491:trailing whitespace.                 {
comment_on_current_database_no_pgdump_v4.1.patch:501:trailing whitespace.             ColId
comment_on_current_database_no_pgdump_v4.1.patch:502:trailing whitespace.                 { warning: squelched 9
whitespaceerrors warning: 14 lines add whitespace errors.
 

It looks like the 'dbname' test is currently failing when you run
'make check-world'.  The Travis CI build log [1] shows the details
of the failure.  From a brief glance, I would guess that some of
the queries are returning unexpected databases that are created in
other tests.

Also, I think this change will require updates to the
documentation.

+    if (dbspecname->dbnametype == DBSPEC_CURRENT_DATABASE )
+        dbname = get_database_name(MyDatabaseId);
+    else
+        dbname = dbspecname->dbname;

This pattern is repeated in the patch several times.  It looks like
the end goal of these code blocks is either to get the database
name or the database OID, so perhaps we should have
get_dbspec_name() and get_dbspec_oid() helper functions (like
get_rolespec_oid() for RoleSpec nodes).

+static bool
+_equalDatabaseSpec(const DBSpecName *a, const DBSpecName *b)
+{
+    COMPARE_SCALAR_FIELD(dbnametype);
+    COMPARE_STRING_FIELD(dbname);
+    COMPARE_LOCATION_FIELD(location);
+
+    return true;
+}

There are some inconsistencies in the naming strategy.  For
example, this function is called _equalDatabaseSpec(), but the
struct is DBSpecName.  I would suggest calling it DatabaseSpec or
DbSpec throughout the patch.

Nathan

[1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/275747367


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

> On 15 Sep 2017, at 16:36, Bossart, Nathan <bossartn@amazon.com> wrote:
>
> A few general comments.
>
> While this patch applies, I am still seeing some whitespace errors:
>
>  comment_on_current_database_no_pgdump_v4.1.patch:488: trailing whitespace.
>              ColId
>  comment_on_current_database_no_pgdump_v4.1.patch:490: trailing whitespace.
>              | CURRENT_DATABASE
>  comment_on_current_database_no_pgdump_v4.1.patch:491: trailing whitespace.
>                  {
>  comment_on_current_database_no_pgdump_v4.1.patch:501: trailing whitespace.
>              ColId
>  comment_on_current_database_no_pgdump_v4.1.patch:502: trailing whitespace.
>                  {
>  warning: squelched 9 whitespace errors
>  warning: 14 lines add whitespace errors.
>
> It looks like the 'dbname' test is currently failing when you run
> 'make check-world'.  The Travis CI build log [1] shows the details
> of the failure.  From a brief glance, I would guess that some of
> the queries are returning unexpected databases that are created in
> other tests.
>
> Also, I think this change will require updates to the
> documentation.
>
> +    if (dbspecname->dbnametype == DBSPEC_CURRENT_DATABASE )
> +        dbname = get_database_name(MyDatabaseId);
> +    else
> +        dbname = dbspecname->dbname;
>
> This pattern is repeated in the patch several times.  It looks like
> the end goal of these code blocks is either to get the database
> name or the database OID, so perhaps we should have
> get_dbspec_name() and get_dbspec_oid() helper functions (like
> get_rolespec_oid() for RoleSpec nodes).
>
> +static bool
> +_equalDatabaseSpec(const DBSpecName *a, const DBSpecName *b)
> +{
> +    COMPARE_SCALAR_FIELD(dbnametype);
> +    COMPARE_STRING_FIELD(dbname);
> +    COMPARE_LOCATION_FIELD(location);
> +
> +    return true;
> +}
>
> There are some inconsistencies in the naming strategy.  For
> example, this function is called _equalDatabaseSpec(), but the
> struct is DBSpecName.  I would suggest calling it DatabaseSpec or
> DbSpec throughout the patch.

Based on this review, and that there hasn’t been a new version submitted, I’m
marking this patch Returned with Feedback.  Please re-submit a new version of
the patch to an upcoming commitfest when ready.

cheers ./daniel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/5/17, 11:53 PM, "Jing Wang" <jingwangian@gmail.com> wrote:
> The patch has been updated according to Nathan's comments. 
> Thanks Nathan's review.

Thanks for the new versions of the patches.  I apologize for
the long delay for this new review.

It looks like the no-pgdump patch needs a rebase at this point.
I was able to apply the code portions with a 3-way merge, but
the documentation changes still did not apply.  I didn't have
any problems applying the pgdump patch.

+    <listitem>
+     <para>
+      The name of a database or keyword current_database. When using 
+      current_database,it means using the name of the connecting database.
+     </para>
+    </listitem>

For commands that accept the CURRENT_USER and SESSION_USER
keywords, the keywords are typically listed in the 'Synopsis'
section.  I think CURRENT_DATABASE should be no different.  For
example, the parameter type above could be
"database_specification," and the following definition could be
included at the bottom of the synopsis:
       where database_specification can be:
           object_name         | CURRENT_DATABASE

Then, in the parameters section, the CURRENT_DATABASE keyword
would be defined:
       CURRENT_DATABASE
           Comment on the current database instead of an           explicitly identified role.

Also, it looks like only the COMMENT and SECURITY LABEL
documentation is being updated in this patch set.  However, this
keyword seems applicable to many other commands, too (e.g.
GRANT, ALTER DATABASE, ALTER ROLE, etc.).

+static ObjectAddress
+get_object_address_database(ObjectType objtype, DbSpec *object, bool missing_ok)
+{
+    char            *dbname;
+    ObjectAddress     address;
+
+    dbname = get_dbspec_name(object);
+
+    address.classId = DatabaseRelationId;
+    address.objectId = get_database_oid(dbname, missing_ok);
+    address.objectSubId = 0;
+
+    return address;
+}

This helper function is only used once, and it seems simple
enough to build the ObjectAddress in the switch statement.
Also, instead of get_database_oid(), could we use
get_dbspec_oid()?
    if (stmt->objtype == OBJECT_DATABASE)    {
-        char       *database = strVal((Value *) stmt->object);
-
-        if (!OidIsValid(get_database_oid(database, true)))
+        if (!OidIsValid(get_dbspec_oid((DbSpec*)stmt->object, true)))        {
+            char        *dbname = NULL;
+
+            dbname = get_dbspec_name((DbSpec*)stmt->object);
+            ereport(WARNING,                    (errcode(ERRCODE_UNDEFINED_DATABASE),
-                     errmsg("database \"%s\" does not exist", database)));
+                     errmsg("database \"%s\" does not exist", dbname)));            return address;        }    }

This section seems to assume that the DbSpec will be of type
DBSPEC_CSTRING in the error handling.  That should be safe for
now, as you cannot drop the current database, but I would
suggest adding assertions here to be sure.

+    dbname = get_dbspec_name((DbSpec*)stmt->dbspec);

As a general note, casts are typically styled as "(DbSpec *)
stmt" (with the spaces) in PostgreSQL.

+        case DBSPEC_CURRENT_DATABASE:
+        {
+            HeapTuple    tuple;
+            Form_pg_database dbForm;

Can we just declare "tuple" and "dbForm" at the beginning of
get_dbspec_name() so we don't need the extra set of braces?

+            if (fout->remoteVersion >= 100000)
+                appendPQExpBuffer(dbQry, "COMMENT ON DATABASE CURRENT_DATABASE IS ");
+            else
+                appendPQExpBuffer(dbQry, "COMMENT ON DATABASE %s IS ", fmtId(datname));

This feature would probably only be added to v11, so the version
checks in the pgdump patch will need to be updated.

Nathan


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Nov 27, 2017 at 11:41 AM, Jing Wang <jingwangian@gmail.com> wrote:
> Hi All,
>
> This is a patch for current_database working on ALTER ROLE/GRANT/REVOKE
> statements which should be applied after the previous patch
> "comment_on_current_database_no_pgdump_v4.4.patch".
>
> By using the patch the CURRENT_DATABASE can working in the following SQL
> statements:
>
> ALTER ROLE ... IN DATABASE CURRENT_DATABASE SET/RESET
> configuration_parameter
> GRANT ... ON DATABASE CURRENT_DATABASE TO role_specification ...
> REVOKE ... ON DATABASE CURRENT_DATABASE FROM ...

Moved to next CF with same status, "needs review".
-- 
Michael

On Wed, Nov 29, 2017 at 11:35 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Nov 27, 2017 at 11:41 AM, Jing Wang <jingwangian@gmail.com> wrote:
>> This is a patch for current_database working on ALTER ROLE/GRANT/REVOKE
>> statements which should be applied after the previous patch
>> "comment_on_current_database_no_pgdump_v4.4.patch".
>>
>> By using the patch the CURRENT_DATABASE can working in the following SQL
>> statements:
>>
>> ALTER ROLE ... IN DATABASE CURRENT_DATABASE SET/RESET
>> configuration_parameter
>> GRANT ... ON DATABASE CURRENT_DATABASE TO role_specification ...
>> REVOKE ... ON DATABASE CURRENT_DATABASE FROM ...
>
> Moved to next CF with same status, "needs review".

Patch no longer applies.

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

On Mon, Dec 4, 2017 at 1:34 PM, Jing Wang <jingwangian@gmail.com> wrote:
> I have rebased the patch on the latest version.

Hi Jing,

According to my testing robot this fails make check-world (or
presumably cd src/bin/pg_dump ; make check), here:

t/001_basic.pl ......... ok
#   Failed test 'binary_upgrade: dumps COMMENT ON DATABASE postgres'
#   at t/002_pg_dump.pl line 6753.
... masses of dump output omitted ...
# Looks like you failed 19 tests of 4727.
t/002_pg_dump.pl .......
Dubious, test returned 19 (wstat 4864, 0x1300)
Failed 19/4727 subtests
t/010_dump_connstr.pl .. ok

Test Summary Report
-------------------
t/002_pg_dump.pl     (Wstat: 4864 Tests: 4727 Failed: 19)
  Failed tests:  63, 224, 412, 702, 992, 1161, 1330, 1503
                1672, 1840, 2008, 2176, 2343, 2495, 2663
                3150, 3901, 4282, 4610
  Non-zero exit status: 19

There is also a warning from my compiler here:

gram.y:1160:19: error: incompatible pointer types assigning to 'char
*' from 'Node *' (aka 'struct Node *')
[-Werror,-Wincompatible-pointer-types]
    { (yyval.str) = (yyvsp[0].node); }
                  ^ ~~~~~~~~~~~~~~~

-- 
Thomas Munro
http://www.enterprisedb.com

Greetings Jing,

* Jing Wang (jingwangian@gmail.com) wrote:
> I have rebased the patch on the latest version.

Thanks!  Looks like there's still more work to be done here, and
unfortunately this ended up on a new thread somehow from the prior one.
I've added this newer thread to the CF app too.

> Because the CURRENT_DATABASE can not only being used on COMMENT ON
> statement but also on other statements as following list so the patch name
> is renamed to "support_CURRENT_DATABASE_keyword_vxx.patch".

Makes sense.

Unfortunately, this still is throwing a warning when building:

/src/backend/parser/gram.y: In function ‘base_yyparse’:
/src/backend/parser/gram.y:1160:19: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
    | IN_P DATABASE db_spec_name { $$ = $3; }
                   ^
Also, the documentation changes aren't quite right, you're doing:

ALTER DATABASE <replaceable class="parameter">{name | CURRENT_DATABASE}</replaceable> OWNER TO {
<replaceable>new_owner</replaceable>| CURRENT_USER | SESSION_USER } 

When it should be:

ALTER DATABASE { <replaceable class="parameter">name</replaceable> | CURRENT_DATABASE } OWNER TO {
<replaceable>new_owner</replaceable>| CURRENT_USER | SESSION_USER } 

Note that the "replaceable class" tag goes directly around 'name', and
doesn't include CURRENT_DATABASE.  Also, keep a space before 'name' and
after 'CURRENT_DATABASE', just like how the "OWNER TO ..." piece works.

Please don't include whitespace-only hunks, like this one:

*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
*************** const ObjectAddress InvalidObjectAddress
*** 724,730 ****
    InvalidOid,
    0
  };
-
  static ObjectAddress get_object_address_unqualified(ObjectType objtype,
                               Value *strval, bool missing_ok);
  static ObjectAddress get_relation_by_qualified_name(ObjectType objtype,

The TAP regression tests for pg_dump are failing.  It looks like you've
changed pg_dump to use CURRENT_DATABASE, which is fine, but please
adjust the regexp's used in the pg_dump TAP tests to handle that.  The
regexp you're looking to change is in src/bin/pg_dump/t/002_pg_dump.pl,
around line 1515 in current head (look for the stanza:

    'COMMENT ON DATABASE postgres' => {

and the regexp:

        regexp    => qr/^COMMENT ON DATABASE postgres IS .*;/m,

That looks to be the only thing that needs to be changed to make this
test pass.

In gram.y, I would have thought to make a db_spec_name a 'dbspec' type,
similar to what was done with 'rolespec' (though, I note, the efforts
around RoleSpec seem to have stalled since COMMENT ON ROLE CURRENT_ROLE
doesn't work and get_object_address seems to still think that the parser
works with roles as strings, when only half of it actually does..) and
then make makeDbSpec() return a DbSpec and then try to minimize the
forced-casting happening.  On a similar vein, I would think that the
various 'dbspec' pointers in AlterRoleSetStmt and others should be of
the DbSpec type instead of just being Node*'s.

Ideally, try to make sure that the changes you're making are pgindent'd
properly.

There's probably more to do on this, but hopefully this gets the patch
into a bit of a cleaner state for further review.  Setting to Waiting on
Author.

Thanks!

Stephen

Jing Wang <jingwangian@gmail.com> writes:
> [ support_CURRENT_DATABASE_keyword_v4.6.patch ]

Not surprisingly, this patch no longer applies in the wake of commit
b3f840120.  Rather than rebasing the pg_dump portions, I would suggest
you just drop them.  It is no longer necessary for pg_dump to worry about
this, because it won't emit COMMENT ON DATABASE or SECURITY LABEL FOR
DATABASE except in cases where it just created the target database
and so knows the DB name for sure.  So, using COMMENT ON DATABASE
CURRENT_DATABASE would just create an unnecessary version dependency
in the output script.  (BTW, using the source database's version to decide
what to emit is not per pg_dump policy.  Also, if you did still want to
modify pg_dump to use CURRENT_DATABASE, you'd have to extend the patch
to handle ACLs and some other ALTER DATABASE commands.)

I'm not really sure how much of the use-case for this feature rested
on the pg_dump issue.  It may still be worthwhile for other use cases,
or maybe we should just drop it.

I notice some other patch application failures in dbcommands.c,
objectaddress.c, and user.c, so there's more work to do besides
this ...

            regards, tom lane

Jing Wang <jingwangian@gmail.com> writes:
> [ support_CURRENT_DATABASE_keyword_v4.7.patch ]

TBH, I think we should reject this patch.  While it's not huge,
it's not trivial either, and I find the grammar changes rather ugly.
The argument for using the feature to fix pg_dump issues has evaporated,
but I don't see anything in the discussion suggesting that people see
a need for it beyond that.

I particularly object to inventing a CURRENT_DATABASE parameterless
function.  That's encroaching on user namespace to no purpose whatever,
as we already have a perfectly good regular function for that.

Also, from a user standpoint, turning CURRENT_DATABASE into a fully
reserved word seems like a bad idea.  If nothing else, that breaks
queries that are relying on the existing current_database() function.
The parallel to CURRENT_ROLE is not very good, because there at least
we can point to the SQL spec and say it's reserved according to the
standard.  CURRENT_DATABASE has no such excuse.

            regards, tom lane

Hi Jing,

On 3/1/18 2:09 PM, Tom Lane wrote:
> Jing Wang <jingwangian@gmail.com> writes:
>> [ support_CURRENT_DATABASE_keyword_v4.7.patch ]
> 
> TBH, I think we should reject this patch.  While it's not huge,
> it's not trivial either, and I find the grammar changes rather ugly.
> The argument for using the feature to fix pg_dump issues has evaporated,
> but I don't see anything in the discussion suggesting that people see
> a need for it beyond that.
> 
> I particularly object to inventing a CURRENT_DATABASE parameterless
> function.  That's encroaching on user namespace to no purpose whatever,
> as we already have a perfectly good regular function for that.
> 
> Also, from a user standpoint, turning CURRENT_DATABASE into a fully
> reserved word seems like a bad idea.  If nothing else, that breaks
> queries that are relying on the existing current_database() function.
> The parallel to CURRENT_ROLE is not very good, because there at least
> we can point to the SQL spec and say it's reserved according to the
> standard.  CURRENT_DATABASE has no such excuse.

Based on Tom's feedback, and hearing no opinions to the contrary, I have
marked this patch Rejected.

Regards,
-- 
-David
david@pgmasters.net

David Steele wrote:

> On 3/1/18 2:09 PM, Tom Lane wrote:
>
> > TBH, I think we should reject this patch.  While it's not huge,
> > it's not trivial either, and I find the grammar changes rather ugly.
> > The argument for using the feature to fix pg_dump issues has evaporated,
> > but I don't see anything in the discussion suggesting that people see
> > a need for it beyond that.

> Based on Tom's feedback, and hearing no opinions to the contrary, I have
> marked this patch Rejected.

I think I opine contrarywise, but I haven't made time to review the
status of this in detail.  I'm fine with keeping it rejected for now,
but I reserve the option to revive it in the future.

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

Hi Álvaro,

On 3/6/18 10:25 AM, Alvaro Herrera wrote:
> David Steele wrote:
> 
>> On 3/1/18 2:09 PM, Tom Lane wrote:
>>
>>> TBH, I think we should reject this patch.  While it's not huge,
>>> it's not trivial either, and I find the grammar changes rather ugly.
>>> The argument for using the feature to fix pg_dump issues has evaporated,
>>> but I don't see anything in the discussion suggesting that people see
>>> a need for it beyond that.
> 
>> Based on Tom's feedback, and hearing no opinions to the contrary, I have
>> marked this patch Rejected.
> 
> I think I opine contrarywise, but I haven't made time to review the
> status of this in detail.  I'm fine with keeping it rejected for now,
> but I reserve the option to revive it in the future.

Absolutely.

From my perspective reviving a patch is pretty much always an option.
I'm attempting to update patches based on what I see as the current
status, but my decision is certainly not final and I do make mistakes.

Regards,
-- 
-David
david@pgmasters.net

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> David Steele wrote:
>> Based on Tom's feedback, and hearing no opinions to the contrary, I have
>> marked this patch Rejected.

> I think I opine contrarywise, but I haven't made time to review the
> status of this in detail.  I'm fine with keeping it rejected for now,
> but I reserve the option to revive it in the future.

I'm fine with reviving it if someone can find a way around the new-
reserved-word problem.  But that's gonna be a bit hard given that
the patch wants to do 

database_name:
            ColId
            | CURRENT_DATABASE

You might be able to preserve the accessibility of the current_database()
function by making CURRENT_DATABASE into a type_func_name_keyword instead
of a fully-reserved word.  But that's ugly (I think you'd need a single-
purpose production for it to be used as a function), and you've still
broken any SQL code using "current_database" as a table or column name.
I'm dubious that the remaining use-case for the feature is worth it.

            regards, tom lane