Re: proposal: schema variables

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
variablesis sometimes a blocking issue for Oracle to Postgres migrations, because the usual emulation with GUC is
sometimesnot enough, in particular when there are security concerns 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
amnot 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".

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
havenot found a satisfying wording to propose.
 

D) Behaviour

I am ok with variables not being transactional by default. That's the most simple, the most efficient, it emulates the
packagevariables 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
wouldbe a great work. We would have at least to find another keyword in the CREATE VARIABLE statement. Something like
"NON-TRANSACTIONALVARIABLE" ?
 

It is possible to create a NOT NULL variable without DEFAULT. When trying to read the variable before a LET statement,
onegets an error massage saying that the NULL value is not allowed (and the documentation is clear about this case).
Justfor the records, I wondered whether it wouldn't be better to forbid a NOT NULL variable creation that wouldn't have
aDEFAULT value. But finally, I think this behaviour provides a good way to force the variable initialisation before its
use.So let's keep it as is.
 

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

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=SW/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
usedin the past for rule privilege. Looking at the postgres sources, I see that this privilege 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
reusednow for this new purpose. Is it important to keep this letter frozen ?
 

F) Extension

I then created an extension, whose installation script creates a schema variable and functions that use it. The schema
variableis 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 generated by pg_dump includes the CREATE
EXTENSIONstatement 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.
 

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
CHECKclauses. 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"

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 DEFAULT 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_variableis 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
presentthe 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
orderto be able to create memory spaces that could be shared by all connections on the database and accessible in SQL
andPL, under the protection of ACL. But that's another story ;-)
 

Best regards. Philippe.

The new status of this patch is: Waiting on Author