Subject: Re: [HACKERS] proposal: schema variables
To: pgsql-hackers@lists.postgresql.org,
 Pavel Stehule <pavel.stehule@gmail.com>
References: 
 <CAFj8pRDY+m9OOxfO10R7J0PAkCCauM-TweaTrdsrsLGMb1VbEQ@mail.gmail.com>
 <623395617.20171113141500@gf.microolap.com>
 <CAFj8pRDdS7ViLBJ6eA93u=C_x15EBv2deiNQDGkBS=LNrjzLLw@mail.gmail.com>
 <CAFj8pRBfb-GTZSHSRVTpMzGr26-7e-_RmOmRpmuk+xuDTgC=mA@mail.gmail.com>
From: Gilles Darold <gilles.darold@dalibo.com>
Message-ID: <28924bcc-9242-9798-e4e8-9d83cea3fef6@dalibo.com>
Date: Wed, 27 Jun 2018 12:21:16 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: 
 <CAFj8pRBfb-GTZSHSRVTpMzGr26-7e-_RmOmRpmuk+xuDTgC=mA@mail.gmail.com>
Content-Type: multipart/alternative;
 boundary="------------C15DAB3ECEAED6192133E1D8"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------C15DAB3ECEAED6192133E1D8
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi,

I'm reviewing the patch as it was flagged in the current commit fest. 
Here are my feedback:

  - The patch need to be rebased due to changes in file 
src/sgml/catalogs.sgml

  - Some compilation warning must be fixed:

    analyze.c: In function ‘transformLetStmt’:
    analyze.c:1568:17: warning: variable ‘rte’ set but not used
    [-Wunused-but-set-variable]
       RangeTblEntry *rte;
                      ^~~
    tab-complete.c:1268:21: warning: initialization from incompatible
    pointer type [-Wincompatible-pointer-types]
       {"VARIABLE", NULL, &Query_for_list_of_variables},

    In the last warning a NULL is missing, should be written:
    {"VARIABLE", NULL, NULL, &Query_for_list_of_variables},


  - How about Peter's suggestion?:
     "In DB2, the privileges for variables are named READ and WRITE. 
That would make more sense to me than reusing the privilege names for 
tables.
     The patch use SELECT and UPDATE which make sense too for SELECT but 
less for UPDATE.

  - The implementation of "ALTER VARIABLE varname SET SCHEMA 
schema_name;" is missing

  - ALTER VARIABLE var1 OWNER TO gilles; ok but not documented and 
missing in regression test

  - ALTER VARIABLE var1 RENAME TO var2; ok but not documented and 
missing in regression test

More generally I think that some comments must be rewritten, especially 
those talking about a PoC. In documentation there is HTML comments that 
can be removed.

Comment at end of file src/backend/commands/schemavar.c generate some 
"indent with spaces" errors with git apply but perhaps the comment can 
be entirely removed or undocumented details moved to the right place.

Otherwise all regression tests passed without issue and especially your 
new regression tests about schema variables.

I have a patch rebased, let me known if you want me to post the new diff.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org


--------------C15DAB3ECEAED6192133E1D8
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">Hi,<br>
      <br>
      I'm reviewing the patch as it was flagged in the current commit
      fest. Here are my feedback:<br>
      <br>
       - The patch need to be rebased due to changes in file
      src/sgml/catalogs.sgml<br>
      <br>
       - Some compilation warning must be fixed:<br>
      <br>
      <blockquote>analyze.c: In function ‘transformLetStmt’:<br>
        analyze.c:1568:17: warning: variable ‘rte’ set but not used
        [-Wunused-but-set-variable]<br>
          RangeTblEntry *rte;<br>
                         ^~~<br>
        tab-complete.c:1268:21: warning: initialization from
        incompatible pointer type [-Wincompatible-pointer-types]<br>
          {"VARIABLE", NULL, &amp;Query_for_list_of_variables},<br>
        <br>
        In the last warning a NULL is missing, should be written:
        {"VARIABLE", NULL, NULL, &amp;Query_for_list_of_variables},</blockquote>
      <br>
       - How about Peter's suggestion?:<br>
          "In DB2, the privileges for variables are named READ and
      WRITE. That would make more sense to me than reusing the privilege
      names for tables.<br>
          The patch use SELECT and UPDATE which make sense too for
      SELECT but less for UPDATE.<br>
      <br>
       - The implementation of "ALTER VARIABLE varname SET SCHEMA
      schema_name;" is missing<br>
      <br>
       - ALTER VARIABLE var1 OWNER TO gilles; ok but not documented and
      missing in regression test<br>
      <br>
       - ALTER VARIABLE var1 RENAME TO var2; ok but not documented and
      missing in regression test<br>
      <br>
      More generally I think that some comments must be rewritten,
      especially those talking about a PoC. In documentation there is
      HTML comments that can be removed.<br>
      <br>
      Comment at end of file src/backend/commands/schemavar.c generate
      some "indent with spaces" errors with git apply but perhaps the
      comment can be entirely removed or undocumented details moved to
      the right place. <br>
      <br>
      Otherwise all regression tests passed without issue and especially
      your new regression tests about schema variables.<br>
      <span class="HOEnZb"></span></div>
    <p>I have a patch rebased, let me known if you want me to post the
      new diff.</p>
    <p><br>
    </p>
    <pre class="moz-signature" cols="72">-- 
Gilles Darold
Consultant PostgreSQL
<a class="moz-txt-link-freetext" href="http://dalibo.com">http://dalibo.com</a> - <a class="moz-txt-link-freetext" href="http://dalibo.org">http://dalibo.org</a>
</pre>
  </body>
</html>

--------------C15DAB3ECEAED6192133E1D8--