Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tRYoH-00ALsH-5o for pgsql-hackers@arkaria.postgresql.org; Sat, 28 Dec 2024 15:33:01 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1tRYoF-000nQB-IJ for pgsql-hackers@arkaria.postgresql.org; Sat, 28 Dec 2024 15:32:59 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tRYoF-000nQ2-4u for pgsql-hackers@lists.postgresql.org; Sat, 28 Dec 2024 15:32:58 +0000 Received: from mail-yb1-xb2e.google.com ([2607:f8b0:4864:20::b2e]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1tRYoB-0023Ps-8W for pgsql-hackers@lists.postgresql.org; Sat, 28 Dec 2024 15:32:56 +0000 Received: by mail-yb1-xb2e.google.com with SMTP id 3f1490d57ef6-e53537d8feeso8747240276.0 for ; Sat, 28 Dec 2024 07:32:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1735399974; x=1736004774; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=URmK03dERsECY/jVlrT0TCIMNZaELdXeoFvDgBxql3c=; b=c4/qWOMtGQ1Cn+cCqi8gWeW7r6d+nMwsSbqxty9pIiQbSPxjBNZsj3ADLvvvluZpyp y4eHaRzUN39Oftjv9CmkZCnAE8iL3FppZgLTDwm21MR8lvP9uDET2EjrxRNuLY+qfAY6 xkH8aJhoLDvOHG2Jf4T1i/VpsjpT3fAgQJ1W+INqYtcIAiTIKZ963xzQhI1rt+7yDQEL Ypl8T7SpRK+omE3UEJ8f1pKUHfn+x5H4f+Jc2woBXtG3Pc0Iyi8sg/ArwD70kK47Hx+y MsASOShbxA6ybql4P5PZLM745Sp6w2zxbyebwGy0zVm3g5rfGJNqzcBL6TyEWbwjHA6w zAOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735399974; x=1736004774; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=URmK03dERsECY/jVlrT0TCIMNZaELdXeoFvDgBxql3c=; b=hjEmBEP9CJflPrEsfTSs9m05fqcqsZzaMl1mzdr9chbA9MoCAiQAcJpyYFXKBsiYnN r3TKumGXA4oztASgPk1sQ9Hvnd7nFaFrpYI2ysrHrQKhHZDFnZPIzHDGXqQe6LY1VVvd DDXms+WZHVAbDYLo374rTiDhYnyDZl1YxrH8D02p95oSn3V2gSynrIWZB8dqdy7n7LD3 Dd8cpK3VWSp8llme+M2qsxsIiNaEBt+MSNuacrvaYrZqAjmWq4M5RGloKljYKIrGhGjz ruf3xnNctv+WkjBxWa7Sj/MLDc5x5YQ8DE2Mq09xew6U9WkJnX+yqQftLQHxo+Y14Eve lhMg== X-Forwarded-Encrypted: i=1; AJvYcCXcDA+AEdUA6QencynQQtjSmSollQrO1NGNQ29fJvb/f64XRUylwR/ckhye7Py58PTma45FmGME0c6SwzeO@lists.postgresql.org X-Gm-Message-State: AOJu0Yx8VW3ClrRqMWUoRiw5f8NGjyiPoTSpFy+RX/0vsLYxKZx/le6d R/LFzf+TEKqTrBa2q3hPWy8oJmegIu6a0AGpRghGRJbvtQLaI0m3bjLyXhWivmxMZHmelaLCLzn GQZg2dQqZIgUMfjhQHNZMS0kFtrc= X-Gm-Gg: ASbGncv1xTqWTaWmbMVoBM+8/19SngIaA9oM6EllnTjrsCAxR9a1y8LiIXwjxSVvcIZ zdRziL2LUZ9mDN03B4/BiYuaNqcARSmZaF7lru7Ha1iXUtm3/vqAWrTdFwrZAIAt3IUF0Y+Q= X-Google-Smtp-Source: AGHT+IHYxkoMUXhwCMFVQp46mzeLrwf/CAElPLbD+G6igMsYHRdil7jBmP9kwrkAoUIdGUsyuNoiF+qaDWZphNsCzVw= X-Received: by 2002:a05:690c:6187:b0:6ee:a81e:6191 with SMTP id 00721157ae682-6f3f815b29fmr237022337b3.22.1735399974009; Sat, 28 Dec 2024 07:32:54 -0800 (PST) MIME-Version: 1.0 References: <3chredgnjcmccym2kczawfih226b4ac6co7p6z4jeofevrcosi@mrsxkx2x2c65> <20241120201313.t4wbhld4ktgielaf@erthalion.local> In-Reply-To: From: Pavel Stehule Date: Sat, 28 Dec 2024 16:32:17 +0100 Message-ID: Subject: Re: Re: proposal: schema variables To: jian he Cc: Dmitry Dolgov <9erthalion6@gmail.com>, Laurenz Albe , Erik Rijkers , Michael Paquier , Amit Kapila , DUVAL REMI , PostgreSQL Hackers Content-Type: multipart/alternative; boundary="000000000000fe8567062a564ae7" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000fe8567062a564ae7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi p=C3=A1 27. 12. 2024 v 16:20 odes=C3=ADlatel jian he 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 =3D 1; > ERROR: type "integer" of target session variable "public.v2" is not a > composite type > LINE 1: let v2.a =3D 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=3D# let x.a =3D 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 =3D 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 =3D=3D 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-abilit= y > 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 =3D '1'::jsonb; > done > > ---------------<<<>>>-------------- > +let_target: > + ColId opt_indirection > + { > + $$ =3D list_make1(makeString($1)); > + if ($2) > + $$ =3D list_concat($$, > + check_indirection($2, yyscanner)); > + } > if you look closely, it seems the indentation level is wrong in > line "$$ =3D 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 --000000000000fe8567062a564ae7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

p=C3=A1 27. 12. 2024 v=C2=A016= :20 odes=C3=ADlatel 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
=C2=A0

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..)

d= one
=C2=A0

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

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
=C2=A0

+ if (coerced_expr =3D=3D 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<= br> so we can change it to:
+errmsg("variable \"%s.%s\" is of type %s, but expression is= of type %s"...)

done
= =C2=A0

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

done
=C2=A0

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

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

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

<= /div>
merged with minor changes in comment's formatting
<= div>
I'll send the patch set with next mail
Best regards

Pavel
--000000000000fe8567062a564ae7--