Skip to content

Commit

Permalink
[BACKPORT pg15-cherrypicks][#18822] YSQL: Write out unmodified cols t…
Browse files Browse the repository at this point in the history
…o main table when update optimizations are enabled

Summary:
**Conflict resolution for PG 15 cherrypick**

- src/postgres/src/backend/executor/nodeModifyTable.c
    - Location: ExecUpdate —> around `else if (IsYBRelation(resultRelationDesc))`
        - YB master 8bae4881dc89205c53ee3c62668c41347394af37 updates the comment explaining what `cols_marked_for_update` does.
        - In YB pg15 be8504df264aff0472e7e91264b095a30d068bf2, this code has moved into the function YBExecUpdateAct.
        - Merge resolution: Keep changes from YB pg15, manually update the comment in YBExecUpdateAct.
    - Location: ExecUpdate —> around `if (updateCxt.crossPartUpdate)`
        - YB master 8bae4881dc89205c53ee3c62668c41347394af37 frees the bitmapset `cols_marked_for_update`.
        - In YB pg15 be8504df264aff0472e7e91264b095a30d068bf2, this code has moved into the function YBExecUpdateAct.
        - Merge resolution: Keep changes from YB pg15, manually free the bitmapset in YBExecUpdateAct.

D34040 introduced a framework to skip redundant secondary index updates and foreign key checks.
As a part of the implementation, this revision skipped writing out unmodified columns to the main table.
For correctness reasons, skipping writes of specific columns to the main table requires the acquisition of row-level locks.
(In the event that no columns are modified, one is still required to acquire a row lock on the affected row)

This created a dependency between the update optimization and the row locking feature.
The latter is controlled by the autoflag `ysql_skip_row_lock_for_update`.
This revision seeks to remove this dependency by continuing to write out unmodified columns to the main table.

There is one notable exception to this behavior: unmodified columns that are a part of the primary key.
If the value of the primary key remains unmodified, its columns are not written out to the main table as this would require an extra round trip to the storage layer.
Jira: DB-7701

Original commit: 8bae488 / D37545

Test Plan:
Run the following tests and ensure that there are no regressions:
```
./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressUpdateOptimized#schedule'
```

Reviewers: jason, tfoucher

Reviewed By: jason

Subscribers: yql, smishra

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D37812
  • Loading branch information
karthik-ramanathan-3006 committed Sep 6, 2024
1 parent 230591b commit 6f83cd8
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 9 deletions.
17 changes: 12 additions & 5 deletions src/postgres/src/backend/executor/nodeModifyTable.c
Original file line number Diff line number Diff line change
Expand Up @@ -2392,11 +2392,17 @@ yb_lreplace:;
resultRelInfo->ri_TrigDesc->trig_update_before_row;

/*
* A bitmapset of columns that have been marked as being updated at
* planning time. This set needs to be updated below if either:
* - The update optimization comparing new and old values to identify
* actually modified columns is enabled.
* - Before row triggers were fired.
* A bitmapset of columns whose values are written to the main table.
* This is initialized to the set of columns marked for update at
* planning time. This set is updated as follows:
* - Columns modified by before row triggers are added.
* - Primary key columns in the setlist that are unmodified are removed.
*
* Maintaining this bitmapset allows us to continue writing out
* unmodified columns to the main table as a safety guardrail, while
* selectively skipping index updates and constraint checks.
* This guardrail may be removed in the future. This also helps avoid
* having a dependency on row locking.
*/
Bitmapset *cols_marked_for_update = bms_copy(rte->updatedCols);

Expand Down Expand Up @@ -2463,6 +2469,7 @@ yb_lreplace:;
? YB_SINGLE_SHARD_TRANSACTION : YB_TRANSACTIONAL,
cols_marked_for_update, canSetTag);

bms_free(cols_marked_for_update);
return row_found;
}

Expand Down
16 changes: 14 additions & 2 deletions src/postgres/src/backend/executor/ybOptimizeModifyTable.c
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ YbComputeModifiedEntities(ResultRelInfo *resultRelInfo, HeapTuple oldtuple,
* skipped as a result of unmodified columns.
* A list of modified columns (updated_cols) is required:
* - To construct the payload of updated values to be send to the storage layer.
* - To determine if column-specific after-row triggers are elgible to fire.
* - To determine if column-specific after-row triggers are eligible to fire.
* ----------------------------------------------------------------
*/
void
Expand Down Expand Up @@ -532,9 +532,21 @@ YbComputeModifiedColumnsAndSkippableEntities(
plan->onConflictAction == ONCONFLICT_UPDATE));
}

*updated_cols = bms_del_members(*updated_cols, unmodified_cols);
*updated_cols = bms_add_members(*updated_cols, modified_cols);

/*
* Exclude unmodified primary key columns from the set of main table columns
* to be updated. This potentially prevents an extra RPC to the main table.
* Do not bother if the any part of the primary key is modified.
*/
if (!bms_overlap(YBGetTablePrimaryKeyBms(rel), modified_cols))
{
Bitmapset *unmodified_pkcols =
bms_intersect(YBGetTablePrimaryKeyBms(rel), unmodified_cols);
*updated_cols = bms_del_members(*updated_cols, unmodified_pkcols);
bms_free(unmodified_pkcols);
}

YbLogInspectedColumns(rel, *updated_cols, modified_cols, unmodified_cols);

bms_free(unmodified_cols);
Expand Down
3 changes: 1 addition & 2 deletions src/postgres/src/backend/utils/misc/pg_yb_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -5158,8 +5158,7 @@ bool
YbIsUpdateOptimizationEnabled()
{
/* TODO(kramanathan): Placeholder until a flag strategy is agreed upon */
return (!YBCIsEnvVarTrue("FLAGS_ysql_skip_row_lock_for_update")) &&
yb_update_optimization_options.num_cols_to_compare > 0 &&
return yb_update_optimization_options.num_cols_to_compare > 0 &&
yb_update_optimization_options.max_cols_size_to_compare > 0;
}

Expand Down

0 comments on commit 6f83cd8

Please sign in to comment.