Fix ERROR when acquiring a tuple lock on OSM chunks on replica #7035

zilder · 2024-06-17T11:04:20Z

Planner attempts acquiring a LockTupleKeyShare tuple lock on an OSM chunk's dimension slice which results in an error on
replica. This patch fixes the issue by avoiding tuple locking when in recovery mode.

Fix for timescale/Support-Dev-Collab#1847

Disable-check: force-changelog-file

codecov · 2024-06-17T11:13:55Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.74%. Comparing base (59f50f2) to head (673b7d4).
Report is 212 commits behind head on main.

Files	Patch %	Lines
src/hypertable.c	0.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7035      +/-   ##
==========================================
+ Coverage   80.06%   81.74%   +1.67%     
==========================================
  Files         190      199       +9     
  Lines       37181    37011     -170     
  Branches     9450     9676     +226     
==========================================
+ Hits        29770    30253     +483     
+ Misses       2997     2859     -138     
+ Partials     4414     3899     -515

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/hypertable.c

mkindahl

The commit description is not very clear. It does fix an error, but a better description would be a functional one about what the pull request actually does, which is something along the lines "Do not lock tuples in OSM chunks on replica servers".

It would also be helpful to add comments describing why, so that your future self knows why you added it.

mkindahl · 2024-06-17T13:36:35Z

src/hypertable.c

@@ -2560,6 +2563,7 @@ ts_hypertable_osm_range_update(PG_FUNCTION_ARGS)
 	bool overlap = false, range_invalid = false;

 	/* Lock tuple FOR UPDATE */
+	Assert(!RecoveryInProgress());


The assert cannot be used as a check in production deployments and is no guarantee that it will never be reached.

What happen if we run this code on a secondary that is in recovery? Will that cause a production issue?

If so, we should have a check in place so that the tuple is not locked when the server is in recovery, but lock it otherwise.

Thanks for the review! I changed the commit message and added a comment.

The assert cannot be used as a check in production deployments and is no guarantee that it will never be reached.

It wasn't meant as a runtime check, rather a documentation for myself and other devs. (Production images are not compiled with asserts enabled, right?). I added a comment there for clarity. This function would not normally be reached from the osm code: on replica it would fail before this call. But even when called directly from psql, it would not cause crash, it would fail with the same error as in the original issue:

ERROR: cannot assign TransactionIds during recovery

when trying to update the catalog.

I can remove the assert altogether though. I only added it because this func calls ts_chunk_get_osm_slice_and_lock and it should not happen in recovery mode, so that we could catch it early if it happens.

Thanks for the review! I changed the commit message and added a comment.

The assert cannot be used as a check in production deployments and is no guarantee that it will never be reached.

It wasn't meant as a runtime check, rather a documentation for myself and other devs. (Production images are not compiled with asserts enabled, right?). I added a comment there for clarity. This function would not normally be reached from the osm code: on replica it would fail before this call. But even when called directly from psql, it would not cause crash, it fail with the same error as in the original issue:

ERROR: cannot assign TransactionIds during recovery

when trying to update catalog.

Thank you for explaining it.

I can remove the assert altogether though. I only added it because this func calls ts_chunk_get_osm_slice_and_lock and it should not happen in recovery mode, so that we could catch it early if it happens.

No, keeping asserts is good for documentation purposes and declaring intentions and help debugging, as you mentioned above. I was just unsure if this meant that the code after would execute in production builds, causing havoc. If that cannot happen, a comment on why this cannot happen and a test (if possible) is good enough to ensure that we do not try to execute bad code in production builds.

mkindahl · 2024-06-18T10:36:24Z

src/hypertable.c

+	/* this function is not meant to be run on a read-only secondary */
+	Assert(!RecoveryInProgress());
+


For your future self, could you extend the comment to describe why we can't reach this point in release builds?

Thanks! Will do

gayyappan · 2024-06-18T13:26:08Z

src/hypertable.c

@@ -2461,14 +2461,23 @@ ts_chunk_get_osm_slice_and_lock(int32 osm_chunk_id, int32 time_dim_id, LockTuple
 				.lockmode = tuplockmode,
 				.waitpolicy = LockWaitBlock,
 			};
+			/*
+			 * We cannot acquire a tuple lock when running in recovery mode


Suggested change

* We cannot acquire a tuple lock when running in recovery mode

it is not possible to acquire tuple lock when running in recovery mode as they require a transaction id and transaction ids cannot be created in recovery mode, so we only acquire the tuple lock if we are not in recovery mode.

Added some more detail to the comment to explain why tuple locks cannot be acquired in recovery mode.

Added a clarification

Planner attempts acquiring a `LockTupleKeyShare` tuple lock on an OSM chunk's dimension slice which results in an error on replica. This patch removes tuple lock when in recovery mode.

For PRs timescale#7088 and timescale#7035 Some more edits for docs compliance.

@Kazmirchuk

This release contains bug fixes since the 2.15.2 release. Best practice is to upgrade at the next available opportunity. **Migrating from self-hosted TimescaleDB v2.14.x and earlier** After you run `ALTER EXTENSION`, you must run [this SQL script](https://github.com/timescale/timescaledb-extras/blob/master/utils/2.15.X-fix_hypertable_foreign_keys.sql). For more details, see the following pull request [#6797](#6797). If you are migrating from TimescaleDB v2.15.0, v2.15.1 or v2.15.2, no changes are required. **Bugfixes** * #7061: Fix the handling of multiple unique indexes in a compressed INSERT. * #7080: Fix the `corresponding equivalence member not found` error. * #7088: Fix the leaks in the DML functions. * #7035: Fix the error when acquiring a tuple lock on the OSM chunks on the replica. **Thanks** * @Kazmirchuk for reporting the issue about leaks with the functions in DML.

@Kazmirchuk

This release contains bug fixes since the 2.15.2 release. Best practice is to upgrade at the next available opportunity. **Migrating from self-hosted TimescaleDB v2.14.x and earlier** After you run `ALTER EXTENSION`, you must run [this SQL script](https://github.com/timescale/timescaledb-extras/blob/master/utils/2.15.X-fix_hypertable_foreign_keys.sql). For more details, see the following pull request [#6797](#6797). If you are migrating from TimescaleDB v2.15.0, v2.15.1 or v2.15.2, no changes are required. **Bugfixes** * #7061: Fix the handling of multiple unique indexes in a compressed INSERT. * #7080: Fix the `corresponding equivalence member not found` error. * #7088: Fix the leaks in the DML functions. * #7035: Fix the error when acquiring a tuple lock on the OSM chunks on the replica. * #7091: Fix ORDER BY/GROUP BY expression not found in targetlist on PG16 **Thanks** * @Kazmirchuk for reporting the issue about leaks with the functions in DML. --------- Signed-off-by: Sven Klemm <31455525+svenklemm@users.noreply.github.com> Co-authored-by: Sven Klemm <31455525+svenklemm@users.noreply.github.com>

zilder marked this pull request as ready for review June 17, 2024 12:07

zilder requested a review from nikkhils June 17, 2024 12:08

gayyappan reviewed Jun 17, 2024

View reviewed changes

src/hypertable.c Show resolved Hide resolved

gayyappan requested review from antekresic and mkindahl June 17, 2024 13:29

mkindahl reviewed Jun 17, 2024

View reviewed changes

zilder force-pushed the zilder/fix_replica_osm_tuple_lock branch from 97ca75d to 35380e7 Compare June 18, 2024 10:08

zilder requested review from gayyappan and mkindahl June 18, 2024 10:30

mkindahl reviewed Jun 18, 2024

View reviewed changes

mkindahl approved these changes Jun 18, 2024

View reviewed changes

zilder force-pushed the zilder/fix_replica_osm_tuple_lock branch from 35380e7 to b9566b6 Compare June 18, 2024 13:15

gayyappan reviewed Jun 18, 2024

View reviewed changes

Do not lock OSM chunk's tuple on replica instances

673b7d4

Planner attempts acquiring a `LockTupleKeyShare` tuple lock on an OSM chunk's dimension slice which results in an error on replica. This patch removes tuple lock when in recovery mode.

zilder force-pushed the zilder/fix_replica_osm_tuple_lock branch from b9566b6 to 673b7d4 Compare June 18, 2024 14:27

zilder requested a review from gayyappan June 18, 2024 14:28

gayyappan approved these changes Jun 18, 2024

View reviewed changes

zilder merged commit f68006b into timescale:main Jun 18, 2024
38 of 40 checks passed

gayyappan added this to the TimescaleDB 2.15.3 milestone Jun 29, 2024

svenklemm added backported-2.15.x bug and removed backported-2.15.x labels Jul 1, 2024

timescale-automation mentioned this pull request Jul 1, 2024

Backport to 2.15.x: #7035: Fix ERROR when acquiring a tuple lock on OSM chunks on replica #7082

Merged

timescale-automation added the backported-2.15.x label Jul 1, 2024

pallavisontakke added a commit to pallavisontakke/timescaledb that referenced this pull request Jul 2, 2024

Edit CHANGELOG.md

4b96417

For PRs timescale#7088 and timescale#7035 Some more edits for docs compliance.

pallavisontakke mentioned this pull request Jul 2, 2024

Release 2.15.3 #7089

Merged

pallavisontakke mentioned this pull request Jul 2, 2024

Release 2.15.3 #7092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ERROR when acquiring a tuple lock on OSM chunks on replica #7035

Fix ERROR when acquiring a tuple lock on OSM chunks on replica #7035

zilder commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 17, 2024 •

edited

Loading

mkindahl left a comment

mkindahl Jun 17, 2024

zilder Jun 18, 2024 •

edited

Loading

mkindahl Jun 18, 2024

mkindahl Jun 18, 2024 •

edited

Loading

zilder Jun 18, 2024

gayyappan Jun 18, 2024

gayyappan Jun 18, 2024

zilder Jun 18, 2024

		/* this function is not meant to be run on a read-only secondary */
		Assert(!RecoveryInProgress());

	* We cannot acquire a tuple lock when running in recovery mode
	it is not possible to acquire tuple lock when running in recovery mode as they require a transaction id and transaction ids cannot be created in recovery mode, so we only acquire the tuple lock if we are not in recovery mode.

Fix ERROR when acquiring a tuple lock on OSM chunks on replica #7035

Fix ERROR when acquiring a tuple lock on OSM chunks on replica #7035

Conversation

zilder commented Jun 17, 2024 • edited Loading

codecov bot commented Jun 17, 2024 • edited Loading

Codecov Report

mkindahl left a comment

Choose a reason for hiding this comment

mkindahl Jun 17, 2024

Choose a reason for hiding this comment

zilder Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

mkindahl Jun 18, 2024

Choose a reason for hiding this comment

mkindahl Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

zilder Jun 18, 2024

Choose a reason for hiding this comment

gayyappan Jun 18, 2024

Choose a reason for hiding this comment

gayyappan Jun 18, 2024

Choose a reason for hiding this comment

zilder Jun 18, 2024

Choose a reason for hiding this comment

zilder commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 17, 2024 •

edited

Loading

zilder Jun 18, 2024 •

edited

Loading

mkindahl Jun 18, 2024 •

edited

Loading