Re: proposal: schema variables

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