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

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

Hi Michael,

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

<https://commitfest.postgresql.org/14/&gt;

Thanks your mention. I will do that.

Regards,
Jing Wang
Fujitsu Australia

On Mon, Jun 5, 2017 at 4:09 AM, Jing Wang <jingwangian@gmail.com> 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

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

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

On Fri, Aug 25, 2017 at 11:16 AM, Jing Wang <jingwangian@gmail.com> 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

Hi Surafel,

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

Regards,
Jing Wang
Fujitsu Australia

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

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

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]https://travis-ci.org/postgresql-cfbot/postgresql/builds/275747367 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