pointless check in RelationBuildPartitionDesc
I noticed this strange hack in RelationBuildPartitionDesc:
/*
* 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;
}
After looking, it seems that this is just self-inflicted pain: for some
reason, we store the pg_inherits row for a partition, and immediately
afterwards compute and store its partition bound, which requires the
above hack. But if we do things in the opposite order, this is no
longer needed. I propose to remove it, as in the attached patch.
--
�lvaro Herrera Developer, https://www.PostgreSQL.org/
Attachments:
ispartition.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f46af41b56..089e9f1b20 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -773,9 +773,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
InvalidOid,
typaddress);
- /* Store inheritance information for new rel. */
- StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
-
/*
* 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 ownerId,
heap_close(parent, NoLock);
}
+ /* Store inheritance information for new rel. */
+ StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
+
/*
* Process the partitioning specification (if any) and store the partition
* key information into the catalog.
diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/partcache.c
index 115a9fe78f..e3cd65b33a 100644
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -302,19 +302,6 @@ RelationBuildPartitionDesc(Relation rel)
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u", inhrelid);
- /*
- * 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 = SysCacheGetAttr(RELOID, tuple,
Anum_pg_class_relpartbound,
&isnull);
On 2018/09/04 6:39, Alvaro Herrera wrote:
I noticed this strange hack in RelationBuildPartitionDesc:
/*
* 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;
}After looking, it seems that this is just self-inflicted pain: for some
reason, we store the pg_inherits row for a partition, and immediately
afterwards compute and store its partition bound, which requires the
above hack. But if we do things in the opposite order, this is no
longer needed. I propose to remove it, as in the attached patch.
+1. I remember having facepalmed at this before and had also written a
patch but never got around to submitting it.
Thanks,
Amit
On Tue, Sep 04, 2018 at 09:47:07AM +0900, Amit Langote wrote:
On 2018/09/04 6:39, Alvaro Herrera wrote:
After looking, it seems that this is just self-inflicted pain: for some
reason, we store the pg_inherits row for a partition, and immediately
afterwards compute and store its partition bound, which requires the
above hack. But if we do things in the opposite order, this is no
longer needed. I propose to remove it, as in the attached patch.+1. I remember having facepalmed at this before and had also written a
patch but never got around to submitting it.
Ok, I see. It seems to me that this could be replaced by an
elog(ERROR), as relispartition ought to be set anyway. This way any
future callers would get things done in the correct order.
--
Michael
On 2018/09/04 10:19, Michael Paquier wrote:
On Tue, Sep 04, 2018 at 09:47:07AM +0900, Amit Langote wrote:
On 2018/09/04 6:39, Alvaro Herrera wrote:
After looking, it seems that this is just self-inflicted pain: for some
reason, we store the pg_inherits row for a partition, and immediately
afterwards compute and store its partition bound, which requires the
above hack. But if we do things in the opposite order, this is no
longer needed. I propose to remove it, as in the attached patch.+1. I remember having facepalmed at this before and had also written a
patch but never got around to submitting it.Ok, I see. It seems to me that this could be replaced by an
elog(ERROR), as relispartition ought to be set anyway. This way any
future callers would get things done in the correct order.
Converting it to elog(ERROR, ...) might be a good idea.
Thanks,
Amit
On 2018-Sep-04, Amit Langote wrote:
On 2018/09/04 10:19, Michael Paquier wrote:
On Tue, Sep 04, 2018 at 09:47:07AM +0900, Amit Langote wrote:
On 2018/09/04 6:39, Alvaro Herrera wrote:
After looking, it seems that this is just self-inflicted pain: for some
reason, we store the pg_inherits row for a partition, and immediately
afterwards compute and store its partition bound, which requires the
above hack. But if we do things in the opposite order, this is no
longer needed. I propose to remove it, as in the attached patch.+1. I remember having facepalmed at this before and had also written a
patch but never got around to submitting it.Ok, I see. It seems to me that this could be replaced by an
elog(ERROR), as relispartition ought to be set anyway. This way any
future callers would get things done in the correct order.Converting it to elog(ERROR, ...) might be a good idea.
I think it'd be pointless noise. If we really want to protect against
that, I think we should promote the Assert for relpartbound's isnull
flag into an if test.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/09/04 13:08, Alvaro Herrera wrote:
On 2018-Sep-04, Amit Langote wrote:
On 2018/09/04 10:19, Michael Paquier wrote:
On Tue, Sep 04, 2018 at 09:47:07AM +0900, Amit Langote wrote:
On 2018/09/04 6:39, Alvaro Herrera wrote:
After looking, it seems that this is just self-inflicted pain: for some
reason, we store the pg_inherits row for a partition, and immediately
afterwards compute and store its partition bound, which requires the
above hack. But if we do things in the opposite order, this is no
longer needed. I propose to remove it, as in the attached patch.+1. I remember having facepalmed at this before and had also written a
patch but never got around to submitting it.Ok, I see. It seems to me that this could be replaced by an
elog(ERROR), as relispartition ought to be set anyway. This way any
future callers would get things done in the correct order.Converting it to elog(ERROR, ...) might be a good idea.
I think it'd be pointless noise. If we really want to protect against
that, I think we should promote the Assert for relpartbound's isnull
flag into an if test.
So that we can elog(ERROR, ...) if isnull is true? If so, I agree,
because that's what should've been done here anyway (for a value read from
the disk that is). I think we should check relispartition then too.
Thanks,
Amit
On 2018-Sep-04, Amit Langote wrote:
On 2018/09/04 13:08, Alvaro Herrera wrote:
I think it'd be pointless noise. If we really want to protect against
that, I think we should promote the Assert for relpartbound's isnull
flag into an if test.So that we can elog(ERROR, ...) if isnull is true? If so, I agree,
because that's what should've been done here anyway (for a value read from
the disk that is).
Yes.
I wonder now what's the value of using an Assert for this, BTW: if those
are enabled, then we'll crash saying "this column is null and shouldn't!".
If they are disabled, we'll instead crash (possibly in production)
trying to decode the field. What was achieved?
With an elog(ERROR) the reaction is the expected one: the current
transaction is aborted, but the server continues to run.
I think we should check relispartition then too.
Well, we don't check every single catalog flag in every possible code
path just to ensure it's sane. I don't see any reason to do differently
here. We rely heavily on the catalog contents to be correct, and
install defenses against user-induced corruption that would cause us to
crash; but going much further than that would be excessive IMO.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
Attachments:
ispartition.patchtext/plain; charset=us-asciiDownload
commit dc5f9933595c95471898e506e004a1799ea4eae5[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/tablecmds.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 ownerId,
InvalidOid,
typaddress);
- /* Store inheritance information for new rel. */
- StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
-
/*
* 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 ownerId,
heap_close(parent, NoLock);
}
+ /* Store inheritance information for new rel. */
+ StoreCatalogInheritance(relationId, inheritOids, stmt->partbound != NULL);
+
/*
* Process the partitioning specification (if any) and store the partition
* key information into the catalog.
@@ -14705,7 +14705,9 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
(void) SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relpartbound,
&isnull);
- Assert(!isnull);
+ if (isnull)
+ elog(ERROR, "null relpartbound for relation %u",
+ RelationGetRelid(partRel));
/* Clear relpartbound and reset relispartition */
memset(new_val, 0, sizeof(new_val));
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/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, PartitionBoundSpec *spec,
datum = SysCacheGetAttr(RELOID, tuple,
Anum_pg_class_relpartbound,
&isnull);
+ if (isnull)
+ elog(ERROR, "null relpartbound for relation %u", inhrelid);
- Assert(!isnull);
bspec = (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);
- /*
- * 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 = SysCacheGetAttr(RELOID, tuple,
Anum_pg_class_relpartbound,
&isnull);
- Assert(!isnull);
+ if (isnull)
+ elog(ERROR, "null relpartbound for partition %u" inhrelid);
boundspec = (Node *) stringToNode(TextDatumGetCString(datum));
/*
@@ -883,9 +871,8 @@ generate_partition_qual(Relation rel)
boundDatum = SysCacheGetAttr(RELOID, tuple,
Anum_pg_class_relpartbound,
&isnull);
- if (isnull) /* should not happen */
- elog(ERROR, "relation \"%s\" has relpartbound = null",
- RelationGetRelationName(rel));
+ if (isnull)
+ elog(ERROR, "null relpartbound for relation %u", RelationGetRelid(rel));
bound = castNode(PartitionBoundSpec,
stringToNode(TextDatumGetCString(boundDatum)));
ReleaseSysCache(tuple);
On 2018/09/04 21:51, Alvaro Herrera wrote:
On 2018-Sep-04, Amit Langote wrote:
On 2018/09/04 13:08, Alvaro Herrera wrote:
I think it'd be pointless noise. If we really want to protect against
that, I think we should promote the Assert for relpartbound's isnull
flag into an if test.So that we can elog(ERROR, ...) if isnull is true? If so, I agree,
because that's what should've been done here anyway (for a value read from
the disk that is).Yes.
I wonder now what's the value of using an Assert for this, BTW: if those
are enabled, then we'll crash saying "this column is null and shouldn't!".
If they are disabled, we'll instead crash (possibly in production)
trying to decode the field. What was achieved?
I can see how that the Assert was pointless all along.
With an elog(ERROR) the reaction is the expected one: the current
transaction is aborted, but the server continues to run.I think we should check relispartition then too.
Well, we don't check every single catalog flag in every possible code
path just to ensure it's sane. I don't see any reason to do differently
here. We rely heavily on the catalog contents to be correct, and
install defenses against user-induced corruption that would cause us to
crash; but going much further than that would be excessive IMO.
Okay, sure anyway.
Thanks,
Amit
On 2018/09/05 1:50, Alvaro Herrera wrote:
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).
Looks good. Thanks for taking care of other sites as well.
@@ -14705,7 +14705,9 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
(void) SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relpartbound,
&isnull);
- Assert(!isnull);
+ if (isnull)
+ elog(ERROR, "null relpartbound for relation %u",
+ RelationGetRelid(partRel));In retrospect, I'm not sure why this piece of code is here at all; maybe
just remove the SycCacheGetAttr and Assert?
Regards,
Amit
On 2018-Sep-05, Amit Langote wrote:
On 2018/09/05 1:50, Alvaro Herrera wrote:
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).Looks good. Thanks for taking care of other sites as well.
@@ -14705,7 +14705,9 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
(void) SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relpartbound, &isnull); - Assert(!isnull); + if (isnull) + elog(ERROR, "null relpartbound for relation %u", + RelationGetRelid(partRel));In retrospect, I'm not sure why this piece of code is here at all; maybe
just remove the SycCacheGetAttr and Assert?
Yeah, good idea, will do.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services