Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1ij6Jl-0004ha-Rk for pgsql-hackers@arkaria.postgresql.org; Sun, 22 Dec 2019 18:51:06 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1ij6Jj-0001Hr-NB for pgsql-hackers@arkaria.postgresql.org; Sun, 22 Dec 2019 18:51:03 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1ij6Jj-0001Hk-AV for pgsql-hackers@lists.postgresql.org; Sun, 22 Dec 2019 18:51:03 +0000 Received: from mail-wr1-x429.google.com ([2a00:1450:4864:20::429]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1ij6Jg-0007QM-Ju for pgsql-hackers@lists.postgresql.org; Sun, 22 Dec 2019 18:51:02 +0000 Received: by mail-wr1-x429.google.com with SMTP id q10so14353476wrm.11 for ; Sun, 22 Dec 2019 10:51:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=c1B7uL26StsjmW6DIdwVw2TI+x3cuBxnjTiadnOnuzo=; b=PmLXYWS6a6XyL+YGaaJi1u7vzU8GaqXV+0tixlHoEMUINuDJPMRnjE6F+BemXqXn0b EayJkwHN8zVewpsXhHU4QWFsCw9b2yCNJuzISap0XdPSY9dCaPO8BMnFq9ePCdrnqfKl 3oXflHkHTTpbQaxxlG2eMXRteR2bMQTy6OodXF7QiQ09ir4lGxS3yOlUWzzrUVLAGpsU V6te/pUltg5VsP3BmyiluT+g/o4lQ5V9Wk3v/9ZmiMQr6CZfPxuWslMRXK555Iw83/p1 rMDuefFiXUikiQDEftqohp1EbI4P2OeDzfkcXYG3lQ5K+Ei6FepUL88CNp8SVula+F2B RZ2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=c1B7uL26StsjmW6DIdwVw2TI+x3cuBxnjTiadnOnuzo=; b=YemFBu7XG33U9V37Q1r1+4+mGP+iuZgD4mMFy4fgjpXLl+gDU+7IFrxK8YylHb/THO fpSRblHh3X44AGXI3m1E4nGnlcJneI3xtkIN0vylyYhQ6wEE6BzqF7m+cjbDly5sFKh0 8c4npLueh4c9tIlk0YaPT4hLOfKUC5VqOlUcmaGDeWCgRj5TzImEGrnk3duyXy48grbS 0kLc8qr2ygQabDwICSlNDJnYkgPIy6L+Ze7+6Dz1fEPchVxG1MXcRVmOvZfpvfr8iTRD Qi3l/ShFayHbdSGaNNN3laW0+THWdsZsfRI/aKs3g2GOtECsX+yHccoE+7+ncHPFC39k lkbg== X-Gm-Message-State: APjAAAVRjwb4FpbGy4Fl2zLw+5ysrizplxnZiyZWy+LauOQh1wC6Tzws QTy311H2fz1vIl81JAr8Z3r0UF2BU7CkXS4ns5k= X-Google-Smtp-Source: APXvYqw7X0ZLyuI4bpXCUw2tYyki5C/2wHnZpOVLusTPRCH1qT0Sl2XR7Us3nPJoo2xY84vCj1dtVm3aK4vgqaVjo58= X-Received: by 2002:adf:f491:: with SMTP id l17mr25947155wro.149.1577040658613; Sun, 22 Dec 2019 10:50:58 -0800 (PST) MIME-Version: 1.0 References: <157701619041.1198.10146593618667092522.pgcf@coridan.postgresql.org> In-Reply-To: <157701619041.1198.10146593618667092522.pgcf@coridan.postgresql.org> From: Pavel Stehule Date: Sun, 22 Dec 2019 19:50:22 +0100 Message-ID: Subject: Re: proposal: schema variables To: Philippe BEAUDOIN Cc: PostgreSQL Hackers Content-Type: multipart/alternative; boundary="00000000000040c940059a4f6421" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000040c940059a4f6421 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi ne 22. 12. 2019 v 13:04 odes=C3=ADlatel Philippe BEAUDOIN napsal: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, failed > Spec compliant: not tested > Documentation: tested, failed > > Hi Pavel, > > First of all, I would like to congratulate you for this great work. This > patch is really cool. The lack of package variables is sometimes a blocki= ng > issue for Oracle to Postgres migrations, because the usual emulation with > GUC is sometimes not enough, in particular when there are security concer= ns > or when the database is used in a public cloud. > > As I look forward to having this patch commited, I decided to spend some > time to participate to the review, although I am not a C specialist and I > have not a good knowledge of the Postgres internals. Here is my report. > > A) Installation > > The patch applies correctly and the compilation is fine. The "make check" > doesn't report any issue. > > B) Basic usage > > I tried some simple schema variables use cases. No problem. > > C) The interface > > The SQL changes look good to me. > > However, in the CREATE VARIABLE command, I would replace the "TRANSACTION= " > word by "TRANSACTIONAL". > There is not technical problem - the problem is in introduction new keyword "transactional" that is near to "transaction". I am not sure if it is practical to have two "similar" keyword and how much the CREATE statement has to use correct English grammar. I am not native speaker, so I am not able to see how bad is using "TRANSACTION" instead "TRANSACTIONAL" in this context. So I see a risk to have two important (it is not syntactic sugar) similar keywords. Just I afraid so using TRANSACTIONAL instead just TRANSACTION is not too user friendly. I have not strong opinion about this - and the implementation is easy, but I am not feel comfortable with introduction this keyword. > I have also tried to replace this word by a ON ROLLBACK clause at the end > of the statement, like for ON COMMIT, but I have not found a satisfying > wording to propose. > > > D) Behaviour > > I am ok with variables not being transactional by default. That's the mos= t > simple, the most efficient, it emulates the package variables of other > RDBMS and it will probably fit the most common use cases. > > Note that I am not strongly opposed to having by default transactional > variables. But I don't know whether this change would be a great work. We > would have at least to find another keyword in the CREATE VARIABLE > statement. Something like "NON-TRANSACTIONAL VARIABLE" ? > Variables almost everywhere (global user settings - GUC is only one planet exception) are non transactional by default. I don't see any reason introduce new different design than is wide used. > It is possible to create a NOT NULL variable without DEFAULT. When trying > to read the variable before a LET statement, one gets an error massage > saying that the NULL value is not allowed (and the documentation is clear > about this case). Just for the records, I wondered whether it wouldn't be > better to forbid a NOT NULL variable creation that wouldn't have a DEFAUL= T > value. But finally, I think this behaviour provides a good way to force t= he > variable initialisation before its use. So let's keep it as is. > This is a question - and there are two possibilities postgres=3D# do $$ declare x int not null; begin raise notice '%', x; end; $$ ; ERROR: variable "x" must have a default value, since it's declared NOT NUL= L LINE 2: declare x int not null; ^ PLpgSQL requires it. But there is not a possibility to enforce future setting. So I know so behave of schema variables is little bit different, but I think so this difference has interesting use case. You can check if the variable was modified somewhere or not. > E) ACL and Rights > > I played a little bit with the GRANT and REVOKE statements. > > I have got an error (Issue 1). The following statement chain: > create variable public.sv1 int; > grant read on variable sv1 to other_user; > drop owned by other_user; > reports : ERROR: unexpected object class 4287 > this is bug and should be fixed > I then tried to use DEFAULT PRIVILEGES. Despite this is not documented, I > successfuly performed: > alter default privileges in schema public grant read on variables to > simple_user; > alter default privileges in schema public grant write on variables to > simple_user; > > When variables are then created, the grants are properly given. > And the psql \ddp command perfectly returns: > Default access privileges > Owner | Schema | Type | Access privileges > ----------+--------+------+------------------------- > postgres | public | | simple_user=3DSW/postgres > (1 row) > > So the ALTER DEFAULT PRIVILEGES documentation chapter has to reflect this > new syntax (Issue 2). > > BTW, in the ACL, the READ privilege is represented by a S letter. A > comment in the source reports that the R letter was used in the past for > rule privilege. Looking at the postgres sources, I see that this privileg= e > on rules has been suppressed in 8.2, so 13 years ago. As this R letter > would be a so much better choice, I wonder whether it couldn't be reused > now for this new purpose. Is it important to keep this letter frozen ? > I have not a idea why it is. I'll recheck it - but in this moment I prefer a consistency with existing ACL - it can be in future as one block if it will be necessary for somebody. > > F) Extension > > I then created an extension, whose installation script creates a schema > variable and functions that use it. The schema variable is correctly link= ed > to the extension, so that dropping the extension drops the variable. > > But there is an issue when dumping the database (Issue 3). The script > generated by pg_dump includes the CREATE EXTENSION statement as expected > but also a redundant CREATE VARIABLE statement for the variable that > belongs to the extension. As a result, one of course gets an error at > restore time. > It is bug and should be fixed > G) Row Level Security > > I did a test activating RLS on a table and creating a POLICY that > references a schema variable in its USING and WITH CHECK clauses. > Everything worked fine. > > H) psql > > A \dV meta-command displays all the created variables. > I would change a little bit the provided view. More precisely I would: > - rename "Constraint" into "Is nullable" and report it as a boolean > - rename "Special behave" into "Is transactional" and report it as a > boolean > - change the order of columns so to have: > Schema | Name | Type | Is nullable | Default | Owner | Is transactional | > Transaction end action > "Is nullable" being aside "Default" > ok > I) Performance > > I just quickly looked at the performance, and didn't notice any issue. > > About variables read performance, I have noticed that: > select sum(1) from generate_series(1,10000000); > and > select sum(sv1) from generate_series(1,10000000); > have similar response times. > > About planning, a condition with a variable used as a constant is > indexable, as if it were a literal. > > J) Documentation > > There are some wordings to improve in the documentation. But I am not the > best person to give advice about english language ;-). > > However, aside the already mentionned lack of changes in the ALTER DEFAUL= T > PRIVILEGES chapter, I also noticed : > - line 50 of the patch, the sentence "(hidden attribute; must be > explicitly selected)" looks false as the oid column of pg_variable is > displayed, as for other tables of the catalog; > - at several places, the word "behave" should be replaced by "behaviour" > - line 433, a get_schema_variable() function is mentionned; is it a > function that can really be called by users ? > > May be it would be interesting to also add a chapter in the Section V of > the documentation, in order to more globally present the schema variables > concept, aside the new or the modified statements. > > K) Coding > > I am not able to appreciate the way the feature has been coded. So I let > this for other reviewers ;-) > > > To conclude, again, thanks a lot for this feature ! > And if I may add this. I dream of an additional feature: adding a SHARED > clause to the CREATE VARIABLE statement in order to be able to create > memory spaces that could be shared by all connections on the database and > accessible in SQL and PL, under the protection of ACL. But that's another > story ;-) > sure, it is another story :-). Thank you for review - I'll try to fix bugs this week. Pavel > Best regards. Philippe. > > The new status of this patch is: Waiting on Author > --00000000000040c940059a4f6421 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

ne 22. 12. 2019 v=C2=A013:04 odes=C3=ADlatel = Philippe BEAUDOIN <phb07@apra.asso= .fr> napsal:
The following review has been posted through the commitfest application= :
make installcheck-world:=C2=A0 tested, passed
Implements feature:=C2=A0 =C2=A0 =C2=A0 =C2=A0tested, failed
Spec compliant:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0not tested
Documentation:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 tested, failed

Hi Pavel,

First of all, I would like to congratulate you for this great work. This pa= tch is really cool. The lack of package variables is sometimes a blocking i= ssue for Oracle to Postgres migrations, because the usual emulation with GU= C is sometimes not enough, in particular when there are security concerns o= r when the database is used in a public cloud.

As I look forward to having this patch commited, I decided to spend some ti= me to participate to the review, although I am not a C specialist and I hav= e not a good knowledge of the Postgres internals. Here is my report.

A) Installation

The patch applies correctly and the compilation is fine. The "make che= ck" doesn't report any issue.

B) Basic usage

I tried some simple schema variables use cases. No problem.

C) The interface

The SQL changes look good to me.

However, in the CREATE VARIABLE command, I would replace the "TRANSACT= ION" word by "TRANSACTIONAL".

There is not technical problem - the problem is in introduction new k= eyword "transactional" that is near to "transaction". I= am not sure if it is practical to have two "similar" keyword and= how much the CREATE statement has to use correct English grammar.

I am not native speaker, so I am not able to see how = bad is using "TRANSACTION" instead "TRANSACTIONAL" in t= his context. So I see a risk to have two important (it is not syntactic sug= ar) similar keywords.

Just I afraid so using TRANS= ACTIONAL instead just TRANSACTION is not too user friendly. I have not stro= ng opinion about this - and the implementation is easy, but I am not feel c= omfortable with introduction this keyword.


I have also tried to replace this word by a ON ROLLBACK clause at the end o= f the statement, like for ON COMMIT, but I have not found a satisfying word= ing to propose.

=C2=A0

D) Behaviour

I am ok with variables not being transactional by default. That's the m= ost simple, the most efficient, it emulates the package variables of other = RDBMS and it will probably fit the most common use cases.

Note that I am not strongly opposed to having by default transactional vari= ables. But I don't know whether this change would be a great work. We w= ould have at least to find another keyword in the CREATE VARIABLE statement= . Something like "NON-TRANSACTIONAL VARIABLE" ?
<= div>
Variables almost everywhere (global user settings - GUC = is only one planet exception) are non transactional by default. I don't= see any reason introduce new different design than is wide used.


It is possible to create a NOT NULL variable without DEFAULT. When trying t= o read the variable before a LET statement, one gets an error massage sayin= g that the NULL value is not allowed (and the documentation is clear about = this case). Just for the records, I wondered whether it wouldn't be bet= ter to forbid a NOT NULL variable creation that wouldn't have a DEFAULT= value. But finally, I think this behaviour provides a good way to force th= e variable initialisation before its use. So let's keep it as is.

This is a question - and there are two possi= bilities

postgres=3D# do $$
declare x int not n= ull;
begin
=C2=A0 raise notice '%', x;
end;
$$ ;
ERR= OR: =C2=A0variable "x" must have a default value, since it's = declared NOT NULL
LINE 2: declare x int not null;
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ^
PLpgSQL requires it. But there is not a possibility to enforce = future setting.

So I know so behave of schema vari= ables is little bit different, but I think so this difference has interesti= ng use case. You can check if the variable was modified somewhere or not.


E) ACL and Rights

I played a little bit with the GRANT and REVOKE statements.

I have got an error (Issue 1). The following statement chain:
=C2=A0 create variable public.sv1 int;
=C2=A0 grant read on variable sv1 to other_user;
=C2=A0 drop owned by other_user;
reports : ERROR:=C2=A0 unexpected object class 4287
this is bug and should be fixed


I then tried to use DEFAULT PRIVILEGES. Despite this is not documented, I s= uccessfuly performed:
=C2=A0 alter default privileges in schema public grant read on variables to= simple_user;
=C2=A0 alter default privileges in schema public grant write on variables t= o simple_user;

When variables are then created, the grants are properly given.
And the psql \ddp command perfectly returns:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Default access privileges =C2=A0 Owner=C2=A0 =C2=A0| Schema | Type |=C2=A0 =C2=A0 Access privileges= =C2=A0 =C2=A0
----------+--------+------+-------------------------
=C2=A0postgres | public |=C2=A0 =C2=A0 =C2=A0 | simple_user=3DSW/postgres (1 row)

So the ALTER DEFAULT PRIVILEGES documentation chapter has to reflect this n= ew syntax (Issue 2).

BTW, in the ACL, the READ privilege is represented by a S letter. A comment= in the source reports that the R letter was used in the past for rule priv= ilege. Looking at the postgres sources, I see that this privilege on rules = has been suppressed=C2=A0 in 8.2, so 13 years ago. As this R letter would b= e a so much better choice, I wonder whether it couldn't be reused now f= or this new purpose. Is it important to keep this letter frozen ?

I have not a idea why it is. I'll recheck it= - but in this moment I prefer a consistency with existing ACL - it can be = in future as one block if it will be necessary for somebody.
= =C2=A0

F) Extension

I then created an extension, whose installation script creates a schema var= iable and functions that use it. The schema variable is correctly linked to= the extension, so that dropping the extension drops the variable.

But there is an issue when dumping the database (Issue 3). The script gener= ated by pg_dump includes the CREATE EXTENSION statement as expected but als= o a redundant CREATE VARIABLE statement for the variable that belongs to th= e extension. As a result, one of course gets an error at restore time.
<= /blockquote>

It is bug and should be fixed

G) Row Level Security

I did a test activating RLS on a table and creating a POLICY that reference= s a schema variable in its USING and WITH CHECK clauses. Everything worked = fine.

H) psql

A \dV meta-command displays all the created variables.
I would change a little bit the provided view. More precisely I would:
- rename "Constraint" into "Is nullable" and report it = as a boolean
- rename "Special behave" into "Is transactional" and r= eport it as a boolean
- change the order of columns so to have:
Schema | Name | Type | Is nullable | Default | Owner | Is transactional | T= ransaction end action
"Is nullable" being aside "Default"

ok


I) Performance

I just quickly looked at the performance, and didn't notice any issue.<= br>
About variables read performance, I have noticed that:
=C2=A0 select sum(1) from generate_series(1,10000000);
and
=C2=A0 select sum(sv1) from generate_series(1,10000000);
have similar response times.

About planning, a condition with a variable used as a constant is indexable= , as if it were a literal.

J) Documentation

There are some wordings to improve in the documentation. But I am not the b= est person to give advice about english language ;-).

However, aside the already mentionned lack of changes in the ALTER DEFAULT = PRIVILEGES chapter, I also noticed :
- line 50 of the patch, the sentence "(hidden attribute; must be expli= citly selected)" looks false as the oid column of pg_variable is displ= ayed, as for other tables of the catalog;
- at several places, the word "behave" should be replaced by &quo= t;behaviour"
- line 433, a get_schema_variable() function is mentionned; is it a functio= n that can really be called by users ?

May be it would be interesting to also add a chapter in the Section V of th= e documentation, in order to more globally present the schema variables con= cept, aside the new or the modified statements.

K) Coding

I am not able to appreciate the way the feature has been coded. So I let th= is for other reviewers ;-)


To conclude, again, thanks a lot for this feature !
And if I may add this. I dream of an additional feature: adding a SHARED cl= ause to the CREATE VARIABLE statement in order to be able to create memory = spaces that could be shared by all connections on the database and accessib= le in SQL and PL, under the protection of ACL. But that's another story= ;-)

sure, it is another story :-).

Thank you for review - I'll try to fix bugs = this week.

Pavel


Best regards. Philippe.

The new status of this patch is: Waiting on Author
--00000000000040c940059a4f6421--