Content-Type: multipart/mixed; boundary="------------BvuBQMTan0qLjF7e26PDWTlF"
Message-ID: <389c986d-fbb4-c644-9280-db7836af7ca9@postgrespro.ru>
Date: Thu, 15 Jun 2023 13:22:28 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Content-Language: en-US
To: pgsql-hackers@lists.postgresql.org
From: Yura Sokolov <y.sokolov@postgrespro.ru>
Subject: When IMMUTABLE is not.
Archived-At: <https://www.postgresql.org/message-id/389c986d-fbb4-c644-9280-db7836af7ca9%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------BvuBQMTan0qLjF7e26PDWTlF
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Good day, hackers.

I found, than declaration of function as IMMUTABLE/STABLE is not enough to be sure
function doesn't manipulate data.

In fact, SPI checks only direct function kind, but fails to check indirect call.

Attached immutable_not.sql creates 3 functions:

- `immutable_direct` is IMMUTABLE and tries to insert into table directly.
   PostgreSQL correctly detects and forbids this action.

- `volatile_direct` is VOLATILE and inserts into table directly.
   It is allowed and executed well.

- `immutable_indirect` is IMMUTABLE and calls `volatile_direct`.
   PostgreSQL failed to detect and prevent this DML manipulation.

Output:

select immutable_direct('immutable_direct'); psql:immutable_not.sql:28: 
ERROR:  INSERT is not allowed in a non-volatile function CONTEXT:  SQL 
statement "insert into xxx values(j)" PL/pgSQL function 
immutable_direct(character varying) line 3 at SQL statement select 
volatile_direct('volatile_direct'); volatile_direct ----------------- 
volatile_direct (1 row) select immutable_indirect('immutable_indirect'); 
immutable_indirect -------------------- immutable_indirect (1 row) 
select * from xxx;         i -------------------- volatile_direct 
immutable_indirect (2 rows) Attached forbid-non-volatile-mutations.diff 
add checks readonly function didn't made data manipulations. Output for 
patched version: select immutable_indirect('immutable_indirect'); 
psql:immutable_not.sql:32: ERROR:  Damn2! Update were done in a 
non-volatile function CONTEXT:  SQL statement "SELECT 
volatile_direct(j)" PL/pgSQL function immutable_indirect(character 
varying) line 3 at PERFORM I doubt check should be done this way. This 
check is necessary, but it should be FATAL instead of ERROR. And ERROR 
should be generated at same place, when it is generated for 
`immutable_direct`, but with check of "read_only" status through whole 
call stack instead of just direct function kind. ----- regards, Yura 
Sokolov Postgres Professional

--------------BvuBQMTan0qLjF7e26PDWTlF
Content-Type: application/sql; name="immutable_not.sql"
Content-Disposition: attachment; filename="immutable_not.sql"
Content-Transfer-Encoding: base64

ZHJvcCB0YWJsZSBpZiBleGlzdHMgeHh4OwpjcmVhdGUgdGFibGUgeHh4IChpIHZhcmNoYXIp
OwoKY3JlYXRlIG9yIHJlcGxhY2UgZnVuY3Rpb24gaW1tdXRhYmxlX2RpcmVjdChqIHZhcmNo
YXIpCnJldHVybnMgdmFyY2hhciBJTU1VVEFCTEUKbGFuZ3VhZ2UgcGxwZ3NxbCBhcyAkJApi
ZWdpbgoJaW5zZXJ0IGludG8geHh4IHZhbHVlcyhqKTsKCXJldHVybiBqOwplbmQgJCQ7Cgpj
cmVhdGUgb3IgcmVwbGFjZSBmdW5jdGlvbiB2b2xhdGlsZV9kaXJlY3QoaiB2YXJjaGFyKQpy
ZXR1cm5zIHZhcmNoYXIgVk9MQVRJTEUKbGFuZ3VhZ2UgcGxwZ3NxbCBhcyAkJApiZWdpbgoJ
aW5zZXJ0IGludG8geHh4IHZhbHVlcyhqKTsKCXJldHVybiBqOwplbmQgJCQ7CgpjcmVhdGUg
b3IgcmVwbGFjZSBmdW5jdGlvbiBpbW11dGFibGVfaW5kaXJlY3QoaiB2YXJjaGFyKQpyZXR1
cm5zIHZhcmNoYXIgSU1NVVRBQkxFCmxhbmd1YWdlIHBscGdzcWwgYXMgJCQKYmVnaW4KCXBl
cmZvcm0gdm9sYXRpbGVfZGlyZWN0KGopOwoJcmV0dXJuIGo7CmVuZCAkJDsKCnNlbGVjdCBp
bW11dGFibGVfZGlyZWN0KCdpbW11dGFibGVfZGlyZWN0Jyk7CgpzZWxlY3Qgdm9sYXRpbGVf
ZGlyZWN0KCd2b2xhdGlsZV9kaXJlY3QnKTsKCnNlbGVjdCBpbW11dGFibGVfaW5kaXJlY3Qo
J2ltbXV0YWJsZV9pbmRpcmVjdCcpOwoKc2VsZWN0ICogZnJvbSB4eHg7Cg==
--------------BvuBQMTan0qLjF7e26PDWTlF
Content-Type: text/x-patch; charset=UTF-8;
 name="forbid-non-volatile-mutations.diff"
Content-Disposition: attachment; filename="forbid-non-volatile-mutations.diff"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL3NwaS5jIGIvc3JjL2JhY2tlbmQv
ZXhlY3V0b3Ivc3BpLmMKaW5kZXggMzM5NzU2ODdiMzguLjgyYjYxMjdkNjUwIDEwMDY0NAot
LS0gYS9zcmMvYmFja2VuZC9leGVjdXRvci9zcGkuYworKysgYi9zcmMvYmFja2VuZC9leGVj
dXRvci9zcGkuYwpAQCAtMTU4NCw2ICsxNTg0LDcgQEAgU1BJX2N1cnNvcl9vcGVuX2ludGVy
bmFsKGNvbnN0IGNoYXIgKm5hbWUsIFNQSVBsYW5QdHIgcGxhbiwKIAlQb3J0YWwJCXBvcnRh
bDsKIAlTUElDYWxsYmFja0FyZyBzcGljYWxsYmFja2FyZzsKIAlFcnJvckNvbnRleHRDYWxs
YmFjayBzcGllcnJjb250ZXh0OworCUNvbW1hbmRJZCAJdGhpc19jb21tYW5kID0gSW52YWxp
ZENvbW1hbmRJZDsKIAogCS8qCiAJICogQ2hlY2sgdGhhdCB0aGUgcGxhbiBpcyBzb21ldGhp
bmcgdGhlIFBvcnRhbCBjb2RlIHdpbGwgc3BlY2lhbC1jYXNlIGFzCkBAIC0xNzMwLDYgKzE3
MzEsNyBAQCBTUElfY3Vyc29yX29wZW5faW50ZXJuYWwoY29uc3QgY2hhciAqbmFtZSwgU1BJ
UGxhblB0ciBwbGFuLAogCWlmIChyZWFkX29ubHkpCiAJewogCQlMaXN0Q2VsbCAgICpsYzsK
KwkJdGhpc19jb21tYW5kID0gR2V0Q3VycmVudENvbW1hbmRJZChmYWxzZSk7CiAKIAkJZm9y
ZWFjaChsYywgc3RtdF9saXN0KQogCQl7CkBAIC0xNzc4LDYgKzE3ODAsMTQgQEAgU1BJX2N1
cnNvcl9vcGVuX2ludGVybmFsKGNvbnN0IGNoYXIgKm5hbWUsIFNQSVBsYW5QdHIgcGxhbiwK
IAkvKiBQb3AgdGhlIFNQSSBzdGFjayAqLwogCV9TUElfZW5kX2NhbGwodHJ1ZSk7CiAKKwlp
ZiAocmVhZF9vbmx5ICYmIHRoaXNfY29tbWFuZCAhPSBHZXRDdXJyZW50Q29tbWFuZElkKGZh
bHNlKSkKKwl7CisJCWVyZXBvcnQoRVJST1IsCisJCQkJKGVycmNvZGUoRVJSQ09ERV9GRUFU
VVJFX05PVF9TVVBQT1JURUQpLAorCQkJCQkJLyogdHJhbnNsYXRvcjogJXMgaXMgYSBTUUwg
c3RhdGVtZW50IG5hbWUgKi8KKwkJCQkJCWVycm1zZygiRGFtbjEhIFVwZGF0ZSB3ZXJlIGRv
bmUgaW4gYSBub24tdm9sYXRpbGUgZnVuY3Rpb24iKSkpOworCX0KKwogCS8qIFJldHVybiB0
aGUgY3JlYXRlZCBwb3J0YWwgKi8KIAlyZXR1cm4gcG9ydGFsOwogfQpAQCAtMjQwNCw2ICsy
NDE0LDcgQEAgX1NQSV9leGVjdXRlX3BsYW4oU1BJUGxhblB0ciBwbGFuLCBjb25zdCBTUElF
eGVjdXRlT3B0aW9ucyAqb3B0aW9ucywKIAlFcnJvckNvbnRleHRDYWxsYmFjayBzcGllcnJj
b250ZXh0OwogCUNhY2hlZFBsYW4gKmNwbGFuID0gTlVMTDsKIAlMaXN0Q2VsbCAgICpsYzE7
CisJQ29tbWFuZElkICAgdGhpc19jb21tYW5kID0gSW52YWxpZENvbW1hbmRJZDsKIAogCS8q
CiAJICogU2V0dXAgZXJyb3IgdHJhY2ViYWNrIHN1cHBvcnQgZm9yIGVyZXBvcnQoKQpAQCAt
MjQ3Myw2ICsyNDg0LDExIEBAIF9TUElfZXhlY3V0ZV9wbGFuKFNQSVBsYW5QdHIgcGxhbiwg
Y29uc3QgU1BJRXhlY3V0ZU9wdGlvbnMgKm9wdGlvbnMsCiAJCQkJKGVycmNvZGUoRVJSQ09E
RV9TWU5UQVhfRVJST1IpLAogCQkJCSBlcnJtc2coImVtcHR5IHF1ZXJ5IGRvZXMgbm90IHJl
dHVybiB0dXBsZXMiKSkpOwogCisJaWYgKG9wdGlvbnMtPnJlYWRfb25seSkKKwl7CisJCXRo
aXNfY29tbWFuZCA9IEdldEN1cnJlbnRDb21tYW5kSWQoZmFsc2UpOworCX0KKwogCWZvcmVh
Y2gobGMxLCBwbGFuLT5wbGFuY2FjaGVfbGlzdCkKIAl7CiAJCUNhY2hlZFBsYW5Tb3VyY2Ug
KnBsYW5zb3VyY2UgPSAoQ2FjaGVkUGxhblNvdXJjZSAqKSBsZmlyc3QobGMxKTsKQEAgLTI3
ODgsNiArMjgwNCwxNCBAQCBfU1BJX2V4ZWN1dGVfcGxhbihTUElQbGFuUHRyIHBsYW4sIGNv
bnN0IFNQSUV4ZWN1dGVPcHRpb25zICpvcHRpb25zLAogCQkJQ29tbWFuZENvdW50ZXJJbmNy
ZW1lbnQoKTsKIAl9CiAKKwlpZiAob3B0aW9ucy0+cmVhZF9vbmx5ICYmIHRoaXNfY29tbWFu
ZCAhPSBHZXRDdXJyZW50Q29tbWFuZElkKGZhbHNlKSkKKwl7CisJCWVyZXBvcnQoRVJST1Is
CisJCQkJKGVycmNvZGUoRVJSQ09ERV9GRUFUVVJFX05PVF9TVVBQT1JURUQpLAorCQkJCQkJ
LyogdHJhbnNsYXRvcjogJXMgaXMgYSBTUUwgc3RhdGVtZW50IG5hbWUgKi8KKwkJCQkJCWVy
cm1zZygiRGFtbjIhIFVwZGF0ZSB3ZXJlIGRvbmUgaW4gYSBub24tdm9sYXRpbGUgZnVuY3Rp
b24iKSkpOworCX0KKwogZmFpbDoKIAogCS8qIFBvcCB0aGUgc25hcHNob3Qgb2ZmIHRoZSBz
dGFjayBpZiB3ZSBwdXNoZWQgb25lICovCg==

--------------BvuBQMTan0qLjF7e26PDWTlF--