Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE
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
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
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
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
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
>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
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
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
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
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
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
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
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
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
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
