From d3e284dcd69a328cb7053e782aeafdf392d34e23 Mon Sep 17 00:00:00 2001 From: Hadi Moshayedi Date: Mon, 5 Nov 2018 15:11:01 -0500 Subject: [PATCH] Use heap_deform_tuple() instead of calling heap_getattr(). (#2464) After Fast ALTER TABLE ADD COLUMN with a non-NULL default in PG11, physical heaps might not contain all attributes after a ALTER TABLE ADD COLUMN happens. heap_getattr() returns NULL when the physical tuple doesn't contain an attribute. So we should use heap_deform_tuple() in these cases, which fills in the missing attributes. Our catalog tables evolve over time, and an upgrade might involve some ALTER TABLE ADD COLUMN commands. Note that we don't need to worry about postgres catalog tables and we can use heap_getattr() for them, because they only change between major versions. This also fixes #2453. --- .../master/master_metadata_utility.c | 34 +++++----- .../distributed/utils/metadata_cache.c | 67 +++++++++---------- src/backend/distributed/utils/node_metadata.c | 48 ++++++------- .../expected/multi_metadata_attributes.out | 15 +++++ .../expected/multi_metadata_attributes_0.out | 10 +++ src/test/regress/multi_schedule | 1 + .../regress/sql/multi_metadata_attributes.sql | 8 +++ 7 files changed, 105 insertions(+), 78 deletions(-) create mode 100644 src/test/regress/expected/multi_metadata_attributes.out create mode 100644 src/test/regress/expected/multi_metadata_attributes_0.out create mode 100644 src/test/regress/sql/multi_metadata_attributes.sql diff --git a/src/backend/distributed/master/master_metadata_utility.c b/src/backend/distributed/master/master_metadata_utility.c index 1ce4c42b6..8b9d042e2 100644 --- a/src/backend/distributed/master/master_metadata_utility.c +++ b/src/backend/distributed/master/master_metadata_utility.c @@ -827,30 +827,32 @@ static GroupShardPlacement * TupleToGroupShardPlacement(TupleDesc tupleDescriptor, HeapTuple heapTuple) { GroupShardPlacement *shardPlacement = NULL; - bool isNull = false; + bool isNullArray[Natts_pg_dist_placement]; + Datum datumArray[Natts_pg_dist_placement]; - Datum placementId = heap_getattr(heapTuple, Anum_pg_dist_placement_placementid, - tupleDescriptor, &isNull); - Datum shardId = heap_getattr(heapTuple, Anum_pg_dist_placement_shardid, - tupleDescriptor, &isNull); - Datum shardLength = heap_getattr(heapTuple, Anum_pg_dist_placement_shardlength, - tupleDescriptor, &isNull); - Datum shardState = heap_getattr(heapTuple, Anum_pg_dist_placement_shardstate, - tupleDescriptor, &isNull); - Datum groupId = heap_getattr(heapTuple, Anum_pg_dist_placement_groupid, - tupleDescriptor, &isNull); if (HeapTupleHeaderGetNatts(heapTuple->t_data) != Natts_pg_dist_placement || HeapTupleHasNulls(heapTuple)) { ereport(ERROR, (errmsg("unexpected null in pg_dist_placement tuple"))); } + /* + * We use heap_deform_tuple() instead of heap_getattr() to expand tuple + * to contain missing values when ALTER TABLE ADD COLUMN happens. + */ + heap_deform_tuple(heapTuple, tupleDescriptor, datumArray, isNullArray); + shardPlacement = CitusMakeNode(GroupShardPlacement); - shardPlacement->placementId = DatumGetInt64(placementId); - shardPlacement->shardId = DatumGetInt64(shardId); - shardPlacement->shardLength = DatumGetInt64(shardLength); - shardPlacement->shardState = DatumGetUInt32(shardState); - shardPlacement->groupId = DatumGetUInt32(groupId); + shardPlacement->placementId = DatumGetInt64( + datumArray[Anum_pg_dist_placement_placementid - 1]); + shardPlacement->shardId = DatumGetInt64( + datumArray[Anum_pg_dist_placement_shardid - 1]); + shardPlacement->shardLength = DatumGetInt64( + datumArray[Anum_pg_dist_placement_shardlength - 1]); + shardPlacement->shardState = DatumGetUInt32( + datumArray[Anum_pg_dist_placement_shardstate - 1]); + shardPlacement->groupId = DatumGetUInt32( + datumArray[Anum_pg_dist_placement_groupid - 1]); return shardPlacement; } diff --git a/src/backend/distributed/utils/metadata_cache.c b/src/backend/distributed/utils/metadata_cache.c index 7cae34bed..2c8c5d625 100644 --- a/src/backend/distributed/utils/metadata_cache.c +++ b/src/backend/distributed/utils/metadata_cache.c @@ -889,13 +889,13 @@ BuildDistTableCacheEntry(DistTableCacheEntry *cacheEntry) { HeapTuple distPartitionTuple = NULL; Relation pgDistPartition = NULL; - Form_pg_dist_partition partitionForm = NULL; Datum partitionKeyDatum = 0; Datum replicationModelDatum = 0; MemoryContext oldContext = NULL; TupleDesc tupleDescriptor = NULL; - bool isNull = false; bool partitionKeyIsNull = false; + Datum datumArray[Natts_pg_dist_partition]; + bool isNullArray[Natts_pg_dist_partition]; pgDistPartition = heap_open(DistPartitionRelationId(), AccessShareLock); distPartitionTuple = @@ -912,14 +912,11 @@ BuildDistTableCacheEntry(DistTableCacheEntry *cacheEntry) cacheEntry->isDistributedTable = true; tupleDescriptor = RelationGetDescr(pgDistPartition); - partitionForm = (Form_pg_dist_partition) GETSTRUCT(distPartitionTuple); + heap_deform_tuple(distPartitionTuple, tupleDescriptor, datumArray, isNullArray); - cacheEntry->partitionMethod = partitionForm->partmethod; - - partitionKeyDatum = heap_getattr(distPartitionTuple, - Anum_pg_dist_partition_partkey, - tupleDescriptor, - &partitionKeyIsNull); + cacheEntry->partitionMethod = datumArray[Anum_pg_dist_partition_partmethod - 1]; + partitionKeyDatum = datumArray[Anum_pg_dist_partition_partkey - 1]; + partitionKeyIsNull = isNullArray[Anum_pg_dist_partition_partkey - 1]; /* note that for reference tables partitionKeyisNull is true */ if (!partitionKeyIsNull) @@ -944,20 +941,14 @@ BuildDistTableCacheEntry(DistTableCacheEntry *cacheEntry) cacheEntry->partitionKeyString = NULL; } - cacheEntry->colocationId = heap_getattr(distPartitionTuple, - Anum_pg_dist_partition_colocationid, - tupleDescriptor, - &isNull); - if (isNull) + cacheEntry->colocationId = datumArray[Anum_pg_dist_partition_colocationid - 1]; + if (isNullArray[Anum_pg_dist_partition_colocationid - 1]) { cacheEntry->colocationId = INVALID_COLOCATION_ID; } - replicationModelDatum = heap_getattr(distPartitionTuple, - Anum_pg_dist_partition_repmodel, - tupleDescriptor, - &isNull); - if (isNull) + replicationModelDatum = datumArray[Anum_pg_dist_partition_repmodel - 1]; + if (isNullArray[Anum_pg_dist_partition_repmodel - 1]) { /* * repmodel is NOT NULL but before ALTER EXTENSION citus UPGRADE the column @@ -3437,26 +3428,17 @@ TupleToShardInterval(HeapTuple heapTuple, TupleDesc tupleDescriptor, Oid interva int32 intervalTypeMod) { ShardInterval *shardInterval = NULL; - bool isNull = false; bool minValueNull = false; bool maxValueNull = false; Oid inputFunctionId = InvalidOid; Oid typeIoParam = InvalidOid; - Datum relationIdDatum = heap_getattr(heapTuple, Anum_pg_dist_shard_logicalrelid, - tupleDescriptor, &isNull); - Datum shardIdDatum = heap_getattr(heapTuple, Anum_pg_dist_shard_shardid, - tupleDescriptor, &isNull); - Datum storageTypeDatum = heap_getattr(heapTuple, Anum_pg_dist_shard_shardstorage, - tupleDescriptor, &isNull); - - Datum minValueTextDatum = heap_getattr(heapTuple, Anum_pg_dist_shard_shardminvalue, - tupleDescriptor, &minValueNull); - Datum maxValueTextDatum = heap_getattr(heapTuple, Anum_pg_dist_shard_shardmaxvalue, - tupleDescriptor, &maxValueNull); - - Oid relationId = DatumGetObjectId(relationIdDatum); - int64 shardId = DatumGetInt64(shardIdDatum); - char storageType = DatumGetChar(storageTypeDatum); + Datum datumArray[Natts_pg_dist_shard]; + bool isNullArray[Natts_pg_dist_shard]; + Datum minValueTextDatum = 0; + Datum maxValueTextDatum = 0; + Oid relationId = InvalidOid; + int64 shardId = InvalidOid; + char storageType = InvalidOid; Datum minValue = 0; Datum maxValue = 0; bool minValueExists = false; @@ -3466,6 +3448,21 @@ TupleToShardInterval(HeapTuple heapTuple, TupleDesc tupleDescriptor, Oid interva char intervalAlign = '0'; char intervalDelim = '0'; + /* + * We use heap_deform_tuple() instead of heap_getattr() to expand tuple + * to contain missing values when ALTER TABLE ADD COLUMN happens. + */ + heap_deform_tuple(heapTuple, tupleDescriptor, datumArray, isNullArray); + + relationId = DatumGetObjectId(datumArray[Anum_pg_dist_shard_logicalrelid - 1]); + shardId = DatumGetInt64(datumArray[Anum_pg_dist_shard_shardid - 1]); + storageType = DatumGetChar(datumArray[Anum_pg_dist_shard_shardstorage - 1]); + minValueTextDatum = datumArray[Anum_pg_dist_shard_shardminvalue - 1]; + maxValueTextDatum = datumArray[Anum_pg_dist_shard_shardmaxvalue - 1]; + + minValueNull = isNullArray[Anum_pg_dist_shard_shardminvalue - 1]; + maxValueNull = isNullArray[Anum_pg_dist_shard_shardmaxvalue - 1]; + if (!minValueNull && !maxValueNull) { char *minValueString = TextDatumGetCString(minValueTextDatum); diff --git a/src/backend/distributed/utils/node_metadata.c b/src/backend/distributed/utils/node_metadata.c index a6420974c..d50d746ac 100644 --- a/src/backend/distributed/utils/node_metadata.c +++ b/src/backend/distributed/utils/node_metadata.c @@ -1523,41 +1523,35 @@ static WorkerNode * TupleToWorkerNode(TupleDesc tupleDescriptor, HeapTuple heapTuple) { WorkerNode *workerNode = NULL; - bool isNull = false; - - Datum nodeId = heap_getattr(heapTuple, Anum_pg_dist_node_nodeid, - tupleDescriptor, &isNull); - Datum groupId = heap_getattr(heapTuple, Anum_pg_dist_node_groupid, - tupleDescriptor, &isNull); - Datum nodeName = heap_getattr(heapTuple, Anum_pg_dist_node_nodename, - tupleDescriptor, &isNull); - Datum nodePort = heap_getattr(heapTuple, Anum_pg_dist_node_nodeport, - tupleDescriptor, &isNull); - Datum nodeRack = heap_getattr(heapTuple, Anum_pg_dist_node_noderack, - tupleDescriptor, &isNull); - Datum hasMetadata = heap_getattr(heapTuple, Anum_pg_dist_node_hasmetadata, - tupleDescriptor, &isNull); - Datum isActive = heap_getattr(heapTuple, Anum_pg_dist_node_isactive, - tupleDescriptor, &isNull); - Datum nodeRole = heap_getattr(heapTuple, Anum_pg_dist_node_noderole, - tupleDescriptor, &isNull); - Datum nodeCluster = heap_getattr(heapTuple, Anum_pg_dist_node_nodecluster, - tupleDescriptor, &isNull); + Datum datumArray[Natts_pg_dist_node]; + bool isNullArray[Natts_pg_dist_node]; + char *nodeName = NULL; + char *nodeRack = NULL; Assert(!HeapTupleHasNulls(heapTuple)); + /* + * We use heap_deform_tuple() instead of heap_getattr() to expand tuple + * to contain missing values when ALTER TABLE ADD COLUMN happens. + */ + heap_deform_tuple(heapTuple, tupleDescriptor, datumArray, isNullArray); + + nodeName = DatumGetCString(datumArray[Anum_pg_dist_node_nodename - 1]); + nodeRack = DatumGetCString(datumArray[Anum_pg_dist_node_noderack - 1]); + workerNode = (WorkerNode *) palloc0(sizeof(WorkerNode)); - workerNode->nodeId = DatumGetUInt32(nodeId); - workerNode->workerPort = DatumGetUInt32(nodePort); - workerNode->groupId = DatumGetUInt32(groupId); + workerNode->nodeId = DatumGetUInt32(datumArray[Anum_pg_dist_node_nodeid - 1]); + workerNode->workerPort = DatumGetUInt32(datumArray[Anum_pg_dist_node_nodeport - 1]); + workerNode->groupId = DatumGetUInt32(datumArray[Anum_pg_dist_node_groupid - 1]); strlcpy(workerNode->workerName, TextDatumGetCString(nodeName), WORKER_LENGTH); strlcpy(workerNode->workerRack, TextDatumGetCString(nodeRack), WORKER_LENGTH); - workerNode->hasMetadata = DatumGetBool(hasMetadata); - workerNode->isActive = DatumGetBool(isActive); - workerNode->nodeRole = DatumGetObjectId(nodeRole); + workerNode->hasMetadata = DatumGetBool(datumArray[Anum_pg_dist_node_hasmetadata - 1]); + workerNode->isActive = DatumGetBool(datumArray[Anum_pg_dist_node_isactive - 1]); + workerNode->nodeRole = DatumGetObjectId(datumArray[Anum_pg_dist_node_noderole - 1]); { - Name nodeClusterName = DatumGetName(nodeCluster); + Name nodeClusterName = DatumGetName(datumArray[Anum_pg_dist_node_nodecluster - + 1]); char *nodeClusterString = NameStr(*nodeClusterName); /* diff --git a/src/test/regress/expected/multi_metadata_attributes.out b/src/test/regress/expected/multi_metadata_attributes.out new file mode 100644 index 000000000..a2af38bd4 --- /dev/null +++ b/src/test/regress/expected/multi_metadata_attributes.out @@ -0,0 +1,15 @@ +-- if the output of following query changes, we might need to change +-- some heap_getattr() calls to heap_deform_tuple(). This errors out in +-- postgres versions before 11. +SELECT attrelid::regclass, attname, atthasmissing, attmissingval +FROM pg_attribute +WHERE atthasmissing +ORDER BY attrelid, attname; + attrelid | attname | atthasmissing | attmissingval +--------------+-------------+---------------+--------------- + pg_dist_node | hasmetadata | t | {f} + pg_dist_node | isactive | t | {t} + pg_dist_node | nodecluster | t | {default} + pg_dist_node | noderole | t | {primary} +(4 rows) + diff --git a/src/test/regress/expected/multi_metadata_attributes_0.out b/src/test/regress/expected/multi_metadata_attributes_0.out new file mode 100644 index 000000000..55e60f1f7 --- /dev/null +++ b/src/test/regress/expected/multi_metadata_attributes_0.out @@ -0,0 +1,10 @@ +-- if the output of following query changes, we might need to change +-- some heap_getattr() calls to heap_deform_tuple(). This errors out in +-- postgres versions before 11. +SELECT attrelid::regclass, attname, atthasmissing, attmissingval +FROM pg_attribute +WHERE atthasmissing +ORDER BY attrelid, attname; +ERROR: column "atthasmissing" does not exist +LINE 1: SELECT attrelid::regclass, attname, atthasmissing, attmissin... + ^ diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index d33f96ac2..f438ec6dd 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -22,6 +22,7 @@ test: multi_test_helpers test: multi_table_ddl test: multi_name_lengths test: multi_metadata_access +test: multi_metadata_attributes test: multi_read_from_secondaries diff --git a/src/test/regress/sql/multi_metadata_attributes.sql b/src/test/regress/sql/multi_metadata_attributes.sql new file mode 100644 index 000000000..f34bd51de --- /dev/null +++ b/src/test/regress/sql/multi_metadata_attributes.sql @@ -0,0 +1,8 @@ + +-- if the output of following query changes, we might need to change +-- some heap_getattr() calls to heap_deform_tuple(). This errors out in +-- postgres versions before 11. +SELECT attrelid::regclass, attname, atthasmissing, attmissingval +FROM pg_attribute +WHERE atthasmissing +ORDER BY attrelid, attname;