A few general comments.
While this patch applies, I am still seeing some whitespace errors:
comment_on_current_database_no_pgdump_v4.1.patch:488: trailing whitespace. ColId
comment_on_current_database_no_pgdump_v4.1.patch:490:trailing whitespace. | CURRENT_DATABASE
comment_on_current_database_no_pgdump_v4.1.patch:491:trailing whitespace. {
comment_on_current_database_no_pgdump_v4.1.patch:501:trailing whitespace. ColId
comment_on_current_database_no_pgdump_v4.1.patch:502:trailing whitespace. { warning: squelched 9
whitespaceerrors warning: 14 lines add whitespace errors.
It looks like the 'dbname' test is currently failing when you run
'make check-world'. The Travis CI build log [1] shows the details
of the failure. From a brief glance, I would guess that some of
the queries are returning unexpected databases that are created in
other tests.
Also, I think this change will require updates to the
documentation.
+ if (dbspecname->dbnametype == DBSPEC_CURRENT_DATABASE )
+ dbname = get_database_name(MyDatabaseId);
+ else
+ dbname = dbspecname->dbname;
This pattern is repeated in the patch several times. It looks like
the end goal of these code blocks is either to get the database
name or the database OID, so perhaps we should have
get_dbspec_name() and get_dbspec_oid() helper functions (like
get_rolespec_oid() for RoleSpec nodes).
+static bool
+_equalDatabaseSpec(const DBSpecName *a, const DBSpecName *b)
+{
+ COMPARE_SCALAR_FIELD(dbnametype);
+ COMPARE_STRING_FIELD(dbname);
+ COMPARE_LOCATION_FIELD(location);
+
+ return true;
+}
There are some inconsistencies in the naming strategy. For
example, this function is called _equalDatabaseSpec(), but the
struct is DBSpecName. I would suggest calling it DatabaseSpec or
DbSpec throughout the patch.
Nathan
[1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/275747367
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers