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