pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Handle default NULL insertion a little better. 5+ messages / 1 participants [nested] [flat]
* pgsql: Handle default NULL insertion a little better. @ 2025-01-29 20:32 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2025-01-29 20:32 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Handle default NULL insertion a little better. If a column is omitted in an INSERT, and there's no column default, the code in preptlist.c generates a NULL Const to be inserted. Furthermore, if the column is of a domain type, we wrap the Const in CoerceToDomain, so as to throw a run-time error if the domain has a NOT NULL constraint. That's fine as far as it goes, but there are two problems: 1. We're being sloppy about the type/typmod that the Const is labeled with. It really should have the domain's base type/typmod, since it's the input to CoerceToDomain not the output. This can result in coerce_to_domain inserting a useless length-coercion function (useless because it's being applied to a null). The coercion would typically get const-folded away later, but it'd be better not to create it in the first place. 2. We're not applying expression preprocessing (specifically, eval_const_expressions) to the resulting expression tree. The planner's primary expression-preprocessing pass already happened, so that means the length coercion step and CoerceToDomain node miss preprocessing altogether. This is at the least inefficient, since it means the length coercion and CoerceToDomain will actually be executed for each inserted row, though they could be const-folded away in most cases. Worse, it seems possible that missing preprocessing for the length coercion could result in an invalid plan (for example, due to failing to perform default-function-argument insertion). I'm not aware of any live bug of that sort with core datatypes, and it might be unreachable for extension types as well because of restrictions of CREATE CAST, but I'm not entirely convinced that it's unreachable. Hence, it seems worth back-patching the fix (although I only went back to v14, as the patch doesn't apply cleanly at all in v13). There are several places in the rewriter that are building null domain constants the same way as preptlist.c. While those are before the planner and hence don't have any reachable bug, they're still applying a length coercion that will be const-folded away later, uselessly wasting cycles. Hence, make a utility routine that all of these places can call to do it right. Making this code more careful about the typmod assigned to the generated NULL constant has visible but cosmetic effects on some of the plans shown in contrib/postgres_fdw's regression tests. Discussion: https://postgr.es/m/1865579.1738113656@sss.pgh.pa.us Backpatch-through: 14 Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/0da39aa7667b06e16189d318f7850d559d446d52 Modified Files -------------- contrib/postgres_fdw/expected/postgres_fdw.out | 24 +++++++-------- src/backend/optimizer/prep/preptlist.c | 34 +++++++++------------ src/backend/parser/parse_coerce.c | 37 +++++++++++++++++++++++ src/backend/rewrite/rewriteHandler.c | 42 ++++++-------------------- src/backend/rewrite/rewriteManip.c | 30 +++++++++--------- src/include/parser/parse_coerce.h | 3 ++ 6 files changed, 92 insertions(+), 78 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
* pgsql: Handle default NULL insertion a little better. @ 2025-01-29 20:32 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2025-01-29 20:32 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Handle default NULL insertion a little better. If a column is omitted in an INSERT, and there's no column default, the code in preptlist.c generates a NULL Const to be inserted. Furthermore, if the column is of a domain type, we wrap the Const in CoerceToDomain, so as to throw a run-time error if the domain has a NOT NULL constraint. That's fine as far as it goes, but there are two problems: 1. We're being sloppy about the type/typmod that the Const is labeled with. It really should have the domain's base type/typmod, since it's the input to CoerceToDomain not the output. This can result in coerce_to_domain inserting a useless length-coercion function (useless because it's being applied to a null). The coercion would typically get const-folded away later, but it'd be better not to create it in the first place. 2. We're not applying expression preprocessing (specifically, eval_const_expressions) to the resulting expression tree. The planner's primary expression-preprocessing pass already happened, so that means the length coercion step and CoerceToDomain node miss preprocessing altogether. This is at the least inefficient, since it means the length coercion and CoerceToDomain will actually be executed for each inserted row, though they could be const-folded away in most cases. Worse, it seems possible that missing preprocessing for the length coercion could result in an invalid plan (for example, due to failing to perform default-function-argument insertion). I'm not aware of any live bug of that sort with core datatypes, and it might be unreachable for extension types as well because of restrictions of CREATE CAST, but I'm not entirely convinced that it's unreachable. Hence, it seems worth back-patching the fix (although I only went back to v14, as the patch doesn't apply cleanly at all in v13). There are several places in the rewriter that are building null domain constants the same way as preptlist.c. While those are before the planner and hence don't have any reachable bug, they're still applying a length coercion that will be const-folded away later, uselessly wasting cycles. Hence, make a utility routine that all of these places can call to do it right. Making this code more careful about the typmod assigned to the generated NULL constant has visible but cosmetic effects on some of the plans shown in contrib/postgres_fdw's regression tests. Discussion: https://postgr.es/m/1865579.1738113656@sss.pgh.pa.us Backpatch-through: 14 Branch ------ REL_17_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/6e41e9e5e0deb39420446f10b26c36a84f7248d2 Modified Files -------------- contrib/postgres_fdw/expected/postgres_fdw.out | 24 +++++++-------- src/backend/optimizer/prep/preptlist.c | 34 +++++++++------------ src/backend/parser/parse_coerce.c | 37 +++++++++++++++++++++++ src/backend/rewrite/rewriteHandler.c | 42 ++++++-------------------- src/backend/rewrite/rewriteManip.c | 30 +++++++++--------- src/include/parser/parse_coerce.h | 3 ++ 6 files changed, 92 insertions(+), 78 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
* pgsql: Handle default NULL insertion a little better. @ 2025-01-29 20:32 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2025-01-29 20:32 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Handle default NULL insertion a little better. If a column is omitted in an INSERT, and there's no column default, the code in preptlist.c generates a NULL Const to be inserted. Furthermore, if the column is of a domain type, we wrap the Const in CoerceToDomain, so as to throw a run-time error if the domain has a NOT NULL constraint. That's fine as far as it goes, but there are two problems: 1. We're being sloppy about the type/typmod that the Const is labeled with. It really should have the domain's base type/typmod, since it's the input to CoerceToDomain not the output. This can result in coerce_to_domain inserting a useless length-coercion function (useless because it's being applied to a null). The coercion would typically get const-folded away later, but it'd be better not to create it in the first place. 2. We're not applying expression preprocessing (specifically, eval_const_expressions) to the resulting expression tree. The planner's primary expression-preprocessing pass already happened, so that means the length coercion step and CoerceToDomain node miss preprocessing altogether. This is at the least inefficient, since it means the length coercion and CoerceToDomain will actually be executed for each inserted row, though they could be const-folded away in most cases. Worse, it seems possible that missing preprocessing for the length coercion could result in an invalid plan (for example, due to failing to perform default-function-argument insertion). I'm not aware of any live bug of that sort with core datatypes, and it might be unreachable for extension types as well because of restrictions of CREATE CAST, but I'm not entirely convinced that it's unreachable. Hence, it seems worth back-patching the fix (although I only went back to v14, as the patch doesn't apply cleanly at all in v13). There are several places in the rewriter that are building null domain constants the same way as preptlist.c. While those are before the planner and hence don't have any reachable bug, they're still applying a length coercion that will be const-folded away later, uselessly wasting cycles. Hence, make a utility routine that all of these places can call to do it right. Making this code more careful about the typmod assigned to the generated NULL constant has visible but cosmetic effects on some of the plans shown in contrib/postgres_fdw's regression tests. Discussion: https://postgr.es/m/1865579.1738113656@sss.pgh.pa.us Backpatch-through: 14 Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/6655d931c659fda68cce090b7214cb1a9d4cfc65 Modified Files -------------- contrib/postgres_fdw/expected/postgres_fdw.out | 24 +++++++-------- src/backend/optimizer/prep/preptlist.c | 34 +++++++++------------ src/backend/parser/parse_coerce.c | 37 +++++++++++++++++++++++ src/backend/rewrite/rewriteHandler.c | 42 ++++++-------------------- src/backend/rewrite/rewriteManip.c | 30 +++++++++--------- src/include/parser/parse_coerce.h | 3 ++ 6 files changed, 92 insertions(+), 78 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
* pgsql: Handle default NULL insertion a little better. @ 2025-01-29 20:32 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2025-01-29 20:32 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Handle default NULL insertion a little better. If a column is omitted in an INSERT, and there's no column default, the code in preptlist.c generates a NULL Const to be inserted. Furthermore, if the column is of a domain type, we wrap the Const in CoerceToDomain, so as to throw a run-time error if the domain has a NOT NULL constraint. That's fine as far as it goes, but there are two problems: 1. We're being sloppy about the type/typmod that the Const is labeled with. It really should have the domain's base type/typmod, since it's the input to CoerceToDomain not the output. This can result in coerce_to_domain inserting a useless length-coercion function (useless because it's being applied to a null). The coercion would typically get const-folded away later, but it'd be better not to create it in the first place. 2. We're not applying expression preprocessing (specifically, eval_const_expressions) to the resulting expression tree. The planner's primary expression-preprocessing pass already happened, so that means the length coercion step and CoerceToDomain node miss preprocessing altogether. This is at the least inefficient, since it means the length coercion and CoerceToDomain will actually be executed for each inserted row, though they could be const-folded away in most cases. Worse, it seems possible that missing preprocessing for the length coercion could result in an invalid plan (for example, due to failing to perform default-function-argument insertion). I'm not aware of any live bug of that sort with core datatypes, and it might be unreachable for extension types as well because of restrictions of CREATE CAST, but I'm not entirely convinced that it's unreachable. Hence, it seems worth back-patching the fix (although I only went back to v14, as the patch doesn't apply cleanly at all in v13). There are several places in the rewriter that are building null domain constants the same way as preptlist.c. While those are before the planner and hence don't have any reachable bug, they're still applying a length coercion that will be const-folded away later, uselessly wasting cycles. Hence, make a utility routine that all of these places can call to do it right. Making this code more careful about the typmod assigned to the generated NULL constant has visible but cosmetic effects on some of the plans shown in contrib/postgres_fdw's regression tests. Discussion: https://postgr.es/m/1865579.1738113656@sss.pgh.pa.us Backpatch-through: 14 Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/bb85d09355832e8249b1af40fd181b6e791d87b5 Modified Files -------------- contrib/postgres_fdw/expected/postgres_fdw.out | 24 +++++++-------- src/backend/optimizer/prep/preptlist.c | 34 +++++++++------------ src/backend/parser/parse_coerce.c | 37 +++++++++++++++++++++++ src/backend/rewrite/rewriteHandler.c | 42 ++++++-------------------- src/backend/rewrite/rewriteManip.c | 30 +++++++++--------- src/include/parser/parse_coerce.h | 3 ++ 6 files changed, 92 insertions(+), 78 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
* pgsql: Handle default NULL insertion a little better. @ 2025-01-29 20:32 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2025-01-29 20:32 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Handle default NULL insertion a little better. If a column is omitted in an INSERT, and there's no column default, the code in preptlist.c generates a NULL Const to be inserted. Furthermore, if the column is of a domain type, we wrap the Const in CoerceToDomain, so as to throw a run-time error if the domain has a NOT NULL constraint. That's fine as far as it goes, but there are two problems: 1. We're being sloppy about the type/typmod that the Const is labeled with. It really should have the domain's base type/typmod, since it's the input to CoerceToDomain not the output. This can result in coerce_to_domain inserting a useless length-coercion function (useless because it's being applied to a null). The coercion would typically get const-folded away later, but it'd be better not to create it in the first place. 2. We're not applying expression preprocessing (specifically, eval_const_expressions) to the resulting expression tree. The planner's primary expression-preprocessing pass already happened, so that means the length coercion step and CoerceToDomain node miss preprocessing altogether. This is at the least inefficient, since it means the length coercion and CoerceToDomain will actually be executed for each inserted row, though they could be const-folded away in most cases. Worse, it seems possible that missing preprocessing for the length coercion could result in an invalid plan (for example, due to failing to perform default-function-argument insertion). I'm not aware of any live bug of that sort with core datatypes, and it might be unreachable for extension types as well because of restrictions of CREATE CAST, but I'm not entirely convinced that it's unreachable. Hence, it seems worth back-patching the fix (although I only went back to v14, as the patch doesn't apply cleanly at all in v13). There are several places in the rewriter that are building null domain constants the same way as preptlist.c. While those are before the planner and hence don't have any reachable bug, they're still applying a length coercion that will be const-folded away later, uselessly wasting cycles. Hence, make a utility routine that all of these places can call to do it right. Making this code more careful about the typmod assigned to the generated NULL constant has visible but cosmetic effects on some of the plans shown in contrib/postgres_fdw's regression tests. Discussion: https://postgr.es/m/1865579.1738113656@sss.pgh.pa.us Backpatch-through: 14 Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/c05268e6ec931724282b5780f228e5a1aad94268 Modified Files -------------- contrib/postgres_fdw/expected/postgres_fdw.out | 24 +++++++-------- src/backend/optimizer/prep/preptlist.c | 31 ++++++++----------- src/backend/parser/parse_coerce.c | 37 +++++++++++++++++++++++ src/backend/rewrite/rewriteHandler.c | 42 ++++++-------------------- src/backend/rewrite/rewriteManip.c | 30 +++++++++--------- src/include/parser/parse_coerce.h | 3 ++ 6 files changed, 90 insertions(+), 77 deletions(-) ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-01-29 20:32 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-01-29 20:32 pgsql: Handle default NULL insertion a little better. Tom Lane <tgl@sss.pgh.pa.us> 2025-01-29 20:32 pgsql: Handle default NULL insertion a little better. Tom Lane <tgl@sss.pgh.pa.us> 2025-01-29 20:32 pgsql: Handle default NULL insertion a little better. Tom Lane <tgl@sss.pgh.pa.us> 2025-01-29 20:32 pgsql: Handle default NULL insertion a little better. Tom Lane <tgl@sss.pgh.pa.us> 2025-01-29 20:32 pgsql: Handle default NULL insertion a little better. 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