Re: Support to COMMENT ON DATABASE CURRENT_DATABASE

On 10/5/17, 11:53 PM, "Jing Wang" <jingwangian(at)gmail(dot)com> wrote:
> The patch has been updated according to Nathan's comments. 
> Thanks Nathan's review.

Thanks for the new versions of the patches. I apologize for
the long delay for this new review.

It looks like the no-pgdump patch needs a rebase at this point.
I was able to apply the code portions with a 3-way merge, but
the documentation changes still did not apply. I didn't have
any problems applying the pgdump patch.

+ <listitem>
+ <para>
+ The name of a database or keyword current_database. When using
+ current_database,it means using the name of the connecting database.
+ </para>
+ </listitem>

For commands that accept the CURRENT_USER and SESSION_USER
keywords, the keywords are typically listed in the 'Synopsis'
section. I think CURRENT_DATABASE should be no different. For
example, the parameter type above could be
"database_specification," and the following definition could be
included at the bottom of the synopsis:

where database_specification can be:

object_name
| CURRENT_DATABASE

Then, in the parameters section, the CURRENT_DATABASE keyword
would be defined:

CURRENT_DATABASE

Comment on the current database instead of an
explicitly identified role.

Also, it looks like only the COMMENT and SECURITY LABEL
documentation is being updated in this patch set. However, this
keyword seems applicable to many other commands, too (e.g.
GRANT, ALTER DATABASE, ALTER ROLE, etc.).

+static ObjectAddress
+get_object_address_database(ObjectType objtype, DbSpec *object, bool missing_ok)
+{
+ char *dbname;
+ ObjectAddress address;
+
+ dbname = get_dbspec_name(object);
+
+ address.classId = DatabaseRelationId;
+ address.objectId = get_database_oid(dbname, missing_ok);
+ address.objectSubId = 0;
+
+ return address;
+}

This helper function is only used once, and it seems simple
enough to build the ObjectAddress in the switch statement.
Also, instead of get_database_oid(), could we use
get_dbspec_oid()?

if (stmt->objtype == OBJECT_DATABASE)
{
- char *database = strVal((Value *) stmt->object);
-
- if (!OidIsValid(get_database_oid(database, true)))
+ if (!OidIsValid(get_dbspec_oid((DbSpec*)stmt->object, true)))
{
+ char *dbname = NULL;
+
+ dbname = get_dbspec_name((DbSpec*)stmt->object);
+
ereport(WARNING,
(errcode(ERRCODE_UNDEFINED_DATABASE),
- errmsg("database \"%s\" does not exist", database)));
+ errmsg("database \"%s\" does not exist", dbname)));
return address;
}
}

This section seems to assume that the DbSpec will be of type
DBSPEC_CSTRING in the error handling. That should be safe for
now, as you cannot drop the current database, but I would
suggest adding assertions here to be sure.

+ dbname = get_dbspec_name((DbSpec*)stmt->dbspec);

As a general note, casts are typically styled as "(DbSpec *)
stmt" (with the spaces) in PostgreSQL.

+ case DBSPEC_CURRENT_DATABASE:
+ {
+ HeapTuple tuple;
+ Form_pg_database dbForm;

Can we just declare "tuple" and "dbForm" at the beginning of
get_dbspec_name() so we don't need the extra set of braces?

+ if (fout->remoteVersion >= 100000)
+ appendPQExpBuffer(dbQry, "COMMENT ON DATABASE CURRENT_DATABASE IS ");
+ else
+ appendPQExpBuffer(dbQry, "COMMENT ON DATABASE %s IS ", fmtId(datname));

This feature would probably only be added to v11, so the version
checks in the pgdump patch will need to be updated.

Nathan