Re: Re: proposal: schema variables

Hi

út 7. 1. 2025 v 10:07 odesílatel jian he <jian(dot)universality(at)gmail(dot)com>
napsal:

> 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