Re: proposal: schema variables

út 30. 7. 2024 v 21:46 odesílatel Laurenz Albe <laurenz.albe@cybertec.at>
napsal:

> This is my review of the second patch in the series.
>
> Again, I mostly changed code comments and documentation.
>
> Noteworthy remarks:
>
> - A general reminder: single line comments should start with a lower case
>   letter and have to period in the end:
>
>   /* this is a typical single line comment */
>
> - 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.
>
>   Perhaps there is a bug in src/backend/executor/execExpr.c.
>
> - IdentifyVariable
>
>   > --- a/src/backend/catalog/namespace.c
>   > +++ b/src/backend/catalog/namespace.c
>   > [...]
>   > +/*
>   > + * IdentifyVariable - try to find variable identified by list of
> names.
>   > [...]
>
>   The function comment doesn't make clear to me what the function does.
>   Perhaps that is so confusing because the function seems to serve several
>   purposes (during parsing, in ANALYZE, and to identify shadowed variables
>   in a later patch).
>
>   I rewrote the function comment, but didn't touch the code or code
> comments.
>
>   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?
>
>   Code comments:
>
>   - There are lots of variables declared at the beginning, but they are
> only
>     used in code blocks further down.  For clarity, you should declare a
>     variable in the code block where it is used.
>
>   - +                   /*
>     +                    * In this case, "a" is used as catalog name -
> check it.
>     +                    */
>     +                   if (strcmp(a, get_database_name(MyDatabaseId)) !=
> 0)
>     +                   {
>     +                       if (!noerror)
>     +                           ereport(ERROR,
>     +
>  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>     +                                    errmsg("cross-database references
> are not implemented: %s",
>     +                                           NameListToString(names))));
>     +                   }
>     +                   else
>     +                   {
>
>     There needn't be an "else", since the ereport() will never return.
>     Less indentation is better.
>
> - 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.
>   > +    */
>   > +   Assert(svar);
>   > +
>   > +   *typid = svar->typid;
>   > +
>   > +   return copy_session_variable_value(svar, isNull);
>   > +}
>
> - 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.
>
> - 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.
>
> Attached are the first two patches with my edits (the first patch is
> unchanged;
> I just add it again to keep the cfbot happy.
>

Probably you didn't attach new files - the second patch is not complete. Or
you didn't make changes there?

Regards

Pavel



>
> I hope to get to review the other patches at some later time.
>
> Yours,
> Laurenz Albe
>