Don't set no_3pc for syncpoint table modification.

It is much more consistent to simply prevent 3pc when only local tables were
modified in the transaction. I've kept the GUC though because it might be useful
for testing/debugging/manual fixing in other circumstances.

(cherry picked from commit 828495ef92b8ed53f288fdb1f6a8b299dce4be65)

Author: arssher

multimaster--1.0.sql

@@ -52,13 +52,6 @@ CREATE TRIGGER on_node_drop
 CREATE FUNCTION mtm.after_node_drop_plpgsql() RETURNS TRIGGER AS $$
 BEGIN
   DELETE FROM mtm.syncpoints WHERE node_id = OLD.id;
-  /*
-   * Trigger on sp table modification will set no_3pc, but in case of node drop
-   * we still want to go through distributed commit. mtm.local_tables ensures
-   * sp table changes won't be streamed though as they are not consistent
-   * among the nodes.
-   */
-  SET multimaster.no_3pc TO FALSE;
   RETURN NULL; -- result is ignored since this is an AFTER trigger
 END
 $$ LANGUAGE plpgsql;
@@ -226,24 +219,11 @@ $$
 LANGUAGE sql;
 
 /*
- * Don't perform 3pc on syncpoint tables updates.
- */
-CREATE FUNCTION mtm.syncpoint_dml() RETURNS TRIGGER AS $$
-BEGIN
-  SET LOCAL multimaster.no_3pc TO TRUE;
-  RETURN NULL; -- result is ignored since this is an AFTER trigger
-END
-$$ LANGUAGE plpgsql;
-
-CREATE TRIGGER on_syncpoint_dml
-    AFTER INSERT OR DELETE OR UPDATE ON mtm.syncpoints
-    FOR EACH ROW EXECUTE FUNCTION mtm.syncpoint_dml();
-
-/*
- * Note that pg_filter_decode_txn currently prevents plain COMMITs replay, so
- * this automatically ensures manual modifications won't be streamed. However, we
- * cleanup node syncpoints on its removal, and drop_node surely ought to go
- * through 3pc -- and there we do need to filter out these changes.
+ * Note that pg_filter_decode_txn currently prevents plain COMMITs replay and if
+ * xact modified only local tables we don't perform 3pc, so there is no empty
+ * sp table modification xacts flowing around. However, we cleanup node
+ * syncpoints on its removal, and drop_node surely ought to go through 3pc.
+ * Thus local_tables semantics does here exactly what we want.
  */
 INSERT INTO mtm.local_tables VALUES ('mtm', 'syncpoints');
 

src/commit.c

@@ -387,10 +387,12 @@ MtmTwoPhaseCommit(void)
 	nodemask_t	pc_success_cohort;
 	MtmGeneration xact_gen;
 
-	/* don't run 3pc on syncpoint table update */
 	if (MtmNo3PC)
 	{
-		/* SET LOCAL semantics ensures GUC value is reset to false on xact commit */
+		/*
+		 * SET LOCAL which ensures GUC value is reset on xact commit is
+		 * strongly recommended for this (internal) variable manipulations.
+		 */
 		return false;
 	}
 

src/ddl.c

@@ -1236,7 +1236,10 @@ MtmExecutorFinish(QueryDesc *queryDesc)
 			{
 				Relation	rel = estate->es_result_relations[i].ri_RelationDesc;
 
-				if (RelationNeedsWAL(rel))
+				/*
+				 * Don't run 3pc unless we modified at least one non-local table.
+				 */
+				if (RelationNeedsWAL(rel) && !MtmIsRelationLocal(rel))
 				{
 					if (MtmIgnoreTablesWithoutPk)
 					{

src/multimaster.c

@@ -551,12 +551,13 @@ NULL,
 NULL);
 
 	DefineCustomBoolVariable("multimaster.no_3pc",
-							 "Don't perform 3pc for current xact. For internal usage only.",
+							 "Don't perform 3pc for current xact. Mostly for internal usage, you should really know what you are doing.",
 							 NULL,
 							 &MtmNo3PC,
 							 false,
 							 PGC_USERSET,
-							 0,
+							 /* construct a few obstacles preventing broad usage */
+							 GUC_NO_SHOW_ALL | GUC_DISALLOW_IN_FILE,
 							 NULL,
 							 NULL,
 							 NULL