Re: Re: proposal: schema variables - Mailing list pgsql-hackers

From Pavel Stehule
Subject Re: Re: proposal: schema variables
Date
Msg-id CAFj8pRBxA868TpLDe9ofXdpVUNmHY8pzkxrjbZ0obCe0g+YZ-Q@mail.gmail.com
Whole thread Raw
In response to Re: proposal: schema variables  (Dmitry Dolgov <9erthalion6@gmail.com>)
Responses Re: Re: proposal: schema variables
List pgsql-hackers
Hi

pá 27. 12. 2024 v 16:20 odesílatel jian he <jian.universality@gmail.com> napsal:
hi.

+ if (!OidIsValid(varid))
+ AcceptInvalidationMessages();
+ else if (OidIsValid(varid))
+ LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);

we can change it to
+ if (!OidIsValid(varid))
+ AcceptInvalidationMessages();
+ else
+ LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);

done
 

inval_count didn't explain at all, other places did actually explain it.
Can we add some text explaining inval_count? (i didn't fully understand
this part, that is why i am asking..)

done
 

seems IdentifyVariable all these three ereport(ERROR...) don't have
regress tests,
i think we should have it. Am I missing something?

done

create variable v2 as int;
let v2.a = 1;
ERROR:  type "integer" of target session variable "public.v2" is not a
composite type
LINE 1: let v2.a = 1;
            ^
the error messages look weird.
IMO, it should either be
"type of session variable "public.v2" is not a composite type"
or
"session variable "public.v2" don't have attribute "a"

changed

I did inspiration from transformAssignmentIndirection now

(2024-12-28 16:07:57) postgres=# let x.a = 20;
ERROR:  cannot assign to field "a" of session variable "public.x" because its type integer is not a composite type
LINE 1: let x.a = 20;
            ^

also, the above can be as a regress test for:
+ if (attrname && !is_rowtype)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("type \"%s\" of target session variable \"%s.%s\" is not a
composite type",
+ format_type_be(typid),
+ get_namespace_name(get_session_variable_namespace(varid)),
+ get_session_variable_name(varid)),
+ parser_errposition(pstate, stmt->location)));
since we don't have coverage tests for it.


done
 

+ if (coerced_expr == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("variable \"%s.%s\" is of type %s,"
+ " but expression is of type %s",
+ get_namespace_name(get_session_variable_namespace(varid)),
+ get_session_variable_name(varid),
+ format_type_be(typid),
+ format_type_be(exprtypid)),
+ errhint("You will need to rewrite or cast the expression."),
+ parser_errposition(pstate, exprLocation((Node *) expr))));

generally, errmsg(...) should put it into one line for better grep-ability
so we can change it to:
+errmsg("variable \"%s.%s\" is of type %s, but expression is of type %s"...)

done
 

also no coverage tests?
simple test case for it:
create variable v2 as int;
let v2 = '1'::jsonb;

done
 

---------------<<<>>>--------------
+let_target:
+ ColId opt_indirection
+ {
+ $$ = list_make1(makeString($1));
+ if ($2)
+  $$ = list_concat($$,
+   check_indirection($2, yyscanner));
+ }
if you look closely, it seems the indentation level is wrong in
line "$$ = list_concat($$,"?

let_target is not needed as separate rule now, so I removed it and little bit cleaned (really only little bit)
 

---------------<<<>>>--------------
i did some refactoring in session_variables.sql for privilege check.
make the tests more neat, please check attached.

merged with minor changes in comment's formatting

I'll send the patch set with next mail

Best regards

Pavel

pgsql-hackers by date:

Previous
From: Tomas Vondra
Date:
Subject: Re: Add the ability to limit the amount of memory that can be allocated to backends.
Next
From: Marcos Pegoraro
Date:
Subject: Re: Document NULL