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

I went through the v20240724-2-0001 patch and mostly changed some documentation
and the comments.  Attached is my updated version.

Comments:

> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c
> [...]
> +bool
> +VariableIsVisible(Oid varid)
> [...]
> +   /*
> +    * Quick check: if it ain't in the path at all, it ain't visible. Items in
> +    * the system namespace are surely in the path and so we needn't even do
> +    * list_member_oid() for them.
> +    */
> +   varnamespace = varform->varnamespace;
> +   if (varnamespace != PG_CATALOG_NAMESPACE &&
> +       !list_member_oid(activeSearchPath, varnamespace))
> +       visible = false;
> +   else

I cannot imagine that we'll ever have session variables in "pg_catalog".
Did you put that there as defensive coding?

> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c
> [...]
> +Datum
> +pg_variable_is_visible(PG_FUNCTION_ARGS)

That function should get documented in functions-info.html#FUNCTIONS-INFO-SCHEMA-TABLE
I have added an entry in the attached patch.

> --- /dev/null
> +++ b/doc/src/sgml/ref/create_variable.sgml
> [...]
> +  <note>
> +   <para>
> +    Inside a query or an expression, the session variable can be shadowed by
> +    column or by routine's variable or routine argument. Such collisions of
> +    identifiers can be resolved by using qualified identifiers. Session variables
> +    can use schema name, columns can use table aliases, routine variables
> +    can use block labels, and routine arguments can use the routine name.
> +   </para>
> +  </note>
> + </refsect1>

I have slightly rewritten the text and moved it to doc/src/sgml/ddl.sgml.
I felt that to be a better place for generic information about session variable's
behavior.  Also, the single paragraph in doc/src/sgml/ddl.sgml felt lonely.
I left the note in CREATE VARIABLE, but it is now just a link to ddl.sgml.

> --- a/src/bin/pg_dump/pg_dump.c
> +++ b/src/bin/pg_dump/pg_dump.c
> [...]
> +void
> +getVariables(Archive *fout)
> +{
> [...]
> +   for (i = 0; i < ntups; i++)
> +   {
> [...]
> +       /* Decide whether we want to dump it */
> +       selectDumpableObject(&(varinfo[i].dobj), fout);
> +
> +       /* Do not try to dump ACL if no ACL exists. */
> +       if (!PQgetisnull(res, i, i_varacl))
> +           varinfo[i].dobj.components |= DUMP_COMPONENT_ACL;
> +
> +       if (strlen(varinfo[i].rolname) == 0)
> +           pg_log_warning("owner of variable \"%s\" appears to be invalid",
> +                          varinfo[i].dobj.name);
> +
> +       /* Decide whether we want to dump it */
> +       selectDumpableObject(&(varinfo[i].dobj), fout);
> +
> +       vtype = findTypeByOid(varinfo[i].vartype);
> +       addObjectDependency(&varinfo[i].dobj, vtype->dobj.dumpId);
> +   }

One of the two selectDumpableObject() calls seems redundant.
I removed the first one; I hope that's OK.

> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> [...]
> @@ -4421,7 +4449,7 @@ psql_completion(const char *text, int start, int end)
> 
>  /* PREPARE xx AS */
>     else if (Matches("PREPARE", MatchAny, "AS"))
> -       COMPLETE_WITH("SELECT", "UPDATE", "INSERT INTO", "DELETE FROM");
> +       COMPLETE_WITH("SELECT", "UPDATE", "INSERT INTO", "DELETE FROM", "LET");

I guess that belongs in the second patch, but I didn't change it.
Since the feature cannot be committed without LET, it doesn't really matter in
which of the two patches it is.

> --- a/src/test/regress/expected/psql.out
> +++ b/src/test/regress/expected/psql.out
> [...]
> +\dV regression|mydb.public.var
> +cross-database references are not implemented: regression|mydb.public.var

That's a weird test - why the pipe?  Is there something I don't get?

There is already another test for cross-database references.

> +\dV "no.such.database"."no.such.schema"."no.such.variable"
> +cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.variable"

And another one.  Do we need so many tests for that?

I hope I didn't create too many conflicts with the rest of the patch set.

I plan to review the other patches too, but I'll go on vacations for three weeks
in a week, and fall promises to be busy.  So it might be a while.

Yours,
Laurenz Albe