pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Make INSERT-from-multiple-VALUES-rows handle targetlist indirect 7+ messages / 1 participants [nested] [flat]
* pgsql: Make INSERT-from-multiple-VALUES-rows handle targetlist indirect @ 2016-08-03 20:37 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2016-08-03 20:37 UTC (permalink / raw) To: pgsql-committers Make INSERT-from-multiple-VALUES-rows handle targetlist indirection better. Previously, if an INSERT with multiple rows of VALUES had indirection (array subscripting or field selection) in its target-columns list, the parser handled that by applying transformAssignedExpr() to each element of each VALUES row independently. This led to having ArrayRef assignment nodes or FieldStore nodes in each row of the VALUES RTE. That works for simple cases, but in bug #14265 Nuri Boardman points out that it fails if there are multiple assignments to elements/fields of the same target column. For such cases to work, rewriteTargetListIU() has to nest the ArrayRefs or FieldStores together to produce a single expression to be assigned to the column. But it failed to find them in the top-level targetlist and issued an error about "multiple assignments to same column". We could possibly fix this by teaching the rewriter to apply rewriteTargetListIU to each VALUES row separately, but that would be messy (it would change the output rowtype of the VALUES RTE, for example) and inefficient. Instead, let's fix the parser so that the VALUES RTE outputs are just the user-specified values, cast to the right type if necessary, and then the ArrayRefs or FieldStores are applied in the top-level targetlist to Vars representing the RTE's outputs. This is the same parsetree representation already used for similar cases with INSERT/SELECT syntax, so it allows simplifications in ruleutils.c, which no longer needs to treat INSERT-from-multiple-VALUES as its own special case. This implementation works by applying transformAssignedExpr to the VALUES entries as before, and then stripping off any ArrayRefs or FieldStores it adds. With lots of VALUES rows it would be noticeably more efficient to not add those nodes in the first place. But that's just an optimization not a bug fix, and there doesn't seem to be any good way to do it without significant refactoring. (A non-invasive answer would be to apply transformAssignedExpr + stripping to just the first VALUES row, and then just forcibly cast remaining rows to the same data types exposed in the first row. But this way would lead to different, not-INSERT-specific errors being reported in casting failure cases, so it doesn't seem very nice.) So leave that for later; this patch at least isn't making the per-row parsing work worse, and it does make the finished parsetree smaller, saving rewriter and planner work. Catversion bump because stored rules containing such INSERTs would need to change. Because of that, no back-patch, even though this is a very long-standing bug. Report: <20160727005725.7438.26021@wrigleys.postgresql.org> Discussion: <9578.1469645245@sss.pgh.pa.us> Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/a3c7a993d5eb29df4d33075b83c75ae25f257897 Modified Files -------------- src/backend/parser/analyze.c | 65 +++++++++++++++++++++++++---- src/backend/utils/adt/ruleutils.c | 60 +++++++++------------------ src/include/catalog/catversion.h | 2 +- src/test/regress/expected/insert.out | 79 ++++++++++++++++++++++++++++++++++++ src/test/regress/sql/insert.sql | 48 ++++++++++++++++++++++ 5 files changed, 205 insertions(+), 49 deletions(-) -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum @ 2024-03-14 18:57 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-03-14 18:57 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Make INSERT-from-multiple-VALUES-rows handle domain target columns. Commit a3c7a993d fixed some cases involving target columns that are arrays or composites by applying transformAssignedExpr to the VALUES entries, and then stripping off any assignment ArrayRefs or FieldStores that the transformation added. But I forgot about domains over arrays or composites :-(. Such cases would either fail with surprising complaints about mismatched datatypes, or insert unexpected coercions that could lead to odd results. To fix, extend the stripping logic to get rid of CoerceToDomain if it's atop an ArrayRef or FieldStore. While poking at this, I realized that there's a poorly documented and not-at-all-tested behavior nearby: we coerce each VALUES column to the domain type separately, and rely on the rewriter to merge those operations so that the domain constraints are checked only once. If that merging did not happen, it's entirely possible that we'd get unexpected domain constraint failures due to checking a partially-updated container value. There's no bug there, but while we're here let's improve the commentary about it and add some test cases that explicitly exercise that behavior. Per bug #18393 from Pablo Kharo. Back-patch to all supported branches. Discussion: https://postgr.es/m/18393-65fedb1a0de9260d@postgresql.org Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/b4a71cf65d70b98aaf890b13ec600e340ff69e3f Modified Files -------------- src/backend/parser/analyze.c | 19 ++++-- src/backend/parser/parse_target.c | 18 +++++- src/backend/rewrite/rewriteHandler.c | 6 +- src/test/regress/expected/insert.out | 116 ++++++++++++++++++++++++++++++++++- src/test/regress/sql/insert.sql | 79 +++++++++++++++++++++++- 5 files changed, 227 insertions(+), 11 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum @ 2024-03-14 18:57 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-03-14 18:57 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Make INSERT-from-multiple-VALUES-rows handle domain target columns. Commit a3c7a993d fixed some cases involving target columns that are arrays or composites by applying transformAssignedExpr to the VALUES entries, and then stripping off any assignment ArrayRefs or FieldStores that the transformation added. But I forgot about domains over arrays or composites :-(. Such cases would either fail with surprising complaints about mismatched datatypes, or insert unexpected coercions that could lead to odd results. To fix, extend the stripping logic to get rid of CoerceToDomain if it's atop an ArrayRef or FieldStore. While poking at this, I realized that there's a poorly documented and not-at-all-tested behavior nearby: we coerce each VALUES column to the domain type separately, and rely on the rewriter to merge those operations so that the domain constraints are checked only once. If that merging did not happen, it's entirely possible that we'd get unexpected domain constraint failures due to checking a partially-updated container value. There's no bug there, but while we're here let's improve the commentary about it and add some test cases that explicitly exercise that behavior. Per bug #18393 from Pablo Kharo. Back-patch to all supported branches. Discussion: https://postgr.es/m/18393-65fedb1a0de9260d@postgresql.org Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/52898c63e724cd6e18728b4bc0f8d9ec49f12b14 Modified Files -------------- src/backend/parser/analyze.c | 19 ++++-- src/backend/parser/parse_target.c | 18 +++++- src/backend/rewrite/rewriteHandler.c | 6 +- src/test/regress/expected/insert.out | 116 ++++++++++++++++++++++++++++++++++- src/test/regress/sql/insert.sql | 79 +++++++++++++++++++++++- 5 files changed, 227 insertions(+), 11 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum @ 2024-03-14 18:57 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-03-14 18:57 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Make INSERT-from-multiple-VALUES-rows handle domain target columns. Commit a3c7a993d fixed some cases involving target columns that are arrays or composites by applying transformAssignedExpr to the VALUES entries, and then stripping off any assignment ArrayRefs or FieldStores that the transformation added. But I forgot about domains over arrays or composites :-(. Such cases would either fail with surprising complaints about mismatched datatypes, or insert unexpected coercions that could lead to odd results. To fix, extend the stripping logic to get rid of CoerceToDomain if it's atop an ArrayRef or FieldStore. While poking at this, I realized that there's a poorly documented and not-at-all-tested behavior nearby: we coerce each VALUES column to the domain type separately, and rely on the rewriter to merge those operations so that the domain constraints are checked only once. If that merging did not happen, it's entirely possible that we'd get unexpected domain constraint failures due to checking a partially-updated container value. There's no bug there, but while we're here let's improve the commentary about it and add some test cases that explicitly exercise that behavior. Per bug #18393 from Pablo Kharo. Back-patch to all supported branches. Discussion: https://postgr.es/m/18393-65fedb1a0de9260d@postgresql.org Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/7c61d23422afece255ed47c84876fb062f40d451 Modified Files -------------- src/backend/parser/analyze.c | 19 ++++-- src/backend/parser/parse_target.c | 18 +++++- src/backend/rewrite/rewriteHandler.c | 6 +- src/test/regress/expected/insert.out | 116 ++++++++++++++++++++++++++++++++++- src/test/regress/sql/insert.sql | 79 +++++++++++++++++++++++- 5 files changed, 227 insertions(+), 11 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum @ 2024-03-14 18:57 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-03-14 18:57 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Make INSERT-from-multiple-VALUES-rows handle domain target columns. Commit a3c7a993d fixed some cases involving target columns that are arrays or composites by applying transformAssignedExpr to the VALUES entries, and then stripping off any assignment ArrayRefs or FieldStores that the transformation added. But I forgot about domains over arrays or composites :-(. Such cases would either fail with surprising complaints about mismatched datatypes, or insert unexpected coercions that could lead to odd results. To fix, extend the stripping logic to get rid of CoerceToDomain if it's atop an ArrayRef or FieldStore. While poking at this, I realized that there's a poorly documented and not-at-all-tested behavior nearby: we coerce each VALUES column to the domain type separately, and rely on the rewriter to merge those operations so that the domain constraints are checked only once. If that merging did not happen, it's entirely possible that we'd get unexpected domain constraint failures due to checking a partially-updated container value. There's no bug there, but while we're here let's improve the commentary about it and add some test cases that explicitly exercise that behavior. Per bug #18393 from Pablo Kharo. Back-patch to all supported branches. Discussion: https://postgr.es/m/18393-65fedb1a0de9260d@postgresql.org Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/3621ffd9f21b313116e84301a1dce0d3f1dc2f8a Modified Files -------------- src/backend/parser/analyze.c | 19 ++++-- src/backend/parser/parse_target.c | 18 +++++- src/backend/rewrite/rewriteHandler.c | 6 +- src/test/regress/expected/insert.out | 116 ++++++++++++++++++++++++++++++++++- src/test/regress/sql/insert.sql | 79 +++++++++++++++++++++++- 5 files changed, 227 insertions(+), 11 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum @ 2024-03-14 18:57 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-03-14 18:57 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Make INSERT-from-multiple-VALUES-rows handle domain target columns. Commit a3c7a993d fixed some cases involving target columns that are arrays or composites by applying transformAssignedExpr to the VALUES entries, and then stripping off any assignment ArrayRefs or FieldStores that the transformation added. But I forgot about domains over arrays or composites :-(. Such cases would either fail with surprising complaints about mismatched datatypes, or insert unexpected coercions that could lead to odd results. To fix, extend the stripping logic to get rid of CoerceToDomain if it's atop an ArrayRef or FieldStore. While poking at this, I realized that there's a poorly documented and not-at-all-tested behavior nearby: we coerce each VALUES column to the domain type separately, and rely on the rewriter to merge those operations so that the domain constraints are checked only once. If that merging did not happen, it's entirely possible that we'd get unexpected domain constraint failures due to checking a partially-updated container value. There's no bug there, but while we're here let's improve the commentary about it and add some test cases that explicitly exercise that behavior. Per bug #18393 from Pablo Kharo. Back-patch to all supported branches. Discussion: https://postgr.es/m/18393-65fedb1a0de9260d@postgresql.org Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/0200398dd39e5db0eba06043129f1a4daf9170c5 Modified Files -------------- src/backend/parser/analyze.c | 19 ++++-- src/backend/parser/parse_target.c | 18 +++++- src/backend/rewrite/rewriteHandler.c | 6 +- src/test/regress/expected/insert.out | 116 ++++++++++++++++++++++++++++++++++- src/test/regress/sql/insert.sql | 79 +++++++++++++++++++++++- 5 files changed, 227 insertions(+), 11 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum @ 2024-03-14 18:57 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-03-14 18:57 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Make INSERT-from-multiple-VALUES-rows handle domain target columns. Commit a3c7a993d fixed some cases involving target columns that are arrays or composites by applying transformAssignedExpr to the VALUES entries, and then stripping off any assignment ArrayRefs or FieldStores that the transformation added. But I forgot about domains over arrays or composites :-(. Such cases would either fail with surprising complaints about mismatched datatypes, or insert unexpected coercions that could lead to odd results. To fix, extend the stripping logic to get rid of CoerceToDomain if it's atop an ArrayRef or FieldStore. While poking at this, I realized that there's a poorly documented and not-at-all-tested behavior nearby: we coerce each VALUES column to the domain type separately, and rely on the rewriter to merge those operations so that the domain constraints are checked only once. If that merging did not happen, it's entirely possible that we'd get unexpected domain constraint failures due to checking a partially-updated container value. There's no bug there, but while we're here let's improve the commentary about it and add some test cases that explicitly exercise that behavior. Per bug #18393 from Pablo Kharo. Back-patch to all supported branches. Discussion: https://postgr.es/m/18393-65fedb1a0de9260d@postgresql.org Branch ------ REL_12_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/82c87af7a0ac97ec6e99277f2deb4ee55e347e1e Modified Files -------------- src/backend/parser/analyze.c | 19 ++++-- src/backend/parser/parse_target.c | 18 +++++- src/backend/rewrite/rewriteHandler.c | 6 +- src/test/regress/expected/insert.out | 116 ++++++++++++++++++++++++++++++++++- src/test/regress/sql/insert.sql | 79 +++++++++++++++++++++++- 5 files changed, 227 insertions(+), 11 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-03-14 18:57 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2016-08-03 20:37 pgsql: Make INSERT-from-multiple-VALUES-rows handle targetlist indirect Tom Lane <tgl@sss.pgh.pa.us> 2024-03-14 18:57 pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum Tom Lane <tgl@sss.pgh.pa.us> 2024-03-14 18:57 pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum Tom Lane <tgl@sss.pgh.pa.us> 2024-03-14 18:57 pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum Tom Lane <tgl@sss.pgh.pa.us> 2024-03-14 18:57 pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum Tom Lane <tgl@sss.pgh.pa.us> 2024-03-14 18:57 pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum Tom Lane <tgl@sss.pgh.pa.us> 2024-03-14 18:57 pgsql: Make INSERT-from-multiple-VALUES-rows handle domain target colum 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