I have gone over patch 3 from the set and worked on the comments.
Apart from that, I have modified your patch as follows:
> +/*
> + * pg_session_variables - designed for testing
> + *
> + * This is a function designed for testing and debugging. It returns the
> + * content of sessionvars as-is, and can therefore display entries about
> + * session variables that were dropped but for which this backend didn't
> + * process the shared invalidations yet.
> + */
> +Datum
> +pg_session_variables(PG_FUNCTION_ARGS)
> +{
> +#define NUM_PG_SESSION_VARIABLES_ATTS 8
> +
> + elog(DEBUG1, "pg_session_variables start");
I don't think that message is necessary, particularly with DEBUG1.
I have removed this message and the "end" message as well.
> + while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
> + {
> + Datum values[NUM_PG_SESSION_VARIABLES_ATTS];
> + bool nulls[NUM_PG_SESSION_VARIABLES_ATTS];
> + HeapTuple tp;
> + bool var_is_valid = false;
> +
> + memset(values, 0, sizeof(values));
> + memset(nulls, 0, sizeof(nulls));
Instead of explicitly zeroing out the arrays, I have used an empty initializer
in the definition, like
bool nulls[NUM_PG_SESSION_VARIABLES_ATTS] = {};
That should have the same effect.
If you don't like that, I have no real problem with your original code.
> + values[0] = ObjectIdGetDatum(svar->varid);
> + values[3] = ObjectIdGetDatum(svar->typid);
You are using the type ID without checking if it exists in the catalog.
I think that is a bug.
There is a dependency between the variable and the type, but if a concurrent
session drops both the variable and the data type, the non-existing type ID
would still show up in the function output.
Even worse, the OID could have been reused for a different type since.
I am attaching just patch number 3 and leave you to adapt the patch set,
but I don't think any of the other patches should be affected.
Yours,
Laurenz Albe
From 422d0b6793b88951cd24a56ab45f5d7699e70c6b Mon Sep 17 00:00:00 2001
From: Laurenz Albe <[email protected]>
Date: Wed, 23 Oct 2024 06:56:02 +0300
Subject: [PATCH vv20241023] function pg_session_variables for cleaning tests
This is a function designed for testing and debugging. It returns the
content of sessionvars as-is, and can therefore display entries about
session variables that were dropped but for which this backend didn't
process the shared invalidations yet.
---
src/backend/commands/session_variable.c | 89 +++++++++++++++++++++++++
src/include/catalog/pg_proc.dat | 8 +++
2 files changed, 97 insertions(+)
diff --git a/src/backend/commands/session_variable.c b/src/backend/commands/session_variable.c
index 548d9835c0d..19f772a9fb6 100644
--- a/src/backend/commands/session_variable.c
+++ b/src/backend/commands/session_variable.c
@@ -569,3 +569,92 @@ ExecuteLetStmt(ParseState *pstate,
PopActiveSnapshot();
}
+
+/*
+ * pg_session_variables - designed for testing
+ *
+ * This is a function designed for testing and debugging. It returns the
+ * content of session variables as-is, and can therefore display data about
+ * session variables that were dropped, but for which this backend didn't
+ * process the shared invalidations yet.
+ */
+Datum
+pg_session_variables(PG_FUNCTION_ARGS)
+{
+#define NUM_PG_SESSION_VARIABLES_ATTS 8
+
+ InitMaterializedSRF(fcinfo, 0);
+
+ if (sessionvars)
+ {
+ ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+ HASH_SEQ_STATUS status;
+ SVariable svar;
+
+ hash_seq_init(&status, sessionvars);
+
+ while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
+ {
+ Datum values[NUM_PG_SESSION_VARIABLES_ATTS] = {};
+ bool nulls[NUM_PG_SESSION_VARIABLES_ATTS] = {};
+ HeapTuple tp;
+ bool var_is_valid = false;
+
+ values[0] = ObjectIdGetDatum(svar->varid);
+ values[3] = ObjectIdGetDatum(svar->typid);
+
+ /*
+ * It is possible that the variable has been dropped from the
+ * catalog, but not yet purged from the hash table.
+ */
+ tp = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(svar->varid));
+
+ if (HeapTupleIsValid(tp))
+ {
+ Form_pg_variable varform = (Form_pg_variable) GETSTRUCT(tp);
+
+ /*
+ * It is also possible that a variable has been dropped and
+ * someone created a new variable with the same object ID. Use
+ * the catalog information only if that is not the case.
+ */
+ if (svar->create_lsn == varform->varcreate_lsn)
+ {
+ values[1] = CStringGetTextDatum(
+ get_namespace_name(varform->varnamespace));
+
+ values[2] = CStringGetTextDatum(NameStr(varform->varname));
+ values[4] = CStringGetTextDatum(format_type_be(svar->typid));
+ values[5] = BoolGetDatum(false);
+
+ values[6] = BoolGetDatum(
+ object_aclcheck(VariableRelationId, svar->varid,
+ GetUserId(), ACL_SELECT) == ACLCHECK_OK);
+
+ values[7] = BoolGetDatum(
+ object_aclcheck(VariableRelationId, svar->varid,
+ GetUserId(), ACL_UPDATE) == ACLCHECK_OK);
+
+ var_is_valid = true;
+ }
+
+ ReleaseSysCache(tp);
+ }
+
+ /* if there is no matching catalog entry, return null values */
+ if (!var_is_valid)
+ {
+ nulls[1] = true;
+ nulls[2] = true;
+ nulls[4] = true;
+ values[5] = BoolGetDatum(true);
+ nulls[6] = true;
+ nulls[7] = true;
+ }
+
+ tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
+ }
+ }
+
+ return (Datum) 0;
+}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 01a118239ce..f60df4db58f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12366,4 +12366,12 @@
proargtypes => 'int2',
prosrc => 'gist_stratnum_identity' },
+# Session variables support
+{ oid => '8488', descr => 'list of used session variables',
+ proname => 'pg_session_variables', prorows => '1000', proretset => 't',
+ provolatile => 's', proparallel => 'r', prorettype => 'record',
+ proargtypes => '', proallargtypes => '{oid,text,text,oid,text,bool,bool,bool}',
+ proargmodes => '{o,o,o,o,o,o,o,o}',
+ proargnames => '{varid,schema,name,typid,typname,removed,can_select,can_update}',
+ prosrc => 'pg_session_variables' },
]
--
2.47.0
