Propagate foreign server ops by agedemenli · Pull Request #5468 · citusdata/citus

Ahmet Gedemenli (agedemenli) · 2021-11-16T17:45:10Z

DESCRIPTION: Adds propagation for foreign server commands

Marco Slot (marcocitus) · 2021-11-18T15:58:03Z


+		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.

Marco Slot (marcocitus) · 2021-11-22T12:46:53Z

+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

Burak Velioglu (velioglu) · 2021-11-30T09:56:48Z

+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

Burak Velioglu (velioglu) · 2021-12-03T12:24:58Z

+/*
+ * 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

Burak Velioglu (velioglu) · 2021-12-03T12:33:29Z

 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

Önder Kalacı (onderkalaci) · 2021-12-03T13:50:14Z

+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

Önder Kalacı (onderkalaci) · 2021-12-03T14:05:07Z

+-- 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?

Önder Kalacı (onderkalaci) · 2021-12-03T14:06:58Z

+	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

Önder Kalacı (onderkalaci) · 2021-12-03T14:09:52Z

+{
+	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!

Önder Kalacı (onderkalaci) · 2021-12-03T14:11:50Z

 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

Marco Slot (marcocitus) · 2021-12-06T15:16:15Z

+		{
+			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

Önder Kalacı (onderkalaci)

I'll continue reviewing & testing

Önder Kalacı (onderkalaci) · 2021-12-08T09:06:55Z

+		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.

Önder Kalacı (onderkalaci) · 2021-12-08T09:13:06Z

+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*

Önder Kalacı (onderkalaci) · 2021-12-08T09:16:47Z

+	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

Önder Kalacı (onderkalaci) · 2021-12-08T09:52:47Z

+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

Önder Kalacı (onderkalaci) · 2021-12-08T10:01:11Z

+	}
+
+	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.. ?

Önder Kalacı (onderkalaci) · 2021-12-08T10:04:02Z

+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)?

Marco Slot (marcocitus) · 2021-12-22T10:44:45Z

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


you might be able to use AppendOptionListToString here

Önder Kalacı (onderkalaci)

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

Önder Kalacı (onderkalaci) · 2021-12-23T08:29:17Z

+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

Önder Kalacı (onderkalaci) · 2021-12-23T08:33:37Z

+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)?

Önder Kalacı (onderkalaci) · 2021-12-23T09:25:34Z

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


can we use GetObjectAddressByServerName?

Önder Kalacı (onderkalaci) · 2021-12-23T09:26:07Z

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


can we use GetObjectAddressByServerName?

Önder Kalacı (onderkalaci) · 2021-12-23T09:30:21Z

+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.

Önder Kalacı (onderkalaci) · 2021-12-23T14:24:46Z

+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

Önder Kalacı (onderkalaci) · 2021-12-23T14:27:22Z

+         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.

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

Marco Slot (marcocitus) reviewed Nov 18, 2021

View reviewed changes

Comment thread src/backend/distributed/commands/foreign_server.c

Marco Slot (marcocitus) reviewed Nov 22, 2021

View reviewed changes

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

Burak Velioglu (velioglu) reviewed Nov 30, 2021

View reviewed changes

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

Önder Kalacı (onderkalaci) mentioned this pull request Dec 3, 2021

Fix fails with metadata syncing in undistribute_table #5503

Merged

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

Burak Velioglu (velioglu) reviewed Dec 3, 2021

View reviewed changes

Önder Kalacı (onderkalaci) reviewed Dec 3, 2021

View reviewed changes

Comment thread src/backend/distributed/deparser/deparse_foreign_server_stmts.c

Önder Kalacı (onderkalaci) reviewed Dec 3, 2021

View reviewed changes

Comment thread src/test/regress/expected/local_table_join.out

Marco Slot (marcocitus) reviewed Dec 6, 2021

View reviewed changes

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

Ahmet Gedemenli (agedemenli) requested review from Marco Slot (marcocitus), Önder Kalacı (onderkalaci) and Burak Velioglu (velioglu) December 7, 2021 16:02

Önder Kalacı (onderkalaci) reviewed Dec 8, 2021

View reviewed changes

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

Marco Slot (marcocitus) reviewed Dec 22, 2021

View reviewed changes

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

Önder Kalacı (onderkalaci) reviewed Dec 23, 2021

View reviewed changes

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

Önder Kalacı (onderkalaci) approved these changes Dec 23, 2021

View reviewed changes

Propagate foreign server ops

042d45b

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

Ahmet Gedemenli (agedemenli) merged commit 63626d4 into master Dec 23, 2021

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

Önder Kalacı (onderkalaci) mentioned this pull request Jan 4, 2022

Support foreign tables in MX #5461

Merged

Uh oh!

Conversation

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Ahmet Gedemenli (agedemenli) Dec 3, 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!

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Önder Kalacı (onderkalaci) left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ahmet Gedemenli (agedemenli) commented Nov 16, 2021 •

edited

Loading

codecov Bot commented Nov 23, 2021 •

edited

Loading

Ahmet Gedemenli (agedemenli) Dec 3, 2021 •

edited

Loading