> On 15 Sep 2017, at 16:36, Bossart, Nathan <bossartn@amazon.com> wrote:
>
> 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 whitespace errors
> 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.
Based on this review, and that there hasn’t been a new version submitted, I’m
marking this patch Returned with Feedback. Please re-submit a new version of
the patch to an upcoming commitfest when ready.
cheers ./daniel
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
