Re: proposal: schema variables - Mailing list pgsql-hackers

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.

I hope to get to review the other patches at some later time.

Yours,
Laurenz Albe