Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Bossart, Nathan]

On 10/5/17, 11:53 PM, "Jing Wang"  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.

+
+ 
+  The name of a database or keyword current_database. When using 
+  current_database,it means using the name of the connecting database.
+ 
+

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 >= 10)
+   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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi

I don't know why the previous email can't be linked with the original email
webpage. It is weird. So supplementing following information for
understanding:

The original email link:
https://www.postgresql.org/message-id/CAF3%2BxM%2BxSswcWQZMP1cjj12gPz8DXHcM9_fT1y-0fVzxi9pmOw%40mail.gmail.com

The recent email with updated patch is as following:
https://www.postgresql.org/message-id/CAF3%2BxMKkKpd8oVRpN9i1BFMau2dFhVrt-Y0BE%3DDsoKtOm%3DC2AQ%40mail.gmail.com

-- 
Regards,
Jing Wang
Fujitsu Australia

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi all,

The patch has been updated according to Nathan's comments.
Thanks Nathan's review.

Please find the updated patch in the attached files:
comment_on_current_database_no_pgdump_v4.3.patch --- support
current_database keyword exclude the pg_dump part.
comment_on_current_database_for_pgdump_v4.3.patch --- only for the pg_dump
part changed based on the previous patch

Regards,
Jing Wang
Fujitsu Australia


comment_on_current_database_no_pgdump_v4.3.patch
Description: Binary data


comment_on_current_database_for_pgdump_v4.3.patch
Description: Binary data

-- 
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] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Daniel Gustafsson]

> On 15 Sep 2017, at 16:36, Bossart, Nathan  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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Bossart, Nathan]

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.

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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi Surafel,

Sorry for that.

Yes. The test case file is forgotten to be added into the previous patch.

Kindly please use the updated patch in the attached file.


Regards,
Jing Wang
Fujitsu Australia


comment_on_current_database_no_pgdump_v4.1.patch
Description: Binary data

-- 
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] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Thomas Munro]

On Tue, Sep 12, 2017 at 1:11 PM, Jing Wang  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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi Surafel,

Please find the rebased patch based on latest version in the attached file.

Regards,
Jing Wang
Fujitsu Australia


comment_on_current_database_for_pgdump_v4.patch
Description: Binary data


comment_on_current_database_no_pgdump_v4.patch
Description: Binary data

-- 
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] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Surafel Temesgen]

On Fri, Aug 25, 2017 at 11:16 AM, Jing Wang  wrote:

> Hi all,
>
> Enclosed please find the updated patch with covering security labels on
> database.
>
> The patch cover the following commands:
>

i can't apply your patch cleanly i think it needs rebase

Regards

Surafel

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Surafel Temesgen]

i can't apply your patch cleanly i think it needs rebase

Regards

Surafel


On Thu, Aug 31, 2017 at 1:38 PM, Jing Wang  wrote:

> Hi All,
>
> Enclosed please find the patch only for the pg_dump using the 'comment on
> current_database' statement.
>
> This patch should be working with the previous patch which is
> comment_on_current_database_no_pgdump_v3.patch
>
> Regards,
> Jing Wang
> Fujitsu Australia
>

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi All,

Enclosed please find the patch only for the pg_dump using the 'comment on
current_database' statement.

This patch should be working with the previous patch which is
comment_on_current_database_no_pgdump_v3.patch

Regards,
Jing Wang
Fujitsu Australia


comment_on_current_database_for_pgdump_v3.patch
Description: Binary data

-- 
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] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi all,

Enclosed please find the updated patch with covering security labels on
database.

The patch cover the following 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
6. SECURITY LABEL ON DATABASE CURRENT_DATABASE is ...

The patch doesn't cover the pg_dump part which will be included in another
patch later.

Regards,
Jing Wang
Fujitsu Australia


comment_on_current_database_no_pgdump_v3.patch
Description: Binary data

-- 
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] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi Surafel,

>Your patch doesn't cover security labels on databases which have similar
issue
>and consider dividing the patch into two one for adding CURRENT_DATABASE
as a
>database specifier and the other for adding database-level information to
pg_dump output
>in a way that allows to load a dump into a differently named database.

Thanks your review and suggestion. I will add them.


Regards,
Jing Wang
Fujitsu Australia

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Surafel Temesgen]

On Mon, Jun 5, 2017 at 4:09 AM, Jing Wang  wrote:

> Hi all,
>
> The attached patch is to support the feature "COMMENT ON DATABASE
> CURRENT_DATABASE". The solution is based on the previous discussion in [2] .
>

Your patch doesn't cover security labels on databases which have similar
issue
and consider dividing the patch into two one for adding CURRENT_DATABASE as
a
database specifier and the other for adding database-level information to
pg_dump output
in a way that allows to load a dump into a differently named database

Regards,

Surafel

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi Michael,

>You should add that to the next commit fest:
>https://commitfest.postgresql.org/14/


Thanks your mention. I will do that.

Regards,
Jing Wang
Fujitsu Australia

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Michael Paquier]

On Mon, Jun 5, 2017 at 10:09 AM, Jing Wang  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


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