Re: proposal: schema variables - Mailing list pgsql-hackers
From | Laurenz Albe |
---|---|
Subject | Re: proposal: schema variables |
Date | |
Msg-id | c29fb07fb1b55725e175e5625a5261baeab4a977.camel@cybertec.at Whole thread Raw |
In response to | Re: proposal: schema variables (Pavel Stehule <pavel.stehule@gmail.com>) |
Responses |
Re: proposal: schema variables
Re: proposal: schema variables |
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
Attachment
pgsql-hackers by date: