BUG #17056: Segmentation fault on altering the type of the foreign table column with a default

PG Bug reporting form <noreply@postgresql.org> writes:

CREATE FOREIGN DATA WRAPPER dummy;
CREATE SERVER s0 FOREIGN DATA WRAPPER dummy;
CREATE FOREIGN TABLE ft1 (c1 integer NOT NULL) SERVER s0;
ALTER FOREIGN TABLE ft1 ADD COLUMN c8 integer DEFAULT 0;
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE char(10);

Hmm. The equivalent DDL on a plain table works fine, but this is
crashing in the code that manipulates attmissingval. I suspect some
confusion about whether a foreign table column should even *have*
attmissingval. Andrew, any thoughts?

regards, tom lane

On 6/10/21 6:10 PM, Tom Lane wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

CREATE FOREIGN DATA WRAPPER dummy;
CREATE SERVER s0 FOREIGN DATA WRAPPER dummy;
CREATE FOREIGN TABLE ft1 (c1 integer NOT NULL) SERVER s0;
ALTER FOREIGN TABLE ft1 ADD COLUMN c8 integer DEFAULT 0;
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE char(10);

Hmm. The equivalent DDL on a plain table works fine, but this is
crashing in the code that manipulates attmissingval. I suspect some
confusion about whether a foreign table column should even *have*
attmissingval. Andrew, any thoughts?

My initial thought would be that it should not. If the foreign table has
rows with missing columns then it should be up to the foreign server to
supply them transparently. We have no notion what the foreign semantics
of missing columns are.

I can take a look at a fix tomorrow. My inclination would be simply to
skip setting attmissingval for foreign tables.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 6/10/21 6:10 PM, Tom Lane wrote:

Hmm. The equivalent DDL on a plain table works fine, but this is
crashing in the code that manipulates attmissingval. I suspect some
confusion about whether a foreign table column should even *have*
attmissingval. Andrew, any thoughts?

My initial thought would be that it should not. If the foreign table has
rows with missing columns then it should be up to the foreign server to
supply them transparently. We have no notion what the foreign semantics
of missing columns are.

Yeah, that was kind of what I thought. Probably only RELKIND_RELATION
rels should ever have attmissingval; but certainly, anything without
local storage should not.

I can take a look at a fix tomorrow. My inclination would be simply to
skip setting attmissingval for foreign tables.

Seems like in addition to that, we'll need a defense in this specific
code to cope with the case where the foreign column already has an
attmissingval. Or maybe, the logic to not store a new one will be enough
to keep us from reaching this crash; but we need to be sure it is enough.

regards, tom lane

On 6/10/21 7:11 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 6/10/21 6:10 PM, Tom Lane wrote:

Hmm. The equivalent DDL on a plain table works fine, but this is
crashing in the code that manipulates attmissingval. I suspect some
confusion about whether a foreign table column should even *have*
attmissingval. Andrew, any thoughts?

My initial thought would be that it should not. If the foreign table has
rows with missing columns then it should be up to the foreign server to
supply them transparently. We have no notion what the foreign semantics
of missing columns are.

Yeah, that was kind of what I thought. Probably only RELKIND_RELATION
rels should ever have attmissingval; but certainly, anything without
local storage should not.

I can take a look at a fix tomorrow. My inclination would be simply to
skip setting attmissingval for foreign tables.

Seems like in addition to that, we'll need a defense in this specific
code to cope with the case where the foreign column already has an
attmissingval. Or maybe, the logic to not store a new one will be enough
to keep us from reaching this crash; but we need to be sure it is enough.

The first piece could be fairly simply accomplished by something like this

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index afa830d924..ac89efefe8 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2287,7 +2287,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();

I'm guessing we want to exclude materialized views and partitioned
tables as well as things without local storage.

How to ignore something that's got into the catalog that shouldn't be
there is less clear. At the point where we fetch missing values all we
have access to is a TupleDesc.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 6/11/21 5:59 PM, Andrew Dunstan wrote:

How to ignore something that's got into the catalog that shouldn't be
there is less clear. At the point where we fetch missing values all we
have access to is a TupleDesc.

On further reflection I guess we'll need to make that check at the point
where we fill in the TupleDesc.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 6/10/21 7:11 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 6/10/21 6:10 PM, Tom Lane wrote:

Hmm. The equivalent DDL on a plain table works fine, but this is
crashing in the code that manipulates attmissingval. I suspect some
confusion about whether a foreign table column should even *have*
attmissingval. Andrew, any thoughts?

My initial thought would be that it should not. If the foreign table has
rows with missing columns then it should be up to the foreign server to
supply them transparently. We have no notion what the foreign semantics
of missing columns are.

Yeah, that was kind of what I thought. Probably only RELKIND_RELATION
rels should ever have attmissingval; but certainly, anything without
local storage should not.

I can take a look at a fix tomorrow. My inclination would be simply to
skip setting attmissingval for foreign tables.

Seems like in addition to that, we'll need a defense in this specific
code to cope with the case where the foreign column already has an
attmissingval. Or maybe, the logic to not store a new one will be enough
to keep us from reaching this crash; but we need to be sure it is enough.

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.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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

Datum
getmissingattr(TupleDesc tupleDesc,
int attnum, bool *isnull)
{
Form_pg_attribute att;

Assert(attnum <= tupleDesc->natts);
Assert(attnum > 0);

att = TupleDescAttr(tupleDesc, attnum - 1);

if (att->atthasmissing)
{
AttrMissing *attrmiss;

Assert(tupleDesc->constr);
Assert(tupleDesc->constr->missing);

attrmiss = tupleDesc->constr->missing + (attnum - 1);

if (attrmiss->am_present)

Greetings,

Andres Freund

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

reposting to -hackers to get more eyeballs.

Summary: only RELKIND_RELATION type relations should have attributes
with atthasmissing/attmissingval

-------- Forwarded Message --------
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
From: Andrew Dunstan <andrew@dunslane.net>
To: Andres Freund <andres@anarazel.de>
CC: Tom Lane <tgl@sss.pgh.pa.us>, exclusion@gmail.com,
pgsql-bugs@lists.postgresql.org

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

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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"

On 6/14/21 3:13 PM, Alvaro Herrera wrote:

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).

OK, will change.

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

Well AFAIK this is the only place we actually need this combination of
tests, and I'm not a huge fan of defining a macro to use in one spot.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com