Introduce GUC citus.use_citus_managed_tables by agedemenli · Pull Request #5440 · citusdata/citus

Ahmet Gedemenli (agedemenli) · 2021-11-05T14:06:49Z

DESCRIPTION: Introduce GUC use_citus_managed_tables

After CREATE TABLE, the newly created table will be added to metadata by default if the GUC citus.use_citus_managed_tables is set to true.
The parameters for the conversion is as follows:

cascadeViaForeignKeys = true
autoConverted = false

Note that this is valid for only the coordinator (and if it's added as worker). Also, we will skip partition creations, as well as temporary tables.

The default value for the GUC is false.

Ahmet Gedemenli (agedemenli) · 2021-11-05T14:07:24Z

citus.so!DeferErrorIfUnsupportedRouterPlannableSelectQuery(Query * query) (\home\ahmet\citus\src\backend\distributed\planner\multi_router_planner.c:3592)
citus.so!CreateRouterPlan(Query * originalQuery, Query * query, PlannerRestrictionContext * plannerRestrictionContext) (\home\ahmet\citus\src\backend\distributed\planner\multi_router_planner.c:194)
citus.so!CreateDistributedPlan(uint64 planId, Query * originalQuery, Query * query, ParamListInfo boundParams, _Bool hasUnresolvedParams, PlannerRestrictionContext * plannerRestrictionContext) (\home\ahmet\citus\src\backend\distributed\planner\distributed_planner.c:916)
citus.so!CreateInsertSelectIntoLocalTablePlan(uint64 planId, Query * originalQuery, ParamListInfo boundParams, _Bool hasUnresolvedParams, PlannerRestrictionContext * plannerRestrictionContext) (\home\ahmet\citus\src\backend\distributed\planner\insert_select_planner.c:381)
citus.so!CreateDistributedPlan(uint64 planId, Query * originalQuery, Query * query, ParamListInfo boundParams, _Bool hasUnresolvedParams, PlannerRestrictionContext * plannerRestrictionContext) (\home\ahmet\citus\src\backend\distributed\planner\distributed_planner.c:883)
citus.so!CreateDistributedPlannedStmt(DistributedPlanningContext * planContext) (\home\ahmet\citus\src\backend\distributed\planner\distributed_planner.c:656)
citus.so!PlanDistributedStmt(DistributedPlanningContext * planContext, int rteIdCounter) (\home\ahmet\citus\src\backend\distributed\planner\distributed_planner.c:593)
citus.so!distributed_planner(Query * parse, const char * query_string, int cursorOptions, ParamListInfo boundParams) (\home\ahmet\citus\src\backend\distributed\planner\distributed_planner.c:229)
pg_stat_statements.so!pgss_planner(Query * parse, const char * query_string, int cursorOptions, ParamListInfo boundParams) (\home\ahmet\.pgenv\src\postgresql-14.0\contrib\pg_stat_statements\pg_stat_statements.c:946)
planner(Query * parse, const char * query_string, int cursorOptions, ParamListInfo boundParams) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\optimizer\plan\planner.c:269)
pg_plan_query(Query * querytree, const char * query_string, int cursorOptions, ParamListInfo boundParams) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\tcop\postgres.c:847)
pg_plan_queries(List * querytrees, const char * query_string, int cursorOptions, ParamListInfo boundParams) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\tcop\postgres.c:939)
BuildCachedPlan(CachedPlanSource * plansource, List * qlist, ParamListInfo boundParams, QueryEnvironment * queryEnv) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\utils\cache\plancache.c:935)
GetCachedPlan(CachedPlanSource * plansource, ParamListInfo boundParams, ResourceOwner owner, QueryEnvironment * queryEnv) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\utils\cache\plancache.c:1217)
_SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI, Snapshot snapshot, Snapshot crosscheck_snapshot, _Bool read_only, _Bool allow_nonatomic, _Bool fire_triggers, uint64 tcount, DestReceiver * caller_dest, ResourceOwner plan_owner) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\spi.c:2411)
SPI_execute(const char * src, _Bool read_only, long tcount) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\spi.c:523)
citus.so!ExecuteQueryViaSPI(char * query, int SPIOK) (\home\ahmet\citus\src\backend\distributed\commands\alter_table.c:1762)
citus.so!ReplaceTable(Oid sourceId, Oid targetId, List * justBeforeDropCommands, _Bool suppressNoticeMessages) (\home\ahmet\citus\src\backend\distributed\commands\alter_table.c:1343)
citus.so!ConvertTable(TableConversionState * con) (\home\ahmet\citus\src\backend\distributed\commands\alter_table.c:743)
citus.so!UndistributeTable(TableConversionParameters * params) (\home\ahmet\citus\src\backend\distributed\commands\alter_table.c:391)
citus.so!DropFKeysAndUndistributeTable(Oid relationId) (\home\ahmet\citus\src\backend\distributed\commands\create_distributed_table.c:785)
citus.so!CreateDistributedTable(Oid relationId, Var * distributionColumn, char distributionMethod, int shardCount, _Bool shardCountIsStrict, char * colocateWithTableName, _Bool viaDeprecatedAPI) (\home\ahmet\citus\src\backend\distributed\commands\create_distributed_table.c:394)
citus.so!create_reference_table(FunctionCallInfo fcinfo) (\home\ahmet\citus\src\backend\distributed\commands\create_distributed_table.c:315)
ExecInterpExpr(ExprState * state, ExprContext * econtext, _Bool * isnull) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\execExprInterp.c:749)
ExecInterpExprStillValid(ExprState * state, ExprContext * econtext, _Bool * isNull) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\execExprInterp.c:1824)
ExecEvalExprSwitchContext(_Bool * isNull, ExprContext * econtext, ExprState * state) (\home\ahmet\.pgenv\src\postgresql-14.0\src\include\executor\executor.h:339)
ExecProject(ProjectionInfo * projInfo) (\home\ahmet\.pgenv\src\postgresql-14.0\src\include\executor\executor.h:373)
ExecResult(PlanState * pstate) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\nodeResult.c:136)
ExecProcNodeFirst(PlanState * node) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\execProcnode.c:463)
ExecProcNode(PlanState * node) (\home\ahmet\.pgenv\src\postgresql-14.0\src\include\executor\executor.h:257)
ExecutePlan(EState * estate, PlanState * planstate, _Bool use_parallel_mode, CmdType operation, _Bool sendTuples, uint64 numberTuples, ScanDirection direction, DestReceiver * dest, _Bool execute_once) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\execMain.c:1551)
standard_ExecutorRun(QueryDesc * queryDesc, ScanDirection direction, uint64 count, _Bool execute_once) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\execMain.c:361)
citus.so!CitusExecutorRun(QueryDesc * queryDesc, ScanDirection direction, uint64 count, _Bool execute_once) (\home\ahmet\citus\src\backend\distributed\executor\multi_executor.c:214)
pg_stat_statements.so!pgss_ExecutorRun(QueryDesc * queryDesc, ScanDirection direction, uint64 count, _Bool execute_once) (\home\ahmet\.pgenv\src\postgresql-14.0\contrib\pg_stat_statements\pg_stat_statements.c:1001)
ExecutorRun(QueryDesc * queryDesc, ScanDirection direction, uint64 count, _Bool execute_once) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\executor\execMain.c:303)
PortalRunSelect(Portal portal, _Bool forward, long count, DestReceiver * dest) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\tcop\pquery.c:919)
PortalRun(Portal portal, long count, _Bool isTopLevel, _Bool run_once, DestReceiver * dest, DestReceiver * altdest, QueryCompletion * qc) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\tcop\pquery.c:763)
exec_simple_query(const char * query_string) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\tcop\postgres.c:1214)
PostgresMain(int argc, char ** argv, const char * dbname, const char * username) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\tcop\postgres.c:4486)
BackendRun(Port * port) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\postmaster\postmaster.c:4506)
BackendStartup(Port * port) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\postmaster\postmaster.c:4228)
ServerLoop() (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\postmaster\postmaster.c:1745)
PostmasterMain(int argc, char ** argv) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\postmaster\postmaster.c:1417)
main(int argc, char ** argv) (\home\ahmet\.pgenv\src\postgresql-14.0\src\backend\main\main.c:209)

Marco Slot (marcocitus) · 2021-11-06T10:08:02Z

 	}
 	PG_END_TRY();
+
+	if (AddAllLocalTablesToMetadata && ShouldConvertTable && IsA(parsetree, CreateStmt) && IsCoordinator() && !IsBinaryUpgrade)


needs a context == PROCESS_UTILITY_TOPLEVEL check to avoid Citus calling this in its own CREATE TABLE statements (causes most of the current test failures)

maybe also a CoordinatorAddedAsWorkerNode()

Marco Slot (marcocitus) · 2021-11-06T12:22:03Z

+	if (AddAllLocalTablesToMetadata && ShouldConvertTable && IsA(parsetree, CreateStmt) && IsCoordinator() && !IsBinaryUpgrade)
+	{
+		CreateStmt *createTableStmt = (CreateStmt *) parsetree;
+		if (createTableStmt->relation->relpersistence != 't')


maybe also createTableStmt->partbound == NULL to avoid CREATE TABLE .. PARTITION OF issues (should be captured already by parent auto-converting.

Marco Slot (marcocitus) · 2021-11-06T12:24:52Z

+	if (!CoordinatorAddedAsWorkerNode() && AddAllLocalTablesToMetadata)
+	{
+		ExecuteQueryViaSPI("select citus_add_node('localhost', 9700, groupid=>0);", SPI_OK_SELECT);
+	}


Shouldn't InsertCoordinatorIfClusterEmpty above already do this?

Marco Slot (marcocitus) · 2021-11-08T11:59:37Z

+			CommandCounterIncrement();
+			Oid relationId = RangeVarGetRelid(createTableStmt->relation, NoLock, false); 
+			CreateCitusLocalTable(relationId, true, false);
+			ShouldConvertTable = true;


now that we have PROCESS_UTILITY_TOPLEVEL check, do we still need ShouldConvertTable?

This isn't a very reliable flag at the moment, e.g. errors may leave the flag as false, but I'm wondering whether we need it.

Marco Slot (marcocitus) · 2021-11-08T12:00:37Z

 	}
 	PG_END_TRY();
+
+	if (AddAllLocalTablesToMetadata && ShouldConvertTable && !IsBinaryUpgrade &&


could make sense to move this into the if (UtilityHookLevel == 1) block above, below the if (ShouldCheckUndistributeCitusLocalTables()) block.

Marco Slot (marcocitus)

I think being able to do this via a (default off) GUC is valuable.

Can we maybe use this GUC in (parts of) auto_undist_citus_local.out to add some test coverage?

Marco Slot (marcocitus) · 2021-11-09T10:15:31Z

+				if (createTableStmt->relation->relpersistence != 't' &&
+					createTableStmt->partbound == NULL)
+				{
+					CommandCounterIncrement();


Can you add a comment on why this is needed?

Marco Slot (marcocitus) · 2021-11-09T10:15:49Z

+
+			if (AddAllLocalTablesToMetadata && !IsBinaryUpgrade &&
+				IsA(parsetree, CreateStmt) && context == PROCESS_UTILITY_TOPLEVEL &&
+				IsCoordinator() && CoordinatorAddedAsWorkerNode())


Would be good to define a function that captures all of these conditions and the ones below.

Marco Slot (marcocitus) · 2021-11-09T16:12:39Z

+
+	CreateStmt *createTableStmt = (CreateStmt *) parsetree;
+
+	if (createTableStmt->relation->relpersistence == 't' ||


't' -> RELPERSISTENCE_TEMP

Marco Slot (marcocitus) · 2021-11-09T16:14:31Z

 			ResetConstraintDropped();
+
+			if (context == PROCESS_UTILITY_TOPLEVEL &&
+				NeedAddToMetadataIfTableCreated(parsetree))


Maybe ShouldAddNewTableToMetadata

mainly, we usually start these function names with Should

Marco Slot (marcocitus) · 2021-11-10T15:40:38Z

+				Oid relationId = RangeVarGetRelid(createTableStmt->relation,
+												  NoLock, false);
+				bool cascade = true;
+				bool autoConverted = false;


maybe add a comment on why autoConverted is false even though we're auto-converting :)

(wondering whether we should rename that column)

I think it becomes insignificant when this new GUC is set to true. Still we shouldn't mark it as autoConverted = true, since the user explicitly wants these tables to be added to metadata, by setting the GUC.

I'll add the comment

Marco Slot (marcocitus) · 2021-11-10T15:41:11Z

+		"citus.use_citus_managed_tables",
+		gettext_noop("Adds local tables to metadata by default when enabled"),
+		gettext_noop("When enabled, calls CreateCitusLocalTable after every "
+					 "CREATE TABLE statement. Set to false by default."),


can you update the descriptions?

Marco Slot (marcocitus) · 2021-11-11T09:52:07Z

 #include "utils/syscache.h"


+bool AddAllLocalTablesToMetadata = true;


maybe add a comment on how this is used, especially since it's not used in this file

Ahmet Gedemenli (agedemenli) force-pushed the default-add-to-metadata-experiment branch from 2c93b67 to 0d8e6ba Compare November 5, 2021 14:32

Marco Slot (marcocitus) reviewed Nov 6, 2021

View reviewed changes

Ahmet Gedemenli (agedemenli) force-pushed the default-add-to-metadata-experiment branch from e6486bf to 7dd3755 Compare November 8, 2021 09:01

Marco Slot (marcocitus) reviewed Nov 8, 2021

View reviewed changes

Marco Slot (marcocitus) reviewed Nov 9, 2021

View reviewed changes

Ahmet Gedemenli (agedemenli) force-pushed the default-add-to-metadata-experiment branch from bec7fa6 to 602a1f7 Compare November 9, 2021 15:55

Ahmet Gedemenli (agedemenli) changed the title ~~Default add to metadata experiment~~ Introduce GUC citus.access_new_local_tables_from_workers Nov 9, 2021

Ahmet Gedemenli (agedemenli) marked this pull request as ready for review November 9, 2021 15:56

Marco Slot (marcocitus) reviewed Nov 9, 2021

View reviewed changes

Marco Slot (marcocitus) reviewed Nov 10, 2021

View reviewed changes

Marco Slot (marcocitus) approved these changes Nov 11, 2021

View reviewed changes

Ahmet Gedemenli (agedemenli) force-pushed the default-add-to-metadata-experiment branch 2 times, most recently from 74fb582 to 2e02826 Compare November 11, 2021 10:25

Ahmet Gedemenli (agedemenli) changed the title ~~Introduce GUC citus.access_new_local_tables_from_workers~~ Introduce GUC citus.use_citus_managed_tables Nov 11, 2021

Introduce GUC citus.use_citus_managed_tables

14a33d4

Ahmet Gedemenli (agedemenli) force-pushed the default-add-to-metadata-experiment branch from 2e02826 to 14a33d4 Compare November 11, 2021 11:09

Ahmet Gedemenli (agedemenli) merged commit 1aa32d5 into master Nov 11, 2021

Ahmet Gedemenli (agedemenli) deleted the default-add-to-metadata-experiment branch November 11, 2021 11:39

This was referenced Jan 6, 2022

Support foreign tables in MX #5461

Merged

IMPORT FOREIGN SCHEMA doesn't add to metadata when use_citus_managed_tables is on #5603

Open


		CreateStmt createTableStmt = (CreateStmt ) parsetree;

		if (createTableStmt->relation->relpersistence == 't' \|\|

		#include "utils/syscache.h"


		bool AddAllLocalTablesToMetadata = true;

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Ahmet Gedemenli (agedemenli) commented Nov 5, 2021

Uh oh!

Marco Slot (marcocitus) Nov 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marco Slot (marcocitus) Nov 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Marco Slot (marcocitus) left a comment

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!

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 Nov 5, 2021 •

edited

Loading

Marco Slot (marcocitus) Nov 6, 2021 •

edited

Loading

Marco Slot (marcocitus) Nov 6, 2021 •

edited

Loading