Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fYE4o-0003dQ-1m for pgsql-hackers@arkaria.postgresql.org; Wed, 27 Jun 2018 17:17:54 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fYE4m-0005I9-I0 for pgsql-hackers@arkaria.postgresql.org; Wed, 27 Jun 2018 17:17:52 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fYE4l-0005I2-Tp for pgsql-hackers@lists.postgresql.org; Wed, 27 Jun 2018 17:17:52 +0000 Received: from mail-wm0-x22c.google.com ([2a00:1450:400c:c09::22c]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fYE4i-0002ne-6F for pgsql-hackers@lists.postgresql.org; Wed, 27 Jun 2018 17:17:50 +0000 Received: by mail-wm0-x22c.google.com with SMTP id w137-v6so6711402wmw.1 for ; Wed, 27 Jun 2018 10:17:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=/fHW3EMOCIePCbhgbtUQbRQipL3iTpJvJCFS87FhPZY=; b=ix0wd7QyRc0swKGpggDS2vwTZop5gLZ39dh+HYerCFIThNtK+z6v1DGR1r15D/pvxz O2bAYXSms8rOqm9bwlFB/6jPBB2m6LlZ+HkVj3EMmnJxfKU7g2yRnMGJvNUpcQ73pxKn xV2ao37fKJ4jusVZHqdoyLH9Fu1OLB1DIyiHu5BacpISieEvnvt4KJLAYUGY2KDB+H8C TUjeqdUO63NWaqCnSV/VCHAiAZkAnEnt1yXaKYUxwvMbQzpIFi4dIiNrDoArt3CMg+Bw z4RZxNsFlnLYWKyuY0jOPxIB80+jcgK5M91I56kkZAutdz4HFq0tqHqOKST+2vZd/NQi mxzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=/fHW3EMOCIePCbhgbtUQbRQipL3iTpJvJCFS87FhPZY=; b=lTiUV2fkOZ50D0Y5ejl8rK7J96rHppJ4+wzAbLgakJsw970EcOvVey9MoGNZhY6J+i tG+GJ9FQy1O7hO9EDa8iHO+7llp8MM4+S1dDcB7HFStVGdtNaBbr0cJ4+8O9ZJlaSrpu 7Tz3ibsuRP1kPdvpNWyx4x+OONMaqxhXoBr79zgdZYVy5/oFMR6NN4nB91MjvqD3oZys KMo4J3WaxV/6viE7gVXgLjIK5L7SiED/uPlaC2P68TpFbx2fnznyN3VPoqtuKzV6rO3z c40abOFVjt+wg05K+MDuFgW1qWHD/SHbXOeleU3KysPDwqSN912UrnlO+RpQ2Nq6utTM L/7A== X-Gm-Message-State: APt69E2xRjbqv/x6/P1AZc+GXUqgft/H8BzXCh68aYC2rb7Yrmqp+Icq uyjscLwnBomM/RWvNDCVdHr1fKUI4gYuPVXWDxA= X-Google-Smtp-Source: AAOMgpeQhVUFknbTNB/6TzWfnSSQc6WzGkOddle0RrGhx/E/MbQqE3uJ37ma6JS+gdogmz71VLsCz2r0krIBCzDrWrs= X-Received: by 2002:a1c:8590:: with SMTP id h138-v6mr5490398wmd.85.1530119865595; Wed, 27 Jun 2018 10:17:45 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a5d:428a:0:0:0:0:0 with HTTP; Wed, 27 Jun 2018 10:17:05 -0700 (PDT) In-Reply-To: References: <623395617.20171113141500@gf.microolap.com> <28924bcc-9242-9798-e4e8-9d83cea3fef6@dalibo.com> From: Pavel Stehule Date: Wed, 27 Jun 2018 19:17:05 +0200 Message-ID: Subject: Re: [HACKERS] proposal: schema variables To: Gilles Darold Cc: PostgreSQL Hackers Content-Type: multipart/alternative; boundary="0000000000000d7d86056fa2cb7d" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000000d7d86056fa2cb7d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 2018-06-27 19:15 GMT+02:00 Gilles Darold : > Le 27/06/2018 =C3=A0 13:22, Pavel Stehule a =C3=A9crit : > > Hi > > 2018-06-27 12:21 GMT+02:00 Gilles Darold : > >> 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 =E2=80=98transformLetStmt=E2=80=99: >> analyze.c:1568:17: warning: variable =E2=80=98rte=E2=80=99 set but not u= sed >> [-Wunused-but-set-variable] >> RangeTblEntry *rte; >> ^~~ >> tab-complete.c:1268:21: warning: initialization from incompatible pointe= r >> 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 missin= g >> 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 b= e >> 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= . >> > > I plan significant refactoring of this patch for next commitfest. There > was anotherstrong Peter's and Robert comments > > 1. The schema variables should to have own system table > 2. The composite schema variables should to use explicitly defined > composite type > 3. The memory management is not nice - transactional drop table with > content is implemented ugly. > > I hope, so I can start on these issues next month. > > Thank you for review - I'll recheck ALTER commands. > >> >> 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= . >> > > I plan significant refactoring of this patch for next commitfest. There > was anotherstrong Peter's and Robert c > Regards > > > Ok Pavel, I've changed the status to "Waiting for authors" so that no one > will make an other review until you send a new patch. > sure Thank you Pavel > > -- > Gilles Darold > Consultant PostgreSQLhttp://dalibo.com - http://dalibo.org > > --0000000000000d7d86056fa2cb7d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


2018-06-27 19:15 GMT+02:00 Gilles Darold <gilles.darold@dalibo= .com>:
=20 =20 =20
Le 27/06/2018 =C3= =A0 13:22, Pavel Stehule a =C3=A9crit=C2=A0:
Hi

2018-06-27 12= :21 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com>:
Hi,

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

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

=C2=A0- Some compilation warning must be fixed:

analyze.c: In function =E2=80=98transformLetStmt=E2=80=99:
analyze.c:1568:17: warning: variable =E2=80=98rte=E2= =80=99 set but not used [-Wunused-but-set-variable]
=C2=A0 RangeTblEntry *rte;
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ^~~
tab-complete.c:1268:21: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
=C2=A0 {"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},

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

=C2=A0- The implementation of "ALTER VARIABLE varn= ame SET SCHEMA schema_name;" is missing

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

=C2=A0- 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 wit= h 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.


I plan significant refactoring of this patch for next commitfest. There was anotherstrong Peter's and Robert comments

1. The schema variables should to have own system table
2. The composite schema variables should to use explicitly defined composite type
3. The memory management is not nice - transactional drop table with content is implemented ugly.

I hope, so I can start on these issues next month.

Thank you for review - I'll recheck ALTER commands. =

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.


I plan significant refactoring of this patch for next commitfest. There was anotherstrong Peter's and Robert c
Regards


Ok Pavel, I've changed the status to "Waiting for authors&q= uot; so that no one will make an other review until you send a new patch.

=
sure

Thank you
<= br>
Pavel
=C2=A0


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

--0000000000000d7d86056fa2cb7d--