Add table mappings to compression settings #6990
Conversation
erimatnor
added
compression
tech-debt
erimatnor
force-pushed
the
refactor-compression-settings
branch
7 times, most recently
from
a48c167
to
f2652ad
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6990 +/- ##
==========================================
+ Coverage 80.06% 81.78% +1.71%
==========================================
Files 190 199 +9
Lines 37181 37038 -143
Branches 9450 9675 +225
==========================================
+ Hits 29770 30290 +520
+ Misses 2997 2861 -136
+ Partials 4414 3887 -527 ☔ View full report in Codecov by Sentry. |
Refactor the compression settings metadata table to include a mapping from a chunk's relid to its compressed chunk's relid. Adding this mapping makes compression settings the main metadata table for compression-related information, while decoupling it from chunk metadata. This simplifies the code that looks up compression metadata as it no longer requires first looking up the corresponding compressed chunk. The new compression settings is a step towards removing a chunk's compression table from the chunk metadata. In other words, the "compressed chunk" will no longer be a chunk, just a relation associated with the regular chunk via compression settings.
erimatnor
force-pushed
the
refactor-compression-settings
branch
from
f2652ad
to
b9df9ea
Compare
There was a problem hiding this comment.
It looks like this PR might need a bit more work. If we're going to store info about compressed and uncompressed assignments in the compression_settings table, we should take it out of the chunk catalog table in this same update. Storing the same thing in two places can lead to confusion and mistakes. We want to keep things tidy and have one source of truth for this information.
While I agree we should get rid of the metadata in the chunk catalog table, I think we should do this in increments. That's a pretty heavy lift and has other consequences on dependent tables, like compression_chunk_size, etc. This change does not mean we will store the same information in multiple places apart from an extra ID. I think we can live with this in the short term until the other changes are merged too. |
There was a problem hiding this comment.
For posterity, lets make a concrete plan for the rest of the refactoring that should follow this PR for clarity and prioritization purposes.
Removing the compressed_chunk_id chunk metadata should be followed up and would clean up the catalog and our code nicely.
| @@ -1916,7 +1916,9 @@ process_rename_column(ProcessUtilityArgs *args, Cache *hcache, Oid relid, Rename | |||
| * we don't do anything. */ | |||
| if (ht) | |||
| { | |||
| ts_compression_settings_rename_column_hypertable(ht, stmt->subname, stmt->newname); | |||
| ts_compression_settings_rename_column_recurse(ht->main_table_relid, | |||
There was a problem hiding this comment.
| ts_compression_settings_rename_column_recurse(ht->main_table_relid, | |
| ts_compression_settings_rename_column_cascade(ht->main_table_relid, |
| } | ||
|
|
||
| TSDLLEXPORT bool | ||
| ts_compression_settings_delete_by_compress_relid(Oid relid) |
There was a problem hiding this comment.
We should comment that these functions are part of an intermediate step while we transition from using compressed table relid to chunk relid.
| } | ||
|
|
||
| TSDLLEXPORT CompressionSettings * | ||
| ts_compression_settings_get_by_compress_relid(Oid relid) |
There was a problem hiding this comment.
Add a todo to make clear this will eventually go away since its an intermediate step towards the complete refactor.
|
|
||
| process_predicates(chunk, settings, predicates, &heap_filters, &index_filters, &is_null); | ||
|
|
||
| chunk_rel = table_open(chunk->table_id, RowExclusiveLock); | ||
| comp_chunk = ts_chunk_get_by_id(chunk->fd.compressed_chunk_id, true); |
There was a problem hiding this comment.
Can you get the compressed table relid from the settings instead of getting the compressed chunk metadata here?
mkindahl
requested review from
antekresic and
svenklemm
and removed request for
mkindahl
Refactor the compression settings metadata table to include a mapping from a chunk's relid to its compressed chunk's relid.
Adding this mapping makes compression settings the main metadata table for compression-related information, while decoupling it from chunk metadata. This simplifies the code that looks up compression metadata as it no longer requires first looking up the corresponding compressed chunk.
The new compression settings is a step towards removing a chunk's compression table from the chunk metadata. In other words, the "compressed chunk" will no longer be a chunk, just a relation associated with the regular chunk via compression settings.
Disable-check: force-changelog-file