Disable foreign distributed tables by agedemenli · Pull Request #5605 · citusdata/citus

Ahmet Gedemenli (agedemenli) · 2022-01-07T00:48:43Z

DESCRIPTION: Disables distributed&reference foreign tables

Disables create_distributed_table and create_reference_table for foreign tables. Foreign tables can now only be added to metadata. Errors out when trying to distribute foreign tables.

We don't want to break users that have distributed&reference foreign tables already. With this PR, we log a WARNING message for them, when they use an existing distributed foreign table. This check is done in distributed planner.

Includes a lot of changes in regression test output, for the existing distributed foreign table test.
Added some test to show the error when trying to distribute a foreign table.
Can't add regression test for the warning, as it's not a case we can see for now. The warning is only for the users that have distributed foreign tables already.

fixes: #5604

codecov · 2022-01-07T00:53:20Z

Codecov Report

Merging #5605 (0ed8226) into master (9d858cb) will decrease coverage by 1.81%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master    #5605      +/-   ##
==========================================
- Coverage   92.66%   90.85%   -1.82%     
==========================================
  Files         219      219              
  Lines       46011    46034      +23     
==========================================
- Hits        42637    41825     -812     
- Misses       3374     4209     +835

Önder Kalacı (onderkalaci)

I think the question Marco Slot (@marcocitus) had earlier was that what about existing foreign distributed tables that were created on 10.2 or earlier.

Can you still (a) query those (b) run alter/rename/DDLs etc (c) drop them

Also, when you do (a) and (b), should we warn users?

Önder Kalacı (onderkalaci) · 2022-01-07T08:10:19Z

Also, when you do (a) and (b), should we warn users?

Well, that might be too much. Maybe warn once per session or such on these occasions.

Ahmet Gedemenli (agedemenli) · 2022-01-07T09:27:48Z

I think the question Marco Slot (@marcocitus) had earlier was that what about existing foreign distributed tables that were created on 10.2 or earlier.

Can you still (a) query those (b) run alter/rename/DDLs etc (c) drop them

Also, when you do (a) and (b), should we warn users?

Yes, all of them are still allowed. I'll add a warning for such cases.

Önder Kalacı (onderkalaci) · 2022-01-07T12:56:09Z

+		IsCitusTable(relationId) && !IsCitusTableType(relationId, CITUS_LOCAL_TABLE))
+	{
+		DistributedForeignTableWarningPrompted = true;
+		ereport(WARNING, (errmsg("support for distributed foreign tables are deprecated"),


support for distributed or reference foreign tables are deprecated, please use Citus managed local tables
(keep the detail)

Önder Kalacı (onderkalaci) · 2022-01-07T12:57:51Z

 		return NIL;
 	}

+	WarnUnsupportedIfForeignDistributedTable(leftRelationId);


Let's only keep WarnUnsupportedIfForeignDistributedTable in distributed planner. These DDL hooks seems excessive. If you have a distributed foreign table, you'd most likely run a query compared to any DDL.

Önder Kalacı (onderkalaci) · 2022-01-07T13:02:04Z


 		if (IsCitusTable(rangeTableEntry->relid))
 		{
+			WarnUnsupportedIfForeignDistributedTable(rangeTableEntry->relid);


I'm really not convinced that calling WarnUnsupportedIfForeignDistributedTable inside ListContainsDistributedTableRTE is a good approach.

What about:

else if (CitusHasBeenLoaded()) { needsDistributedPlanning = ListContainsDistributedTableRTE(rangeTableList, maybeHasForeignTable); if (needsDistributedPlanning) { fastPathRouterQuery = FastPathRouterQuery(parse, &distributionKeyValue); if (maybeHasForeignTable) { WarnUnsupportedIfForeignDistributedTable(rangeTableEntry->relid); } } }

We actually need a relationId to pass to WarnUnsupportedIfForeignDistributedTable, to verify the relation is a distributed foreign table. But here we don't have a relationId, instead we have a list that might contain a relation that is a distributed foreign table.

Besides, there is one more place that we call ListContainsDistributedTableRTE, that has nothing to do with foreign tables, so passing such a parameter doesn't make sense in that place.

We can consider iterating the list once more, to find out if it contains any distributed foreign table; but this option adds some unnecessary complexity. Maybe we can avoid this complexity by doing the iteration only when the variable DistributedForeignTableWarningPrompted is false ?

Something like:

else if (CitusHasBeenLoaded()) { needsDistributedPlanning = ListContainsDistributedTableRTE(rangeTableList); if (needsDistributedPlanning) { fastPathRouterQuery = FastPathRouterQuery(parse, &distributionKeyValue); WarnIfListHasForeignDistributedTable(rangeTableList); } }

and

static void WarnIfListHasForeignDistributedTable(List *rangeTableList) { RangeTblEntry *rangeTableEntry = NULL; foreach_ptr(rangeTableEntry, rangeTableList) { if (DistributedForeignTableWarningPrompted) { return; } WarnUnsupportedIfForeignDistributedTable(rangeTableEntry->relid); } }

Besides, there is one more place that we call ListContainsDistributedTableRTE, that has nothing to do with foreign tables, so passing such a parameter doesn't make sense in that place.

Similarly, calling WarnUnsupportedIfForeignDistributedTable doesn't make sense :)

Can't we re-iterate the list only when we know there is a foreign table? In majority of the cases DistributedForeignTableWarningPrompted will be false, hence we'd re-iterate unnecessarily with the above suggestion

Önder Kalacı (onderkalaci) · 2022-01-07T14:27:42Z

+ * We do that because now we only support Citus Local Tables for foreign tables.
+ */
+void
+WarnUnsupportedIfForeignDistributedTable(Oid relationId)


do we really need this function now? Can't we inline into WarnIfListHasForeignDistributedTable? It seems confusing to have an external function with this name.

And, when we do that, we should probably be able to remove DistributedForeignTableWarningPrompted check from this part?

Önder Kalacı (onderkalaci) · 2022-01-07T14:32:15Z


-	return ListContainsDistributedTableRTE(allRTEs);
+	/* redundant parameter for here */
+	bool *maybeHasForeignDistributedTable = false;


Minor: can't we pass NULL? See my other comment as well

return ListContainsDistributedTableRTE(allRTEs, NULL);

Önder Kalacı (onderkalaci) · 2022-01-07T14:52:47Z

+ * We do that because now we only support Citus Local Tables for foreign tables.
+ */
+void
+WarnIfListHasForeignDistributedTable(List *rangeTableList)


minor: probably better to move where it is used, and make it a static function

Önder Kalacı (onderkalaci) · 2022-01-07T14:58:15Z

+	RangeTblEntry *rangeTableEntry = NULL;
+	foreach_ptr(rangeTableEntry, rangeTableList)
+	{
+		if (DistributedForeignTableWarningPrompted)


minor: might be good to move this check out of the loop

Önder Kalacı (onderkalaci) reviewed Jan 7, 2022

View reviewed changes

Ahmet Gedemenli (agedemenli) added 10 commits January 7, 2022 17:05

Disable foreign distributed tables

2e08548

Fix tests

dcbce3f

Style

5f6918c

Add Warning for alter distributed foreign table

b496ad9

Style

ea7ae7c

Add check for distributed planner

514667b

Dont warn when doing alter foreign table

882ff09

Edit warning message

6fd2398

Add WarnIfListHasForeignDistributedTable

3322c16

Style

1d99531

Ahmet Gedemenli (agedemenli) force-pushed the disable-foreign-distributed-tables branch from c0f1b37 to 1d99531 Compare January 7, 2022 14:05

Önder Kalacı (onderkalaci) reviewed Jan 7, 2022

View reviewed changes

Unify the check&warning function

1dd3df9

Önder Kalacı (onderkalaci) approved these changes Jan 7, 2022

View reviewed changes

Move WarnIfListHasForeignDistributedTable to distributed_planner.c

0ed8226

Ahmet Gedemenli (agedemenli) merged commit 3c834e6 into master Jan 7, 2022

Ahmet Gedemenli (agedemenli) deleted the disable-foreign-distributed-tables branch January 7, 2022 15:12

Uh oh!

Conversation

Ahmet Gedemenli (agedemenli) commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Önder Kalacı (onderkalaci) left a comment

Choose a reason for hiding this comment

Uh oh!

Önder Kalacı (onderkalaci) commented Jan 7, 2022

Uh oh!

Ahmet Gedemenli (agedemenli) commented Jan 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Ahmet Gedemenli (agedemenli) commented Jan 7, 2022 •

edited

Loading

codecov Bot commented Jan 7, 2022 •

edited

Loading