Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

Greetings Jing,

* Jing Wang (jingwangian(at)gmail(dot)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