Emit warning that an injector of a panic should consider disabling FTS.

- Adds hint to user on how to disable FTS.

- Registers warnings outside of the core injection framework

  This allows us to create warnings that are GPDB specific, for
  things like FTS without tainting the core framework which is
  theoretically Postgres-only dependent.

gpcontrib/gp_inject_fault/gp_inject_fault.c

@@ -11,6 +11,7 @@
 #include "libpq/ip.h"
 #include "libpq-fe.h"
 #include "postmaster/postmaster.h"
+#include "postmaster/fts.h"
 #include "utils/builtins.h"
 #include "utils/faultinjector.h"
 #include "utils/fmgroids.h"
@@ -20,6 +21,32 @@ PG_MODULE_MAGIC;
 
 extern Datum gp_inject_fault(PG_FUNCTION_ARGS);
 extern Datum gp_inject_fault2(PG_FUNCTION_ARGS);
+void _PG_init(void);
+
+static void
+fts_with_panic_warning(FaultInjectorEntry_s faultEntry)
+{
+	if (Gp_role == GP_ROLE_DISPATCH && !FtsIsActive()) return;
+
+	if (faultEntry.faultInjectorType == FaultInjectorTypePanic)
+		ereport(WARNING, (
+			errmsg("consider disabling FTS probes while injecting a panic."),
+				errhint("Inject an infinite 'skip' into the 'fts_probe' fault to disable FTS probing.")));
+}
+
+/*
+ * Register warning when extension is loaded.
+ *
+ */
+void
+_PG_init(void)
+{
+	InjectFaultInit();
+
+	MemoryContext oldContext = MemoryContextSwitchTo(TopMemoryContext);
+	register_fault_injection_warning(fts_with_panic_warning);
+	MemoryContextSwitchTo(oldContext);
+}
 
 PG_FUNCTION_INFO_V1(gp_inject_fault);
 Datum
@@ -105,7 +132,6 @@ gp_inject_fault2(PG_FUNCTION_ARGS)
 	int		port = PG_GETARG_INT32(10);
 	char	*response;
 
-
 	/* Fast path if injecting fault in our postmaster. */
 	if (GpIdentity.dbid == dbid)
 	{

src/backend/mock.mk

@@ -10,6 +10,7 @@ include $(top_srcdir)/src/Makefile.mock
 override CPPFLAGS+= -I$(top_srcdir)/src/backend/libpq \
 					-I$(libpq_srcdir) \
 					-I$(top_srcdir)/src/backend/postmaster \
+					-I$(top_srcdir)/src/test/unit/mock/ \
 					-I. -I$(top_builddir)/src/port \
 					-DDLSUFFIX=$(DLSUFFIX) \
 					-I$(top_srcdir)/src/backend/utils/stat

src/backend/utils/misc/Makefile

@@ -19,7 +19,7 @@ override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
 
 OBJS = guc.o help_config.o pg_rusage.o ps_status.o rbtree.o \
        gpexpand.o \
-       faultinjector.o uriparser.o \
+       faultinjector.o faultinjector_warnings.o uriparser.o \
        bitstream.o bitmap_compression.o guc_gp.o backend_cancel.o \
        superuser.o timeout.o tzparser.o
 

src/backend/utils/misc/faultinjector.c

@@ -31,13 +31,17 @@
 #include "libpq/pqformat.h"
 #include "postmaster/autovacuum.h"
 #include "postmaster/bgwriter.h"
-#include "postmaster/fts.h"
 #include "storage/spin.h"
 #include "storage/shmem.h"
 #include "utils/faultinjector.h"
 #include "utils/hsearch.h"
 #include "miscadmin.h"
 
+/*
+ * internal include
+ */
+#include "faultinjector_warnings.h"
+
 #ifdef FAULT_INJECTOR
 
 /*
@@ -153,6 +157,13 @@ FiLockRelease(void)
 	SpinLockRelease(&faultInjectorShmem->lock);
 }
 
+
+void InjectFaultInit(void)
+{
+	warnings_init();
+}
+
+
 /****************************************************************
  * FAULT INJECTOR routines
  ****************************************************************/
@@ -967,6 +978,16 @@ InjectFault(char *faultName, char *type, char *ddlStatement, char *databaseName,
 		 faultName, type, ddlStatement, databaseName, tableName,
 		 startOccurrence, endOccurrence, extraArg );
 
+	faultEntry.faultInjectorType = FaultInjectorTypeStringToEnum(type);
+	faultEntry.ddlStatement = FaultInjectorDDLStringToEnum(ddlStatement);
+	faultEntry.extraArg = extraArg;
+	faultEntry.startOccurrence = startOccurrence;
+	faultEntry.endOccurrence = endOccurrence;
+
+	/*
+	 * Validations:
+	 *
+	 */
 	if (strlcpy(faultEntry.faultName, faultName, sizeof(faultEntry.faultName)) >=
 		sizeof(faultEntry.faultName))
 		ereport(ERROR,
@@ -975,7 +996,6 @@ InjectFault(char *faultName, char *type, char *ddlStatement, char *databaseName,
 				 errdetail("Fault name should be no more than %d characters.",
 						   FAULT_NAME_MAX_LENGTH-1)));
 
-	faultEntry.faultInjectorType = FaultInjectorTypeStringToEnum(type);
 	if (faultEntry.faultInjectorType == FaultInjectorTypeMax)
 		ereport(ERROR,
 				(errcode(ERRCODE_PROTOCOL_VIOLATION),
@@ -988,7 +1008,6 @@ InjectFault(char *faultName, char *type, char *ddlStatement, char *databaseName,
 				(errcode(ERRCODE_PROTOCOL_VIOLATION),
 				 errmsg("invalid fault name '%s'", faultName)));
 
-	faultEntry.extraArg = extraArg;
 	if (faultEntry.faultInjectorType == FaultInjectorTypeSleep)
 	{
 		if (extraArg < 0 || extraArg > 7200)
@@ -997,7 +1016,6 @@ InjectFault(char *faultName, char *type, char *ddlStatement, char *databaseName,
 					 errmsg("invalid sleep time, allowed range [0, 7200 sec]")));
 	}
 
-	faultEntry.ddlStatement = FaultInjectorDDLStringToEnum(ddlStatement);
 	if (faultEntry.ddlStatement == DDLMax)
 		ereport(ERROR,
 				(errcode(ERRCODE_PROTOCOL_VIOLATION),
@@ -1031,8 +1049,13 @@ InjectFault(char *faultName, char *type, char *ddlStatement, char *databaseName,
 				(errcode(ERRCODE_PROTOCOL_VIOLATION),
 				 errmsg("invalid end occurrence number, allowed range [startOccurrence, ] or -1")));
 
-	faultEntry.startOccurrence = startOccurrence;
-	faultEntry.endOccurrence = endOccurrence;
+
+	/*
+	 * Warnings:
+	 *
+	 */
+	emit_warnings(faultEntry);
+
 
 	if (FaultInjector_SetFaultInjection(&faultEntry) == STATUS_OK)
 	{

src/backend/utils/misc/faultinjector_warnings.c

+/*
+ * faultinjector_warnings.c
+ *
+ * Plugin system for collecting warning functions and processing warnings
+ * at a later time.
+ *
+ * Portions Copyright (c) 2019-Present Pivotal Software, Inc.
+ *
+ * IDENTIFICATION
+ *	    src/backend/utils/misc/faultinjector_warnings.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+#include "nodes/pg_list.h"
+
+
+/*
+ * Implements
+ */
+#include "utils/faultinjector.h"
+#include "faultinjector_warnings.h"
+
+
+static List *warnings_list;
+
+
+void
+warnings_init(void)
+{
+	warnings_list = NULL;
+}
+
+
+/*
+ * Provide a function that inspects an entry and warns the user
+ * of an problematic configuration.
+ *
+ * void some_warning_function(FaultInjectorEntry_s faultEntry)
+ * {
+ *     if (isProblematic(faultEntry))
+ *         elog(WARNING, "this fault injection configuration might have problems");
+ * }
+ *
+ * add_fault_injection_warning(some_warning_function);
+ */
+void
+register_fault_injection_warning(fault_injection_warning_function warning)
+{
+	warnings_list = lappend(warnings_list, warning);
+}
+
+
+/*
+ * Process all configured warnings for a given faultEntry.
+ *
+ */
+void
+emit_warnings(FaultInjectorEntry_s faultEntry)
+{
+	ListCell *list_cell = NULL;
+
+	foreach(list_cell, warnings_list)
+	{
+		fault_injection_warning_function warning_function =
+			(fault_injection_warning_function) lfirst(list_cell);
+		warning_function(faultEntry);
+	}
+}

src/backend/utils/misc/faultinjector_warnings.h

+/*
+ * faultinjector_warnings.h
+ *
+ * Functions for internal usage by the fault injector framework
+ *
+ * Plugin system for collecting warning functions and processing warnings
+ * at a later time.
+ *
+ * Portions Copyright (c) 2019-Present Pivotal Software, Inc.
+ *
+ * IDENTIFICATION
+ *	    src/backend/utils/misc/faultinjector_warnings.h
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef FAULTINJECTOR_WARNINGS_H
+#define FAULTINJECTOR_WARNINGS_H
+
+#include "postgres.h"
+#include "nodes/pg_list.h"
+#include "utils/faultinjector.h"
+
+extern void warnings_init(void);
+extern void emit_warnings(FaultInjectorEntry_s faultEntry);
+
+#endif // FAULTINJECTOR_WARNINGS_H

src/backend/utils/misc/test/Makefile

@@ -2,6 +2,9 @@ subdir=src/backend/utils/misc
 top_builddir=../../../../..
 include $(top_builddir)/src/Makefile.global
 
-TARGETS = ps_status bitstream bitmap_compression
+TARGETS = ps_status bitstream bitmap_compression faultinjector_warnings
+
+faultinjector_warnings.t: ../faultinjector_warnings.o faultinjector_warnings_test.o
+
 
 include $(top_builddir)/src/backend/mock.mk

src/backend/utils/misc/test/faultinjector_warnings_test.c

+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include "cmockery.h"
+
+
+#include "postgres.h"
+#include "nodes/pg_list.h"
+#include "utils/memutils.h"
+
+#include "utils/faultinjector.h"
+#include "../faultinjector_warnings.h"
+
+
+static bool warning_was_called = false;
+
+
+void some_warning(FaultInjectorEntry_s faultEntry)
+{
+	warning_was_called = true;
+}
+
+
+void setup()
+{
+	warning_was_called = false;
+
+	warnings_init();
+}
+
+
+
+void test_fault_injection_warnings_can_be_added_and_emitted(void **state)
+{
+	setup();
+
+	FaultInjectorEntry_s faultEntry;
+	faultEntry.startOccurrence = 0;
+
+	register_fault_injection_warning(some_warning);
+	emit_warnings(faultEntry);
+
+	assert_true(warning_was_called);
+}
+
+
+void test_fault_injection_warnings_not_called_when_empty(void **state)
+{
+	setup();
+
+	FaultInjectorEntry_s faultEntry;
+	faultEntry.startOccurrence = 0;
+
+	emit_warnings(faultEntry);
+
+	assert_false(warning_was_called);
+}
+
+
+int main(int argc, char* argv[]) {
+	cmockery_parse_arguments(argc, argv);
+
+	const UnitTest tests[] = {
+		unit_test(test_fault_injection_warnings_can_be_added_and_emitted),
+		unit_test(test_fault_injection_warnings_not_called_when_empty)
+	};
+
+	MemoryContextInit();
+
+	return run_tests(tests);
+}

src/include/utils/faultinjector.h

@@ -79,6 +79,7 @@ typedef struct FaultInjectorEntry_s {
 	
 } FaultInjectorEntry_s;
 
+extern void InjectFaultInit(void);
 
 extern Size FaultInjector_ShmemSize(void);
 
@@ -96,6 +97,9 @@ extern char *InjectFault(
 
 extern void HandleFaultMessage(const char* msg);
 
+typedef void (*fault_injection_warning_function)(FaultInjectorEntry_s faultEntry);
+void register_fault_injection_warning(fault_injection_warning_function warning);
+
 #ifdef FAULT_INJECTOR
 extern bool am_faulthandler;
 #define IsFaultHandler am_faulthandler

src/test/regress/expected/bitmap_index.out

@@ -627,6 +627,16 @@ drop table unlogged_test;
 --
 -- Test crash recovery
 --
+--
+-- disable fault-tolerance service (FTS) probing to ensure
+-- the mirror does not accidentally get promoted
+--
+SELECT gp_inject_fault2('fts_probe', 'skip', dbid, hostname, port) FROM gp_segment_configuration WHERE role = 'p' and content = -1;
+ gp_inject_fault2 
+------------------
+ Success:
+(1 row)
+
 CREATE TABLE bm_test_insert(a int) DISTRIBUTED BY (a);
 CREATE INDEX bm_a_idx ON bm_test_insert USING bitmap(a);
 CREATE TABLE bm_test_update(a int, b int) DISTRIBUTED BY (a);
@@ -650,9 +660,15 @@ INSERT INTO bm_test_insert SELECT generate_series (1, 10000);
 UPDATE bm_test_update SET b=b+1;
 -- trigger recovery on primaries 
 SELECT gp_inject_fault_infinite('finish_prepared_after_record_commit_prepared', 'panic', dbid) FROM gp_segment_configuration WHERE role = 'p' AND content > -1;
-NOTICE:  Success:  (seg0 172.17.0.10:25435 pid=355751)
-NOTICE:  Success:  (seg1 172.17.0.10:25433 pid=355752)
-NOTICE:  Success:  (seg2 172.17.0.10:25434 pid=355753)
+WARNING:  consider disabling FTS probes while injecting a panic.  (seg0 127.0.0.1:7002 pid=27680)
+HINT:  Inject a 'skip' into the 'fts_probe' fault to disable FTS probing.
+NOTICE:  Success:  (seg0 127.0.0.1:7002 pid=27680)
+WARNING:  consider disabling FTS probes while injecting a panic.  (seg1 127.0.0.1:7003 pid=27681)
+HINT:  Inject a 'skip' into the 'fts_probe' fault to disable FTS probing.
+NOTICE:  Success:  (seg1 127.0.0.1:7003 pid=27681)
+WARNING:  consider disabling FTS probes while injecting a panic.  (seg2 127.0.0.1:7004 pid=27682)
+HINT:  Inject a 'skip' into the 'fts_probe' fault to disable FTS probing.
+NOTICE:  Success:  (seg2 127.0.0.1:7004 pid=27682)
  gp_inject_fault_infinite 
 --------------------------
  t
@@ -703,6 +719,15 @@ SELECT * FROM bm_test_update WHERE b=1;
 DROP TABLE trigger_recovery_on_primaries;
 DROP TABLE bm_test_insert;
 DROP TABLE bm_test_update;
+--
+-- re-enable fault-tolerance service (FTS) probing after recovery completed.
+--
+SELECT gp_inject_fault2('fts_probe', 'reset', dbid, hostname, port) FROM gp_segment_configuration WHERE role = 'p' and content = -1;
+ gp_inject_fault2 
+------------------
+ Success:
+(1 row)
+
 -- If the table is AO table, it need generate some fake tuple pointer,
 -- this pointer is a little different from the heap tables pointer,
 -- If the Offset in pointer is 0(If the row number is 32768, the Offset

src/test/regress/sql/bitmap_index.sql

@@ -266,6 +266,12 @@ drop table unlogged_test;
 --
 -- Test crash recovery
 --
+
+--
+-- disable fault-tolerance service (FTS) probing to ensure
+-- the mirror does not accidentally get promoted
+--
+SELECT gp_inject_fault2('fts_probe', 'skip', dbid, hostname, port) FROM gp_segment_configuration WHERE role = 'p' and content = -1;
 CREATE TABLE bm_test_insert(a int) DISTRIBUTED BY (a);
 CREATE INDEX bm_a_idx ON bm_test_insert USING bitmap(a);
 CREATE TABLE bm_test_update(a int, b int) DISTRIBUTED BY (a);
@@ -294,6 +300,12 @@ DROP TABLE trigger_recovery_on_primaries;
 DROP TABLE bm_test_insert;
 DROP TABLE bm_test_update;
 
+--
+-- re-enable fault-tolerance service (FTS) probing after recovery completed.
+--
+SELECT gp_inject_fault2('fts_probe', 'reset', dbid, hostname, port) FROM gp_segment_configuration WHERE role = 'p' and content = -1;
+
+
 -- If the table is AO table, it need generate some fake tuple pointer,
 -- this pointer is a little different from the heap tables pointer,
 -- If the Offset in pointer is 0(If the row number is 32768, the Offset

src/test/unit/mock/faultinjector_warnings.h

+#ifndef FAULTINJECTOR_WARNINGS_MOCK_H
+#define FAULTINJECTOR_WARNINGS_MOCK_H
+
+/*
+ * Dummy implementation of src/backend/utils/misc/faultinjector_warnings.h
+ */
+
+#endif  // FAULTINJECTOR_WARNINGS_MOCK_H
+