Switch to sequential execution if the index name is long #4209

onderkalaci · 2020-09-30T13:09:41Z

/*

Citus has the logic to truncate the long shard names to prevent
various issues, including self-deadlocks. However, for partitioned
tables, when index is created on the parent table, the index names
on the partitions are auto-generated by Postgres. We use the same
Postgres function to generate the index names on the shards of the
partitions. If the length exceeds the limit, we switch to sequential
execution mode.
*/

DESCRIPTION: Fixes a bug that could cause deadlocks on CREATE INDEX

onderkalaci · 2020-09-30T14:06:06Z

src/backend/distributed/commands/index_pg_source.c

+ * The argument list is pretty ad-hoc :-(
+ */
+char *
+ChooseIndexName(const char *tabname, Oid namespaceId,


we can even use this function to support unnamed indexes?

I was thinking of the same https://github.com/citusdata/citus/pull/3994/files#diff-2fb367404fd67f00cf27350700dce00dR48

codecov · 2020-09-30T14:23:11Z

Codecov Report

Merging #4209 into master will increase coverage by 0.04%.
The diff coverage is 83.83%.

@@            Coverage Diff             @@
##           master    #4209      +/-   ##
==========================================
+ Coverage   90.84%   90.88%   +0.04%     
==========================================
  Files         188      189       +1     
  Lines       37056    37155      +99     
==========================================
+ Hits        33664    33770     +106     
+ Misses       3392     3385       -7

src/backend/distributed/commands/index_pg_source.c

src/backend/distributed/commands/index.c

marcocitus · 2020-09-30T15:58:22Z

src/test/regress/expected/multi_index_statements.out

+	CREATE INDEX ix_test_index_creation4
+	ON test_index_creation1 USING btree
+	(tenant_id, timeperiod);
+DEBUG:  the index name on the shards of the partition is too long, switching to sequential execution mode to prevent self deadlocks: test_index_creation1_p2020_09_26_10311_tenant_id_timeperiod_idx


would it be worth using a NOTICE? not sure whether it will inform or scare the user

We do not have NOTICE when we switch to sequential execution due to fkey relationships. That's why I followed the same approach by adding DEBUG1.

If table is empty, NOTICE would be confusing. If the table has reasonable amount of data, then it might be beneficial.

Do we have a trick to get that information without calling master_update_shard_stats?

onderkalaci · 2020-10-01T08:05:07Z

src/backend/distributed/commands/index_pg_source.c

+									   namespaceId,
+									   true);
+	}
+	else if (isconstraint)


This is not supported on PG, that's why I cannot add tests:

alter table test_index_creation1 ADD CONSTRAINT c1 PRIMARY KEY usiNG INDEX ix_test_index_creation6; ALTER TABLE / ADD CONSTRAINT USING INDEX is not supported on partitioned tables

onderkalaci · 2020-10-01T08:07:39Z

src/backend/distributed/commands/index_pg_source.c

+									   namespaceId,
+									   true);
+	}
+	else if (exclusionOpNames != NIL)


This is not supported on PG, that's why I cannot add tests:

ERROR: exclusion constraints are not supported on partitioned tables

onderkalaci · 2020-10-01T08:10:37Z

src/backend/distributed/commands/index_pg_source.c

+{
+	char *indexname;
+
+	if (primary)


we cannot add tests, because we hit #1664. The reason we hit it is that this type of index only adds table_name_pkey. However, shard names are longer than _pkey. We could add a test by manually setting shardId less than 4 chars, but that seems overkill

marcocitus · 2020-10-01T08:44:46Z

src/backend/distributed/metadata/metadata_utility.c

+
+	int largestShardIndex = cacheEntry->shardIntervalArrayLength - 1;
+	ShardInterval *shardInterval =
+		CopyShardInterval(cacheEntry->sortedShardIntervalArray[largestShardIndex]);


isn't this the shard with the largest hash range?

I don't think we are interested in the hash ranges here? The idea is to find the shard name in longest length, so we are searching some edge cases like the table has shards table_99999 and table_100000, we want to pick the latter

but this array is sorted by (hash) range

which would no longer be the biggest shard ID after tenant isolation

Oh, yes I see what you mean now

We could be more defensive for future change and always loop over the shards, but that seems not very likely that we change the shardIds monotonically increasing per table?

/* * Citus has the logic to truncate the long shard names to prevent * various issues, including self-deadlocks. However, for partitioned * tables, when index is created on the parent table, the index names * on the partitions are auto-generated by Postgres. We use the same * Postgres function to generate the index names on the shards of the * partitions. If the length exceeds the limit, we switch to sequential * execution mode. */

src/backend/distributed/metadata/metadata_utility.c

marcocitus · 2020-10-01T15:11:34Z

src/backend/distributed/metadata/metadata_utility.c

+	/* this cannot happen, still be defensive */
+	if (largestShardId == 0)
+	{
+		ereport(ERROR, (errmsg("unexpected shardId: %lu", largestShardId)));


append-distributed tables can have 0 shards

as far as I see, we already don't allow partitioning append distributed tables, so do you think that error check/message is sufficient @marcocitus ?

SELECT create_distributed_table('measurement','city_id','append');
ERROR: distributing partitioned tables in only supported for hash-distributed tables

onderkalaci commented Sep 30, 2020

View reviewed changes

onderkalaci requested a review from marcocitus September 30, 2020 14:09

onurctirtir mentioned this pull request Sep 30, 2020

Prototype: Enable no name indexes on citus tables #3994

Closed

2 tasks

onurctirtir reviewed Sep 30, 2020

View reviewed changes

src/backend/distributed/commands/index_pg_source.c Outdated Show resolved Hide resolved

marcocitus reviewed Sep 30, 2020

View reviewed changes

src/backend/distributed/commands/index.c Show resolved Hide resolved

marcocitus changed the title ~~Switch to sequetial execution if the index name is long~~ Switch to sequential execution if the index name is long Sep 30, 2020

marcocitus reviewed Sep 30, 2020

View reviewed changes

onderkalaci commented Oct 1, 2020

View reviewed changes

onderkalaci force-pushed the switch_to_seq_execution_index branch from a0785ee to 6b0fc88 Compare October 1, 2020 08:12

marcocitus reviewed Oct 1, 2020

View reviewed changes

onderkalaci requested a review from marcocitus October 1, 2020 12:47

onderkalaci force-pushed the switch_to_seq_execution_index branch from 6b0fc88 to db0e5c5 Compare October 1, 2020 13:09

onderkalaci force-pushed the switch_to_seq_execution_index branch from db0e5c5 to 8a70123 Compare October 1, 2020 13:13

marcocitus reviewed Oct 1, 2020

View reviewed changes

marcocitus approved these changes Oct 1, 2020

View reviewed changes

onurctirtir added 3 commits October 2, 2020 11:48

remove unnecessary optimization & improve error

2d733e8

Merge branch 'master' into switch_to_seq_execution_index_onur

7e4b60d

early exit if relation has no shards

1c2068a

onurctirtir merged commit df5aa0f into master Oct 2, 2020

onderkalaci mentioned this pull request Oct 13, 2020

Self deadlock if the index name is long when the placements are on the coordinator #4238

Closed

onurctirtir deleted the switch_to_seq_execution_index branch October 14, 2020 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to sequential execution if the index name is long #4209

Switch to sequential execution if the index name is long #4209

onderkalaci commented Sep 30, 2020

onderkalaci Sep 30, 2020

onurctirtir Sep 30, 2020

codecov bot commented Sep 30, 2020 •

edited

Loading

marcocitus Sep 30, 2020

onderkalaci Oct 1, 2020

onderkalaci Oct 1, 2020

onderkalaci Oct 1, 2020

onderkalaci Oct 1, 2020

marcocitus Oct 1, 2020

onderkalaci Oct 1, 2020

marcocitus Oct 1, 2020

onderkalaci Oct 1, 2020

onderkalaci Oct 1, 2020

marcocitus Oct 1, 2020

onurctirtir Oct 2, 2020 •

edited

Loading

Switch to sequential execution if the index name is long #4209

Switch to sequential execution if the index name is long #4209

Conversation

onderkalaci commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 30, 2020 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

onurctirtir Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 30, 2020 •

edited

Loading

onurctirtir Oct 2, 2020 •

edited

Loading