public inbox for [email protected]
help / color / mirror / Atom feedFrom: Laurenz Albe <[email protected]>
To: Pavel Stehule <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: DUVAL REMI <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: proposal: schema variables
Date: Wed, 23 Oct 2024 07:11:08 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAFj8pRBWMLP3Vyr8z+19eaiJKQoVtBfmDhNJFKXDX6uFzd4vBQ@mail.gmail.com>
References: <CAFj8pRBoGBKopkiTa4ki3dMgy-cSTRZ-BQPKOq7=Tk0SSNsowA@mail.gmail.com>
<CAFj8pRBCiWn12H9FHymYs17fk68nRd9Xpn+SYf18TLdb2YSUrQ@mail.gmail.com>
<CAA4eK1JV-Ox0oRFdXnPqXSzM84wTR5QFkRzCpNVF_+0FNjS5Mg@mail.gmail.com>
<CAFj8pRByCCcgDkeXyafAnH6LgxtAyCVwg6yfJAhyTY6GLscfZg@mail.gmail.com>
<CAFj8pRBvUonC_ug3F=w1Q55Dp=DggojvAeL7Vmh14Q-WhFHxzw@mail.gmail.com>
<CAFj8pRDj72P-f=SUygtOXnTOBQ0RzmL_fN=wLfaCzcbPVpGgzw@mail.gmail.com>
<CAFj8pRDD2GQaJ_iDT4vSVe658+oHRXU2S2af7Y1-it9jaP8VFg@mail.gmail.com>
<[email protected]>
<CAFj8pRAFktynx5wkanv5SRuzXkZgXu77XpVACiSE=v7i1xHFbw@mail.gmail.com>
<CAFj8pRA=bn_g5T2AZduy5gNOQoOnUJ+pMHmnRMHi6mR0n=TAsA@mail.gmail.com>
<[email protected]>
<CAFj8pRC9de05HSb4tEHDUwJ98+4Wh30W-rJrNOPnTz6ARcv0Fw@mail.gmail.com>
<CAFj8pRBC5Wz1xHKKBmKsM0xYN0+PdSZ5oXPsk5SZt+VprdUW3A@mail.gmail.com>
<CAFj8pRAh4pzMoZrKCLt9h+Lr2L=vhgs2PjAF45uLbp_7sijM5w@mail.gmail.com>
<CAFj8pRA-kxQ1oErcuDeUKYsrgwB5XGLhquatwxOe3dCVy1gcyQ@mail.gmail.com>
<[email protected]>
<CAFj8pRBbt2xhY9PyabOY0ZN+Aig6ee3oCon-DM9qi0Uw_3qfbw@mail.gmail.com>
<CAFj8pRDSa52J7kPmCYXgq1BBbu3YBXwpdSOVpjgU=hnE2k04Cw@mail.gmail.com>
<CAFj8pRD+QiWOoPrFk2NnPs3t5Eaf4X=aGRV-9ww11cnPP+fV4g@mail.gmail.com>
<[email protected]>
<CAFj8pRCGTjqHvH9oeiSf4T6Bydhk9pm033DxxibgF+B7SHC6MQ@mail.gmail.com>
<CAFj8pRAzNDhFgbZnT0T0mJ7ygA1Qje1Hc0TiKwXM8++kGooPYg@mail.gmail.com>
<[email protected]>
<CAFj8pRBarjJYfkN-0-i=JRZJ4PTOYC+K7XgAhfdDqWGqRiPkyw@mail.gmail.com>
<CAFj8pRDqdWdCULxd5asbKs5C4e9kT2TuKBkR5L-e1=hP5wF2uw@mail.gmail.com>
<[email protected]>
<CAFj8pRCPW56pFr0F0BcasdXjFeo3SFixNSpWKaBk0ibvznum-A@mail.gmail.com>
<CAFj8pRD1Feit93CgwmYm1Q=X+M+AZqffCEZPFQ7qEMNHZRN4fA@mail.gmail.com>
<CAFj8pRCc=B9-FRQg5eWDSkGwS2vpkq88hR6042cmPPizHuEGSA@mail.gmail.com>
<CAFj8pRBk8x7afUXKLBOU-Ctg6A7QJvTAGGVEi0b6Jc8YTe8nUg@mail.gmail.com>
<CAFj8pRCSwHQ4BJUbjF2YEausK1Z6+ejMyedpqAnWJbG+FEJDLw@mail.gmail.com>
<CAFj8pRAbY+N+UqjqgESL5x-bsGmV+aVyyUkxUSgaGDZToZjDqQ@mail.gmail.com>
<CAFj8pRBzKcqzj=23BHfv1QaXHt=2_SN=uhdR3rb_dAVQoit7ug@mail.gmail.com>
<CAFj8pRCi-n6SzkAB+OHG=TZvL13xxta_qgffBLDOY0HEBqDhvg@mail.gmail.com>
<[email protected]>
<CAFj8pRBaD0_bMrCREWnVLfcTMdc0v7ns7Rt=sEvd1EoFmLfarQ@mail.gmail.com>
<[email protected]>
<CAFj8pRDXo-RRcy2VFDm_vzv3Eaaz6Ex=X19up=x8W4COyBNmaQ@mail.gmail.com>
<[email protected]>
<CAFj8pRDgraz64HHcCoFGCKDM1Cxv1ukELsqfsxux4JP3PM6RyQ@mail.gmail.com>
<[email protected]>
<CAFj8pRAsjr0pzQpQeRrQ-wR-ew1oTc6qkW=4355KveN1jKQ+1Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAFj8pRDF-F5diNTXr206QfQFauFNK1vu_7NARXd0dtdwGTWLMQ@mail.gmail.com>
<[email protected]>
<CAFj8pRCy8NTG3ewOrH0KLQgrMpVMSzPjhoLKW=c1Jjz8QWNSKw@mail.gmail.com>
<CAFj8pRB-ey7mCnKqMhBBrkv2mXOvTLfAEBy2LZxQsWPCBvguGQ@mail.gmail.com>
<[email protected]>
<CAFj8pRBvUwfs96MX2_=unHQ107diUOK4S_WD=E4XBXkpXG3VjA@mail.gmail.com>
<CAFj8pRCDWjgo=yLEK-jRDe5VwuNSQSmMqPPmysQf9nHA8gifnw@mail.gmail.com>
<CAFj8pRA2jbXRTGgM1co8gLCNttgaH8miRUSSauMJKWgousDsDg@mail.gmail.com>
<CAFj8pRA6nJRg8oq5k_Cdk5oH5_FjtViG8XLmHKrkqOuiMT_CKQ@mail.gmail.com>
<CAFj8pRBWMLP3Vyr8z+19eaiJKQoVtBfmDhNJFKXDX6uFzd4vBQ@mail.gmail.com>
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
Attachments:
[text/x-patch] v20241023-0003-function-pg_session_variables-for-cleanin.patch (4.2K, 2-v20241023-0003-function-pg_session_variables-for-cleanin.patch)
download | inline diff:
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
view thread (439+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: proposal: schema variables
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox