Fix assorted bugs in pg_get_partition_constraintdef().

It failed if passed a nonexistent relation OID, or one that was a non-heap
relation, because of blindly applying heap_open to a user-supplied OID.
This is not OK behavior for a SQL-exposed function; we have a project
policy that we should return NULL in such cases.  Moreover, since
pg_get_partition_constraintdef ought now to work on indexes, restricting
it to heaps is flat wrong anyway.

The underlying function generate_partition_qual() wasn't on board with
indexes having partition quals either, nor for that matter with rels
having relispartition set but yet null relpartbound.  (One wonders
whether the person who wrote the function comment blocks claiming that
these functions allow a missing relpartbound had ever tested it.)

Fix by testing relispartition before opening the rel, and by using
relation_open not heap_open.  (If any other relkinds ever grow the
ability to have relispartition set, the code will work with them
automatically.)  Also, don't reject null relpartbound in
generate_partition_qual.

Back-patch to v11, and all but the null-relpartbound change to v10.
(It's not really necessary to change generate_partition_qual at all
in v10, but I thought s/heap_open/relation_open/ would be a good
idea anyway just to keep the code in sync with later branches.)

Per report from Justin Pryzby.

Discussion: https://postgr.es/m/20180927200020.GJ776@telsasoft.com

diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 0c116b32efd51b1299203ea798e1b605e98c8fa3..12b2532d95725e2ffd0d142b480a8b958d2ad734 100644 (file)
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -1846,6 +1846,30 @@ get_rel_relkind(Oid relid)
                return '\0';
 }
 
+/*
+ * get_rel_relispartition
+ *
+ *             Returns the relispartition flag associated with a given relation.
+ */
+bool
+get_rel_relispartition(Oid relid)
+{
+       HeapTuple       tp;
+
+       tp = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+       if (HeapTupleIsValid(tp))
+       {
+               Form_pg_class reltup = (Form_pg_class) GETSTRUCT(tp);
+               bool            result;
+
+               result = reltup->relispartition;
+               ReleaseSysCache(tp);
+               return result;
+       }
+       else
+               return false;
+}
+
 /*
  * get_rel_tablespace
  *

diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/partcache.c
index e35a43405eb5d82395af8b257d44680ccac75be5..5757301d0547ceb6017956771b38b1531496169f 100644 (file)
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -797,31 +797,38 @@ RelationGetPartitionQual(Relation rel)
  * get_partition_qual_relid
  *
  * Returns an expression tree describing the passed-in relation's partition
- * constraint. If there is no partition constraint returns NULL; this can
- * happen if the default partition is the only partition.
+ * constraint.
+ *
+ * If the relation is not found, or is not a partition, or there is no
+ * partition constraint, return NULL.  We must guard against the first two
+ * cases because this supports a SQL function that could be passed any OID.
+ * The last case can happen even if relispartition is true, when a default
+ * partition is the only partition.
  */
 Expr *
 get_partition_qual_relid(Oid relid)
 {
-       Relation        rel = heap_open(relid, AccessShareLock);
        Expr       *result = NULL;
-       List       *and_args;
 
-       /* Do the work only if this relation is a partition. */
-       if (rel->rd_rel->relispartition)
+       /* Do the work only if this relation exists and is a partition. */
+       if (get_rel_relispartition(relid))
        {
+               Relation        rel = relation_open(relid, AccessShareLock);
+               List       *and_args;
+
                and_args = generate_partition_qual(rel);
 
+               /* Convert implicit-AND list format to boolean expression */
                if (and_args == NIL)
                        result = NULL;
                else if (list_length(and_args) > 1)
                        result = makeBoolExpr(AND_EXPR, and_args, -1);
                else
                        result = linitial(and_args);
-       }
 
-       /* Keep the lock. */
-       heap_close(rel, NoLock);
+               /* Keep the lock, to allow safe deparsing against the rel by caller. */
+               relation_close(rel, NoLock);
+       }
 
        return result;
 }
@@ -845,7 +852,6 @@ generate_partition_qual(Relation rel)
        MemoryContext oldcxt;
        Datum           boundDatum;
        bool            isnull;
-       PartitionBoundSpec *bound;
        List       *my_qual = NIL,
                           *result = NIL;
        Relation        parent;
@@ -859,8 +865,8 @@ generate_partition_qual(Relation rel)
                return copyObject(rel->rd_partcheck);
 
        /* Grab at least an AccessShareLock on the parent table */
-       parent = heap_open(get_partition_parent(RelationGetRelid(rel)),
-                                          AccessShareLock);
+       parent = relation_open(get_partition_parent(RelationGetRelid(rel)),
+                                                  AccessShareLock);
 
        /* Get pg_class.relpartbound */
        tuple = SearchSysCache1(RELOID, RelationGetRelid(rel));
@@ -871,13 +877,17 @@ generate_partition_qual(Relation rel)
        boundDatum = SysCacheGetAttr(RELOID, tuple,
                                                                 Anum_pg_class_relpartbound,
                                                                 &isnull);
-       if (isnull)
-               elog(ERROR, "null relpartbound for relation %u", RelationGetRelid(rel));
-       bound = castNode(PartitionBoundSpec,
-                                        stringToNode(TextDatumGetCString(boundDatum)));
-       ReleaseSysCache(tuple);
+       if (!isnull)
+       {
+               PartitionBoundSpec *bound;
+
+               bound = castNode(PartitionBoundSpec,
+                                                stringToNode(TextDatumGetCString(boundDatum)));
 
-       my_qual = get_qual_from_partbound(rel, parent, bound);
+               my_qual = get_qual_from_partbound(rel, parent, bound);
+       }
+
+       ReleaseSysCache(tuple);
 
        /* Add the parent's quals to the list (if any) */
        if (parent->rd_rel->relispartition)
@@ -903,7 +913,7 @@ generate_partition_qual(Relation rel)
        MemoryContextSwitchTo(oldcxt);
 
        /* Keep the parent locked until commit */
-       heap_close(parent, NoLock);
+       relation_close(parent, NoLock);
 
        return result;
 }

diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index e0eea2a0dc503334944f01afcf8c38022a13c395..8c57de77c0656ab45cde6234e0149f92502ca25b 100644 (file)
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -128,6 +128,7 @@ extern char *get_rel_name(Oid relid);
 extern Oid     get_rel_namespace(Oid relid);
 extern Oid     get_rel_type_id(Oid relid);
 extern char get_rel_relkind(Oid relid);
+extern bool get_rel_relispartition(Oid relid);
 extern Oid     get_rel_tablespace(Oid relid);
 extern char get_rel_persistence(Oid relid);
 extern Oid     get_transform_fromsql(Oid typid, Oid langid, List *trftypes);

diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index b9297c98d2932eea953a22e19d8dac479afc9b5c..8ab543ae317639cd46016aabbc25f67334a25efd 100644 (file)
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -75,6 +75,25 @@ Indexes:
     "idxpart1_a_idx" btree (a)
     "idxpart1_b_c_idx" btree (b, c)
 
+\d+ idxpart1_a_idx
+                 Index "public.idxpart1_a_idx"
+ Column |  Type   | Key? | Definition | Storage | Stats target 
+--------+---------+------+------------+---------+--------------
+ a      | integer | yes  | a          | plain   | 
+Partition of: idxparti 
+No partition constraint
+btree, for table "public.idxpart1"
+
+\d+ idxpart1_b_c_idx
+                Index "public.idxpart1_b_c_idx"
+ Column |  Type   | Key? | Definition | Storage  | Stats target 
+--------+---------+------+------------+----------+--------------
+ b      | integer | yes  | b          | plain    | 
+ c      | text    | yes  | c          | extended | 
+Partition of: idxparti2 
+No partition constraint
+btree, for table "public.idxpart1"
+
 drop table idxpart;
 -- If a partition already has an index, don't create a duplicative one
 create table idxpart (a int, b int) partition by range (a, b);

diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 2091a87ff5928cd8e3606feb0bd2fa9deb167773..48b885321941e6a9e88bf508cb01b66688767375 100644 (file)
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -44,6 +44,8 @@ create table idxpart1 (like idxpart);
 \d idxpart1
 alter table idxpart attach partition idxpart1 for values from (0) to (10);
 \d idxpart1
+\d+ idxpart1_a_idx
+\d+ idxpart1_b_c_idx
 drop table idxpart;
 
 -- If a partition already has an index, don't create a duplicative one