Fix relcache callback handling causing crashes #4193

erimatnor · 2022-03-24T16:19:55Z

Fix a crash that could corrupt indexes when running VACUUM FULL pg_class.

The crash happens when caches are queried/updated within a cache
invalidation function, which can lead to corruption and recursive
cache invalidations.

To solve the issue, make sure the relcache invalidation callback is
simple and never invokes the relcache or syscache directly or
indirectly.

Some background: The extension is preloaded and thus have planner
hooks installed irrespective of whether the extension is actually
installed or not in the current database. However, the hooks need to
be disabled as long as the extension is not installed. To avoid always
having to dynamically check for the presence of the extension, the
state is cached in the session.

However, the cached state needs to be updated if the extension changes
(altered/dropped/created). Therefore, the relcache invalidation
callback mechanism is (ab)used in TimescaleDB to signal updates to the
extension state across all active backends.

The signaling is implemented by installing a dummy table as part of
the extension and any invalidation on the relid for that table signals
a change in the extension state. However, as of this change, the
actual state is no longer determined in the callback itself, since it
requires use of the relcache and causes the bad behavior. Therefore,
the only thing that remains in the callback after this change is to
reset the extension state.

The actual state is instead resolved on-demand, but can still be
cached when the extension is in the installed state and the dummy
table is present with a known relid. However, if the extension is not
installed, the extension state can no longer be cached as there is no
way to signal other backends that the state should be reset when they
don't know the dummy table's relid, and cannot resolve it from within
the callback itself.

Fixes #3924

Disable-Check: Commit-Count

src/cache_invalidate.c

codecov · 2022-03-25T07:18:25Z

Codecov Report

Merging #4193 (1b3f478) into main (566a4ff) will decrease coverage by 0.01%.
The diff coverage is 96.46%.

❗ Current head 1b3f478 differs from pull request most recent head 6cddc41. Consider uploading reports for the commit 6cddc41 to get more accurate results

@@            Coverage Diff             @@
##             main    #4193      +/-   ##
==========================================
- Coverage   90.77%   90.75%   -0.02%     
==========================================
  Files         215      215              
  Lines       39384    39464      +80     
==========================================
+ Hits        35750    35815      +65     
- Misses       3634     3649      +15

Impacted Files	Coverage Δ
src/chunk.h	`100.00% <ø> (ø)`
src/ts_catalog/catalog.h	`100.00% <ø> (ø)`
tsl/src/fdw/data_node_scan_plan.c	`98.21% <ø> (ø)`
src/dimension_vector.c	`86.36% <50.00%> (-0.85%)`	⬇️
src/extension.c	`85.85% <88.23%> (-4.44%)`	⬇️
src/hypertable_restrict_info.c	`95.84% <95.83%> (+0.33%)`	⬆️
src/dimension_slice.c	`93.88% <97.50%> (+0.06%)`	⬆️
src/cache_invalidate.c	`91.48% <100.00%> (-0.35%)`	⬇️
src/chunk.c	`94.16% <100.00%> (-0.64%)`	⬇️
src/chunk_constraint.c	`94.19% <100.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 915bd03...6cddc41. Read the comment docs.

erimatnor · 2022-03-25T08:23:35Z

I tested manually these two things:

That the PR code no longer reproduces the VACUUM FULL pg_class issue in [Bug]: Segmentation Fault when VACUUM FULL pg_class #3924. Any help in trying to reproduce the bug with this code is appreciated. Hopefully no one can.
That extension state tracking still works across multiple sessions:
- E.g., create extension in one session and make sure the state is "created" in another
- Make sure that dropping the extension in one session moves the state to "unknown" in another session.

mkindahl

A few comments, but I need to continue checking.

src/extension_utils.c

src/cache_invalidate.c

src/extension.c

src/cache_invalidate.c

mkindahl

Approving this since the last comment is not critical.

mfundul · 2022-03-28T06:36:31Z

Will look into this PR more seriously as well.

src/cache_invalidate.c

mfundul · 2022-03-28T18:32:18Z

We should probably have a disclaimer in comments above every function that is potentially called in syscache callback context.

Something in the spirit of:

/*
 * WARNING: this function may be called in SysCache invalidation context. No SysCache accesses allowed.
 */

mfundul

Other than the above LGTM.

Add a TAP test that checks that the extensions state is updated across concurrent sessions/backends when the extension is "dropped" or "created".

mfundul · 2022-03-31T17:31:40Z

tsl/src/fdw/relinfo.c

@@ -425,8 +425,7 @@ fdw_relinfo_create(PlannerInfo *root, RelOptInfo *rel, Oid server_oid, Oid local
 	 */
 	fpinfo->fdw_startup_cost = DEFAULT_FDW_STARTUP_COST;
 	fpinfo->fdw_tuple_cost = DEFAULT_FDW_TUPLE_COST;
-	Assert(ts_extension_oid != InvalidOid);
-	fpinfo->shippable_extensions = list_make1_oid(ts_extension_oid);
+	fpinfo->shippable_extensions = list_make1_oid(ts_extension_get_oid());


Maybe add this here:

Assert(OidIsValid(fpinfo->shippable_extensions));

I removed the assertion since the function ts_extension_get_oid() cannot return an invalid OID.

mfundul · 2022-03-31T17:42:56Z

test/t/CMakeLists.txt

+endforeach(P_FILE)
+
+set(MODULE_PATHNAME "$libdir/timescaledb-${PROJECT_VERSION_MOD}")
+configure_file(functions.sql.in functions.sql)


All .sql files we have that use @MODULE_PATHNAME@ don't have an .in extension. We're not very consistent. Also, I think this is the first test script that actually uses @MODULE_PATHNAME@.

Some test sql files use :MODULE_PATHNAME, because it is set at runtime via psql variables. The solution I use here is different and sets the @MODULE_PATHNAME@ to replace it at configuration (cmake) time. This is also why I added the .in extension. I think both are valid approaches, but the runtime-option isn't easily available for TAP tests.

Also, this sql file isn't strictly a test, but rather a library file. But not sure that matters.

If we are inconsistent elsewhere, we can fix that separately.

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#3974 Fix remote EXPLAIN with parameterized queries * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * #3974 Fix remote EXPLAIN with parameterized queries * #4122 Fix segfault on INSERT into distributed hypertable * #4142 Ignore invalid relid when deleting hypertable * #4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * #4161 Fix memory handling during scans * #4186 Fix owner change for distributed hypertable * #4192 Abort sessions after extension reload * #4193 Fix relcache callback handling causing crashes **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is a patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * timescale#4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * timescale#4122 Fix segfault on INSERT into distributed hypertable * timescale#4142 Ignore invalid relid when deleting hypertable * timescale#4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * timescale#4161 Fix memory handling during scans * timescale#4176 Fix remote EXPLAIN with parameterized queries * timescale#4181 Fix spelling errors and omissions * timescale#4186 Fix owner change for distributed hypertable * timescale#4192 Abort sessions after extension reload * timescale#4193 Fix relcache callback handling causing crashes * timescale#4199 Remove signal-unsafe calls from signal handlers * timescale#4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

@abrownsword

This release is a patch release. We recommend that you upgrade at the next available opportunity. **Bugfixes** * #4121 Fix RENAME TO/SET SCHEMA on distributed hypertable * #4122 Fix segfault on INSERT into distributed hypertable * #4142 Ignore invalid relid when deleting hypertable * #4159 Fix ADD COLUMN IF NOT EXISTS error on compressed hypertable * #4161 Fix memory handling during scans * #4176 Fix remote EXPLAIN with parameterized queries * #4181 Fix spelling errors and omissions * #4186 Fix owner change for distributed hypertable * #4192 Abort sessions after extension reload * #4193 Fix relcache callback handling causing crashes * #4199 Remove signal-unsafe calls from signal handlers * #4219 Do not modify aggregation state in finalize **Thanks** * @abrownsword for reporting a crash in the telemetry reporter * @daydayup863 for reporting issue with remote explain

erimatnor added bug tech-debt Needs refactoring and improvement tasks related to the source code and its architecture. labels Mar 24, 2022

erimatnor self-assigned this Mar 24, 2022

erimatnor force-pushed the fix-vacuum-full-crash branch 2 times, most recently from 15e8b72 to 343726c Compare March 24, 2022 16:27

mfundul reviewed Mar 24, 2022

View reviewed changes

src/cache_invalidate.c Outdated Show resolved Hide resolved

erimatnor force-pushed the fix-vacuum-full-crash branch from 343726c to 7fb64f1 Compare March 25, 2022 07:09

erimatnor requested a review from mkindahl March 25, 2022 07:29

erimatnor force-pushed the fix-vacuum-full-crash branch from 7fb64f1 to ecf804f Compare March 25, 2022 07:31

erimatnor marked this pull request as ready for review March 25, 2022 08:25

erimatnor requested a review from a team as a code owner March 25, 2022 08:25

erimatnor requested review from afiskon, pmwkaa, fabriziomello and nikkhils and removed request for a team March 25, 2022 08:25

mkindahl reviewed Mar 25, 2022

View reviewed changes

erimatnor force-pushed the fix-vacuum-full-crash branch from ecf804f to 1b3f478 Compare March 25, 2022 12:20

mkindahl reviewed Mar 25, 2022

View reviewed changes

src/extension.c Show resolved Hide resolved

src/extension.c Show resolved Hide resolved

src/cache_invalidate.c Show resolved Hide resolved

src/cache_invalidate.c Show resolved Hide resolved

mkindahl approved these changes Mar 28, 2022

View reviewed changes

mfundul mentioned this pull request Mar 28, 2022

Abort sessions after extension reload #4192

Merged

mfundul reviewed Mar 28, 2022

View reviewed changes

src/cache_invalidate.c Show resolved Hide resolved

mfundul approved these changes Mar 29, 2022

View reviewed changes

erimatnor force-pushed the fix-vacuum-full-crash branch 3 times, most recently from 6cddc41 to e8c8857 Compare March 30, 2022 13:19

erimatnor force-pushed the fix-vacuum-full-crash branch 2 times, most recently from 8366543 to 9e32a92 Compare March 31, 2022 11:48

Add TAP tests for extension state

43bedf3

Add a TAP test that checks that the extensions state is updated across concurrent sessions/backends when the extension is "dropped" or "created".

erimatnor force-pushed the fix-vacuum-full-crash branch from 9e32a92 to 43bedf3 Compare March 31, 2022 11:53

mfundul self-requested a review March 31, 2022 12:15

mfundul reviewed Mar 31, 2022

View reviewed changes

erimatnor merged commit 972afe0 into timescale:main Apr 1, 2022

erimatnor deleted the fix-vacuum-full-crash branch April 1, 2022 06:31

NunoFilipeSantos modified the milestones: TimescaleDB 2.7, TimescaleDB 2.6.1 Apr 2, 2022

This was referenced Apr 5, 2022

Release 2.6.1 #4212

Merged

Release 2.6.1 #4213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix relcache callback handling causing crashes #4193

Fix relcache callback handling causing crashes #4193

erimatnor commented Mar 24, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading

erimatnor commented Mar 25, 2022 •

edited

Loading

mkindahl left a comment

mkindahl left a comment

mfundul commented Mar 28, 2022

mfundul commented Mar 28, 2022

mfundul left a comment

mfundul Mar 31, 2022

erimatnor Apr 1, 2022

mfundul Mar 31, 2022

erimatnor Apr 1, 2022 •

edited

Loading

Fix relcache callback handling causing crashes #4193

Fix relcache callback handling causing crashes #4193

Conversation

erimatnor commented Mar 24, 2022 • edited Loading

codecov bot commented Mar 25, 2022 • edited Loading

Codecov Report

erimatnor commented Mar 25, 2022 • edited Loading

mkindahl left a comment

Choose a reason for hiding this comment

mkindahl left a comment

Choose a reason for hiding this comment

mfundul commented Mar 28, 2022

mfundul commented Mar 28, 2022

mfundul left a comment

Choose a reason for hiding this comment

mfundul Mar 31, 2022

Choose a reason for hiding this comment

erimatnor Apr 1, 2022

Choose a reason for hiding this comment

mfundul Mar 31, 2022

Choose a reason for hiding this comment

erimatnor Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

erimatnor commented Mar 24, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading

erimatnor commented Mar 25, 2022 •

edited

Loading

erimatnor Apr 1, 2022 •

edited

Loading