Propagate foreign server ops #5468

agedemenli · 2021-11-16T17:45:10Z

DESCRIPTION: Adds propagation for foreign server commands

marcocitus · 2021-11-18T15:58:03Z

src/backend/distributed/commands/distribute_object_ops.c

@@ -837,6 +865,16 @@ GetDistributeObjectOps(Node *node)
 			return &Any_CreateFunction;
 		}

+		case T_CreateFdwStmt:
+		{
+			return &Any_CreateFdw;


Since foreign data wrappers are almost always created by an extension and we explicitly do not want to propagate objects created by an extension, we could skip CREATE FOREIGN DATA WRAPPER.

What if it's not created by an extension? Like the one we have in the regression tests:

CREATE FUNCTION fake_fdw_handler() RETURNS fdw_handler AS 'citus' LANGUAGE C STRICT; CREATE FOREIGN DATA WRAPPER fake_fdw HANDLER fake_fdw_handler;

I know that it doesn't seem like something a user would do :)
So you're suggesting to skip those CREATE FDW statements because (almost) no one will try to create fdw's manually, right?

The utility seems low and there is some cost (e.g. making sure we don't accidentally break anything, making sure deparsing logic does not have SQL injection bugs). I'm ok supporting it, but I'm also ok not supporting it.

src/backend/distributed/commands/foreign_server.c

marcocitus · 2021-11-22T12:46:53Z

src/backend/distributed/commands/foreign_data_wrapper.c

+PreprocessCreateFdwStmt(Node *node, const char *queryString,
+						ProcessUtilityContext processUtilityContext)
+{
+	EnsureCoordinator();


EnsureCoordinator should be below ShouldPropagate, otherwise we block this command on workers altogether

ah, makes sense

codecov · 2021-11-23T16:05:43Z

Codecov Report

Merging #5468 (7958d44) into master (77d948a) will increase coverage by 0.02%.
The diff coverage is 97.20%.

❗ Current head 7958d44 differs from pull request most recent head 945f174. Consider uploading reports for the commit 945f174 to get more accurate results

@@            Coverage Diff             @@
##           master    #5468      +/-   ##
==========================================
+ Coverage   92.75%   92.77%   +0.02%     
==========================================
  Files         215      217       +2     
  Lines       45294    45509     +215     
==========================================
+ Hits        42012    42222     +210     
- Misses       3282     3287       +5

src/backend/distributed/commands/foreign_server.c

velioglu · 2021-11-30T09:56:48Z

src/backend/distributed/commands/foreign_server.c

+PreprocessAlterForeignServerStmt(Node *node, const char *queryString,
+								 ProcessUtilityContext processUtilityContext)
+{
+	if (!ShouldPropagate())


We can use ShouldPropagateObject instead of calling ShouldPropagate and IsObjectDistributed separately, as it looks more clear to me for objects. It is same for ones below as well.

makes sense, done

src/test/regress/sql/multi_create_fdw.sql

velioglu · 2021-12-03T12:24:58Z

src/backend/distributed/commands/foreign_server.c

+/*
+ * CreateForeignServerStmtObjectAddress finds the ObjectAddress for the server
+ * that is created by given CreateForeignServerStmt. If missingOk is false and if
+ * the statistics does not exist, then it errors out.


Stale statistics comment

fixed, thanks

velioglu · 2021-12-03T12:33:29Z

src/test/regress/sql/multi_create_fdw.sql

@@ -12,5 +12,44 @@ RETURNS fdw_handler
 AS 'citus'
 LANGUAGE C STRICT;

+set citus.enable_ddl_propagation to off;


Why do we disable ddl propagation here?

We don't want to propagate the commands between set citus.enable_ddl_propagation to off; and set citus.enable_ddl_propagation to on;, because these objects are already created on the workers, in pg_regress_multi.pl. Because of that, we get an "already exists" error on the worker, if we don't disable ddl prop.

We also don't want to remove them since some other tests depend on these.

We should probably remove from pg_regress_multi.pl and let this create

onderkalaci · 2021-12-03T13:50:14Z

src/backend/distributed/deparser/deparse_foreign_server_stmts.c

+static char * GetDefElemActionString(DefElemAction action);
+
+char *
+DeparseCreateForeignServerStmt(Node *node)


minor: needs comment

Well, it seems we don't have much comments on these deparse functions, so I'm fine with not having here as well

src/backend/distributed/deparser/deparse_foreign_server_stmts.c

onderkalaci · 2021-12-03T14:05:07Z

src/test/regress/sql/multi_create_fdw.sql

+-- verify that the server is created on the worker
+SELECT COUNT(*)=1 FROM pg_foreign_server WHERE srvname = 'foreign_server';
+\c - - - :master_port
+ALTER SERVER foreign_server OPTIONS (ADD passfile 'to_be_dropped');


can we have any quoted/escaped options and/or values?

onderkalaci · 2021-12-03T14:06:58Z

src/backend/distributed/deparser/deparse_foreign_server_stmts.c

+	foreach_ptr(def, stmt->options)
+	{
+		const char *value = defGetString(def);
+		appendStringInfo(buf, "%s \'%s\'", def->defname, value);


similarly: should we call quote_literal_cstr for name/value?

Defnames don't have quotes according to pg syntax.
Values have single quotes. I've added quote_identifier for that

onderkalaci · 2021-12-03T14:09:52Z

src/backend/distributed/commands/foreign_server.c

+{
+	bool missingOk = false;
+	ObjectAddress typeAddress = GetObjectAddressFromParseTree(node, missingOk);
+	EnsureDependenciesExistOnAllNodes(&typeAddress);


What kind of dependency a foreign server might have? Do we have a test for that?

So, I think a server can depend to an extension, and an extension can depend on a schema? So, such a test might be nice:

cREATE SCHEMa sc1; CREATE EXTENSION postgres_fdw WITH SCHEMA sc1; CREATE SERVER .... -- make sure that schema is also distributed

Added, thanks!

onderkalaci · 2021-12-03T14:11:50Z

src/test/regress/expected/multi_create_fdw.out

@@ -9,3 +9,71 @@ AS 'citus'
 LANGUAGE C STRICT;
 CREATE FOREIGN DATA WRAPPER fake_fdw HANDLER fake_fdw_handler;
 CREATE SERVER fake_fdw_server FOREIGN DATA WRAPPER fake_fdw;
+-- test propagating foreign server creation
+CREATE EXTENSION postgres_fdw;
+CREATE SERVER foreign_server TYPE 'test_type' VERSION 'v1'


I think we should have a very basic test with citus_add_node. Probably here is not the best place to add such a test, but can you find a place where we ensure that adding node sends the foreign server to the new node as well?

I went over the test schedule but this file seemed like the most relevant place for this test. So I've added it to multi_create_fdw.sql.

If you have a suggestion I could consider moving it

src/test/regress/expected/local_table_join.out

marcocitus · 2021-12-06T15:16:15Z

src/backend/distributed/deparser/deparse_foreign_server_stmts.c

+		{
+			const char *value = defGetString(def);
+
+			appendStringInfo(buf, " \'%s\'", value);


Any time you see quotes in an appendStringInfo, assume that there's a SQL injection bug.

Quoting should always be done separately to take into account escaping.

I've added that for CREATE SERVER OPTIONS: https://github.com/citusdata/citus/pull/5468/files#diff-c2f60db4cb9c4046fb711bd8ded1841ad0cede72d4ed8239aaf91e6151e9f90eR150-R151

But apparently missed that for ALTER SERVER.

Could you please check the one that is for CREATE SERVER? I'm considering applying the same here

onderkalaci

I'll continue reviewing & testing

src/backend/distributed/commands/foreign_server.c

onderkalaci · 2021-12-08T09:06:55Z

src/backend/distributed/commands/foreign_server.c

+		UnmarkObjectDistributed(address);
+	}
+
+	/*


This feels dangerous. Can't we create a new DropStmt with distributedServerNames?

In fact, I'm in favor of throwing an error if not all of them are distributed servers.

As we discussed earlier, these kinds of operations are dangerous for MX anyway.

onderkalaci · 2021-12-08T09:13:06Z

src/backend/distributed/commands/foreign_server.c

+CreateForeignServerStmtObjectAddress(Node *node, bool missing_ok)
+{
+	CreateForeignServerStmt *stmt = castNode(CreateForeignServerStmt, node);
+	ForeignServer *server = GetForeignServerByName(stmt->servername, missing_ok);


I think passing servername to this function can help to remove lots of repetition in the code, say we can use it in PreprocessRenameForeignServerStmt and others

Don't we expect all object address functions to have this signature?

https://github.com/citusdata/citus/blob/master/src/backend/distributed/deparser/objectaddress.c#L36

What I meant is move the following -- by passing servername as parameter:

ForeignServer *server = GetForeignServerByName(servername, missing_ok); Oid serverOid = server->serverid; ObjectAddress address = { 0 }; ObjectAddressSet(address, ForeignServerRelationId, serverOid);

to a function, and use it in functions such as PreprocessRenameForeignServerStmt. But you need to palloc and return address*

onderkalaci · 2021-12-08T09:16:47Z

src/backend/distributed/commands/foreign_server.c

+	CreateForeignServerStmt *createStmt = makeNode(CreateForeignServerStmt);
+
+	/* set server name and if_not_exists fields */
+	createStmt->servername = server->servername;


don't we need to createStmt->servername in this memory context? createStmt->servername = pstrdup(server->servername);

We should do same for all the pointers in the new CreateForeignServerStmt. As an example, see RecreateExtensionStmt

src/backend/distributed/commands/distribute_object_ops.c

src/test/regress/expected/propagate_foreign_servers.out

onderkalaci · 2021-12-08T09:52:47Z

src/test/regress/expected/propagate_foreign_servers.out

+SELECT srvowner FROM pg_foreign_server WHERE srvname = 'foreign''server_1!';
+ srvowner
+---------------------------------------------------------------------
+     3373


can we join with pg_roles to print the rolname instead of oid? This applies to all role ids in the test

something like:

SELECT rolname FROM pg_roles JOIN pg_foreign_server ON (pg_roles.oid=pg_foreign_server.srvowner) WHERE srvname = 'foreign_server'; rolname ------------- onderkalaci (1 row)

so that the tests do not rely on OIDs

onderkalaci · 2021-12-08T10:01:11Z

src/backend/distributed/commands/foreign_server.c

+	}
+
+	EnsureCoordinator();
+


Do we need EnsureDependenciesExistOnAllNodes here? I think with AlterServer you cannot alter the server to depend on a new object, right? Such as ALTER SERVER .. SET SCHEMA new_schema or such? If so, we should be fine.

As far as I can read from the docs, ALTER SERVER can only change Server's options?

It's allowed to alter a server's owner, name and options. I thought like we might need to ensure the new owner role exists on the workers?

Ah, that's why we do it on PostProcess.. ?

onderkalaci · 2021-12-08T10:04:02Z

src/backend/distributed/commands/foreign_server.c

+PreprocessAlterForeignServerOwnerStmt(Node *node, const char *queryString,
+									  ProcessUtilityContext processUtilityContext)
+{
+	AlterOwnerStmt *stmt = castNode(AlterOwnerStmt, node);


One thing that is tricky in this PR is alter owner. What if the new owner role is not on the workers?

alter server myserver2 OWNER TO my_role; ERROR: role "my_role" does not exist CONTEXT: while executing command on localhost:9702 Time: 52.760 ms

I think that might be fine on the community, but on the enterprise we probably need EnsureDependencies call to make sure that the necessary objects(e.g., roles) are on the workers. So, we should still add EnsureDependenciesExistOnAllNodes here.

And, on the enteprise specific tests, we should add something like

-- prevent creating the roles just yet, let ALTER SERVER do that SET citus.enable_object_propagation TO off; create role ... ; SET citus.enable_object_propagation TO on; ALTER SERVER .. SET OWNER ...; .. check owner is set properly on the workers ..

I think we have the code and the same test for schemas, so probably fine not to add a specific test on this.

After getting this to enterprise, can you manually verify that it works expected (e.g., we send the roles)?

src/test/regress/expected/propagate_foreign_servers.out

marcocitus · 2021-12-22T10:44:45Z

src/backend/distributed/deparser/deparse_foreign_server_stmts.c

+		return;
+	}
+
+	appendStringInfoString(buf, "OPTIONS (");


you might be able to use AppendOptionListToString here

onderkalaci

I think this is almost ready to merge, I had few minor questions

src/test/regress/expected/propagate_foreign_servers.out

onderkalaci · 2021-12-23T08:29:17Z

src/test/regress/expected/propagate_foreign_servers.out

+SELECT srvowner FROM pg_foreign_server WHERE srvname = 'foreign''server_1!';
+ srvowner
+---------------------------------------------------------------------
+     3373


something like:

SELECT rolname FROM pg_roles JOIN pg_foreign_server ON (pg_roles.oid=pg_foreign_server.srvowner) WHERE srvname = 'foreign_server'; rolname ------------- onderkalaci (1 row)

so that the tests do not rely on OIDs

onderkalaci · 2021-12-23T08:33:37Z

src/backend/distributed/commands/foreign_server.c

+PreprocessAlterForeignServerOwnerStmt(Node *node, const char *queryString,
+									  ProcessUtilityContext processUtilityContext)
+{
+	AlterOwnerStmt *stmt = castNode(AlterOwnerStmt, node);


I think we have the code and the same test for schemas, so probably fine not to add a specific test on this.

After getting this to enterprise, can you manually verify that it works expected (e.g., we send the roles)?

src/test/regress/expected/propagate_foreign_servers.out

src/backend/distributed/commands/foreign_server.c

onderkalaci · 2021-12-23T09:25:34Z

src/backend/distributed/commands/foreign_server.c

+	foreach_ptr(serverValue, serverNames)
+	{
+		char *serverNameString = strVal(serverValue);
+		ForeignServer *server = GetForeignServerByName(serverNameString, false);


can we use GetObjectAddressByServerName?

src/backend/distributed/commands/foreign_server.c

onderkalaci · 2021-12-23T09:26:07Z

src/backend/distributed/commands/foreign_server.c

+	Assert(stmt->renameType == OBJECT_FOREIGN_SERVER);
+
+	char *serverName = strVal(stmt->object);
+	ForeignServer *server = GetForeignServerByName(serverName, false);


can we use GetObjectAddressByServerName?

onderkalaci · 2021-12-23T09:30:21Z

src/backend/distributed/commands/foreign_server.c

+GetForeignServerCreateDDLCommand(Oid serverId)
+{
+	/* generate a statement for creation of the server in "if not exists" construct */
+	Node *stmt = RecreateForeignServerStmt(serverId);


I really couldn't follow why we only have RecreateForeignServerStmt but say we don't have ReCreateDropForeignServerStmt or ReCreateAlterForeignServerStmt.

What makes RecreateForeignServerStmt special?

If you copy & pasted the logic from somewhere else, can you ask the original author why that is the case?

This function is getting used to create the server on a worker when, say, we are doing a citus_add_node.
Here we generate a CreateForeignServerStmt with if_not_exists = true, then deparse it and send the command to workers.

onderkalaci · 2021-12-23T14:24:46Z

src/backend/distributed/deparser/deparse_foreign_server_stmts.c

+static char * GetDefElemActionString(DefElemAction action);
+
+char *
+DeparseCreateForeignServerStmt(Node *node)


Well, it seems we don't have much comments on these deparse functions, so I'm fine with not having here as well

onderkalaci · 2021-12-23T14:27:22Z

src/test/regress/sql/propagate_foreign_servers.sql

+         FOREIGN DATA WRAPPER postgres_fdw
+         OPTIONS (host 'testhost', port '5432', dbname 'testdb');
+
+\c - - - :master_port


I think we should not leave any objects behind. So, maybe recreate test_dependent_schema and make sure once we drop, no objects left behind.

In general, left over objects can become painful once you do large refactors etc.

agedemenli force-pushed the propagate-foreign-server-ops branch from 5a408f1 to 9bf6664 Compare November 17, 2021 17:33

marcocitus reviewed Nov 18, 2021

View reviewed changes

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

marcocitus reviewed Nov 22, 2021

View reviewed changes

agedemenli marked this pull request as ready for review November 24, 2021 14:01

velioglu reviewed Nov 30, 2021

View reviewed changes

agedemenli force-pushed the propagate-foreign-server-ops branch from 99c7d59 to 917903b Compare December 2, 2021 14:35

onderkalaci mentioned this pull request Dec 3, 2021

Fix fails with metadata syncing in undistribute_table #5503

Merged

agedemenli force-pushed the propagate-foreign-server-ops branch 2 times, most recently from 5ae5230 to b13bf4f Compare December 3, 2021 11:53

velioglu reviewed Dec 3, 2021

View reviewed changes

onderkalaci reviewed Dec 3, 2021

View reviewed changes

src/backend/distributed/deparser/deparse_foreign_server_stmts.c Show resolved Hide resolved

onderkalaci reviewed Dec 3, 2021

View reviewed changes

src/test/regress/expected/local_table_join.out Show resolved Hide resolved

marcocitus reviewed Dec 6, 2021

View reviewed changes

agedemenli force-pushed the propagate-foreign-server-ops branch 4 times, most recently from df10a6a to 2c1ed64 Compare December 7, 2021 15:47

agedemenli requested review from marcocitus, onderkalaci and velioglu December 7, 2021 16:02

onderkalaci reviewed Dec 8, 2021

View reviewed changes

agedemenli force-pushed the propagate-foreign-server-ops branch from 5a895ee to 0fab75e Compare December 22, 2021 10:02

marcocitus reviewed Dec 22, 2021

View reviewed changes

agedemenli force-pushed the propagate-foreign-server-ops branch from 66c8eb0 to 818579c Compare December 22, 2021 15:34

onderkalaci reviewed Dec 23, 2021

View reviewed changes

agedemenli force-pushed the propagate-foreign-server-ops branch 2 times, most recently from fc2819b to dc36a4c Compare December 23, 2021 12:09

onderkalaci approved these changes Dec 23, 2021

View reviewed changes

Propagate foreign server ops

042d45b

agedemenli force-pushed the propagate-foreign-server-ops branch from dc36a4c to 042d45b Compare December 23, 2021 14:54

agedemenli merged commit 63626d4 into master Dec 23, 2021

agedemenli deleted the propagate-foreign-server-ops branch December 23, 2021 16:04

onderkalaci mentioned this pull request Jan 4, 2022

Support foreign tables in MX #5461

Merged

Propagate foreign server ops #5468

Propagate foreign server ops #5468

Conversation

agedemenli commented Nov 16, 2021 • 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

codecov bot commented Nov 23, 2021 • edited

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

agedemenli Dec 3, 2021 • 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

onderkalaci 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

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

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 Dec 23, 2021 • edited

Choose a reason for hiding this comment

agedemenli commented Nov 16, 2021 •

edited

codecov bot commented Nov 23, 2021 •

edited

agedemenli Dec 3, 2021 •

edited

onderkalaci Dec 23, 2021 •

edited