From 44859c0b876d5aac105dc660eaf27b55f55a3ef2 Mon Sep 17 00:00:00 2001
From: Vik Fearing <vik.fearing@protonmail.com>
Date: Fri, 13 Sep 2019 21:22:39 +0200
Subject: [PATCH] Fix a few bugs in excluded columns

Base on peer review by Andrew Gierth of commit 47353c1.
---
 periods.c | 47 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/periods.c b/periods.c
index 97f8aca..0a359ab 100644
--- a/periods.c
+++ b/periods.c
@@ -128,7 +128,8 @@ OnlyExcludedColumnsChanged(Relation rel, HeapTuple old_row, HeapTuple new_row)
 	Oid				types[1];
 	Datum			values[1];
 	TupleDesc		tupdesc = RelationGetDescr(rel);
-	Bitmapset	   *excluded_attnums;
+	Bitmapset	   *excluded_attnums = NULL;
+	MemoryContext	mcxt = CurrentMemoryContext; /* The context outside of SPI */
 
 	const char *sql =
 		"SELECT u.name "
@@ -136,10 +137,6 @@ OnlyExcludedColumnsChanged(Relation rel, HeapTuple old_row, HeapTuple new_row)
 		"CROSS JOIN unnest(stp.excluded_column_names) AS u (name) "
 		"WHERE stp.table_name = $1";
 
-	/* Create an empty bitmapset outside of the SPI context */
-	excluded_attnums = bms_make_singleton(0);
-	excluded_attnums = bms_del_member(excluded_attnums, 0);
-
 	if (SPI_connect() != SPI_OK_CONNECT)
 		elog(ERROR, "SPI_connect failed");
 
@@ -156,20 +153,44 @@ OnlyExcludedColumnsChanged(Relation rel, HeapTuple old_row, HeapTuple new_row)
 	if (SPI_processed > 0 && SPI_tuptable != NULL)
 	{
 		TupleDesc	spitupdesc = SPI_tuptable->tupdesc;
+		bool		isnull;
 
 		for (int i = 0; i < SPI_processed; i++)
 		{
 			HeapTuple	tuple = SPI_tuptable->vals[i];
+			Datum		attdatum;
 			char	   *attname;
 			int16		attnum;
 
 			/* Get the attnum from the column name */
-			attname = SPI_getvalue(tuple, spitupdesc, 1);
+			attdatum = SPI_getbinval(tuple, spitupdesc, 1, &isnull);
+			attname = NameStr(*(DatumGetName(attdatum)));
 			attnum = SPI_fnumber(tupdesc, attname);
-			pfree(attname);
 
+			/* Make sure it's valid (should always be) */
+			if (attnum == SPI_ERROR_NOATTRIBUTE)
+				ereport(ERROR,
+						(errcode(ERRCODE_UNDEFINED_COLUMN),
+						 errmsg("column \"%s\" does not exist", attname)));
+
+			/* Just ignore system columns (should never happen) */
+			if (attnum < 0)
+				continue;
+
+			/* Add it to the bitmap set */
 			excluded_attnums = bms_add_member(excluded_attnums, attnum);
 		}
+
+		/*
+		 * If we have excluded columns, move the bitmapset out of the SPI
+		 * context.
+		 */
+		if (excluded_attnums != NULL)
+		{
+			MemoryContext spicontext = MemoryContextSwitchTo(mcxt);
+			excluded_attnums = bms_copy(excluded_attnums);
+			MemoryContextSwitchTo(spicontext);
+		}
 	}
 
 	/* Don't need SPI anymore */
@@ -177,24 +198,24 @@ OnlyExcludedColumnsChanged(Relation rel, HeapTuple old_row, HeapTuple new_row)
 		elog(ERROR, "SPI_finish failed");
 
 	/* If there are no excluded columns, then we're done */
-	if (bms_is_empty(excluded_attnums))
+	if (excluded_attnums == NULL)
 		return false;
 
 	for (int i = 1; i <= tupdesc->natts; i++)
 	{
 		Datum	old_datum, new_datum;
 		bool	old_isnull, new_isnull;
-		Oid		typid;
 		int16	typlen;
 		bool	typbyval;
 
+		/* Ignore if deleted column */
+		if (TupleDescAttr(tupdesc, i-1)->attisdropped)
+			continue;
+
 		/* Ignore if excluded column */
 		if (bms_is_member(i, excluded_attnums))
 			continue;
 
-		typid = SPI_gettypeid(tupdesc, i);
-		get_typlenbyval(typid, &typlen, &typbyval);
-
 		old_datum = SPI_getbinval(old_row, tupdesc, i, &old_isnull);
 		new_datum = SPI_getbinval(new_row, tupdesc, i, &new_isnull);
 
@@ -210,6 +231,8 @@ OnlyExcludedColumnsChanged(Relation rel, HeapTuple old_row, HeapTuple new_row)
 			continue;
 
 		/* Do a fairly strict binary comparison of the values */
+		typlen = TupleDescAttr(tupdesc, i-1)->attlen;
+		typbyval = TupleDescAttr(tupdesc, i-1)->attbyval;
 		if (!datumIsEqual(old_datum, new_datum, typbyval, typlen))
 			return false;
 	}