Re: Re: proposal: schema variables

Hi

pá 27. 12. 2024 v 16:20 odesílatel jian he <jian(dot)universality(at)gmail(dot)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