On 6/17/21 11:13 AM, Andrew Dunstan wrote:
> On 6/17/21 11:05 AM, Tom Lane wrote:
>> Andrew Dunstan <and...@dunslane.net> writes:
>>> Here's a patch I propose to apply to fix this bug (See
>>> <https://www.postgresql.org/message-id/flat/759e997e-e1ca-91cd-84db-f4ae963fada1%40dunslane.net#b1cf11c3eb1f450bed97c79ad473909f>)
>> If I'm reading the code correctly, your change in RelationBuildTupleDesc
>> is scribbling directly on the disk buffer, which is surely not okay.
>> I don't understand why you need that at all given the other defenses
>> you added ... but if you need it, you have to modify the tuple AFTER
>> copying it.
>
> OK, will fix. I think we do need it (See Andres' comment in the bug
> thread). It should be a fairly simple fix.
>
>
revised patch attached.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
>From 06b073a01da5f3e69417ffb88fea1320c0e0a08b Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <and...@dunslane.net>
Date: Thu, 17 Jun 2021 11:49:33 -0400
Subject: [PATCH] Don't set a fast default for anything but a plain table
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The fast default code added in Release 11 omitted to check that the
table a fast default was being added to was a plain table. Thus one
could be added to a foreign table, which predicably blows up. Here we
perform that check, and in addition if we encounter a missing value for
an attribute of something other than a plain table we ignore it.
Fixes bug #17056
Backpatch to release 11,
Reviewed by: Andres Freund, Álvaro Herrera and Tom Lane
---
src/backend/catalog/heap.c | 10 +++++++++-
src/backend/commands/tablecmds.c | 5 +++--
src/backend/utils/cache/relcache.c | 19 ++++++++++++++++++-
src/test/regress/expected/fast_default.out | 19 +++++++++++++++++++
src/test/regress/sql/fast_default.sql | 14 ++++++++++++++
5 files changed, 63 insertions(+), 4 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index afa830d924..1293dc04ca 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 plain table */
+ 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..2eef07e452 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12215,9 +12215,10 @@ 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 plain tables, 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..446cfcc4e8 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -551,6 +551,7 @@ RelationBuildTupleDesc(Relation relation)
{
Form_pg_attribute attp;
int attnum;
+ bool atthasmissing;
attp = (Form_pg_attribute) GETSTRUCT(pg_attribute_tuple);
@@ -563,6 +564,22 @@ RelationBuildTupleDesc(Relation relation)
attp,
ATTRIBUTE_FIXED_PART_SIZE);
+ /*
+ * Fix atthasmissing flag - it's only for plain tables. 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.
+ */
+ atthasmissing = attp->atthasmissing;
+ if (relation->rd_rel->relkind != RELKIND_RELATION && atthasmissing)
+ {
+ Form_pg_attribute nattp;
+
+ atthasmissing = false;
+ nattp = TupleDescAttr(relation->rd_att, attnum - 1);
+ nattp->atthasmissing = false;
+ }
+
/* Update constraint/default info */
if (attp->attnotnull)
constr->has_not_null = true;
@@ -572,7 +589,7 @@ RelationBuildTupleDesc(Relation relation)
ndef++;
/* If the column has a "missing" value, put it in the attrmiss array */
- if (attp->atthasmissing)
+ if (atthasmissing)
{
Datum missingval;
bool missingNull;
diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out
index 10bc5ff757..91f25717b5 100644
--- a/src/test/regress/expected/fast_default.out
+++ b/src/test/regress/expected/fast_default.out
@@ -797,7 +797,26 @@ SELECT * FROM t WHERE a IS NULL;
(1 row)
ROLLBACK;
+-- verify that a default set on a non-plain table doesn't set a missing
+-- value on the attribute
+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);
+SELECT count(*)
+ FROM pg_attribute
+ WHERE attrelid = 'ft1'::regclass AND
+ (attmissingval IS NOT NULL OR atthasmissing);
+ count
+-------
+ 0
+(1 row)
+
-- cleanup
+DROP FOREIGN TABLE ft1;
+DROP SERVER s0;
+DROP FOREIGN DATA WRAPPER dummy;
DROP TABLE vtype;
DROP TABLE vtype2;
DROP TABLE follower;
diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql
index 4589b9e58d..16a3b7ca51 100644
--- a/src/test/regress/sql/fast_default.sql
+++ b/src/test/regress/sql/fast_default.sql
@@ -524,8 +524,22 @@ SET LOCAL enable_seqscan = false;
SELECT * FROM t WHERE a IS NULL;
ROLLBACK;
+-- verify that a default set on a non-plain table doesn't set a missing
+-- value on the attribute
+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);
+SELECT count(*)
+ FROM pg_attribute
+ WHERE attrelid = 'ft1'::regclass AND
+ (attmissingval IS NOT NULL OR atthasmissing);
-- cleanup
+DROP FOREIGN TABLE ft1;
+DROP SERVER s0;
+DROP FOREIGN DATA WRAPPER dummy;
DROP TABLE vtype;
DROP TABLE vtype2;
DROP TABLE follower;
--
2.25.4
