Re: proposal: schema variables

Hi

ne 22. 12. 2019 v 13:04 odesílatel Philippe BEAUDOIN <phb07(at)apra(dot)asso(dot)fr>
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 blocking
> issue for Oracle to Postgres migrations, because the usual emulation with
> GUC is sometimes not 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 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".
>
> 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.
>

I propose compromise solution - I introduced new not reserved keyword
"TRANSACTIONAL". User can use TRANSACTION or TRANSACTIONAL. It is similar
relation like "TEMP" or "TEMPORAL"

>
> D) Behaviour
>
> I am ok with variables not being transactional by default. That's the most
> 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" ?
>
> 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 DEFAULT
> 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
>

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;
>

should be fixed

> 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 used in 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 reused
> now for this new purpose. Is it important to keep this letter frozen ?
>

I use ACL_READ constant in my patch. The value of ACL_READ is defined
elsewhere. So the changing from S to R should be done by separate patch and
by separate discussion.

> F) Extension
>
> I then created an extension, whose installation script creates a schema
> variable 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
> 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.
>

should be fixed now

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

I implemented your proposal

> 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_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 ?
>

should be fixed

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

We can finalize documentation little bit later, when will be clear what
related functionality is implemented.

updated patch attached

> 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 ;-)
>
> Best regards. Philippe.
>

Thank you very much for review

>
> The new status of this patch is: Waiting on Author
>