Date: Tue, 4 Sep 2018 13:50:25 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Michael Paquier <michael@paquier.xyz>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: pointless check in RelationBuildPartitionDesc
Message-ID: <20180904165025.zhby7ahrbkp7jf5l@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="chw3rqes4lwo6ohf"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20180904125128.ntliwrygsylaar4b@alvherre.pgsql>
User-Agent: NeoMutt/20170306-137-4415bd-dirty (1.8.0)
Precedence: bulk


--chw3rqes4lwo6ohf
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Proposed patch.  Checking isnull in a elog(ERROR) is important, because
the column is not marked NOT NULL.  This is not true for other columns
where we simply do Assert(!isnull).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--chw3rqes4lwo6ohf
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="ispartition.patch"
Content-Transfer-Encoding: quoted-printable

commit dc5f9933595c95471898e506e004a1799ea4eae5=1B[m
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org>
AuthorDate: Tue Sep 4 13:24:25 2018 -0300
CommitDate: Tue Sep 4 13:27:30 2018 -0300

    fix relpartbound accesses

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablec=
mds.c
index f46af41b56..ffdd14b819 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -773,9 +773,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid owne=
rId,
 										  InvalidOid,
 										  typaddress);
=20
-	/* Store inheritance information for new rel. */
-	StoreCatalogInheritance(relationId, inheritOids, stmt->partbound !=3D NUL=
L);
-
 	/*
 	 * We must bump the command counter to make the newly-created relation
 	 * tuple visible for opening.
@@ -869,6 +866,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid owne=
rId,
 		heap_close(parent, NoLock);
 	}
=20
+	/* Store inheritance information for new rel. */
+	StoreCatalogInheritance(relationId, inheritOids, stmt->partbound !=3D NUL=
L);
+
 	/*
 	 * Process the partitioning specification (if any) and store the partition
 	 * key information into the catalog.
@@ -14705,7 +14705,9 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
=20
 	(void) SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relpartbound,
 						   &isnull);
-	Assert(!isnull);
+	if (isnull)
+		elog(ERROR, "null relpartbound for relation %u",
+			 RelationGetRelid(partRel));
=20
 	/* Clear relpartbound and reset relispartition */
 	memset(new_val, 0, sizeof(new_val));
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioni=
ng/partbounds.c
index 9015a05d32..4ed9920618 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -1641,8 +1641,9 @@ get_qual_for_range(Relation parent, PartitionBoundSpe=
c *spec,
 			datum =3D SysCacheGetAttr(RELOID, tuple,
 									Anum_pg_class_relpartbound,
 									&isnull);
+			if (isnull)
+				elog(ERROR, "null relpartbound for relation %u", inhrelid);
=20
-			Assert(!isnull);
 			bspec =3D (PartitionBoundSpec *)
 				stringToNode(TextDatumGetCString(datum));
 			if (!IsA(bspec, PartitionBoundSpec))
diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/=
partcache.c
index 115a9fe78f..2fc876b7d8 100644
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -302,23 +302,11 @@ RelationBuildPartitionDesc(Relation rel)
 		if (!HeapTupleIsValid(tuple))
 			elog(ERROR, "cache lookup failed for relation %u", inhrelid);
=20
-		/*
-		 * It is possible that the pg_class tuple of a partition has not been
-		 * updated yet to set its relpartbound field.  The only case where
-		 * this happens is when we open the parent relation to check using its
-		 * partition descriptor that a new partition's bound does not overlap
-		 * some existing partition.
-		 */
-		if (!((Form_pg_class) GETSTRUCT(tuple))->relispartition)
-		{
-			ReleaseSysCache(tuple);
-			continue;
-		}
-
 		datum =3D SysCacheGetAttr(RELOID, tuple,
 								Anum_pg_class_relpartbound,
 								&isnull);
-		Assert(!isnull);
+		if (isnull)
+			elog(ERROR, "null relpartbound for partition %u" inhrelid);
 		boundspec =3D (Node *) stringToNode(TextDatumGetCString(datum));
=20
 		/*
@@ -883,9 +871,8 @@ generate_partition_qual(Relation rel)
 	boundDatum =3D SysCacheGetAttr(RELOID, tuple,
 								 Anum_pg_class_relpartbound,
 								 &isnull);
-	if (isnull)					/* should not happen */
-		elog(ERROR, "relation \"%s\" has relpartbound =3D null",
-			 RelationGetRelationName(rel));
+	if (isnull)
+		elog(ERROR, "null relpartbound for relation %u", RelationGetRelid(rel));
 	bound =3D castNode(PartitionBoundSpec,
 					 stringToNode(TextDatumGetCString(boundDatum)));
 	ReleaseSysCache(tuple);

--chw3rqes4lwo6ohf--