Fix issue creating dimensional constraints

[erimatnor]

During chunk creation, the chunk's dimensional CHECK constraints are created via an "upcall" to PL/pgSQL code. However, creating dimensional constraints in PL/pgSQL code sometimes fails, especially during high-concurrency inserts, because PL/pgSQL code scans metadata using a snapshot that might not see the same metadata as the C code. As a result, chunk creation sometimes fail during constraint creation.

To fix this issue, implement dimensional CHECK-constraint creation in C code. Other constraints (FK, PK, etc.) are still created via an upcall, but should probably also be rewritten in C. However, since these constraints don't depend on recently updated metadata, this is left to a future change.

Fixes #5456

[erimatnor]

I have not been able to recreate this bug in an isolation test, because it only happens when running really high-concurrency inserts (e.g., parallel-copy with many workers).

Still, this is valuable just as a refactor, getting rid of some very legacy PL/pgSQL code. We should try to get rid of the rest of this upcall code in the future.

[github-actions]

@shhnwz, @konskov: please review this pull request.

Powered by pull-review

[codecov]

Codecov Report

Merging #5459 (3fae9cd) into main (72c0f5b) will increase coverage by 0.00%.
The diff coverage is 99.46%.

@@           Coverage Diff           @@
##             main    #5459   +/-   ##
=======================================
  Coverage   90.73%   90.74%           
=======================================
  Files         229      229           
  Lines       53563    53655   +92     
=======================================
+ Hits        48602    48689   +87     
- Misses       4961     4966    +5     

Impacted Files	Coverage Δ
src/chunk.h	100.00% <ø> (ø)
src/chunk_constraint.c	94.35% <99.18%> (+0.78%)	⬆️
src/chunk.c	93.71% <100.00%> (+0.04%)	⬆️
src/process_utility.c	94.43% <100.00%> (ø)
tsl/src/compression/api.c	96.15% <100.00%> (-0.04%)	⬇️
tsl/test/src/test_merge_chunk.c	100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[akuzm]

I have not been able to recreate this bug in an isolation test, because it only happens when running really high-concurrency inserts (e.g., parallel-copy with many workers).

Maybe we should start adding probabilistic tests that just simulate this high-concurrency situation for a couple of minutes. Still better than nothing. Probably can be done with TAP tests.

[nikkhils]

LGTM overall.

[pmwkaa]

I am curious why are we still using SQL functions internally? I am thinking about performance for those calls if they are on a hot path

[erimatnor]

This is just legacy stuff and I think it is way overdue for a refactor into C. It also turned out to not be a great idea from a visibility/snapshot perspective.

Don't think it is that big of a performance issue, though, given that the function is only called when creating a new chunk.

[timescale-automation]

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

Git status

HEAD detached at origin/2.10.x
You are currently cherry-picking commit a51d21ef.
  (fix conflicts and 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)

Changes to be committed:
	modified:   CHANGELOG.md
	modified:   sql/chunk_constraint.sql
	modified:   sql/size_utils.sql
	modified:   sql/util_internal_table_ddl.sql
	modified:   src/chunk.c
	modified:   src/chunk.h
	modified:   src/chunk_constraint.c
	modified:   src/chunk_constraint.h
	modified:   src/process_utility.c
	modified:   tsl/test/shared/expected/extension.out
	modified:   tsl/test/src/test_merge_chunk.c

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   sql/updates/latest-dev.sql
	both modified:   sql/updates/reverse-dev.sql
	both modified:   tsl/src/compression/api.c

Job log

[mkindahl]

Is it OK to exit this loop without finding a dimension to update? If not, you might want to have a check after the loop to make sure that you actually found a dimension to update. Regardless, you might want to add a comment describing if this is OK or not.

[mkindahl]

Variable slice can in theory be NULL here if there is something wrong with the metadata. You might want to ensure that it is not NULL before starting to de-reference it.

Suggested change

	dim = ts_hyperspace_get_dimension_by_id(ht->space, slice->fd.dimension_id);
	Ensure(slice, "dimension slice was missing");
	dim = ts_hyperspace_get_dimension_by_id(ht->space, slice->fd.dimension_id);