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