Re: Fast default stuff versus pg_upgrade

On 06/20/2018 12:58 PM, Andres Freund wrote:
> Hi,
>
> Just a quick scan-through review:
>
> On 2018-06-20 12:51:07 -0400, Andrew Dunstan wrote:
>> diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c
>> index 0c54b02..2666ab2 100644
>> --- a/src/backend/utils/adt/pg_upgrade_support.c
>> +++ b/src/backend/utils/adt/pg_upgrade_support.c
>> @@ -11,14 +11,19 @@
>>   
>>   #include "postgres.h"
>>   
>> +#include "access/heapam.h"
>> +#include "access/htup_details.h"
>>   #include "catalog/binary_upgrade.h"
>> +#include "catalog/indexing.h"
>>   #include "catalog/namespace.h"
>>   #include "catalog/pg_type.h"
>>   #include "commands/extension.h"
>>   #include "miscadmin.h"
>>   #include "utils/array.h"
>>   #include "utils/builtins.h"
>> -
>> +#include "utils/lsyscache.h"
>> +#include "utils/rel.h"
>> +#include "utils/syscache.h"
>>
> Seems to delete a newline.


Will fix

>
>
>>   #define CHECK_IS_BINARY_UPGRADE									\
>>   do {															\
>> @@ -192,3 +197,66 @@ binary_upgrade_set_record_init_privs(PG_FUNCTION_ARGS)
>>   
>>   	PG_RETURN_VOID();
>>   }
>> +
>> +Datum
>> +binary_upgrade_set_missing_value(PG_FUNCTION_ARGS)
>> +{
>> +	Oid		table_id = PG_GETARG_OID(0);
>> +	text    *attname = PG_GETARG_TEXT_P(1);
>> +	text    *value = PG_GETARG_TEXT_P(2);
>> +	Datum    valuesAtt[Natts_pg_attribute];
>> +	bool     nullsAtt[Natts_pg_attribute];
>> +	bool     replacesAtt[Natts_pg_attribute];
>> +	Datum    missingval;
>> +	Form_pg_attribute attStruct;
>> +	Relation    attrrel;
>> +	HeapTuple   atttup, newtup;
>> +	Oid         inputfunc, inputparam;
>> +	char    *cattname = text_to_cstring(attname);
>> +	char    *cvalue = text_to_cstring(value);
>> +
>> +	CHECK_IS_BINARY_UPGRADE;
>> +
>> +	/* Lock the attribute row and get the data */
>> +	attrrel = heap_open(AttributeRelationId, RowExclusiveLock);
> Maybe I'm being paranoid, but I feel like the relation should also be
> locked here.


I wondered about that. Other opinions?


>
>
>> +	atttup = SearchSysCacheAttName(table_id,cattname);
> Missing space before 'cattname'.


Will fix.

>
>> +	if (!HeapTupleIsValid(atttup))
>> +		elog(ERROR, "cache lookup failed for attribute %s of relation %u",
>> +			 cattname, table_id);
>> +	attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
>> +
>> +	/* get an array value from the value string */
>> +
>> +	/* find the io info for an arbitrary array type to get array_in Oid */
>> +	getTypeInputInfo(INT4ARRAYOID, &inputfunc, &inputparam);
> Maybe I'm confused, but why INT4ARRAYOID? If you're just looking for the
> oid of array_in, why not use F_ARRAY_IN?


Brain fade? Will fix.


>
>> +	missingval = OidFunctionCall3(
>> +		inputfunc, /* array_in */
>> +		CStringGetDatum(cvalue),
>> +		ObjectIdGetDatum(attStruct->atttypid),
>> +		Int32GetDatum(attStruct->atttypmod));
>> +
>> +	/* update the tuple - set atthasmissing and attmissingval */
>> +	MemSet(valuesAtt, 0, sizeof(valuesAtt));
>> +	MemSet(nullsAtt, false, sizeof(nullsAtt));
>> +	MemSet(replacesAtt, false, sizeof(replacesAtt));
>> +
>> +	valuesAtt[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(true);
>> +	replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true;
>> +	valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval;
>> +	replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
>> +
>> +	newtup = heap_modify_tuple(atttup, RelationGetDescr(attrrel),
>> +							   valuesAtt, nullsAtt, replacesAtt);
>> +	CatalogTupleUpdate(attrrel, &newtup->t_self, newtup);
>> +	/* clean up */
>> +	heap_freetuple(newtup);
>> +	ReleaseSysCache(atttup);
>> +	pfree(cattname);
>> +	pfree(cvalue);
>> +	pfree(DatumGetPointer(missingval));
> Is this worth bothering with (except the ReleaseSysCache)? We'll clean
> up via memory context soon, no?
>



Done from an abundance of caution. I'll remove them.


Thanks for the quick review.

Attached deals with all those issues except locking.

cheers

andrew

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