Add missing lock to Constraint-aware append

[erimatnor]

During parallel scans of Constraint-aware append, it seems like runtime chunk exclusion happens in every parallel worker. However, parallel workers don't grab a lock on a relation before calling relation_excluded_by_constraints(), which makes them hit an assertion. An AccessShareLock or higher must be held on the relation.

Ideally, runtime chunk exclusion should happen only once in the parallel worker "leader", but that requires a bigger refactor left for the future.

[erimatnor]

Unfortunately, the assertion failure is not easy to reproduce in a test because it doesn't happen all the time. It typically requires a clean-slate session. Might spend some more time doing a test for it later.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.31%. Comparing base (1a08294) to head (76bbbca).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7515      +/-   ##
==========================================
+ Coverage   82.25%   82.31%   +0.05%     
==========================================
  Files         254      254              
  Lines       47642    47615      -27     
  Branches    12005    12001       -4     
==========================================
+ Hits        39186    39192       +6     
- Misses       3650     3667      +17     
+ Partials     4806     4756      -50     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[erimatnor]

Here's a script that reproduces the assertion crash:

select setseed(0.2);

create table readings(time timestamptz, device int, temp float);

select create_hypertable('readings', 'time', create_default_indexes => false);

insert into readings
select t, ceil(random()*10), random()*40
from generate_series('2022-06-01'::timestamptz, '2022-06-20 00:01:00', '1s') t;

alter table readings set (
      timescaledb.compress,
      timescaledb.compress_orderby = 'time',
      timescaledb.compress_segmentby = 'device'
);

--insert into readings values ('2022-06-01', 1, 1.0), ('2022-06-02', 2, 2.0), ('2022-08-02', 3, 3.0);

create index on readings (time);

select format('%I.%I', chunk_schema, chunk_name)::regclass as chunk
  from timescaledb_information.chunks
 where format('%I.%I', hypertable_schema, hypertable_name)::regclass = 'readings'::regclass
 limit 1 \gset

set timescaledb.enable_chunk_append=off;

select compress_chunk(ch) from show_chunks('readings') ch;
select decompress_chunk('_timescaledb_internal._hyper_1_3_chunk');

--set timescaledb.enable_chunk_append=on;

explain (analyze)
select time, avg(temp), device  from readings
where time  > now() - interval '2 years 5 months 20 days' group by time, device order by time;

[erimatnor]

@akuzm After some more investigation it seems this is related to parallel query plans, where the spawned workers don't have the relation locked. I don't know if parallel workers are expected to have relations referenced in plan nodes locked at the beginning of plan execution.

[akuzm]

@akuzm After some more investigation it seems this is related to parallel query plans, where the spawned workers don't have the relation locked. I don't know if parallel workers are expected to have relations referenced in plan nodes locked at the beginning of plan execution.

Not sure, e.g. the parallel seq scans seemingly doesn't lock tables in the workers, athough I only looked through the code and didn't check. In ChunkAppend, we ultimately switched to performing chunk exclusion in the parallel leader. Maybe it makes sense to do the same here as well. #5857

[erimatnor]

@akuzm After some more investigation it seems this is related to parallel query plans, where the spawned workers don't have the relation locked. I don't know if parallel workers are expected to have relations referenced in plan nodes locked at the beginning of plan execution.

Not sure, e.g. the parallel seq scans seemingly doesn't lock tables in the workers, athough I only looked through the code and didn't check. In ChunkAppend, we ultimately switched to performing chunk exclusion in the parallel leader. Maybe it makes sense to do the same here as well. #5857

If we can do chunk exclusion only once it is better. But TBH, I am not sure it is worth the effort given how seldom this scan node is used, so for now I think just taking this lock in the worker is enough. We can always implement leader exclusion later.

[erimatnor]

@akuzm Are we good to proceed with this fix. It seems the lack of locking is only in parallel queries so it seems correct to grab the lock if it is not already taken. Also seems like a very low-risk fix.

[akuzm]

@akuzm Are we good to proceed with this fix. It seems the lack of locking is only in parallel queries so it seems correct to grab the lock if it is not already taken. Also seems like a very low-risk fix.

Sure. Let's move the lock before constify_restrictinfos, that's a complicated function that is usually called during planning, it might have some obscure path that requires a lock as well.

[erimatnor]

@akuzm Are we good to proceed with this fix. It seems the lack of locking is only in parallel queries so it seems correct to grab the lock if it is not already taken. Also seems like a very low-risk fix.

Sure. Let's move the lock before constify_restrictinfos, that's a complicated function that is usually called during planning, it might have some obscure path that requires a lock as well.

Moved the lock to before constify_restrictinfos.

[mkindahl]

LGTM

[mkindahl]

If this lock is already taken in the same session, it will increase the lock count, so there should be no error as a result of taking the lock twice (or more).

The lock will also automatically be released at the end of the transaction, or when the process shuts down, so there is no risk of a dangling lock.

[timescale-automation]

Automated backport to 2.20.x not done: cherry-pick failed.

Git status

HEAD detached at origin/2.20.x
You are currently cherry-picking commit 2a539e2b7.
  (all conflicts fixed: run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

nothing to commit, working tree clean

Job log