From: Andrew Dunstan <andrew@dunslane.net>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: exclusion@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17056: Segmentation fault on altering the type of the foreign table column with a default
Date: Sat, 12 Jun 2021 21:59:24 -0400
Message-ID: <759e997e-e1ca-91cd-84db-f4ae963fada1@dunslane.net> (raw)
In-Reply-To: <20210612215003.ypu5uf2bfceofkkd@alap3.anarazel.de>
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>
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
Attachments:nonlocal-missing-values-fix3.patch (text/x-patch, 2.7K) downloaddiff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.cindex 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.cindex 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.cindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: andrew@dunslane.net, andres@anarazel.de, 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
In-Reply-To: <759e997e-e1ca-91cd-84db-f4ae963fada1@dunslane.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox