Fix pg_extension_config_dump() to handle update cases more sanely.

tglsfdc · tglsfdc · commit bfe738d7f367 · 2012-12-20T16:31:58.000-05:00
If pg_extension_config_dump() is executed again for a table already listed
in the extension's extconfig, the code was blindly making a new array entry.
This does not seem useful.  Fix it to replace the existing array entry
instead, so that it's possible for extension update scripts to alter the
filter conditions for configuration tables.

In addition, teach ALTER EXTENSION DROP TABLE to check for an extconfig
entry for the target table, and remove it if present.  This is not a 100%
solution because it's allowed for an extension update script to just
summarily DROP a member table, and that code path doesn't go through
ExecAlterExtensionContentsStmt.  We could probably make that case clean
things up if we had to, but it would involve sticking a very ugly wart
somewhere in the guts of dependency.c.  Since on the whole it seems quite
unlikely that extension updates would want to remove pre-existing
configuration tables, making the case possible with an explicit command
seems sufficient.

Per bug #7756 from Regina Obe.  Back-patch to 9.1 where extensions were
introduced.
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
@@ -665,6 +665,10 @@ SET LOCAL search_path TO @extschema@;
      and reload.
     </para>
 
+   <indexterm>
+    <primary>pg_extension_config_dump</primary>
+   </indexterm>
+
     <para>
      To solve this problem, an extension's script file can mark a table
      it has created as a configuration table, which will cause
@@ -703,6 +707,14 @@ SELECT pg_catalog.pg_extension_config_dump('my_config', 'WHERE NOT standard_entr
      be modified by users, can be handled by creating triggers on the
      configuration table to ensure that modified rows are marked correctly.
     </para>
+
+    <para>
+     You can alter the filter condition associated with a configuration table
+     by calling <function>pg_extension_config_dump</> again.  (This would
+     typically be useful in an extension update script.)  The only way to mark
+     a table as no longer a configuration table is to dissociate it from the
+     extension with <command>ALTER EXTENSION ... DROP TABLE</>.
+    </para>
    </sect2>
 
    <sect2>
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
@@ -2047,6 +2047,7 @@ pg_extension_config_dump(PG_FUNCTION_ARGS)
 	HeapTuple	extTup;
 	Datum		arrayDatum;
 	Datum		elementDatum;
+	int			arrayLength;
 	int			arrayIndex;
 	bool		isnull;
 	Datum		repl_val[Natts_pg_extension];
@@ -2066,8 +2067,8 @@ pg_extension_config_dump(PG_FUNCTION_ARGS)
 
 	/*
 	 * Check that the table exists and is a member of the extension being
-	 * created.  This ensures that we don't need to register a dependency to
-	 * protect the extconfig entry.
+	 * created.  This ensures that we don't need to register an additional
+	 * dependency to protect the extconfig entry.
 	 */
 	tablename = get_rel_name(tableoid);
 	if (tablename == NULL)
@@ -2084,6 +2085,9 @@ pg_extension_config_dump(PG_FUNCTION_ARGS)
 	/*
 	 * Add the table OID and WHERE condition to the extension's extconfig and
 	 * extcondition arrays.
+	 *
+	 * If the table is already in extconfig, treat this as an update of the
+	 * WHERE condition.
 	 */
 
 	/* Find the pg_extension tuple */
@@ -2114,18 +2118,41 @@ pg_extension_config_dump(PG_FUNCTION_ARGS)
 							  RelationGetDescr(extRel), &isnull);
 	if (isnull)
 	{
+		/* Previously empty extconfig, so build 1-element array */
+		arrayLength = 0;
+		arrayIndex = 1;
+
 		a = construct_array(&elementDatum, 1,
 							OIDOID,
 							sizeof(Oid), true, 'i');
 	}
 	else
 	{
+		/* Modify or extend existing extconfig array */
+		Oid		   *arrayData;
+		int			i;
+
 		a = DatumGetArrayTypeP(arrayDatum);
-		Assert(ARR_ELEMTYPE(a) == OIDOID);
-		Assert(ARR_NDIM(a) == 1);
-		Assert(ARR_LBOUND(a)[0] == 1);
 
-		arrayIndex = ARR_DIMS(a)[0] + 1;		/* add after end */
+		arrayLength = ARR_DIMS(a)[0];
+		if (ARR_NDIM(a) != 1 ||
+			ARR_LBOUND(a)[0] != 1 ||
+			arrayLength < 0 ||
+			ARR_HASNULL(a) ||
+			ARR_ELEMTYPE(a) != OIDOID)
+			elog(ERROR, "extconfig is not a 1-D Oid array");
+		arrayData = (Oid *) ARR_DATA_PTR(a);
+
+		arrayIndex = arrayLength + 1;	/* set up to add after end */
+
+		for (i = 0; i < arrayLength; i++)
+		{
+			if (arrayData[i] == tableoid)
+			{
+				arrayIndex = i + 1;		/* replace this element instead */
+				break;
+			}
+		}
 
 		a = array_set(a, 1, &arrayIndex,
 					  elementDatum,
@@ -2145,19 +2172,26 @@ pg_extension_config_dump(PG_FUNCTION_ARGS)
 							  RelationGetDescr(extRel), &isnull);
 	if (isnull)
 	{
+		if (arrayLength != 0)
+			elog(ERROR, "extconfig and extcondition arrays do not match");
+
 		a = construct_array(&elementDatum, 1,
 							TEXTOID,
 							-1, false, 'i');
 	}
 	else
 	{
 		a = DatumGetArrayTypeP(arrayDatum);
-		Assert(ARR_ELEMTYPE(a) == TEXTOID);
-		Assert(ARR_NDIM(a) == 1);
-		Assert(ARR_LBOUND(a)[0] == 1);
 
-		arrayIndex = ARR_DIMS(a)[0] + 1;		/* add after end */
+		if (ARR_NDIM(a) != 1 ||
+			ARR_LBOUND(a)[0] != 1 ||
+			ARR_HASNULL(a) ||
+			ARR_ELEMTYPE(a) != TEXTOID)
+			elog(ERROR, "extcondition is not a 1-D text array");
+		if (ARR_DIMS(a)[0] != arrayLength)
+			elog(ERROR, "extconfig and extcondition arrays do not match");
 
+		/* Add or replace at same index as in extconfig */
 		a = array_set(a, 1, &arrayIndex,
 					  elementDatum,
 					  false,
@@ -2182,6 +2216,182 @@ pg_extension_config_dump(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
 
+/*
+ * extension_config_remove
+ *
+ * Remove the specified table OID from extension's extconfig, if present.
+ * This is not currently exposed as a function, but it could be;
+ * for now, we just invoke it from ALTER EXTENSION DROP.
+ */
+static void
+extension_config_remove(Oid extensionoid, Oid tableoid)
+{
+	Relation	extRel;
+	ScanKeyData key[1];
+	SysScanDesc extScan;
+	HeapTuple	extTup;
+	Datum		arrayDatum;
+	int			arrayLength;
+	int			arrayIndex;
+	bool		isnull;
+	Datum		repl_val[Natts_pg_extension];
+	bool		repl_null[Natts_pg_extension];
+	bool		repl_repl[Natts_pg_extension];
+	ArrayType  *a;
+
+	/* Find the pg_extension tuple */
+	extRel = heap_open(ExtensionRelationId, RowExclusiveLock);
+
+	ScanKeyInit(&key[0],
+				ObjectIdAttributeNumber,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(extensionoid));
+
+	extScan = systable_beginscan(extRel, ExtensionOidIndexId, true,
+								 SnapshotNow, 1, key);
+
+	extTup = systable_getnext(extScan);
+
+	if (!HeapTupleIsValid(extTup))		/* should not happen */
+		elog(ERROR, "extension with oid %u does not exist",
+			 extensionoid);
+
+	/* Search extconfig for the tableoid */
+	arrayDatum = heap_getattr(extTup, Anum_pg_extension_extconfig,
+							  RelationGetDescr(extRel), &isnull);
+	if (isnull)
+	{
+		/* nothing to do */
+		a = NULL;
+		arrayLength = 0;
+		arrayIndex = -1;
+	}
+	else
+	{
+		Oid		   *arrayData;
+		int			i;
+
+		a = DatumGetArrayTypeP(arrayDatum);
+
+		arrayLength = ARR_DIMS(a)[0];
+		if (ARR_NDIM(a) != 1 ||
+			ARR_LBOUND(a)[0] != 1 ||
+			arrayLength < 0 ||
+			ARR_HASNULL(a) ||
+			ARR_ELEMTYPE(a) != OIDOID)
+			elog(ERROR, "extconfig is not a 1-D Oid array");
+		arrayData = (Oid *) ARR_DATA_PTR(a);
+
+		arrayIndex = -1;		/* flag for no deletion needed */
+
+		for (i = 0; i < arrayLength; i++)
+		{
+			if (arrayData[i] == tableoid)
+			{
+				arrayIndex = i; /* index to remove */
+				break;
+			}
+		}
+	}
+
+	/* If tableoid is not in extconfig, nothing to do */
+	if (arrayIndex < 0)
+	{
+		systable_endscan(extScan);
+		heap_close(extRel, RowExclusiveLock);
+		return;
+	}
+
+	/* Modify or delete the extconfig value */
+	memset(repl_val, 0, sizeof(repl_val));
+	memset(repl_null, false, sizeof(repl_null));
+	memset(repl_repl, false, sizeof(repl_repl));
+
+	if (arrayLength <= 1)
+	{
+		/* removing only element, just set array to null */
+		repl_null[Anum_pg_extension_extconfig - 1] = true;
+	}
+	else
+	{
+		/* squeeze out the target element */
+		Datum	   *dvalues;
+		bool	   *dnulls;
+		int			nelems;
+		int			i;
+
+		deconstruct_array(a, OIDOID, sizeof(Oid), true, 'i',
+						  &dvalues, &dnulls, &nelems);
+
+		/* We already checked there are no nulls, so ignore dnulls */
+		for (i = arrayIndex; i < arrayLength - 1; i++)
+			dvalues[i] = dvalues[i + 1];
+
+		a = construct_array(dvalues, arrayLength - 1,
+							OIDOID, sizeof(Oid), true, 'i');
+
+		repl_val[Anum_pg_extension_extconfig - 1] = PointerGetDatum(a);
+	}
+	repl_repl[Anum_pg_extension_extconfig - 1] = true;
+
+	/* Modify or delete the extcondition value */
+	arrayDatum = heap_getattr(extTup, Anum_pg_extension_extcondition,
+							  RelationGetDescr(extRel), &isnull);
+	if (isnull)
+	{
+		elog(ERROR, "extconfig and extcondition arrays do not match");
+	}
+	else
+	{
+		a = DatumGetArrayTypeP(arrayDatum);
+
+		if (ARR_NDIM(a) != 1 ||
+			ARR_LBOUND(a)[0] != 1 ||
+			ARR_HASNULL(a) ||
+			ARR_ELEMTYPE(a) != TEXTOID)
+			elog(ERROR, "extcondition is not a 1-D text array");
+		if (ARR_DIMS(a)[0] != arrayLength)
+			elog(ERROR, "extconfig and extcondition arrays do not match");
+	}
+
+	if (arrayLength <= 1)
+	{
+		/* removing only element, just set array to null */
+		repl_null[Anum_pg_extension_extcondition - 1] = true;
+	}
+	else
+	{
+		/* squeeze out the target element */
+		Datum	   *dvalues;
+		bool	   *dnulls;
+		int			nelems;
+		int			i;
+
+		deconstruct_array(a, TEXTOID, -1, false, 'i',
+						  &dvalues, &dnulls, &nelems);
+
+		/* We already checked there are no nulls, so ignore dnulls */
+		for (i = arrayIndex; i < arrayLength - 1; i++)
+			dvalues[i] = dvalues[i + 1];
+
+		a = construct_array(dvalues, arrayLength - 1,
+							TEXTOID, -1, false, 'i');
+
+		repl_val[Anum_pg_extension_extcondition - 1] = PointerGetDatum(a);
+	}
+	repl_repl[Anum_pg_extension_extcondition - 1] = true;
+
+	extTup = heap_modify_tuple(extTup, RelationGetDescr(extRel),
+							   repl_val, repl_null, repl_repl);
+
+	simple_heap_update(extRel, &extTup->t_self, extTup);
+	CatalogUpdateIndexes(extRel, extTup);
+
+	systable_endscan(extScan);
+
+	heap_close(extRel, RowExclusiveLock);
+}
+
 /*
  * Execute ALTER EXTENSION SET SCHEMA
  */
@@ -2742,6 +2952,13 @@ ExecAlterExtensionContentsStmt(AlterExtensionContentsStmt *stmt)
 											ExtensionRelationId,
 											DEPENDENCY_EXTENSION) != 1)
 			elog(ERROR, "unexpected number of extension dependency records");
+
+		/*
+		 * If it's a relation, it might have an entry in the extension's
+		 * extconfig array, which we must remove.
+		 */
+		if (object.classId == RelationRelationId)
+			extension_config_remove(extension.objectId, object.objectId);
 	}
 
 	/*
