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