Re: Fast default stuff versus pg_upgrade

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.


>  #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.


> +    atttup = SearchSysCacheAttName(table_id,cattname);

Missing space before 'cattname'.


> +    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?


> +    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?

Greetings,

Andres Freund