Allow up to INT16_MAX rows per batch

[akuzm]

We're getting requests to make the number of rows per batch configurable, so this PR makes some refactoring to make that easier. E.g. it removes some large stack allocations and change width of variables to accomodate this. It doesn't actually add an option to change the compressed batch size yet.

The new potential upper limit is INT16_MAX rows per batch, because dictionary compression uses signed int16 indexes. The actual target compressed batch size is kept hardcoded at 1000 rows for now.

I also had to make some tweaks to memory management to avoid performance regressions, but after that there are no changes in tsbench that I could reproduce: https://grafana.ops.savannah-dev.timescale.com/d/fasYic_4z/compare-akuzm?orgId=1&var-branch=All&var-run1=3331&var-run2=3341&var-threshold=0&var-use_historical_thresholds=true&var-threshold_expression=2.5%20%2A%20percentile_cont%280.90%29&var-exact_suite_version=false

Disable-check: force-changelog-file

[codecov]

Codecov Report

Attention: Patch coverage is 94.44444% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 80.97%. Comparing base (59f50f2) to head (b56e88d).
Report is 70 commits behind head on main.

Files	Patch %	Lines
tsl/src/compression/simple8b_rle_bitmap.h	94.11%	0 Missing and 2 partials ⚠️
tsl/src/compression/simple8b_rle_decompress_all.h	80.00%	0 Missing and 2 partials ⚠️
tsl/src/compression/gorilla_impl.c	85.71%	0 Missing and 1 partial ⚠️
tsl/src/nodes/decompress_chunk/exec.c	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6734      +/-   ##
==========================================
+ Coverage   80.06%   80.97%   +0.90%     
==========================================
  Files         190      191       +1     
  Lines       37181    36383     -798     
  Branches     9450     9447       -3     
==========================================
- Hits        29770    29462     -308     
- Misses       2997     3162     +165     
+ Partials     4414     3759     -655     

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

[svenklemm]

do we really need a guc for this for testing, we can just set the corresponding option directly in tests

[akuzm]

It worked when we had inheritance, but now the compressed chunk is re-created at compression w/o inheritance, so I'm not sure it's even possible.

[akuzm]

The GUC is debug-only, in release builds it's just a static variable that is never changed.

[jnidzwetzki]

I am wondering if these large batches would be compatible with the TAM implementation indexes. As far as I remember, we have only a limited number of bits to address the tuples.

[akuzm]

Hard to tell until we have a design doc at least. This refactoring doesn't really enforce a lower limit on batch sizes, so TAM tables can use a low limit if they want to.

[jnidzwetzki]

I am a bit worried about the migration path. If we have larger batches, the compressed table might not be processable using TAM (or we could not build an index). So, it might be good to know more about the requirements. What do you think @mkindahl / @erimatnor?

[erimatnor]

For TAM indexing we need to encode a value's index in a compressed batch into the TID, which has limited number of bits (and we also need block and offset in there). So, yes, there's a limit for sure. However, we could always enforce a lower batch size in TAM if we rewrite the table.

It would be interesting to know the real-world benefits of larger batch sizes.

[jnidzwetzki]

In build_decompressor we do .decompressed_slots = palloc0(sizeof(void *) * GLOBAL_MAX_ROWS_PER_COMPRESSION),. I am wondering if this should be made more dynamic to avoid the large memory allocation for smaller batches.

[jnidzwetzki]

Since the batches can become larger and consume more space in memory, does the cost model for the sorted merged optimization need to be adjusted as well? What about batch decompression and memory limits?

[akuzm]

Since the batches can become larger and consume more space in memory, does the cost model for the sorted merged optimization need to be adjusted as well? What about batch decompression and memory limits?

This PR doesn't even allow bigger batches, so this is mostly for the future consideration. But yes, I think when we make the size configurable, we'll have to look at the batch size configured for a given table, instead of the upper limit. For now, I'll double-check the costs, I think I forgot to update the max size with current target size there.

In build_decompressor we do .decompressed_slots = palloc0(sizeof(void *) * GLOBAL_MAX_ROWS_PER_COMPRESSION),. I am wondering if this should be made more dynamic to avoid the large memory allocation for smaller batches.

Good point, I'll change it to use the target compressed batch size instead of the max allowed size.

[jnidzwetzki]

Rest LGTM, will approve as soon as the question regarding TAM has been solved.

[akuzm]

Rest LGTM, will approve as soon as the question regarding TAM has been solved.

The main question with TAM is whether it can provide meaningful improvement of customer use cases at a reasonable engineering cost, compared e.g. to the existing sparse index implementation. This doesn't seem certain at the moment. We should demonstrate this, publish a design for index support and so on. Until this is done, and at the moment we don't even have a finalized design for TAM index support, I don't think we should be blocking other work based on the remote possibility that it might have conflicts with a purported TAM index implementation.

I am a bit worried about the migration path. If we have larger batches, the compressed table might not be processable using TAM (or we could not build an index).

If we make the batch size configurable, the migration path would be to recompress all chunks with the smaller batch size. Probalby we're going to apply TAM per-chunk, for the same reason we made the settings per-chunk. So I think it shouldn't be a big problem.

It would be interesting to know the real-world benefits of larger batch sizes.

There is some benefit, e.g. vectorized aggregation can be 2x faster with 4k batches. This needs more careful benchmarking if we decide to increase the default batch size, because for small limit queries it can actually slow them down. We might require a cost-based choice of iterator-based vs bulk decompression, which is not very easy to implement correctly. As for the other products, IIRC DuckDB uses 2k batches and ClickHouse uses 65k.

Here's some old benchmark I did: https://grafana.ops.savannah-dev.timescale.com/d/fasYic_4z/compare-akuzm?orgId=1&var-branch=All&var-run1=3263&var-run2=3269&var-threshold=0.02&var-use_historical_thresholds=true&var-threshold_expression=2.5%20%2A%20percentile_cont%280.90%29&var-exact_suite_version=true

I did not measure the compression ratios, probably they also become better with larger batches, especially for json or dictionary-compressed text.

But mostly I was thinking about smaller sizes that are multiples of an hour/minute, e.g. 720 or 1440 or 3600. I remember Rob asked for this on Slack, can't find this now.