Support column renames on tables that have compression enabled. #2909

gayyappan · 2021-02-05T22:20:53Z

ALTER TABLE RENAME <column_name> TO <new_column_name>
is now supported for hypertables that have compression enabled.

To reviewers:
This PR has multiple commits to help with reviewing test changes. These will be merged.

codecov · 2021-02-05T22:26:25Z

Codecov Report

Merging #2909 (9f555fb) into master (d823987) will increase coverage by 0.14%.
The diff coverage is 95.26%.

@@            Coverage Diff             @@
##           master    #2909      +/-   ##
==========================================
+ Coverage   90.12%   90.27%   +0.14%     
==========================================
  Files         212      212              
  Lines       34793    34937     +144     
==========================================
+ Hits        31358    31540     +182     
+ Misses       3435     3397      -38

Impacted Files	Coverage Δ
src/compat.h	`100.00% <ø> (ø)`
src/cross_module_fn.c	`68.54% <ø> (ø)`
tsl/src/compression/compression.c	`95.32% <ø> (ø)`
tsl/src/init.c	`82.35% <ø> (ø)`
tsl/src/remote/connection_cache.c	`90.40% <ø> (ø)`
tsl/src/bgw_policy/continuous_aggregate_api.c	`93.64% <90.75%> (-4.01%)`	⬇️
src/hypertable_compression.c	`96.22% <92.30%> (-1.28%)`	⬇️
tsl/src/continuous_aggs/invalidation.c	`97.94% <97.14%> (-0.17%)`	⬇️
tsl/src/continuous_aggs/refresh.c	`97.58% <98.14%> (-0.16%)`	⬇️
src/chunk_append/chunk_append.c	`97.38% <100.00%> (+0.01%)`	⬆️
... and 20 more

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 cc287f9...74d874a. Read the comment docs.

mkindahl

Consider if you should move all the code to the tsl/ directory.

src/hypertable_compression.h

mkindahl · 2021-02-15T07:40:10Z

tsl/src/compression/compression.c

+		 * Assumption: column names are the same on compressed and
+		 *       uncompressed chunk.


Is this something you should add a check for? Metadata can break, will the code work correctly if it is broken?

This is a pre-existing assumption. This code does not change any pre-existing assumptions.

What happens if this assumption is not correct, does the code crash or does it just give an error?

I added this comment to clarify existing code and the assumptions made by it. This would crash if the rename were to go through without respecting the existing assumption.

mkindahl · 2021-02-15T07:43:35Z

tsl/src/compression/create.c

+	if (TS_HYPERTABLE_HAS_COMPRESSION_TABLE(ht))
+	{
+		int32 compress_htid = ht->fd.compressed_hypertable_id;
+		Hypertable *compress_ht = ts_hypertable_get_by_id(compress_htid);


Suggested change

Hypertable *compress_ht = ts_hypertable_get_by_id(compress_htid);

const Hypertable *compress_ht = ts_hypertable_get_by_id(compress_htid);

tsl/src/compression/create.c

mkindahl · 2021-02-15T07:46:38Z

tsl/src/process_utility.c

+	if (stmt->renameType == OBJECT_COLUMN)
+	{
+		if (TS_HYPERTABLE_HAS_COMPRESSION_TABLE(ht) || TS_HYPERTABLE_HAS_COMPRESSION_ENABLED(ht))


Suggest to combine these conditions: it's a lot easier to read.

tsl/test/sql/dist_compression.sql

mkindahl · 2021-02-15T07:56:38Z

tsl/test/sql/include/compression_alter.sql

+WHERE attname = 'coli' and hypertable_id = (SELECT id from _timescaledb_catalog.hypertable
+                       WHERE table_name = 'test1' );


A little hard to read, suggesting an alternative that you can ignore if you like.

Suggested change

WHERE attname = 'coli' and hypertable_id = (SELECT id from _timescaledb_catalog.hypertable

WHERE table_name = 'test1' );

WHERE attname = 'coli'

AND hypertable_id = (SELECT id from _timescaledb_catalog.hypertable WHERE table_name = 'test1' );

mkindahl · 2021-02-15T08:00:32Z

tsl/test/sql/include/compression_alter.sql

+SELECT COUNT(*) AS count_compressed
+FROM
+(
+SELECT compress_chunk(chunk.schema_name|| '.' || chunk.table_name)


Any reason to not use format? It is used elsewhere.

tsl/test/sql/include/compression_alter.sql

mkindahl · 2021-02-15T08:02:57Z

tsl/test/sql/include/compression_alter.sql

+SELECT format('%I.%I', cht.schema_name, cht.table_name) AS "COMPRESSION_TBLNM"
+FROM _timescaledb_catalog.hypertable ht, _timescaledb_catalog.hypertable cht
+WHERE ht.table_name = 'test1' and cht.id = ht.compressed_hypertable_id \gset
+
+SELECT count(*) 
+FROM ( SELECT attrelid::regclass, attname FROM pg_attribute 
+       WHERE attrelid in (SELECT inhrelid::regclass from pg_inherits 
+                          where inhparent = :'COMPRESSION_TBLNM'::regclass )
+       and attname = 'bigintcol' ) q;


Nit: you're only using the variable here so suggest to use a CTE: it's a lot easier to read.

svenklemm

Can you add some tests where identifiers are getting truncated.

ALTER TABLE t RENAME c TO cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc;

ALTER TABLE t RENAME cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
TO accccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc;

pmwkaa · 2021-02-18T12:11:04Z

src/process_utility.c

-	ts_dimension_set_name(dim, stmt->newname);
+	if (dim)
+		ts_dimension_set_name(dim, stmt->newname);
+	if (ts_cm_functions->process_rename_cmd)


Do we really this this new cross module function? We can use existing tsl_ddl_command_start hook instead

I tried to refactor the process_utility code to directly call tsl_ddl_command_start without any additional tsl support functions. It doesn't work as expected as some ddl functions are executed in multi transaction contexts. For this particular ddl command, the original hypertable has already been modified and the hypertable cache cannot find it by the time we get here with the original name.

I share @pmwkaa's concern (and I have given similar feedback in the past). If the existing DDL forwarding doesn't work for some reason (I am not sure I understand why not), then we should at least use just one cross-module function for all DDL statements instead of adding a new one for each DDL. The reason the the hypertable cannot be found in the cache is because CommandCounterIncrement(). We have catalog update functions to deal with that without doing command-counter increment.

@erimatnor This is the refactoring change we discussed. I spent a few days trying to refactor the code to get this to work: Setting the hypertable cache and passing it around didn't work as expected. I didn't want to spend more time fixing this at the cost of derailing this feature support. process_utility code needs a wider overhaul to work without adding new functions to the tsl code. We also have exactly the same issue calling out to sql functions that have implementations under tsl. This should also be addressed as part of the refactoring effort.

ALTER TABLE <hypertable> RENAME <column_name> TO <new_column_name> is now supported for hypertables that have compression enabled. Note: Column renaming is not supported for distributed hypertables. So this will not work on distributed hypertables that have compression enabled.

erimatnor · 2021-02-19T14:37:51Z

tsl/src/compression/create.c

+												   -1);
+		ExecRenameStmt(compress_col_stmt);
+	}
+	// update catalog entries for the renamed column for the hypertable


Nit: C++ style comment.

erimatnor · 2021-02-19T14:39:39Z

src/hypertable_compression.c

@@ -158,3 +158,49 @@ ts_hypertable_compression_delete_by_hypertable_id(int32 htid)
 	}
 	return count > 0;
 }
+
+TSDLLEXPORT void
+ts_hypertable_compression_rename_column(int32 htid, char *old_column_name, char *new_column_name)


Suggested change

ts_hypertable_compression_rename_column(int32 htid, char *old_column_name, char *new_column_name)

ts_hypertable_compression_rename_column(int32 htid, const char *old_column_name, const char *new_column_name)

erimatnor · 2021-02-19T14:41:18Z

src/hypertable_compression.c

+		}
+	}
+	if (found == false)
+		elog(ERROR, "column %s not found in hypertable_compression catalog table", old_column_name);


Should this be an assert? AFAIK, it shouldn't happen so this will be dead code that we cannot test for. If it is a valid error, I think it is preferred that this is a more user-friendly message that doesn't reference specific internal catalogs.

erimatnor · 2021-02-19T15:16:53Z

src/cross_module_fn.h

@@ -99,6 +99,7 @@ typedef struct CrossModuleFunctions
 	bool (*process_compress_table)(AlterTableCmd *cmd, Hypertable *ht,
 								   WithClauseResult *with_clause_options);
 	void (*process_altertable_cmd)(Hypertable *ht, const AlterTableCmd *cmd);
+	void (*process_rename_cmd)(Hypertable *ht, const RenameStmt *stmt);


I think this has come up before, and I don't know the conclusion so I'll reiterate.

I think creating a new cross-module function for every process utility (DDL) command is not a good idea and doesn't really scale. We already "forward" DDL commands to TSL and we should try to use that mechanism or improve it if it isn't sufficient in its current form.

The next best approach, IMO, is to have a "general" cross-module function that we can reuse for all DDL commands, e.g.:

void (*process_ddl_cmd)(Hypertable *ht, const Node *cmd);

then use a switch on it in the process_ddl_cmd function:

switch (nodeTag(cmd)) { case T_RenameStmt: .... break; case T_AlterTableCmd: ... break; }

erimatnor · 2021-02-19T15:22:07Z

src/process_utility.c

-	ts_dimension_set_name(dim, stmt->newname);
+	if (dim)
+		ts_dimension_set_name(dim, stmt->newname);
+	if (ts_cm_functions->process_rename_cmd)


I share @pmwkaa's concern (and I have given similar feedback in the past). If the existing DDL forwarding doesn't work for some reason (I am not sure I understand why not), then we should at least use just one cross-module function for all DDL statements instead of adding a new one for each DDL. The reason the the hypertable cannot be found in the cache is because CommandCounterIncrement(). We have catalog update functions to deal with that without doing command-counter increment.

This release adds major new features since the 2.0.2 release. We deem it moderate priority for upgrading. This release adds the long-awaited support for PostgreSQL 13 to TimescaleDB. This release also relaxes some restrictions for compressed hypertables; namely, TimescaleDB now supports adding columns to compressed hypertables and renaming columns of compressed hypertables. **Major Features** * timescale#2779 Add support for PostgreSQL 13 **Minor features** * timescale#2736 Support adding columns to hypertables with compression enabled * timescale#2909 Support renaming columns of hypertables with compression enabled

This release adds major new features since the 2.0.2 release. We deem it moderate priority for upgrading. This release adds the long-awaited support for PostgreSQL 13 to TimescaleDB. The minimum required PostgreSQL 13 version is 13.2 due to a security vulnerability affecting TimescaleDB functionality present in earlier versions of PostgreSQL 13. This release also relaxes some restrictions for compressed hypertables; namely, TimescaleDB now supports adding columns to compressed hypertables and renaming columns of compressed hypertables. **Major Features** * timescale#2779 Add support for PostgreSQL 13 **Minor features** * timescale#2736 Support adding columns to hypertables with compression enabled * timescale#2909 Support renaming columns of hypertables with compression enabled

This release adds major new features since the 2.0.2 release. We deem it moderate priority for upgrading. This release adds the long-awaited support for PostgreSQL 13 to TimescaleDB. The minimum required PostgreSQL 13 version is 13.2 due to a security vulnerability affecting TimescaleDB functionality present in earlier versions of PostgreSQL 13. This release also relaxes some restrictions for compressed hypertables; namely, TimescaleDB now supports adding columns to compressed hypertables and renaming columns of compressed hypertables. **Major Features** * #2779 Add support for PostgreSQL 13 **Minor features** * #2736 Support adding columns to hypertables with compression enabled * #2909 Support renaming columns of hypertables with compression enabled

gayyappan force-pushed the compress_alter branch 4 times, most recently from 29da96f to 1d9865a Compare February 8, 2021 20:24

gayyappan marked this pull request as ready for review February 8, 2021 20:38

gayyappan requested a review from a team as a code owner February 8, 2021 20:38

gayyappan requested review from pmwkaa, erimatnor and svenklemm and removed request for a team February 8, 2021 20:38

mkindahl self-requested a review February 12, 2021 14:04

mkindahl reviewed Feb 15, 2021

View reviewed changes

NunoFilipeSantos added the compression label Feb 15, 2021

svenklemm reviewed Feb 15, 2021

View reviewed changes

mkindahl approved these changes Feb 16, 2021

View reviewed changes

pmwkaa reviewed Feb 18, 2021

View reviewed changes

gayyappan force-pushed the compress_alter branch from 1d9865a to 5d1a666 Compare February 18, 2021 20:44

gayyappan requested a review from svenklemm February 18, 2021 22:20

gayyappan force-pushed the compress_alter branch from b324c1f to 07999aa Compare February 19, 2021 14:29

gayyappan force-pushed the compress_alter branch from 07999aa to 74d874a Compare February 19, 2021 15:02

svenklemm approved these changes Feb 19, 2021

View reviewed changes

gayyappan merged commit 5be6a3e into timescale:master Feb 19, 2021

erimatnor reviewed Feb 19, 2021

View reviewed changes

svenklemm mentioned this pull request Feb 22, 2021

Release 2.1.0 #2962

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support column renames on tables that have compression enabled. #2909

Support column renames on tables that have compression enabled. #2909

gayyappan commented Feb 5, 2021 •

edited

Loading

codecov bot commented Feb 5, 2021 •

edited

Loading

mkindahl left a comment

mkindahl Feb 15, 2021

gayyappan Feb 16, 2021

mkindahl Feb 16, 2021

gayyappan Feb 16, 2021

mkindahl Feb 15, 2021

mkindahl Feb 15, 2021

mkindahl Feb 15, 2021

mkindahl Feb 15, 2021

mkindahl Feb 15, 2021

svenklemm left a comment

pmwkaa Feb 18, 2021

gayyappan Feb 18, 2021

erimatnor Feb 19, 2021

gayyappan Feb 19, 2021

erimatnor Feb 19, 2021

erimatnor Feb 19, 2021

erimatnor Feb 19, 2021

erimatnor Feb 19, 2021

erimatnor Feb 19, 2021

		* Assumption: column names are the same on compressed and
		* uncompressed chunk.

	Hypertable *compress_ht = ts_hypertable_get_by_id(compress_htid);
	const Hypertable *compress_ht = ts_hypertable_get_by_id(compress_htid);

		WHERE attname = 'coli' and hypertable_id = (SELECT id from _timescaledb_catalog.hypertable
		WHERE table_name = 'test1' );

	ts_hypertable_compression_rename_column(int32 htid, char old_column_name, char new_column_name)
	ts_hypertable_compression_rename_column(int32 htid, const char old_column_name, const char new_column_name)

Support column renames on tables that have compression enabled. #2909

Support column renames on tables that have compression enabled. #2909

Conversation

gayyappan commented Feb 5, 2021 • edited Loading

codecov bot commented Feb 5, 2021 • edited Loading

Codecov Report

mkindahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenklemm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gayyappan commented Feb 5, 2021 •

edited

Loading

codecov bot commented Feb 5, 2021 •

edited

Loading