Re: Schema variables - new implementation for Postgres 15 - Mailing list pgsql-hackers

Hi Pavel,

On Sun, Aug 21, 2022 at 09:54:03AM +0200, Pavel Stehule wrote:
>
> should be fixed now

I started reviewing the patchset, beginning with 0001 (at least the parts that
don't substantially change later) and have a few comments.

- you define new AclMode READ and WRITE.  Those bits are precious and I don't
  think it's ok to consume 2 bits for session variables, especially since those
  are the last two bits available since the recent GUC access control patch
  (ACL_SET and ACL_ALTER_SYSTEM).  Maybe we could existing INSERT and UPDATE
  privileges instead, like it's done for sequences?

- make check and make-check-world don't pass with this test only.  Given that
  the split is mostly done to ease review and probably not intended to be
  committed this way, we probably shouldn't spend efforts to clean up the split
  apart from making sure that each patch compiles cleanly on its own.  But in
  this case it brought my attention to misc_sanity.sql test.  Looking at patch
  0010, I see:

diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out
index a57fd142a9..ce9bad7211 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -60,7 +60,9 @@ ORDER BY 1, 2;
  pg_index                | indpred       | pg_node_tree
  pg_largeobject          | data          | bytea
  pg_largeobject_metadata | lomacl        | aclitem[]
-(11 rows)
+ pg_variable             | varacl        | aclitem[]
+ pg_variable             | vardefexpr    | pg_node_tree
+(13 rows)

This is the test for relations with varlena columns without TOAST table.  I
don't think that's correct to add those exceptions, and there should be a TOAST
table declared for pg_variable too, as noted in the comment above that query.

- nitpicking: s/catalogue/catalog/

Some other comments on other patches while testing things around:

- For sessionvariable.c (in 0002), I see that there are still all the comments
  and code about checking type validity based on a generation number and other
  heuristics.  I still fail to understand why this is needed at all as the
  stored datum should remain compatible as long as we prevent the few
  incompatible DDL that are also prevented when there's a relation dependency.
  As an example, I try to quickly disable all that code with the following:

diff --git a/src/backend/commands/sessionvariable.c b/src/backend/commands/sessionvariable.c
index 9b4f9482a4..7c8808dc46 100644
--- a/src/backend/commands/sessionvariable.c
+++ b/src/backend/commands/sessionvariable.c
@@ -794,6 +794,8 @@ svartype_verify_composite_fast(SVariableType svt)
 static int64
 get_svariable_valid_type_gennum(SVariableType svt)
 {
+   return 1;
+
    HeapTuple   tuple;
    bool        fast_check = true;

@@ -905,6 +907,8 @@ get_svariabletype(Oid typid)
 static bool
 session_variable_use_valid_type(SVariable svar)
 {
+   return true;
+
    Assert(svar);
    Assert(svar->svartype);

And session_variable.sql regression test still works just fine.  Am I missing
something?

While at it, the initial comment should probably say "free local memory" rather
than "purge memory".

- doc are missing for GRANT/REVOKE ... ON ALL VARIABLES

- plpgsql.sgml:
+   <sect3>
+    <title><command>Session variables and constants</command></title>

I don't think it's ok to use "constant" as an alias for immutable session
variable as immutable session variable can actually be changed.

Similarly, in catalogs.sgml:

+       <structfield>varisimmutable</structfield> <type>boolean</type>
+      </para>
+      <para>
+       True if the variable is immutable (cannot be modified). The default value is false.
+      </para></entry>
+     </row>

I think there should be a note and a link to the corresponding part in
create_variable.sgml to explain what exactly is an immutable variable, as the
implemented behavior (for nullable immutable variable) is somewhat unexpected.

- other nitpicking: pg_variable and struct Variable seems a bit inconsistent.
  For instance one uses vartype and vartypmod and the other typid and typmod,
  while both use varname and varnamespace.  I think we should avoid discrepancy
  here.

Also, there's a sessionvariable.c and a session_variable.h.  Let's use
session_variable.[ch], as it seems more readable?

-typedef patch: missing SVariableTypeData, some commits need a pgindent, e.g:

+typedef SVariableTypeData * SVariableType;

+typedef SVariableData * SVariable;

+static SessionVariableValue * RestoreSessionVariables(char **start_address,
+                                                   int *num_session_variables);

+static Query *transformLetStmt(ParseState *pstate,
+                              LetStmt * stmt);

(and multiple others)