Re: proposal: schema variables - Mailing list pgsql-hackers
From | Laurenz Albe |
---|---|
Subject | Re: proposal: schema variables |
Date | |
Msg-id | 04ec666686e9e21cb515617df06885c66f3d34ce.camel@cybertec.at Whole thread Raw |
Responses |
Re: proposal: schema variables
|
List | pgsql-hackers |
On Thu, 2024-08-15 at 07:55 +0200, Pavel Stehule wrote: > út 30. 7. 2024 v 21:46 odesílatel Laurenz Albe <laurenz.albe@cybertec.at> napsal: > > - A general reminder: single line comments should start with a lower case > > letter and have to period in the end: > > Should it be "have not to period in the end" ? I made a typo. I mean "have *no* period in the end". > I fixed almost all without parts related to psql and executor - there almost all > current comments break the mentioned rule. So I use the style used in these files. > I can fix it (if you like it) - or it can be fixed generally in a separated patch set. Thanks. I also noticed that a lot of existing comments break that rule, so I think that it is fine if you stick with the way that the surrounding code does it. > > - Variable as parameter: > > > > CREATE VARIABLE var AS date; > > LET var = current_date; > > PREPARE stmt(date) AS SELECT $1; > > EXECUTE stmt(var); > > ERROR: paramid of PARAM_VARIABLE param is out of range > > > > Is that working as intended? I don't understand the error message. > > fixed in 0002 patch (variables cannot be used as EXECUTE argument) and 0014 (enable usage variables as an argument of EXECUTE) Thanks. > > > --- a/src/backend/catalog/namespace.c > > > +++ b/src/backend/catalog/namespace.c > > > [...] > > > +/* > > > + * IdentifyVariable - try to find variable identified by list of names. > > > [...] > > > > Perhaps part of the reason why this function is so complicated is that > > you support things like this: > > > > CREATE SCHEMA sch; > > CREATE TYPE cp AS (a integer, b integer); > > CREATE VARIABLE sch.v AS cp; > > LET sch.v = (1, 2); > > SELECT sch.v.b; > > b > > ═══ > > 2 > > (1 row) > > > > test=# SELECT test.sch.v.b; > > b > > ═══ > > 2 > > (1 row) > > > > We don't support that for tables: > > > > CREATE TABLE tab (c cp); > > INSERT INTO tab VALUES (ROW(42, 43)); > > SELECT tab.c.b FROM tab; > > ERROR: cross-database references are not implemented: tab.c.b > > > > You have to use extra parentheses: > > > > SELECT (tab.c).b FROM tab; > > b > > ════ > > 43 > > (1 row) > > > > I'd say that this should be the same for session variables. > > What do you think? > > I prefer the current state, but I don't have a very strong opinion about it. > It can save 115 lines of almost trivial code, but I think the user experience > can be much worse. Using composite types in tables is not too common a > pattern (and when I read some recommendations for Oracle, it is an antipattern), > but usage of composite variables is common (it can hold a row). Moreover, > we talked long about possible identifier's collisions, and the pattern > schema.variable is very good protection against possible collisions. > Probably the pattern catalog.schema.variable.field is not too interesting > but the support has 50 lines. > > More, the plpgsql supports pattern label.variable.field, so can be little bit > unfriendly if session variables doesn't support "similar" pattern I see your point, and I'm fine with leaving it as it is. > > > > - src/backend/commands/session_variable.c > > > > There is one comment that confuses me and that I did not edit: > > > > > +Datum > > > +GetSessionVariable(Oid varid, bool *isNull, Oid *typid) > > > +{ > > > + SVariable svar; > > > + > > > + svar = get_session_variable(varid); > > > + > > > + /* > > > + * Although svar is freshly validated in this point, the svar->is_valid can > > > + * be false, due possible accepting invalidation message inside domain > > > + * check. Now, the validation is done after lock, that can also accept > > > + * invalidation message, so validation should be trustful. > > > + * > > > + * For now, we don't need to repeat validation. Only svar should be valid > > > + * pointer. > > > + */ > > This comment is related to assertions. Before I had there `Assert(svar->is_valid)`, > because I expected it. But it was not always true. And although it is true, > we don't need to validate a variable, because at this moment, the variable > should be locked, and then we can return content safely. I guess my main problem is the word "trustful". I don't recognize that word. Perhaps you can reword the comment along the lines of your above explanation. > > > - src/backend/executor/execMain.c > > > > > @@ -196,6 +198,51 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags) > > > Assert(queryDesc->sourceText != NULL); > > > estate->es_sourceText = queryDesc->sourceText; > > > > > > + /* > > > + * The executor doesn't work with session variables directly. Values of > > > + * related session variables are copied to dedicated array, and this array > > > + * is passed to executor. > > > + */ > > > > Why? Is that a performance measure? The comment should explain that. > > Session variables are volatile internally - some function that uses LET statements > can be called more times inside a query. This behavior is not consistent with > plpgsql's variables or external parameters. And if we want to support parallel > queries, then volatile session variables can be pretty messy (and unusable). > If we want the same behaviour for queries with or without parallel support, > then the session variables should be "stable" (like stable functions). > Simple implementation is using "snapshot" of values of used session variables > when query is started. This "snapshot" is immutable, so we don't need to make > a copy more times. > > I changed this comment Thanks. > > - parallel safety > > > > > --- a/src/backend/optimizer/util/clauses.c > > > +++ b/src/backend/optimizer/util/clauses.c > > > @@ -935,6 +936,13 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context) > > > if (param->paramkind == PARAM_EXTERN) > > > return false; > > > > > > + /* We doesn't support passing session variables to workers */ > > > + if (param->paramkind == PARAM_VARIABLE) > > > + { > > > + if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)) > > > + return true; > > > + } > > > > Even if a later patch alleviates that restriction, this patch should document that > > session variables imply "parallel restricted". I have added that to doc/src/sgml/parallel.sgml. > > > > merged (and removed in 0015) Thanks. I hope I can do some more review at some point in the future... I sincerely hope that this patch set gets merged at some point. The big obstacle is that it is so large. That's probably because it is pretty feature-complete (but, as we have found, not totally free of bugs). Judging from the amount of time I put into my review so far, I guess that this patch set would keep a committer busy for several weeks. Perhaps the only way to get this done would be to make you a committer... Yours, Laurenz Albe
pgsql-hackers by date: