Introduce citus local tables #4143

onurctirtir · 2020-09-07T15:29:41Z

DESCRIPTION: Introduce citus local tables

This is the final pr to merge Citus Local Tables into master 🎵 🕺

The commits in this pr are merged from other sub-pr's:

Implement "citus local table" creation logic and a udf #3852: Brings lazy&fast table creation logic for create_citus_local_table udf
Extend citus local table utility command support #3995: Brings extended utility command support
for citus local tables
Citus Local Tables: planner + some other last pieces of changes #4133: Brings changes in planner and in several
places to integrate citus local tables into our distributed execution logic

We are introducing citus local tables, which a new table type to citus.

To be able to create a citus local table, first we need to add coordinator as a worker
node.
Then, we can create a citus local table via SELECT create_citus_local_table(<tableName>).

Calling this udf from coordinator will actually create a single-shard table whose shard
is on the coordinator.
Also, from the citus metadata perspective, for citus local tables:

partitionMethod is set to DISTRIBUTE_BY_NONE (like reference tables) and
replicationModel is set to the current value of citus.replication_model, which
already can't be equal to REPLICATION_MODEL_2PC, which is only used for reference
tables internally.

Note that currently we support creating citus local tables only from postgres tables
living in the coordinator.
That means, it is not allowed to execute this udf from worker nodes or it is not allowed
to move shard of a citus local table to any other nodes.

Also, run-time complexity of calling create_citus_local_table udf does not depend
on the size of the relation, that means, creating citus local tables is actually a
non-blocking operation.
This is because, instead of copying the data to a new shard, this udf just does the
following:

convert input postgres table to the single-shard of the citus local table by suffixing
the shardId to it's name, constraints, indexes and triggers etc.,
create a shell table for citus local table in coordinator and in mx-worker nodes when
metadata sycn is enabled.
create necessary objects on shell table.

Here, we should also note we can execute queries/dml's from mx worker nodes
as citus local tables are already first class citus tables.

Even more, we brought trigger support for citus local tables.
That means, we can define triggers on citus local tables so that users can define trigger
objects to perform execution of custom functions that might even modify other citus tables
and other postgres tables.

Other than trigger support, citus local tables can also be involved in foreign key relationships
with reference tables.
Here the only restriction is, foreign keys from reference tables to citus local tables cannot
have behaviors other than RESTRICT & NO ACTION behavior.
Other than that, foreign keys between citus local tables and reference tables just work fine.

All in all, citus local tables are actually just local tables living in the coordinator, but natively
accessible from other nodes like other first class citus tables and this enables us to set foreign
keys constraints between very big coordinator tables and reference tables without having to
do any data replication to worker nodes for local tables.

Parent issue that describes current limitations & TO-DO's can be found here.

onurctirtir · 2020-09-07T15:53:05Z

src/backend/distributed/commands/call.c

@@ -96,13 +96,12 @@ CallFuncExprRemotely(CallStmt *callStmt, DistObjectCacheEntry *procedure,
 	CitusTableCacheEntry *distTable = GetCitusTableCacheEntry(colocatedRelationId);
 	Var *partitionColumn = distTable->partitionColumn;
 	bool colocatedWithReferenceTable = false;
-	if (IsCitusTableTypeCacheEntry(distTable, REFERENCE_TABLE))
+	if (IsCitusTableTypeCacheEntry(distTable, CITUS_TABLE_WITH_NO_DIST_KEY))


Does that change make sense ?

I think we should punt on this change for now, because we have a lot of reference table usage: https://github.com/citusdata/citus/compare/single-placement-table/master-cache-entry-rebased...add_funct_call_delegation?expand=1

I'd suggest we make this back as REFERENCE_TABLE and add Support function call delegation to the todo list. Maybe share this super basic change that potentially allows it.

marcocitus · 2020-09-08T08:15:40Z

src/include/distributed/commands.h

+ * Flags that can be passed to GetForeignKeyOids to indicate
+ * which foreign key constraint OIDs are to be extracted
+ */
+typedef enum ExtractForeignKeyConstrainstMode


Constrainst -> Constraints

lets not forget this

onurctirtir · 2020-09-08T12:10:52Z

Just a heads up, in 5012c6b, the commit disabling check-merge-to-enterprise is just reverted

onderkalaci · 2020-09-08T13:16:28Z

src/backend/distributed/commands/call.c

@@ -96,13 +96,12 @@ CallFuncExprRemotely(CallStmt *callStmt, DistObjectCacheEntry *procedure,
 	CitusTableCacheEntry *distTable = GetCitusTableCacheEntry(colocatedRelationId);
 	Var *partitionColumn = distTable->partitionColumn;
 	bool colocatedWithReferenceTable = false;
-	if (IsCitusTableTypeCacheEntry(distTable, REFERENCE_TABLE))
+	if (IsCitusTableTypeCacheEntry(distTable, CITUS_TABLE_WITH_NO_DIST_KEY))


I think we should punt on this change for now, because we have a lot of reference table usage: https://github.com/citusdata/citus/compare/single-placement-table/master-cache-entry-rebased...add_funct_call_delegation?expand=1

I'd suggest we make this back as REFERENCE_TABLE and add Support function call delegation to the todo list. Maybe share this super basic change that potentially allows it.

onderkalaci · 2020-09-08T13:23:37Z

src/backend/distributed/commands/create_citus_local_table.c

+	/* assert that we created the shell table properly in the same schema */
+	Assert(OidIsValid(shellRelationId));
+
+	DropAndMoveDefaultSequenceOwnerships(shardRelationId, shellRelationId);


might be nice to have a comment on why and how we handle sequences on citus local tables

onderkalaci · 2020-09-08T13:45:14Z

src/include/distributed/commands.h

+ * Flags that can be passed to GetForeignKeyOids to indicate
+ * which foreign key constraint OIDs are to be extracted
+ */
+typedef enum ExtractForeignKeyConstrainstMode


lets not forget this

onderkalaci · 2020-09-08T13:45:59Z

src/include/distributed/commands/utility_hook.h

@@ -30,6 +30,7 @@ typedef enum
 } PropSetCmdBehavior;
 extern PropSetCmdBehavior PropagateSetCommands;
 extern bool EnableDDLPropagation;
+extern bool AllCitusLocalTable;


What is this new variable? We seem to not use it?

this was part of vanilla testing, removing it :)

SaitTalhaNisanci · 2020-09-08T18:02:01Z

src/backend/distributed/commands/create_citus_local_table.c

+
+
+/*
+ * create_citus_local_table creates a citus table from the table with relationId.


Minor: a citus table -> a citus local table

SaitTalhaNisanci · 2020-09-08T18:04:01Z

src/backend/distributed/commands/create_citus_local_table.c

+
+	PG_RETURN_VOID();
+}
+


Minor:

It could be nice to order the functions in a way that they are called, for example you could place CreateCitusLocalTable below this function for better readability

SaitTalhaNisanci · 2020-09-08T18:05:48Z

src/backend/distributed/commands/create_citus_local_table.c

+static void
+CreateCitusLocalTable(Oid relationId)
+{
+	/* these checks should be done before acquiring the lock on the table */


Minor: it could be nicer to explain "why" these checks should be done "before". That way when adding something new, it could help people place it to the right place

SaitTalhaNisanci · 2020-09-08T18:07:27Z

src/backend/distributed/commands/create_citus_local_table.c

+
+/*
+ * ErrorIfUnsupportedCreateCitusLocalTable errors out if we cannot create the
+ * citus local table from the relation with relationId.


from the relation with relationId -> from the relation

we don't have relationId as a parameter

SaitTalhaNisanci · 2020-09-08T18:17:28Z

src/backend/distributed/commands/create_citus_local_table.c

+
+
+/*
+ * FinalizeCitusLocalTableCreation performs completes creation of the citus


either remove performs or completes

SaitTalhaNisanci · 2020-09-08T18:40:39Z

src/backend/distributed/commands/table.c

@@ -1401,6 +1666,87 @@ InterShardDDLTaskList(Oid leftRelationId, Oid rightRelationId,
 }


+/*
+ * CreateRightShardListForInterShardDDLTask is an helper function that creates


an helper -> a helper

SaitTalhaNisanci · 2020-09-08T18:45:27Z

src/backend/distributed/planner/multi_router_planner.c

+
+
+/*
+ * DeferErrorIfUnsupportedModifyQueryWithCitusLocalTable is an helper function


an helper -> a helper

SaitTalhaNisanci · 2020-09-08T18:45:50Z

src/backend/distributed/planner/multi_router_planner.c

+
+
+/*
+ * DeferErrorIfUnsupportedModifyQueryWithPostgresLocalTable is an helper


s/an helper/ a helper/g

SaitTalhaNisanci · 2020-09-08T18:48:15Z

src/backend/distributed/operations/worker_node_manager.c

+{
+	bool groupContainsNodes = false;
+
+	LockRelationOid(DistNodeRelationId(), ShareLock);


What was the difference between not releasing a lock and releasing it with NO_LOCK?

we can't unlock a relation with NO_LOCK

SaitTalhaNisanci · 2020-09-08T18:49:26Z

src/backend/distributed/operations/worker_node_manager.c

+ * returns coordinator node.
+ */
+WorkerNode *
+CoordinatorNode()


CoordinatorNode -> CoordinatorNodeIfAddedAsWorker

what about CoordinatorNodeIfAddedAsWorkerOrError ?

Yeah even better :)

onderkalaci · 2020-09-09T06:37:23Z

src/test/regress/sql/citus_local_tables.sql

+FROM (SELECT shardid FROM pg_dist_shard WHERE logicalrelid='citus_local_table_1'::regclass) as shardid;
+-- undistribute_table is supported
+BEGIN;
+  SELECT undistribute_table('citus_local_table_1');


We should also make sure that master_copy_shard_placement() has an explicit error check for citus local tables. The correct place could be ErrorIfTableCannotBeReplicated()

If you create a citus local table with statement replication, it gives a weird error now

SELECT master_copy_shard_placement(102082, 'localhost', 5432, 'localhost', 9700); ERROR: could not find placement matching "localhost:9700" HINT: Confirm the placement still exists and try again.

this should be outdated, I added necessary changes for that in

citus/src/backend/distributed/operations/repair_shards.c

Lines 186 to 213 in e36b26e

/*

* ErrorIfTableCannotBeReplicated function errors out if the given table is not suitable

* for its shard being replicated. Shard replications is not allowed only for MX tables,

* since RF=1 is a must MX tables.

*/

static void

ErrorIfTableCannotBeReplicated(Oid relationId)

{

/*

* Note that ShouldSyncTableMetadata() returns true for both MX tables

* and reference tables.

*/

bool shouldSyncMetadata = ShouldSyncTableMetadata(relationId);

if (!shouldSyncMetadata)

{

return;

}

CitusTableCacheEntry *tableEntry = GetCitusTableCacheEntry(relationId);

char *relationName = get_rel_name(relationId);

if (IsCitusTableTypeCacheEntry(tableEntry, CITUS_LOCAL_TABLE))

{

ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),

(errmsg("Table %s is a citus local table. Replicating "

"shard of a citus local table currently is not "

"supported", quote_literal_cstr(relationName)))));

}

onderkalaci · 2020-09-09T06:45:18Z

src/test/regress/sql/citus_local_tables_mx.sql

+SELECT * FROM citus_local_table_3;
+
+-- finally show that we do not allow defining foreign key in mx nodes
+ALTER TABLE citus_local_table_3 ADD CONSTRAINT fkey_local_to_local_2 FOREIGN KEY(l1) REFERENCES citus_local_table_4(l1);


Lets add some savepoint regression tests at this moment. It shouldn't take a lot of time, you can check the existing savepoint tests.

To simple tests like,

Do a modification to citus local table;
rollback to savepoint;
see that the modification is rolled back

===

Do a modification to citus local table;
Do a modification to distributed table;
rollback to savepoint;
see that the modificatios are rolled back

===
Do a modification to citus local table;
Do a modification to reference table;
rollback to savepoint;
see that the modifications are rolled back

onderkalaci

I think we are good to merge this now. It has been a long journey, we started with the idea of enabling foreign keys between local tables and reference tables, and we ended-up with a first class table type in Citus. This work will be essential for hybrid local/distributed table clusters.

We'll need later to iterate to implement all the missing pieces (#4145).

This commit brings following features: Foreign key support from citus local tables to reference tables * Foreign key support from reference tables to citus local tables (only with RESTRICT & NO ACTION behavior) * ALTER TABLE ENABLE/DISABLE trigger command support * CREATE/DROP/ALTER trigger command support and disallows: * ALTER TABLE ATTACH/DETACH PARTITION commands * CREATE TABLE <postgres table> ATTACH PARTITION <citus local table> commands * Foreign keys from postgres tables to citus local tables (the other way was already disallowed) for citus local tables.

onurctirtir commented Sep 7, 2020

View reviewed changes

marcocitus reviewed Sep 8, 2020

View reviewed changes

onderkalaci reviewed Sep 8, 2020

View reviewed changes

onurctirtir mentioned this pull request Sep 8, 2020

Adding local tables to metadata: Limitations & TO-DO's #4145

Open

onurctirtir requested a review from onderkalaci September 8, 2020 15:27

onurctirtir added this to the 9.5 Release milestone Sep 8, 2020

onurctirtir self-assigned this Sep 8, 2020

onurctirtir mentioned this pull request Sep 8, 2020

Foreign keys between reference tables and coordinator local tables: A different approach #3641

Closed

SaitTalhaNisanci reviewed Sep 8, 2020

View reviewed changes

SaitTalhaNisanci added the feature label Sep 9, 2020

onderkalaci reviewed Sep 9, 2020

View reviewed changes

onderkalaci approved these changes Sep 9, 2020

View reviewed changes

onurctirtir added 4 commits September 9, 2020 11:50

Implement "citus local table" creation logic

17cc810

Adapt other cache entry changes for citus local tables

0b1cc11

Apply planner changes for citus local tables

3a73fba

onurctirtir force-pushed the single-placement-table/master-cache-entry-rebased branch from ab643b5 to 3a73fba Compare September 9, 2020 08:53

onurctirtir merged commit 5e5ba46 into master Sep 9, 2020

onderkalaci mentioned this pull request Oct 20, 2020

Automate table type decision: distributed vs reference vs citus local vs pg local #4261

Open

This was referenced Nov 19, 2020

Improve support for single shard tables #3583

Closed

Automatically add local tables on the coordinator to the metadata #3335

Closed

onurctirtir deleted the single-placement-table/master-cache-entry-rebased branch December 2, 2020 09:21

agedemenli mentioned this pull request Jan 6, 2022

Support foreign tables in MX #5461

Merged

aykut-bozkurt mentioned this pull request May 30, 2022

Propagate ALTER TABLE foo ENABLE TRIGGER to workers #2924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce citus local tables #4143

Introduce citus local tables #4143

onurctirtir commented Sep 7, 2020 •

edited

onurctirtir Sep 7, 2020

onderkalaci Sep 8, 2020

marcocitus Sep 8, 2020

onderkalaci Sep 8, 2020

onurctirtir commented Sep 8, 2020

onderkalaci Sep 8, 2020

onderkalaci Sep 8, 2020

onderkalaci Sep 8, 2020

onderkalaci Sep 8, 2020

onurctirtir Sep 8, 2020 •

edited

SaitTalhaNisanci Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

onurctirtir Sep 8, 2020

SaitTalhaNisanci Sep 8, 2020

onurctirtir Sep 8, 2020

SaitTalhaNisanci Sep 9, 2020

onderkalaci Sep 9, 2020

onurctirtir Sep 9, 2020

onderkalaci Sep 9, 2020

onderkalaci left a comment



		/*
		* create_citus_local_table creates a citus table from the table with relationId.



		/*
		* FinalizeCitusLocalTableCreation performs completes creation of the citus



		/*
		* DeferErrorIfUnsupportedModifyQueryWithCitusLocalTable is an helper function

	/*
	* ErrorIfTableCannotBeReplicated function errors out if the given table is not suitable
	* for its shard being replicated. Shard replications is not allowed only for MX tables,
	* since RF=1 is a must MX tables.
	*/
	static void
	ErrorIfTableCannotBeReplicated(Oid relationId)
	{
	/*
	* Note that ShouldSyncTableMetadata() returns true for both MX tables
	* and reference tables.
	*/
	bool shouldSyncMetadata = ShouldSyncTableMetadata(relationId);
	if (!shouldSyncMetadata)
	{
	return;
	}

	CitusTableCacheEntry *tableEntry = GetCitusTableCacheEntry(relationId);
	char *relationName = get_rel_name(relationId);

	if (IsCitusTableTypeCacheEntry(tableEntry, CITUS_LOCAL_TABLE))
	{
	ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
	(errmsg("Table %s is a citus local table. Replicating "
	"shard of a citus local table currently is not "
	"supported", quote_literal_cstr(relationName)))));
	}

Introduce citus local tables #4143

Introduce citus local tables #4143

Conversation

onurctirtir commented Sep 7, 2020 • edited

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 commented Sep 8, 2020

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 Sep 8, 2020 • edited

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

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

onderkalaci left a comment

Choose a reason for hiding this comment

onurctirtir commented Sep 7, 2020 •

edited

onurctirtir Sep 8, 2020 •

edited