Re: proposal: schema variables

Hi

so 27. 7. 2024 v 16:19 odesílatel Laurenz Albe <laurenz.albe@cybertec.at>
napsal:

> 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?
>

No, I just used a pattern for relations. I removed the test against
PG_CATALOG_NAMESPACE and wrote comment

<-->/*
<--> * Quick check: if it ain't in the path at all, it ain't visible. We
<--> * don't expect usage of session variables in the system namespace.
<--> */




>
> > --- 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.
>

merged


>
> > --- /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.
>

merged


>
> > --- 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.
>

sure


>
> > --- 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?
>

This test is not related to pipe, but usage of invalid multipart names

These tests look weird, but when you look at the psql.out file, then it is
consistent with others.

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?
>

It is crash psql parser test - these tests are designed like \dt or \di


>
>
> 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.
>

I am sending patches with merged your changes (related to first patch)

Regards

Pavel



>
> Yours,
> Laurenz Albe
>