Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE - Mailing list pgsql-hackers

From Daniel Gustafsson
Subject Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE
Date
Msg-id E4F7198E-40C6-4A3C-8A11-10012A5B1C12@yesql.se
Whole thread Raw
In response to Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE  ("Bossart, Nathan" <bossartn@amazon.com>)
List pgsql-hackers
> 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

pgsql-hackers by date:

Previous
From: Pavel Stehule
Date:
Subject: Re: [HACKERS] proposal - Default namespaces for XPath expressions(PostgreSQL 11)
Next
From: Daniel Gustafsson
Date:
Subject: Re: [HACKERS] generated columns