pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Ensure we preprocess expressions before checking their volatilit 6+ messages / 1 participants [nested] [flat]
* pgsql: Ensure we preprocess expressions before checking their volatilit @ 2023-11-16 15:05 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-11-16 15:05 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Ensure we preprocess expressions before checking their volatility. contain_mutable_functions and contain_volatile_functions give reliable answers only after expression preprocessing (specifically eval_const_expressions). Some places understand this, but some did not get the memo --- which is not entirely their fault, because the problem is documented only in places far away from those functions. Introduce wrapper functions that allow doing the right thing easily, and add commentary in hopes of preventing future mistakes from copy-and-paste of code that's only conditionally safe. Two actual bugs of this ilk are fixed here. We failed to preprocess column GENERATED expressions before checking mutability, so that the code could fail to detect the use of a volatile function default-argument expression, or it could reject a polymorphic function that is actually immutable on the datatype of interest. Likewise, column DEFAULT expressions weren't preprocessed before determining if it's safe to apply the attmissingval mechanism. A false negative would just result in an unnecessary table rewrite, but a false positive could allow the attmissingval mechanism to be used in a case where it should not be, resulting in unexpected initial values in a new column. In passing, re-order the steps in ComputePartitionAttrs so that its checks for invalid column references are done before applying expression_planner, rather than after. The previous coding would not complain if a partition expression contains a disallowed column reference that gets optimized away by constant folding, which seems to me to be a behavior we do not want. Per bug #18097 from Jim Keener. Back-patch to all supported versions. Discussion: https://postgr.es/m/18097-ebb179674f22932f@postgresql.org Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/f07a3039c72b4a7afbe2b898fd1194575d13c37e Modified Files -------------- src/backend/catalog/heap.c | 7 +++- src/backend/commands/copyfrom.c | 6 ++- src/backend/commands/indexcmds.c | 31 +------------- src/backend/commands/tablecmds.c | 50 +++++++++++----------- src/backend/optimizer/util/clauses.c | 66 ++++++++++++++++++++++++++++++ src/include/optimizer/optimizer.h | 2 + src/test/regress/expected/fast_default.out | 18 ++++++++ src/test/regress/expected/generated.out | 3 ++ src/test/regress/sql/fast_default.sql | 11 +++++ src/test/regress/sql/generated.sql | 3 ++ 10 files changed, 141 insertions(+), 56 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Ensure we preprocess expressions before checking their volatilit @ 2023-11-16 15:05 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-11-16 15:05 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Ensure we preprocess expressions before checking their volatility. contain_mutable_functions and contain_volatile_functions give reliable answers only after expression preprocessing (specifically eval_const_expressions). Some places understand this, but some did not get the memo --- which is not entirely their fault, because the problem is documented only in places far away from those functions. Introduce wrapper functions that allow doing the right thing easily, and add commentary in hopes of preventing future mistakes from copy-and-paste of code that's only conditionally safe. Two actual bugs of this ilk are fixed here. We failed to preprocess column GENERATED expressions before checking mutability, so that the code could fail to detect the use of a volatile function default-argument expression, or it could reject a polymorphic function that is actually immutable on the datatype of interest. Likewise, column DEFAULT expressions weren't preprocessed before determining if it's safe to apply the attmissingval mechanism. A false negative would just result in an unnecessary table rewrite, but a false positive could allow the attmissingval mechanism to be used in a case where it should not be, resulting in unexpected initial values in a new column. In passing, re-order the steps in ComputePartitionAttrs so that its checks for invalid column references are done before applying expression_planner, rather than after. The previous coding would not complain if a partition expression contains a disallowed column reference that gets optimized away by constant folding, which seems to me to be a behavior we do not want. Per bug #18097 from Jim Keener. Back-patch to all supported versions. Discussion: https://postgr.es/m/18097-ebb179674f22932f@postgresql.org Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/743ddafc7124604c1f59b5d9f661be47a987a8d0 Modified Files -------------- src/backend/catalog/heap.c | 7 +++- src/backend/commands/copyfrom.c | 6 ++- src/backend/commands/indexcmds.c | 31 +------------- src/backend/commands/tablecmds.c | 50 +++++++++++----------- src/backend/optimizer/util/clauses.c | 66 ++++++++++++++++++++++++++++++ src/include/optimizer/optimizer.h | 2 + src/test/regress/expected/fast_default.out | 18 ++++++++ src/test/regress/expected/generated.out | 3 ++ src/test/regress/sql/fast_default.sql | 11 +++++ src/test/regress/sql/generated.sql | 3 ++ 10 files changed, 141 insertions(+), 56 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Ensure we preprocess expressions before checking their volatilit @ 2023-11-16 15:05 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-11-16 15:05 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Ensure we preprocess expressions before checking their volatility. contain_mutable_functions and contain_volatile_functions give reliable answers only after expression preprocessing (specifically eval_const_expressions). Some places understand this, but some did not get the memo --- which is not entirely their fault, because the problem is documented only in places far away from those functions. Introduce wrapper functions that allow doing the right thing easily, and add commentary in hopes of preventing future mistakes from copy-and-paste of code that's only conditionally safe. Two actual bugs of this ilk are fixed here. We failed to preprocess column GENERATED expressions before checking mutability, so that the code could fail to detect the use of a volatile function default-argument expression, or it could reject a polymorphic function that is actually immutable on the datatype of interest. Likewise, column DEFAULT expressions weren't preprocessed before determining if it's safe to apply the attmissingval mechanism. A false negative would just result in an unnecessary table rewrite, but a false positive could allow the attmissingval mechanism to be used in a case where it should not be, resulting in unexpected initial values in a new column. In passing, re-order the steps in ComputePartitionAttrs so that its checks for invalid column references are done before applying expression_planner, rather than after. The previous coding would not complain if a partition expression contains a disallowed column reference that gets optimized away by constant folding, which seems to me to be a behavior we do not want. Per bug #18097 from Jim Keener. Back-patch to all supported versions. Discussion: https://postgr.es/m/18097-ebb179674f22932f@postgresql.org Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/5c11104c7d20abbf845a2f03435641f7c6bc67de Modified Files -------------- src/backend/catalog/heap.c | 7 +++- src/backend/commands/copy.c | 6 ++- src/backend/commands/indexcmds.c | 31 +------------- src/backend/commands/tablecmds.c | 50 +++++++++++----------- src/backend/optimizer/util/clauses.c | 66 ++++++++++++++++++++++++++++++ src/include/optimizer/optimizer.h | 2 + src/test/regress/expected/fast_default.out | 18 ++++++++ src/test/regress/expected/generated.out | 3 ++ src/test/regress/sql/fast_default.sql | 11 +++++ src/test/regress/sql/generated.sql | 3 ++ 10 files changed, 141 insertions(+), 56 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Ensure we preprocess expressions before checking their volatilit @ 2023-11-16 15:05 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-11-16 15:05 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Ensure we preprocess expressions before checking their volatility. contain_mutable_functions and contain_volatile_functions give reliable answers only after expression preprocessing (specifically eval_const_expressions). Some places understand this, but some did not get the memo --- which is not entirely their fault, because the problem is documented only in places far away from those functions. Introduce wrapper functions that allow doing the right thing easily, and add commentary in hopes of preventing future mistakes from copy-and-paste of code that's only conditionally safe. Two actual bugs of this ilk are fixed here. We failed to preprocess column GENERATED expressions before checking mutability, so that the code could fail to detect the use of a volatile function default-argument expression, or it could reject a polymorphic function that is actually immutable on the datatype of interest. Likewise, column DEFAULT expressions weren't preprocessed before determining if it's safe to apply the attmissingval mechanism. A false negative would just result in an unnecessary table rewrite, but a false positive could allow the attmissingval mechanism to be used in a case where it should not be, resulting in unexpected initial values in a new column. In passing, re-order the steps in ComputePartitionAttrs so that its checks for invalid column references are done before applying expression_planner, rather than after. The previous coding would not complain if a partition expression contains a disallowed column reference that gets optimized away by constant folding, which seems to me to be a behavior we do not want. Per bug #18097 from Jim Keener. Back-patch to all supported versions. Discussion: https://postgr.es/m/18097-ebb179674f22932f@postgresql.org Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/9057ddbefe7e8a1aed8ea91fc0f98f31b8e693d7 Modified Files -------------- src/backend/catalog/heap.c | 7 +++- src/backend/commands/copyfrom.c | 6 ++- src/backend/commands/indexcmds.c | 31 +------------- src/backend/commands/tablecmds.c | 50 +++++++++++----------- src/backend/optimizer/util/clauses.c | 66 ++++++++++++++++++++++++++++++ src/include/optimizer/optimizer.h | 2 + src/test/regress/expected/fast_default.out | 18 ++++++++ src/test/regress/expected/generated.out | 3 ++ src/test/regress/sql/fast_default.sql | 11 +++++ src/test/regress/sql/generated.sql | 3 ++ 10 files changed, 141 insertions(+), 56 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Ensure we preprocess expressions before checking their volatilit @ 2023-11-16 15:05 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-11-16 15:05 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Ensure we preprocess expressions before checking their volatility. contain_mutable_functions and contain_volatile_functions give reliable answers only after expression preprocessing (specifically eval_const_expressions). Some places understand this, but some did not get the memo --- which is not entirely their fault, because the problem is documented only in places far away from those functions. Introduce wrapper functions that allow doing the right thing easily, and add commentary in hopes of preventing future mistakes from copy-and-paste of code that's only conditionally safe. Two actual bugs of this ilk are fixed here. We failed to preprocess column GENERATED expressions before checking mutability, so that the code could fail to detect the use of a volatile function default-argument expression, or it could reject a polymorphic function that is actually immutable on the datatype of interest. Likewise, column DEFAULT expressions weren't preprocessed before determining if it's safe to apply the attmissingval mechanism. A false negative would just result in an unnecessary table rewrite, but a false positive could allow the attmissingval mechanism to be used in a case where it should not be, resulting in unexpected initial values in a new column. In passing, re-order the steps in ComputePartitionAttrs so that its checks for invalid column references are done before applying expression_planner, rather than after. The previous coding would not complain if a partition expression contains a disallowed column reference that gets optimized away by constant folding, which seems to me to be a behavior we do not want. Per bug #18097 from Jim Keener. Back-patch to all supported versions. Discussion: https://postgr.es/m/18097-ebb179674f22932f@postgresql.org Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/9bd0f74eac199597980aea45eaf88c502a202665 Modified Files -------------- src/backend/catalog/heap.c | 7 +++- src/backend/commands/copyfrom.c | 6 ++- src/backend/commands/indexcmds.c | 31 +------------- src/backend/commands/tablecmds.c | 50 +++++++++++----------- src/backend/optimizer/util/clauses.c | 66 ++++++++++++++++++++++++++++++ src/include/optimizer/optimizer.h | 2 + src/test/regress/expected/fast_default.out | 18 ++++++++ src/test/regress/expected/generated.out | 3 ++ src/test/regress/sql/fast_default.sql | 11 +++++ src/test/regress/sql/generated.sql | 3 ++ 10 files changed, 141 insertions(+), 56 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Ensure we preprocess expressions before checking their volatilit @ 2023-11-16 15:05 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2023-11-16 15:05 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Ensure we preprocess expressions before checking their volatility. contain_mutable_functions and contain_volatile_functions give reliable answers only after expression preprocessing (specifically eval_const_expressions). Some places understand this, but some did not get the memo --- which is not entirely their fault, because the problem is documented only in places far away from those functions. Introduce wrapper functions that allow doing the right thing easily, and add commentary in hopes of preventing future mistakes from copy-and-paste of code that's only conditionally safe. Two actual bugs of this ilk are fixed here. We failed to preprocess column GENERATED expressions before checking mutability, so that the code could fail to detect the use of a volatile function default-argument expression, or it could reject a polymorphic function that is actually immutable on the datatype of interest. Likewise, column DEFAULT expressions weren't preprocessed before determining if it's safe to apply the attmissingval mechanism. A false negative would just result in an unnecessary table rewrite, but a false positive could allow the attmissingval mechanism to be used in a case where it should not be, resulting in unexpected initial values in a new column. In passing, re-order the steps in ComputePartitionAttrs so that its checks for invalid column references are done before applying expression_planner, rather than after. The previous coding would not complain if a partition expression contains a disallowed column reference that gets optimized away by constant folding, which seems to me to be a behavior we do not want. Per bug #18097 from Jim Keener. Back-patch to all supported versions. Discussion: https://postgr.es/m/18097-ebb179674f22932f@postgresql.org Branch ------ REL_12_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/abd1b1325d60eefcf66942fc332f990255daccb1 Modified Files -------------- src/backend/catalog/heap.c | 7 +++- src/backend/commands/copy.c | 6 ++- src/backend/commands/indexcmds.c | 31 +------------- src/backend/commands/tablecmds.c | 50 +++++++++++----------- src/backend/optimizer/util/clauses.c | 66 ++++++++++++++++++++++++++++++ src/include/optimizer/optimizer.h | 2 + src/test/regress/expected/fast_default.out | 18 ++++++++ src/test/regress/expected/generated.out | 3 ++ src/test/regress/sql/fast_default.sql | 11 +++++ src/test/regress/sql/generated.sql | 3 ++ 10 files changed, 141 insertions(+), 56 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-11-16 15:05 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-11-16 15:05 pgsql: Ensure we preprocess expressions before checking their volatilit Tom Lane <tgl@sss.pgh.pa.us> 2023-11-16 15:05 pgsql: Ensure we preprocess expressions before checking their volatilit Tom Lane <tgl@sss.pgh.pa.us> 2023-11-16 15:05 pgsql: Ensure we preprocess expressions before checking their volatilit Tom Lane <tgl@sss.pgh.pa.us> 2023-11-16 15:05 pgsql: Ensure we preprocess expressions before checking their volatilit Tom Lane <tgl@sss.pgh.pa.us> 2023-11-16 15:05 pgsql: Ensure we preprocess expressions before checking their volatilit Tom Lane <tgl@sss.pgh.pa.us> 2023-11-16 15:05 pgsql: Ensure we preprocess expressions before checking their volatilit Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox