Re: Schema variables - new implementation for Postgres 15

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)