ADBDEV-3951: Backport of "Implemented InPlaceUpdate to be used for updates made on non-distribution columns" #609

KnightMurloc · 2023-09-01T07:42:54Z

Implemented InPlaceUpdate to be used for updates made on non-distribution columns.
Currently, ORCA uses Split-Update for updates on both distribution
and non-distribution columns. With this commit,
ORCA uses an InPlaceUpdate whenever updates are made to
non-distribution columns or non-partition keys, and Split Update
if any of modified columns are either distribution or partition keys.
Consider below setup where we are updating
non-distibution column, b in the table foo.

create table foo(a int, b int); explain update foo set b=4;
ORCA produces plan with Split and Update nodes

 Update
   Output: foo.a, foo.b, (DMLAction), foo.ctid
   ->  Split
         Output: foo.a, foo.b, foo.ctid, foo.gp_segment_id, DMLAction
         ->  Result
               Output: foo.a, foo.b, 4, foo.ctid, foo.gp_segment_id
               ->  Seq Scan on public.foo
                     Output: foo.a, foo.b, foo.ctid, foo.gp_segment_id

There is no point in using a Split and Update for this as we are updating a
non-distribution column which do not require any redistribution. This
commit uses an InPlace Update to perform updates on non-distribution
columns like Planner. Below is the new plan produced with this commit.

New Plan

 Update
   Output: foo.a, "outer".b, foo.ctid
   ->  Result
         Output: foo.a, 4, foo.ctid, foo.gp_segment_id
         ->  Seq Scan on public.foo
               Output: foo.a, foo.ctid, foo.gp_segment_id

greenplum 6 specific changes:

Some constructors have been changed because the list of arguments in 6X and 7X
are different.
fixed a bug in CParseHandlerPhysicalDML::startElement where preserve_oids_xml
was used instead of fSplit, which could lead to SIGSEGV during DML node parsing.
Changed create_index_hot test. Removed disabling the optimizer before updating
since ORCA no longer uses split update in this case.
Implement In-Place Update for DML node. Added the exec Update call in the DML
node. Added partitioning selection in case of In-Place update by sorted table.
Added a call to reconstructMatchingTupleSlot in execUpdate to update plans
generated by ORCA to handle cases of dropped attributes.

original commit: https://github.com/greenplum-db/gpdb/commit/3ced85b65732728edff66fd9a2ce6d7485d65a06

BenderArenadata · 2023-09-01T08:17:47Z

Allure report https://allure-ee.adsw.io/launch/53331

BenderArenadata · 2023-09-01T09:27:51Z

Allure report https://allure-ee.adsw.io/launch/53341

BenderArenadata · 2023-09-04T07:56:28Z

Allure report https://allure-ee.adsw.io/launch/53456

BenderArenadata · 2023-09-04T12:57:01Z

Allure report https://allure-ee.adsw.io/launch/53506

BenderArenadata · 2023-09-05T05:55:53Z

Allure report https://allure-ee.adsw.io/launch/53536

BenderArenadata · 2023-09-05T06:56:39Z

Allure report https://allure-ee.adsw.io/launch/53552

RekGRpth · 2023-09-06T09:16:56Z

If it's a backport, then maybe you should have cherrypicked it to keep the authorship.

BenderArenadata · 2023-12-15T09:43:34Z

Allure report https://allure-ee.adsw.io/launch/60486

BenderArenadata · 2023-12-15T09:55:15Z

Failed job Resource group isolation tests on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/890515

BenderArenadata · 2023-12-15T10:08:39Z

Failed job Resource group isolation tests on ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/890516

BenderArenadata · 2023-12-15T12:11:47Z

Failed job Regression tests with ORCA on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/890509

BenderArenadata · 2023-12-15T12:14:35Z

Failed job Regression tests with ORCA on ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/890510

BenderArenadata · 2023-12-20T08:51:50Z

Allure report https://allure-ee.adsw.io/launch/60723

BenderArenadata · 2023-12-20T08:53:57Z

Failed job Orca unittests on ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/905014

BenderArenadata · 2023-12-20T08:55:32Z

Failed job ORCA code linter on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/905019

BenderArenadata · 2023-12-20T08:57:14Z

Failed job Orca unittests on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/905013

RekGRpth · 2023-12-20T09:02:53Z

implement execUpdate call for DML node
not compute actionColIdx for InPlace Update

If it doesn't apply to the specified backport, then might it be better to make it a separate commit(s) (perhaps a backport(s) of some other commit(s))?

BenderArenadata · 2023-12-20T09:03:48Z

Failed job Resource group isolation tests on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/905017

BenderArenadata · 2023-12-20T09:07:45Z

Failed job Resource group isolation tests on ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/905018

KnightMurloc · 2023-12-20T09:15:59Z

implement execUpdate call for DML node
not compute actionColIdx for InPlace Update

If it doesn't apply to the specified backport, then might it be better to make it a separate commit(s) (perhaps a backport(s) of some other commit(s))?

These changes are necessary for the patch to work. GP7 uses the Modify Table node instead of DML. I'm not sure if we want to port these changes.

src/backend/gporca/libgpopt/src/operators/CLogicalDML.cpp

src/backend/gporca/libgpopt/src/operators/CLogicalUpdate.cpp

src/backend/gporca/libnaucrates/src/parser/CParseHandlerPhysicalDML.cpp

BenderArenadata · 2024-01-12T09:43:44Z

Allure report https://allure-ee.adsw.io/launch/61589

BenderArenadata · 2024-01-12T09:54:09Z

Failed job Resource group isolation tests on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/958078

BenderArenadata · 2024-01-12T10:00:29Z

Failed job Resource group isolation tests on ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/958079

BenderArenadata · 2024-01-12T11:51:43Z

Failed job Regression tests with ORCA on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/958072

BenderArenadata · 2024-01-12T12:15:22Z

Failed job Regression tests with ORCA on ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/958073

BenderArenadata · 2024-01-15T04:42:52Z

Allure report https://allure-ee.adsw.io/launch/61705

BenderArenadata · 2024-01-15T04:52:52Z

Failed job Resource group isolation tests on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/962859

src/backend/executor/nodeDML.c

BenderArenadata · 2024-01-15T04:59:49Z

Failed job Resource group isolation tests on ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/962861

src/backend/executor/nodeDML.c

Added the exec Update call in the DML node. Added partitioning selection in case of In-Place update by sorted table. added a call to reconstructMatchingTupleSlot in execUpdate to update plans generated by ORCA to handle cases of dropped attributes.

…tion columns. Currently, ORCA uses Split-Update for updates on both distribution and non-distribution columns. With this commit, ORCA uses an InPlaceUpdate whenever updates are made to non-distribution columns or non-partition keys, and Split Update if any of modified columns are either distribution or partition keys. Consider below setup where we are updating non-distibution column, b in the table foo. ` create table foo(a int, b int); explain update foo set b=4; ` ORCA produces plan with Split and Update nodes ``` Update on public.foo -> Result Output: foo_1.a, foo_1.b, (DMLAction), foo_1.ctid, foo_1.gp_segment_id -> Split Output: foo_1.a, foo_1.b, foo_1.ctid, foo_1.gp_segment_id, DMLAction -> Seq Scan on public.foo foo_1 Output: foo_1.a, foo_1.b, 4, foo_1.ctid, foo_1.gp_segment_id ``` There is no point in using a Split and Update for this as we are updating a non-distribution column which do not require any redistribution. This commit uses an InPlace Update to perform updates on non-distribution columns like Planner. Below is the new plan produced with this commit. New Plan ``` Update on public.foo -> Seq Scan on public.foo foo_1 Output: foo_1.a, 4, foo_1.ctid, foo_1.gp_segment_id Optimizer: Pivotal Optimizer (GPORCA) ``` greenplum 6 specific changes: 1. Some constructors have been changed because the list of arguments in 6X and 7X are different. 2. fixed a bug in CParseHandlerPhysicalDML::startElement where preserve_oids_xml was used instead of fSplit, which could lead to SIGSEGV during DML node parsing. 3. Changed create_index_hot test. Removed disabling the optimizer before updating since ORCA no longer uses split update in this case. (cherry picked from commit 3ced85b)

src/test/isolation2/expected/modify_table_data_corrupt_optimizer.out

BenderArenadata · 2024-01-15T06:04:57Z

Allure report https://allure-ee.adsw.io/launch/61708

BenderArenadata · 2024-01-15T06:15:09Z

Failed job Resource group isolation tests on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/962989

BenderArenadata · 2024-01-15T06:29:41Z

Failed job Resource group isolation tests on ppc64le: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/962990

BenderArenadata · 2024-01-15T08:07:41Z

Failed job Regression tests with ORCA on x86_64: https://gitlab.adsw.io/arenadata/github_mirroring/gpdb/-/jobs/962981

src/backend/executor/nodeModifyTable.c

bimboterminator1 · 2024-01-16T09:08:58Z

src/backend/executor/nodeDML.c

+		/*
+		 * Perform partition selection for in place update
+		 */
+		if (isUpdate && !AttributeNumberIsValid(plannode->actionColIdx))


We don't need to perform partition selection in case of updating leaf partition. Do we?

RekGRpth · 2024-01-17T05:11:40Z

src/backend/executor/nodeDML.c

+		 * Perform partition selection for in place update
+		 */
+		if (isUpdate && !AttributeNumberIsValid(plannode->actionColIdx))
+			node->ps.state->es_result_relation_info =


Do the tests check this code branch?

RekGRpth · 2024-01-17T05:11:55Z

src/backend/executor/nodeModifyTable.c

 		checkPartitionUpdate(estate, slot, resultRelInfo);

+		if (planGen == PLANGEN_OPTIMIZER)
+			slot = reconstructMatchingTupleSlot(slot, resultRelInfo);


Do the tests check this code branch?

KnightMurloc force-pushed the ADBDEV-3951 branch from a199223 to 64582c6 Compare September 1, 2023 08:50

KnightMurloc force-pushed the ADBDEV-3951 branch 3 times, most recently from 6d7cfac to ddc5dde Compare September 4, 2023 07:18

KnightMurloc force-pushed the ADBDEV-3951 branch from ddc5dde to 20e1c18 Compare September 4, 2023 12:01

KnightMurloc force-pushed the ADBDEV-3951 branch from 20e1c18 to 92a9469 Compare September 5, 2023 05:18

KnightMurloc force-pushed the ADBDEV-3951 branch from 92a9469 to f7148c0 Compare September 5, 2023 06:17

KnightMurloc force-pushed the ADBDEV-3951 branch 3 times, most recently from 129e453 to a02d221 Compare December 20, 2023 08:17

RekGRpth reviewed Jan 12, 2024

View reviewed changes

src/backend/gporca/libgpopt/src/operators/CLogicalDML.cpp Show resolved Hide resolved

RekGRpth reviewed Jan 12, 2024

View reviewed changes

src/backend/gporca/libgpopt/src/operators/CLogicalDML.cpp Outdated Show resolved Hide resolved

RekGRpth reviewed Jan 12, 2024

View reviewed changes

src/backend/gporca/libgpopt/src/operators/CLogicalUpdate.cpp Outdated Show resolved Hide resolved

RekGRpth reviewed Jan 12, 2024

View reviewed changes

src/backend/gporca/libnaucrates/src/parser/CParseHandlerPhysicalDML.cpp Show resolved Hide resolved

KnightMurloc force-pushed the ADBDEV-3951 branch from ee9bb4e to c6b7dd6 Compare January 12, 2024 09:09

This comment was marked as resolved.

Sign in to view

KnightMurloc force-pushed the ADBDEV-3951 branch from c6b7dd6 to 0031781 Compare January 15, 2024 04:08

RekGRpth reviewed Jan 15, 2024

View reviewed changes

src/backend/executor/nodeDML.c Show resolved Hide resolved

KnightMurloc force-pushed the ADBDEV-3951 branch from 0031781 to f2124d9 Compare January 15, 2024 05:24

RekGRpth reviewed Jan 15, 2024

View reviewed changes

src/backend/executor/nodeDML.c Outdated Show resolved Hide resolved

KnightMurloc and others added 2 commits January 15, 2024 12:28

KnightMurloc force-pushed the ADBDEV-3951 branch from f2124d9 to c89d8ac Compare January 15, 2024 05:29

RekGRpth reviewed Jan 15, 2024

View reviewed changes

src/test/isolation2/expected/modify_table_data_corrupt_optimizer.out Show resolved Hide resolved

RekGRpth reviewed Jan 16, 2024

View reviewed changes

src/backend/executor/nodeModifyTable.c Show resolved Hide resolved

bimboterminator1 reviewed Jan 16, 2024

View reviewed changes

RekGRpth reviewed Jan 17, 2024

View reviewed changes

ADBDEV-3951: Backport of "Implemented InPlaceUpdate to be used for updates made on non-distribution columns" #609

Are you sure you want to change the base?

ADBDEV-3951: Backport of "Implemented InPlaceUpdate to be used for updates made on non-distribution columns" #609

Conversation

KnightMurloc commented Sep 1, 2023 • edited Loading

BenderArenadata commented Sep 1, 2023

BenderArenadata commented Sep 1, 2023

BenderArenadata commented Sep 4, 2023

BenderArenadata commented Sep 4, 2023

BenderArenadata commented Sep 5, 2023

BenderArenadata commented Sep 5, 2023

RekGRpth commented Sep 6, 2023

BenderArenadata commented Dec 15, 2023

BenderArenadata commented Dec 15, 2023

BenderArenadata commented Dec 15, 2023

BenderArenadata commented Dec 15, 2023

BenderArenadata commented Dec 15, 2023

BenderArenadata commented Dec 20, 2023

BenderArenadata commented Dec 20, 2023

BenderArenadata commented Dec 20, 2023

BenderArenadata commented Dec 20, 2023

RekGRpth commented Dec 20, 2023 • edited Loading

BenderArenadata commented Dec 20, 2023

BenderArenadata commented Dec 20, 2023

KnightMurloc commented Dec 20, 2023

BenderArenadata commented Jan 12, 2024

BenderArenadata commented Jan 12, 2024

BenderArenadata commented Jan 12, 2024

BenderArenadata commented Jan 12, 2024

This comment was marked as resolved.

BenderArenadata commented Jan 12, 2024

BenderArenadata commented Jan 15, 2024

BenderArenadata commented Jan 15, 2024

BenderArenadata commented Jan 15, 2024

BenderArenadata commented Jan 15, 2024

BenderArenadata commented Jan 15, 2024

BenderArenadata commented Jan 15, 2024

BenderArenadata commented Jan 15, 2024

bimboterminator1 Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

RekGRpth Jan 17, 2024

Choose a reason for hiding this comment

RekGRpth Jan 17, 2024

Choose a reason for hiding this comment

KnightMurloc commented Sep 1, 2023 •

edited

Loading

RekGRpth commented Dec 20, 2023 •

edited

Loading

bimboterminator1 Jan 16, 2024 •

edited

Loading