Date: Mon, 14 Jun 2021 15:13:21 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
	exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17056: Segmentation fault on altering the type of the
 foreign table column with a default
Message-ID: <202106141913.cbzfrrcoinxx@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <759e997e-e1ca-91cd-84db-f4ae963fada1@dunslane.net>
Archived-At: 
 <https://www.postgresql.org/message-id/202106141913.cbzfrrcoinxx%40alvherre.pgsql>
Precedence: bulk

On 2021-Jun-12, Andrew Dunstan wrote:

> +	/* Don't do anything unless it's a RELKIND type relation */
> +	if (tablerel->rd_rel->relkind != RELKIND_RELATION)
> +	{
> +		table_close(tablerel, AccessExclusiveLock);
> +		return;
> +	}

"RELKIND type relation" is the wrong phrase ... maybe "it's a plain
table" is good enough?  (Ditto in RelationBuildTupleDesc).

>  	/*
>  	 * 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)

Would it be sensible to have a macro "AttributeHasMissingVal(rel,
attTup)", to use instead of reading atthasmissing directly?  The macro
would check the relkind, and also serve as documentation that said check
is necessary.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"