hi.
some minor issues.
'transformMergeStmt also needs
"qry->hasSessionVariables = pstate->p_hasSessionVariables;"
?
yes, done
<function>acldefault</function> in doc/src/sgml/func.sgml
Need an update for SESSION VARIABLE?
Table 9.70. Access Privilege Inquiry Functions
sorting order: has_session_variable_privilege should after has_server_privilege?
swapped
Similar to src/test/regress/sql/event_trigger.sql,
we can use the following query to test four functions in
Table 9.79. Object Information and Addressing Functions
(9.27.5. Object Information and Addressing Functions)
for session variables.
SELECT
e.varname,
pg_describe_object('pg_variable'::regclass, e.oid, 0) as descr,
b.type, b.object_names, b.object_args,
pg_identify_object(a.classid, a.objid, a.objsubid) as ident
FROM pg_variable as e,
LATERAL pg_identify_object_as_address('pg_variable'::regclass, e.oid, 0) as b,
LATERAL pg_get_object_address(b.type, b.object_names, b.object_args) as a;
done
in function transformColumnRef
if (expr_kind_allows_session_variables(pstate->p_expr_kind))
{
}
can change to
if (!node &&
!(relname && crerr == CRERR_NO_COLUMN) &&
expr_kind_allows_session_variables(pstate->p_expr_kind))
{
}
can reduce the function call of expr_kind_allows_session_variables,
also not lose readability.
In this case, I prefer current code. It is more accented so the code is related to places where variables are allowed.
+ following patches like GUC-session_variables_ambiguity_warning or variable-fence-syntax~upport-and-variable-fence-usa
are less invasive and much more readable.
My opinion about it is not extra strong, and surely it can be subjective. I think the current form is quite well readable
and the proposed change is not significantly better.
typedef struct SVariableData says:
/*
* Stored value and type description can be outdated when we receive a
* sinval message. We then have to check if the stored data are still
* trustworthy.
*/
bool is_valid;
CREATE VARIABLE var3 AS int;
LET var3 = 10;
GRANT SELECT ON VARIABLE var3 TO regress_noowner;
I don't understand why the last statement GRANT SELECT needs to call
pg_variable_cache_callback?
and it will invalidate the original var3.
Reason is simple - you did the change of pg_variable. Unfortunately, the system of system cache invalidation is sometimes too simple and too aggressive.
Inside pg_variable_cache_callback I cannot identify if a related row in pg_variable was removed (by another session) or just updated.
Theoretically I can miss the information of what row was touched too. Important note - invalidation in this case doesn't means so variable or the
value of variable should be thrown. It just means, so before any usage of the current variable's value, we should recheck our cached data against
fresh syscache. Cache invalidation is relative common in Postgres, but validation is very quick - just the check of varcreate_lsn
+/*
+ * Returns a copy of the value of the session variable (in the current memory
+ * context). The caller is responsible for permission checks.
+ */
+Datum
+GetSessionVariable(Oid varid, bool *isNull, Oid *typid)
+{
+ SVariable svar;
+
+ svar = get_session_variable(varid);
+
+ /*
+ * Although "svar" is freshly validated in this point, svar->is_valid can
+ * be false, if an invalidation message ws processed during the domain check.
+ * But the variable and all its dependencies are locked now, so we don't need
+ * to repeat the validation.
+ */
+ Assert(svar);
+
+ *typid = svar->typid;
+
+ return copy_session_variable_value(svar, isNull);
+}
typo, "ws processed" should be "was processed".
fixed
also the Assert is not helpful? since svar is NULL,
get_session_variable would have segfault.
also SVariable svar; is not initialized to NULL, so generally it will
not be NULL by default.
assert removed
also the comments seem to imply that before
copy_session_variable_value, svar->is_valid can be false.
if svar->is_valid is false, then why do copy_session_variable_value.
When the flag is_valid is true, then we are sure, so the stored value is valid. When this flag is false, then we are not sure, so this value is valid. But inside one statement,
and if the variable was locked, then we don't need to repeat validation (if we did it immediately before), because we are sure, so the related system catalog should be valid, because it is locked, and then we can safely return the value. The flag is_valid can be changed any time when we are using catalog cache - in this case when we do domain check. So although it can look
weird after execution get_session_variable we can return the correct value, but the flag is_valid can be false. It doesn't mean, so the current value is wrong. It means, so the next command
should repeat validation.
In the attached version I rewrote the check if we can or cannot to ACL SELECT check for variable used as base node of assignment indirection. I significantly simplified the code - it is based on fact so we know varid of the base node variable. It should be the same session variable like resultVariable. Then I don't need to use bitmapset of used session variables. Just I can check if this variable is used by param that is not basenode param.
Regards
Pavel