From: | Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> |
---|---|
To: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
Cc: | Erik Rijkers <er(at)xs4all(dot)nl>, Michael Paquier <michael(at)paquier(dot)xyz>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, DUVAL REMI <REMI(dot)DUVAL(at)cheops(dot)fr>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: proposal: schema variables |
Date: | 2024-07-27 14:19:23 |
Message-ID: | c29fb07fb1b55725e175e5625a5261baeab4a977.camel@cybertec.at |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
Attachment | Content-Type | Size |
---|---|---|
v20240727-0001-Catalog-CREATE-ALTER-DROP-privileges-pg_du.patch | text/x-patch | 105.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2024-07-27 14:20:29 | Re: why is pg_upgrade's regression run so slow? |
Previous Message | Dmitry Dolgov | 2024-07-27 13:49:42 | Re: Pluggable cumulative statistics |