Subject: Re: BUG #17056: Segmentation fault on altering the type of the
 foreign table column with a default
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, exclusion@gmail.com,
 pgsql-bugs@lists.postgresql.org
References: <17056-4f6f7243d322be6b@postgresql.org>
 <1806408.1623363029@sss.pgh.pa.us>
 <4726f847-68fb-a7fc-c5e9-7b9536046fc4@dunslane.net>
 <1816295.1623366663@sss.pgh.pa.us>
 <cad1e46d-260b-2794-299b-1c6112f6c389@dunslane.net>
 <20210612215003.ypu5uf2bfceofkkd@alap3.anarazel.de>
From: Andrew Dunstan <andrew@dunslane.net>
Message-ID: <759e997e-e1ca-91cd-84db-f4ae963fada1@dunslane.net>
Date: Sat, 12 Jun 2021 21:59:24 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <20210612215003.ypu5uf2bfceofkkd@alap3.anarazel.de>
Content-Type: multipart/mixed;
 boundary="------------E1794EE43B83FFB1B1EFECD7"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/759e997e-e1ca-91cd-84db-f4ae963fada1%40dunslane.net>
Precedence: bulk

This is a multi-part message in MIME format.
--------------E1794EE43B83FFB1B1EFECD7
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


On 6/12/21 5:50 PM, Andres Freund wrote:
> Hi,
>
> On 2021-06-12 17:40:38 -0400, Andrew Dunstan wrote:
>> Ok, I think the attached is the least we need to do. Given this I
>> haven't been able to induce a crash even when the catalog is hacked with
>> bogus missing values on a foreign table. But I'm not 100% convinced I
>> have fixed all the places that need to be fixed.
> Hm. There's a few places that look at atthasmissing and just assume that
> there's corresponding information about the missing field. And as far as
> I can see the proposed changes in RelationBuildTupleDesc() don't unset
> atthasmissing, they just prevent the constraint part of the tuple desc
> from being filled. Wouldn't this cause problems if we reach code like
>

Yes, you're right. This version should take care of things better.


Thanks for looking.


cheers


andrew


--------------E1794EE43B83FFB1B1EFECD7
Content-Type: text/x-patch; charset=UTF-8;
 name="nonlocal-missing-values-fix3.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="nonlocal-missing-values-fix3.patch"

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index afa830d924..bcf80d865f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2161,6 +2161,13 @@ SetAttrMissing(Oid relid, char *attname, char *value)
 	/* lock the table the attribute belongs to */
 	tablerel = table_open(relid, AccessExclusiveLock);
 
+	/* Don't do anything unless it's a RELKIND type relation */
+	if (tablerel->rd_rel->relkind != RELKIND_RELATION)
+	{
+		table_close(tablerel, AccessExclusiveLock);
+		return;
+	}
+
 	/* Lock the attribute row and get the data */
 	attrrel = table_open(AttributeRelationId, RowExclusiveLock);
 	atttup = SearchSysCacheAttName(relid, attname);
@@ -2287,7 +2294,8 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
 		valuesAtt[Anum_pg_attribute_atthasdef - 1] = true;
 		replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;
 
-		if (add_column_mode && !attgenerated)
+		if (rel->rd_rel->relkind == RELKIND_RELATION  && add_column_mode &&
+			!attgenerated)
 		{
 			expr2 = expression_planner(expr2);
 			estate = CreateExecutorState();
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 028e8ac46b..2b18eb89bf 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12215,9 +12215,11 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 	/*
 	 * Here we go --- change the recorded column type and collation.  (Note
 	 * heapTup is a copy of the syscache entry, so okay to scribble on.) First
-	 * fix up the missing value if any.
+	 * fix up the missing value if any. There shouldn't be any missing values
+	 * for anything except RELKIND_RELATION relations, but if there are, ignore
+	 * them.
 	 */
-	if (attTup->atthasmissing)
+	if (rel->rd_rel->relkind == RELKIND_RELATION  && attTup->atthasmissing)
 	{
 		Datum		missingval;
 		bool		missingNull;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index fd05615e76..588f9f0067 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -559,6 +559,15 @@ RelationBuildTupleDesc(Relation relation)
 			elog(ERROR, "invalid attribute number %d for relation \"%s\"",
 				 attp->attnum, RelationGetRelationName(relation));
 
+		/*
+		 * Fix atthasmissing flag - it's only for RELKIND relations. Others
+		 * should not have missing values set, but there may be some left from
+		 * before when we placed that check, so this code defensively ignores
+		 * such values.
+		 */
+		if (relation->rd_rel->relkind != RELKIND_RELATION)
+			attp->atthasmissing = false;
+
 		memcpy(TupleDescAttr(relation->rd_att, attnum - 1),
 			   attp,
 			   ATTRIBUTE_FIXED_PART_SIZE);

--------------E1794EE43B83FFB1B1EFECD7--