Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Tom Lane]

Alvaro Herrera  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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[David Steele]

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
da...@pgmasters.net

Re: Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[David Steele]

Hi Jing,

On 3/1/18 2:09 PM, Tom Lane wrote:
> Jing Wang  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
da...@pgmasters.net

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Tom Lane]

Jing Wang  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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

>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  has been removed from the pg_dump codes.

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

Enclosed please find the latest patch fixed above.


Regards,
Jing Wang
Fujitsu Australia


support_CURRENT_DATABASE_keyword_v4.7.patch
Description: Binary data

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Tom Lane]

Jing Wang  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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi Stephen and Thomas,

Thanks your review comments.
Enclosed please find the latest patch.

>/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; }
The warning has been dismissed.

>When it should be:
>ALTER DATABASE { name |
CURRENT_DATABASE } OWNER TO { new_owner |
CURRENT_USER | SESSION_USER }
Yes. It should be.

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

>The TAP regression tests for pg_dump are failing.
The test case has been updated.

>make makeDbSpec() return a DbSpec and then try to minimize the
>forced-casting happening.
Makes sense. It has been changed.


Regards,
Jing Wang
Fujitsu Australia


support_CURRENT_DATABASE_keyword_v4.6.patch
Description: Binary data

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Stephen Frost]

Greetings Jing,

* Jing Wang (jingwang...@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 {name | 
CURRENT_DATABASE} OWNER TO { new_owner 
| CURRENT_USER | SESSION_USER }

When it should be:

ALTER DATABASE { name | 
CURRENT_DATABASE } OWNER TO { new_owner | 
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


signature.asc
Description: PGP signature

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Thomas Munro]

On Mon, Dec 4, 2017 at 1:34 PM, Jing Wang  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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi,

I have rebased the patch on the latest version.

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".


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. ALTER DATABASE CURRENT_DATABASE RESET ALL
6. SELECT CURRENT_DATABASE
7. SECURITY LABEL ON DATABASE CURRENT_DATABASE
8. ALTER ROLE ... IN DATABASE CURRENT_DATABASE SET/RESET
configuration_parameter
9. GRANT ... ON DATABASE CURRENT_DATABASE TO role_specification ...
10. REVOKE ... ON DATABASE CURRENT_DATABASE FROM ...


Regards,
Jing Wang
Fujitsu Australia


support_CURRENT_DATABASE_keyword_v4.5.patch
Description: Binary data

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Robert Haas]

On Wed, Nov 29, 2017 at 11:35 PM, Michael Paquier
 wrote:
> On Mon, Nov 27, 2017 at 11:41 AM, Jing Wang  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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Michael Paquier]

On Mon, Nov 27, 2017 at 11:41 AM, Jing Wang  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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

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 ...


Regards,
Jing Wang
Fujitsu Australia


current_database_on_grant_revoke_role_v4.4.patch
Description: Binary data

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi Nathan,

Thanks for review comments.

Enclosed please find the patch which has been updated according to your
suggestion.

The CURRENT_DATABASE can be used as following SQL statements and people can
find information from sgml files:
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. ALTER DATABASE CURRENT_DATABASE RESET ALL
6. SELECT CURRENT_DATABASE
7. SECURITY LABEL ON DATABASE CURRENT_DATABASE

As your mentioned the database_name are also present in the
GRANT/REVOKE/ALTER ROLE, so a patch will be present later for supporting
CURRENT_DATABASE on these SQL statements.

Regards,
Jing Wang
Fujitsu Australia


comment_on_current_database_no_pgdump_v4.4.patch
Description: Binary data


comment_on_current_database_for_pgdump_v4.4.patch
Description: Binary data