Re: BUG #15446: Crash on ALTER TABLE

2019-01-10 Thread Dean Rasheed
On Wed, 9 Jan 2019 at 23:24, Andrew Dunstan
 wrote:
> Here's another attempt. For the rewrite case it kept the logic of the
> previous patch to clear all the missing attributes, but if we're not
> rewriting we reconstruct the missing value according to the new type
> settings.
>

Looks good to me.

Regards,
Dean



Re: BUG #15446: Crash on ALTER TABLE

2019-01-09 Thread Andrew Dunstan

On 1/8/19 7:41 PM, Andrew Dunstan wrote:
> On 1/8/19 4:48 PM, Dean Rasheed wrote:
>> On Tue, 8 Jan 2019 at 19:34, Andrew Dunstan
>>  wrote:
>>> Here's a patch that I think cures the problem.
>>>
>> Hmm, that doesn't quite work because the table might not actually be
>> rewritten as a result of the type change. For example:
>>
>> DROP TABLE IF EXISTS foo;
>> CREATE TABLE foo (a int);
>> INSERT INTO foo VALUES (1);
>> ALTER TABLE foo ADD COLUMN b varchar(10) DEFAULT 'xxx';
>> ALTER TABLE foo ALTER COLUMN b SET DEFAULT 'yyy';
>> INSERT INTO foo VALUES (2);
>> SELECT * FROM foo;
>>  a |  b
>> ---+-
>>  1 | xxx
>>  2 | yyy
>> (2 rows)
>>
>> ALTER TABLE foo ALTER COLUMN b TYPE varchar(20) USING b::varchar(20);
>> SELECT * FROM foo;
>>  a |  b
>> ---+-
>>  1 |
>>  2 | yyy
>> (2 rows)
>>
>
> Ouch, OK, looks like we need something more complex.
>
>

Here's another attempt. For the rewrite case it kept the logic of the
previous patch to clear all the missing attributes, but if we're not
rewriting we reconstruct the missing value according to the new type
settings.


cheers


andrew



-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c8c50e8c98..e72d0b43ed 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9285,6 +9285,21 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 	HeapTuple	depTup;
 	ObjectAddress address;
 
+	/*
+	 * Clear all the missing values if we're rewriting the table, since this
+	 * renders them pointless.
+	 */
+	if (tab->rewrite)
+	{
+		Relationnewrel;
+
+		newrel = heap_open(RelationGetRelid(rel), NoLock);
+		RelationClearMissing(newrel);
+		relation_close(newrel, NoLock);
+		/* make sure we don't conflict with later attribute modifications */
+		CommandCounterIncrement();
+	}
+
 	attrelation = heap_open(AttributeRelationId, RowExclusiveLock);
 
 	/* Look up the target column */
@@ -9601,7 +9616,69 @@ 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.
 	 */
+	if (attTup->atthasmissing)
+	{
+		Datum   missingval;
+		boolmissingNull;
+
+		/* if rewrite is true the missing value should already be cleared */
+		Assert(tab->rewrite == 0);
+
+		/* Get the missing value datum */
+		missingval = heap_getattr(heapTup,
+  Anum_pg_attribute_attmissingval,
+  attrelation->rd_att,
+  );
+
+		/* if it's a null array there is nothing to do */
+
+		if (! missingNull)
+		{
+			/*
+			 * Get the datum out of the array and repack it in a new array
+			 * built with the new type data. We assume that since the table
+			 * doesn't need rewriting, the actual Datum doesn't need to be
+			 * changed, only the array metadata.
+			 */
+
+			int one = 1;
+			bool isNull;
+			Datum   valuesAtt[Natts_pg_attribute];
+			boolnullsAtt[Natts_pg_attribute];
+			boolreplacesAtt[Natts_pg_attribute];
+
+			MemSet(valuesAtt, 0, sizeof(valuesAtt));
+			MemSet(nullsAtt, false, sizeof(nullsAtt));
+			MemSet(replacesAtt, false, sizeof(replacesAtt));
+
+			missingval = array_get_element(missingval,
+		   1,
+		   ,
+		   0,
+		   attTup->attlen,
+		   attTup->attbyval,
+		   attTup->attalign,
+		   );
+			missingval = PointerGetDatum(
+construct_array(,
+1,
+targettype,
+tform->typlen,
+tform->typbyval,
+tform->typalign));
+
+			valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval;
+			replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
+			nullsAtt[Anum_pg_attribute_attmissingval - 1] = false;
+
+			heapTup = heap_modify_tuple(heapTup, RelationGetDescr(attrelation),
+		valuesAtt, nullsAtt, replacesAtt);
+			attTup = (Form_pg_attribute) GETSTRUCT(heapTup);
+		}
+	}
+
 	attTup->atttypid = targettype;
 	attTup->atttypmod = targettypmod;
 	attTup->attcollation = targetcollid;
diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out
index 1c1924cd5c..40a15bd2d6 100644
--- a/src/test/regress/expected/fast_default.out
+++ b/src/test/regress/expected/fast_default.out
@@ -735,7 +735,44 @@ INSERT INTO leader VALUES (1, 1), (2, 2);
 ALTER TABLE leader ADD c int;
 ALTER TABLE leader DROP c;
 DELETE FROM leader;
+-- check that ALTER TABLE ... ALTER TYPE does the right thing
+CREATE TABLE vtype( a integer);
+INSERT INTO vtype VALUES (1);
+ALTER TABLE vtype ADD COLUMN b DOUBLE PRECISION DEFAULT 0.2;
+ALTER TABLE vtype ADD COLUMN c BOOLEAN DEFAULT true;
+SELECT * FROM vtype;
+ a |  b  | c 
+---+-+---
+ 1 | 0.2 | t
+(1 row)
+
+ALTER TABLE vtype
+  ALTER b TYPE text USING b::text,
+  ALTER c TYPE 

Re: BUG #15446: Crash on ALTER TABLE

2019-01-08 Thread Andrew Dunstan


On 1/8/19 4:48 PM, Dean Rasheed wrote:
> On Tue, 8 Jan 2019 at 19:34, Andrew Dunstan
>  wrote:
>> Here's a patch that I think cures the problem.
>>
> Hmm, that doesn't quite work because the table might not actually be
> rewritten as a result of the type change. For example:
>
> DROP TABLE IF EXISTS foo;
> CREATE TABLE foo (a int);
> INSERT INTO foo VALUES (1);
> ALTER TABLE foo ADD COLUMN b varchar(10) DEFAULT 'xxx';
> ALTER TABLE foo ALTER COLUMN b SET DEFAULT 'yyy';
> INSERT INTO foo VALUES (2);
> SELECT * FROM foo;
>  a |  b
> ---+-
>  1 | xxx
>  2 | yyy
> (2 rows)
>
> ALTER TABLE foo ALTER COLUMN b TYPE varchar(20) USING b::varchar(20);
> SELECT * FROM foo;
>  a |  b
> ---+-
>  1 |
>  2 | yyy
> (2 rows)
>


Ouch, OK, looks like we need something more complex.


cheers


andrew


-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services