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