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 <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
signature.asc
Description: PGP signature
