Missing can't-assign-to-constant checks in plpgsql
I happened to notice that there are a couple of places in plpgsql
that will let you assign a new value to a variable that's marked
CONSTANT:
* We don't complain if an output parameter in a CALL statement
is constant.
* We don't complain if a refcursor variable is constant, even
though OPEN may assign a new value to it.
The attached quick-hack patch closes both of these oversights.
Perhaps the OPEN change is a little too aggressive, since if
you give the refcursor variable some non-null initial value,
OPEN won't change it; in that usage a CONSTANT marking could
be allowed. But I really seriously doubt that anybody out
there is marking such variables as constants, so I thought
throwing the error at compile time was better than postponing
it to runtime so we could handle that.
Regardless of which way we handle that point, I'm inclined to
change this only in HEAD. Probably people wouldn't thank us
for making the back branches more strict.
regards, tom lane
PS: I didn't do it here, but I'm kind of tempted to pull out
all the cursor-related tests in plpgsql.sql and move them to
a new test file under src/pl/plpgsql/src/sql/. They look
pretty self-contained, and I doubt they're worth keeping in
the core tests.
Attachments:
plpgsql-add-missing-checks-for-constant-1.patchtext/x-diff; charset=us-ascii; name=plpgsql-add-missing-checks-for-constant-1.patchDownload+95-1
čt 28. 4. 2022 v 23:52 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
I happened to notice that there are a couple of places in plpgsql
that will let you assign a new value to a variable that's marked
CONSTANT:* We don't complain if an output parameter in a CALL statement
is constant.* We don't complain if a refcursor variable is constant, even
though OPEN may assign a new value to it.The attached quick-hack patch closes both of these oversights.
Perhaps the OPEN change is a little too aggressive, since if
you give the refcursor variable some non-null initial value,
OPEN won't change it; in that usage a CONSTANT marking could
be allowed. But I really seriously doubt that anybody out
there is marking such variables as constants, so I thought
throwing the error at compile time was better than postponing
it to runtime so we could handle that.Regardless of which way we handle that point, I'm inclined to
change this only in HEAD. Probably people wouldn't thank us
for making the back branches more strict.
+1
I can implement these checks in plpgsql_check. So possible issues can be
detected and fixed on older versions by using plpgsql_check.
Regards
Pavel
Show quoted text
regards, tom lane
PS: I didn't do it here, but I'm kind of tempted to pull out
all the cursor-related tests in plpgsql.sql and move them to
a new test file under src/pl/plpgsql/src/sql/. They look
pretty self-contained, and I doubt they're worth keeping in
the core tests.
Pavel Stehule <pavel.stehule@gmail.com> writes:
čt 28. 4. 2022 v 23:52 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
Perhaps the OPEN change is a little too aggressive, since if
you give the refcursor variable some non-null initial value,
OPEN won't change it; in that usage a CONSTANT marking could
be allowed. But I really seriously doubt that anybody out
there is marking such variables as constants, so I thought
throwing the error at compile time was better than postponing
it to runtime so we could handle that.Regardless of which way we handle that point, I'm inclined to
change this only in HEAD. Probably people wouldn't thank us
for making the back branches more strict.
+1
After sleeping on it, I got cold feet about breaking arguably
legal code, so I made OPEN check at runtime instead. Which
was probably a good thing anyway, because it made me notice
that exec_stmt_forc() needed a check too. AFAICS there are no
other places in pl_exec.c that are performing assignments to
variables not checked at parse time.
Pushed that way.
regards, tom lane
Hi
Regardless of which way we handle that point, I'm inclined to
change this only in HEAD. Probably people wouldn't thank us
for making the back branches more strict.+1
I can implement these checks in plpgsql_check. So possible issues can be
detected and fixed on older versions by using plpgsql_check.
new related checks are implemented on plpgsql_check 2.1.4
Regards
Pavel
Show quoted text
Regards
Pavel
regards, tom lane
PS: I didn't do it here, but I'm kind of tempted to pull out
all the cursor-related tests in plpgsql.sql and move them to
a new test file under src/pl/plpgsql/src/sql/. They look
pretty self-contained, and I doubt they're worth keeping in
the core tests.