pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpointless check in RelationBuildPartitionDesc
11+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2018-09-05 2fbdf1b38b 52ab02d593 Simplify partitioned table creation vs. relcache
* pointless check in RelationBuildPartitionDesc
@ 2018-09-03 21:39 Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2018-09-03 21:39 UTC (permalink / raw)
To: pgsql-hackers
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.patch (text/plain, 1.8K) download
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);
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2018-09-04 00:47 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Amit Langote @ 2018-09-04 00:47 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
@ 2018-09-04 01:19 ` Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2018-09-04 01:19 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
@ 2018-09-04 01:23 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 04:08 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 11+ messages in thread
From: Amit Langote @ 2018-09-04 01:23 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
@ 2018-09-04 04:08 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 06:19 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2018-09-04 04:08 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 04:08 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
@ 2018-09-04 06:19 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 12:51 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 11+ messages in thread
From: Amit Langote @ 2018-09-04 06:19 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 04:08 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 06:19 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
@ 2018-09-04 12:51 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 16:50 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-05 00:45 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 2 replies; 11+ messages in thread
From: Alvaro Herrera @ 2018-09-04 12:51 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 04:08 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 06:19 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 12:51 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
@ 2018-09-04 16:50 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-05 00:53 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
1 sibling, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2018-09-04 16:50 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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.patch (text/plain, 3.7K) download
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);
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 04:08 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 06:19 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 12:51 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 16:50 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
@ 2018-09-05 00:53 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-05 13:05 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 11+ messages in thread
From: Amit Langote @ 2018-09-05 00:53 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 04:08 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 06:19 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 12:51 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 16:50 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-05 00:53 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
@ 2018-09-05 13:05 ` Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 0 replies; 11+ messages in thread
From: Alvaro Herrera @ 2018-09-05 13:05 UTC (permalink / raw)
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: pointless check in RelationBuildPartitionDesc
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Re: pointless check in RelationBuildPartitionDesc Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 04:08 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 06:19 ` Re: pointless check in RelationBuildPartitionDesc Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 12:51 ` Re: pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@2ndquadrant.com>
@ 2018-09-05 00:45 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
1 sibling, 0 replies; 11+ messages in thread
From: Amit Langote @ 2018-09-05 00:45 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2018-09-05 13:05 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-09-03 21:39 pointless check in RelationBuildPartitionDesc Alvaro Herrera <alvherre@alvh.no-ip.org>
2018-09-04 00:47 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 01:19 ` Michael Paquier <michael@paquier.xyz>
2018-09-04 01:23 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 04:08 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 06:19 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-04 12:51 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-04 16:50 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-05 00:53 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2018-09-05 13:05 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2018-09-05 00:45 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox