Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DirectFunctionCall crash in distributed_exec #3727

Conversation

erimatnor
Copy link
Contributor

@erimatnor erimatnor commented Oct 22, 2021

This change fixes a crash that occurred when calling
distributed_exec via a direct function call.

The crash was triggered by a dynamic lookup of the function name via
the function Oid in the FunctionCallInfo struct in order to generate
error messages for read-only and transaction block checks. However,
this information is provided by the parsing stage, which is not
executed when doing direct function calls, thus leading to a
segmentation fault when trying to dereference a pointer in the
FunctionCallInfo that wasn't set.

Note that this problem is not limited to distributed_exec; it is
present in all SQL-callable functions that use the same pattern and
macros.

To fix the problem, update the macros and patterns used for checking
for read-only mode and transaction blocks to avoid doing the function
name lookup when the pointer is not set. Instead fall back to the C
function name in that case (via C macro __func__).

A test case is added in C code to call distributed_exec via a direct
function call within a transaction block in order to hit the
previously crashing error message.

The distributed_exec function has also been updated with better
handling of input parameters, like empty arrays of data nodes, or
arrays containing NULL elements.

@erimatnor erimatnor added the bug label Oct 22, 2021
@erimatnor erimatnor self-assigned this Oct 22, 2021
@erimatnor erimatnor requested a review from a team as a code owner October 22, 2021 07:57
@erimatnor erimatnor force-pushed the fix-read-only-transaction-block-prevention branch 2 times, most recently from 804a703 to 5089fdd Compare October 22, 2021 08:02
@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #3727 (ddebd5b) into master (221437e) will increase coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3727   +/-   ##
=======================================
  Coverage   90.29%   90.29%           
=======================================
  Files         212      213    +1     
  Lines       37628    37652   +24     
=======================================
+ Hits        33976    33998   +22     
- Misses       3652     3654    +2     
Impacted Files Coverage Δ
src/compat/compat.h 94.28% <ø> (ø)
src/process_utility.c 94.34% <ø> (ø)
src/utils.h 80.00% <ø> (ø)
tsl/test/src/test_dist_util.c 85.71% <ø> (ø)
tsl/src/remote/dist_commands.c 92.78% <81.81%> (+0.31%) ⬆️
tsl/test/src/test_dist_commands.c 84.61% <84.61%> (ø)
tsl/src/continuous_aggs/invalidation.c 96.00% <100.00%> (+0.02%) ⬆️
tsl/src/data_node.c 95.47% <100.00%> (ø)

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 6d9715a...ddebd5b. Read the comment docs.

Copy link
Contributor

@pmwkaa pmwkaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

src/utils.h Outdated Show resolved Hide resolved
src/utils.h Outdated Show resolved Hide resolved
tsl/src/remote/dist_commands.c Show resolved Hide resolved
tsl/test/expected/dist_commands.out Show resolved Hide resolved
@erimatnor erimatnor force-pushed the fix-read-only-transaction-block-prevention branch 2 times, most recently from 3a12164 to 112db15 Compare October 26, 2021 06:57
Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This change fixes a crash that occurred when calling
`distributed_exec` via a direct function call.

The crash was triggered by a dynamic lookup of the function name via
the function Oid in the `FunctionCallInfo` struct in order to generate
error messages for read-only and transaction block checks. However,
this information is provided by the parsing stage, which is not
executed when doing direct function calls, thus leading to a
segmentation fault when trying to dereference a pointer in the
`FunctionCallInfo` that wasn't set.

Note that this problem is not limited to `distributed_exec`; it is
present in all SQL-callable functions that use the same pattern and
macros.

To fix the problem, update the macros and patterns used for checking
for read-only mode and transaction blocks to avoid doing the function
name lookup when the pointer is not set. Instead fall back to the C
function name in that case (via C macro `__func__`).

A test case is added in C code to call `distributed_exec` via a direct
function call within a transaction block in order to hit the
previously crashing error message.

The `distributed_exec` function has also been updated with better
handling of input parameters, like empty arrays of data nodes, or
arrays containing NULL elements.
@erimatnor erimatnor force-pushed the fix-read-only-transaction-block-prevention branch from 112db15 to ddebd5b Compare October 26, 2021 11:10
@erimatnor erimatnor merged commit d740e19 into timescale:master Oct 26, 2021
@erimatnor erimatnor deleted the fix-read-only-transaction-block-prevention branch October 26, 2021 13:21
fabriziomello added a commit to fabriziomello/timescaledb that referenced this pull request Oct 27, 2021
This release adds major new features since the 2.4.2 release.
We deem it moderate priority for upgrading.

This release includes these noteworthy features:

* Continuous Aggregates for Distributed Hypertables
* Support for PostgreSQL 14
* Experimental: Support for timezones in `time_bucket_ng()`, including
the `origin` argument

This release also includes several bug fixes.

**Features**
* timescale#3034 Add support for PostgreSQL 14
* timescale#3435 Add continuous aggregates for distributed hypertables
* timescale#3505 Add support for timezones in `time_bucket_ng()`
* timescale#3598 Improve evaluation of stable functions such as now() on access node
* timescale#3717 Support transparent decompression on individual chunks

**Bugfixes**
* timescale#3580 Fix memory context bug executing TRUNCATE
* timescale#3592 Allow alter column type on distributed hypertable
* timescale#3618 Fix execution of refresh_caggs from user actions
* timescale#3625 Add shared dependencies when creating chunk
* timescale#3626 Fix memory context bug executing TRUNCATE
* timescale#3627 Schema qualify UDTs in multi-node
* timescale#3638 Allow owner change of a data node
* timescale#3654 Fix index attnum mapping in reorder_chunk
* timescale#3661 Fix SkipScan path generation with constant DISTINCT column
* timescale#3667 Fix compress_policy for multi txn handling
* timescale#3673 Fix distributed hypertable DROP within a procedure
* timescale#3701 Allow anyone to use size utilities on distributed hypertables
* timescale#3708 Fix crash in get_aggsplit
* timescale#3709 Fix ordered append pathkey check
* timescale#3712 Fix GRANT/REVOKE ALL IN SCHEMA handling
* timescale#3724 Fix inserts into compressed chunks on hypertables with caggs
* timescale#3727 Fix DirectFunctionCall crash in distributed_exec
* timescale#3728 Fix SkipScan with varchar column
* timescale#3733 Fix ANALYZE crash with custom statistics for custom types
* timescale#3747 Always reset expr context in DecompressChunk

**Thanks**
* @binakot and @sebvett for reporting an issue with DISTINCT queries
* @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE
* @mjf for reporting an issue with ordered append and JOINs
* @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now()
* @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg
* @tanglebones for reporting the ANALYZE crash with custom types on multinode
@fabriziomello fabriziomello mentioned this pull request Oct 27, 2021
fabriziomello added a commit to fabriziomello/timescaledb that referenced this pull request Oct 27, 2021
This release adds major new features since the 2.4.2 release.
We deem it moderate priority for upgrading.

This release includes these noteworthy features:

* Continuous Aggregates for Distributed Hypertables
* Support for PostgreSQL 14
* Experimental: Support for timezones in `time_bucket_ng()`, including
the `origin` argument

This release also includes several bug fixes.

**Features**
* timescale#3034 Add support for PostgreSQL 14
* timescale#3435 Add continuous aggregates for distributed hypertables
* timescale#3505 Add support for timezones in `time_bucket_ng()`

**Bugfixes**
* timescale#3580 Fix memory context bug executing TRUNCATE
* timescale#3592 Allow alter column type on distributed hypertable
* timescale#3598 Improve evaluation of stable functions such as now() on access
node
* timescale#3618 Fix execution of refresh_caggs from user actions
* timescale#3625 Add shared dependencies when creating chunk
* timescale#3626 Fix memory context bug executing TRUNCATE
* timescale#3627 Schema qualify UDTs in multi-node
* timescale#3638 Allow owner change of a data node
* timescale#3654 Fix index attnum mapping in reorder_chunk
* timescale#3661 Fix SkipScan path generation with constant DISTINCT column
* timescale#3667 Fix compress_policy for multi txn handling
* timescale#3673 Fix distributed hypertable DROP within a procedure
* timescale#3701 Allow anyone to use size utilities on distributed hypertables
* timescale#3708 Fix crash in get_aggsplit
* timescale#3709 Fix ordered append pathkey check
* timescale#3712 Fix GRANT/REVOKE ALL IN SCHEMA handling
* timescale#3717 Support transparent decompression on individual chunks
* timescale#3724 Fix inserts into compressed chunks on hypertables with caggs
* timescale#3727 Fix DirectFunctionCall crash in distributed_exec
* timescale#3728 Fix SkipScan with varchar column
* timescale#3733 Fix ANALYZE crash with custom statistics for custom types
* timescale#3747 Always reset expr context in DecompressChunk

**Thanks**
* @binakot and @sebvett for reporting an issue with DISTINCT queries
* @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE
* @mjf for reporting an issue with ordered append and JOINs
* @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now()
* @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg
* @tanglebones for reporting the ANALYZE crash with custom types on multinode
fabriziomello added a commit that referenced this pull request Oct 27, 2021
This release adds major new features since the 2.4.2 release.
We deem it moderate priority for upgrading.

This release includes these noteworthy features:

* Continuous Aggregates for Distributed Hypertables
* Support for PostgreSQL 14
* Experimental: Support for timezones in `time_bucket_ng()`, including
the `origin` argument

This release also includes several bug fixes.

**Features**
* #3034 Add support for PostgreSQL 14
* #3435 Add continuous aggregates for distributed hypertables
* #3505 Add support for timezones in `time_bucket_ng()`

**Bugfixes**
* #3580 Fix memory context bug executing TRUNCATE
* #3592 Allow alter column type on distributed hypertable
* #3598 Improve evaluation of stable functions such as now() on access
node
* #3618 Fix execution of refresh_caggs from user actions
* #3625 Add shared dependencies when creating chunk
* #3626 Fix memory context bug executing TRUNCATE
* #3627 Schema qualify UDTs in multi-node
* #3638 Allow owner change of a data node
* #3654 Fix index attnum mapping in reorder_chunk
* #3661 Fix SkipScan path generation with constant DISTINCT column
* #3667 Fix compress_policy for multi txn handling
* #3673 Fix distributed hypertable DROP within a procedure
* #3701 Allow anyone to use size utilities on distributed hypertables
* #3708 Fix crash in get_aggsplit
* #3709 Fix ordered append pathkey check
* #3712 Fix GRANT/REVOKE ALL IN SCHEMA handling
* #3717 Support transparent decompression on individual chunks
* #3724 Fix inserts into compressed chunks on hypertables with caggs
* #3727 Fix DirectFunctionCall crash in distributed_exec
* #3728 Fix SkipScan with varchar column
* #3733 Fix ANALYZE crash with custom statistics for custom types
* #3747 Always reset expr context in DecompressChunk

**Thanks**
* @binakot and @sebvett for reporting an issue with DISTINCT queries
* @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE
* @mjf for reporting an issue with ordered append and JOINs
* @phemmer for reporting the issues on multinode with aggregate queries and evaluation of now()
* @abolognino for reporting an issue with INSERTs into compressed hypertables that have cagg
* @tanglebones for reporting the ANALYZE crash with custom types on multinode
fabriziomello added a commit that referenced this pull request Oct 27, 2021
This release adds major new features since the 2.4.2 release.
We deem it moderate priority for upgrading.

This release includes these noteworthy features:

* Continuous Aggregates for Distributed Hypertables
* Support for PostgreSQL 14
* Experimental: Support for timezones in `time_bucket_ng()`, including
the `origin` argument

This release also includes several bug fixes.

**Features**
* #3034 Add support for PostgreSQL 14
* #3435 Add continuous aggregates for distributed hypertables
* #3505 Add support for timezones in `time_bucket_ng()`

**Bugfixes**
* #3580 Fix memory context bug executing TRUNCATE
* #3592 Allow alter column type on distributed hypertable
* #3598 Improve evaluation of stable functions such as now() on access
node
* #3618 Fix execution of refresh_caggs from user actions
* #3625 Add shared dependencies when creating chunk
* #3626 Fix memory context bug executing TRUNCATE
* #3627 Schema qualify UDTs in multi-node
* #3638 Allow owner change of a data node
* #3654 Fix index attnum mapping in reorder_chunk
* #3661 Fix SkipScan path generation with constant DISTINCT column
* #3667 Fix compress_policy for multi txn handling
* #3673 Fix distributed hypertable DROP within a procedure
* #3701 Allow anyone to use size utilities on distributed hypertables
* #3708 Fix crash in get_aggsplit
* #3709 Fix ordered append pathkey check
* #3712 Fix GRANT/REVOKE ALL IN SCHEMA handling
* #3717 Support transparent decompression on individual chunks
* #3724 Fix inserts into compressed chunks on hypertables with caggs
* #3727 Fix DirectFunctionCall crash in distributed_exec
* #3728 Fix SkipScan with varchar column
* #3733 Fix ANALYZE crash with custom statistics for custom types
* #3747 Always reset expr context in DecompressChunk

**Thanks**
* @binakot and @sebvett for reporting an issue with DISTINCT queries
* @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE
* @mjf for reporting an issue with ordered append and JOINs
* @phemmer for reporting the issues on multinode with aggregate queries
and evaluation of now()
* @abolognino for reporting an issue with INSERTs into compressed
hypertables that have cagg
* @tanglebones for reporting the ANALYZE crash with custom types on
multinode
fabriziomello added a commit that referenced this pull request Oct 27, 2021
This release adds major new features since the 2.4.2 release.
We deem it moderate priority for upgrading.

This release includes these noteworthy features:

* Continuous Aggregates for Distributed Hypertables
* Support for PostgreSQL 14
* Experimental: Support for timezones in `time_bucket_ng()`, including
the `origin` argument

This release also includes several bug fixes.

**Features**
* #3034 Add support for PostgreSQL 14
* #3435 Add continuous aggregates for distributed hypertables
* #3505 Add support for timezones in `time_bucket_ng()`

**Bugfixes**
* #3580 Fix memory context bug executing TRUNCATE
* #3592 Allow alter column type on distributed hypertable
* #3598 Improve evaluation of stable functions such as now() on access
node
* #3618 Fix execution of refresh_caggs from user actions
* #3625 Add shared dependencies when creating chunk
* #3626 Fix memory context bug executing TRUNCATE
* #3627 Schema qualify UDTs in multi-node
* #3638 Allow owner change of a data node
* #3654 Fix index attnum mapping in reorder_chunk
* #3661 Fix SkipScan path generation with constant DISTINCT column
* #3667 Fix compress_policy for multi txn handling
* #3673 Fix distributed hypertable DROP within a procedure
* #3701 Allow anyone to use size utilities on distributed hypertables
* #3708 Fix crash in get_aggsplit
* #3709 Fix ordered append pathkey check
* #3712 Fix GRANT/REVOKE ALL IN SCHEMA handling
* #3717 Support transparent decompression on individual chunks
* #3724 Fix inserts into compressed chunks on hypertables with caggs
* #3727 Fix DirectFunctionCall crash in distributed_exec
* #3728 Fix SkipScan with varchar column
* #3733 Fix ANALYZE crash with custom statistics for custom types
* #3747 Always reset expr context in DecompressChunk

**Thanks**
* @binakot and @sebvett for reporting an issue with DISTINCT queries
* @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE
* @mjf for reporting an issue with ordered append and JOINs
* @phemmer for reporting the issues on multinode with aggregate queries
and evaluation of now()
* @abolognino for reporting an issue with INSERTs into compressed
hypertables that have cagg
* @tanglebones for reporting the ANALYZE crash with custom types on
multinode
fabriziomello added a commit that referenced this pull request Oct 27, 2021
This release adds major new features since the 2.4.2 release.
We deem it moderate priority for upgrading.

This release includes these noteworthy features:

* Continuous Aggregates for Distributed Hypertables
* Support for PostgreSQL 14
* Experimental: Support for timezones in `time_bucket_ng()`, including
the `origin` argument

This release also includes several bug fixes.

**Features**
* #3034 Add support for PostgreSQL 14
* #3435 Add continuous aggregates for distributed hypertables
* #3505 Add support for timezones in `time_bucket_ng()`

**Bugfixes**
* #3580 Fix memory context bug executing TRUNCATE
* #3592 Allow alter column type on distributed hypertable
* #3598 Improve evaluation of stable functions such as now() on access
node
* #3618 Fix execution of refresh_caggs from user actions
* #3625 Add shared dependencies when creating chunk
* #3626 Fix memory context bug executing TRUNCATE
* #3627 Schema qualify UDTs in multi-node
* #3638 Allow owner change of a data node
* #3654 Fix index attnum mapping in reorder_chunk
* #3661 Fix SkipScan path generation with constant DISTINCT column
* #3667 Fix compress_policy for multi txn handling
* #3673 Fix distributed hypertable DROP within a procedure
* #3701 Allow anyone to use size utilities on distributed hypertables
* #3708 Fix crash in get_aggsplit
* #3709 Fix ordered append pathkey check
* #3712 Fix GRANT/REVOKE ALL IN SCHEMA handling
* #3717 Support transparent decompression on individual chunks
* #3724 Fix inserts into compressed chunks on hypertables with caggs
* #3727 Fix DirectFunctionCall crash in distributed_exec
* #3728 Fix SkipScan with varchar column
* #3733 Fix ANALYZE crash with custom statistics for custom types
* #3747 Always reset expr context in DecompressChunk

**Thanks**
* @binakot and @sebvett for reporting an issue with DISTINCT queries
* @hardikm10, @DavidPavlicek and @pafiti for reporting bugs on TRUNCATE
* @mjf for reporting an issue with ordered append and JOINs
* @phemmer for reporting the issues on multinode with aggregate queries
and evaluation of now()
* @abolognino for reporting an issue with INSERTs into compressed
hypertables that have cagg
* @tanglebones for reporting the ANALYZE crash with custom types on
multinode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants