Re: Support to COMMENT ON DATABASE CURRENT_DATABASE

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Jing Wang <jingwangian(at)gmail(dot)com>
Cc: "Bossart, Nathan" <bossartn(at)amazon(dot)com>, Surafel Temesgen <surafel3000(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Support to COMMENT ON DATABASE CURRENT_DATABASE
Date: 2017-10-02 10:49:58
Message-ID: E4F7198E-40C6-4A3C-8A11-10012A5B1C12@yesql.se
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

> On 15 Sep 2017, at 16:36, Bossart, Nathan <bossartn(at)amazon(dot)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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2017-10-02 11:51:55 Re: generated columns
Previous Message Pavel Stehule 2017-10-02 10:43:19 Re: proposal - Default namespaces for XPath expressions (PostgreSQL 11)