pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedlist_free() in index_get_partition()
3+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2020-11-07 8ad6a0c1bb d94d37f8c0 623644f02c Plug memory leak in index_get_partition
* list_free() in index_get_partition()
@ 2020-11-05 20:36 Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 3+ messages in thread
From: Justin Pryzby @ 2020-11-05 20:36 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers
Seems to be missing.
The 2nd patch does some more cleanup - Before, a failed syscache lookup would
ERROR, but I don't think that's supposed to happen. get_rel_relispartition()
would instead return false, and we won't call get_partition_parent().
commit a8ad949f22b8dd7b23049b0b0704e5be9233e319
Author: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu Nov 5 12:06:49 2020 -0600
list_free() in index_get_partition()
which was added at: a6da0047158b8a227f883aeed19eb7fcfbef11fb
inline patch 1: src/backend/catalog/partition.c (+2/-1 lines) download inline-1.patch
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 239ac017fa..4dfac39adf 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -153,44 +153,45 @@ Oid
index_get_partition(Relation partition, Oid indexId)
{
List *idxlist = RelationGetIndexList(partition);
ListCell *l;
foreach(l, idxlist)
{
Oid partIdx = lfirst_oid(l);
HeapTuple tup;
Form_pg_class classForm;
bool ispartition;
tup = SearchSysCache1(RELOID, ObjectIdGetDatum(partIdx));
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for relation %u", partIdx);
classForm = (Form_pg_class) GETSTRUCT(tup);
ispartition = classForm->relispartition;
ReleaseSysCache(tup);
if (!ispartition)
continue;
- if (get_partition_parent(lfirst_oid(l)) == indexId)
+ if (get_partition_parent(partIdx) == indexId)
{
list_free(idxlist);
return partIdx;
}
}
+ list_free(idxlist);
return InvalidOid;
}
inline patch 2: src/backend/catalog/partition.c (+3/-11 lines) download inline-2.patch
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 4dfac39adf..3d78bc0872 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -28,6 +28,7 @@
#include "partitioning/partbounds.h"
#include "rewrite/rewriteManip.h"
#include "utils/fmgroids.h"
+#include "utils/lsyscache.h"
#include "utils/partcache.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -158,17 +159,8 @@ index_get_partition(Relation partition, Oid indexId)
foreach(l, idxlist)
{
Oid partIdx = lfirst_oid(l);
- HeapTuple tup;
- Form_pg_class classForm;
- bool ispartition;
-
- tup = SearchSysCache1(RELOID, ObjectIdGetDatum(partIdx));
- if (!HeapTupleIsValid(tup))
- elog(ERROR, "cache lookup failed for relation %u", partIdx);
- classForm = (Form_pg_class) GETSTRUCT(tup);
- ispartition = classForm->relispartition;
- ReleaseSysCache(tup);
- if (!ispartition)
+
+ if (!get_rel_relispartition(partIdx))
continue;
if (get_partition_parent(partIdx) == indexId)
{
0001-Fixes-to-index_get_partition.patch (text/x-diff, 992B) download
From 90e155f660314ec70de14177241dd1ec029d9311 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 5 Nov 2020 12:06:49 -0600
Subject: [PATCH 1/2] Fixes to index_get_partition()
which was added at: a6da0047158b8a227f883aeed19eb7fcfbef11fb
free list in cases where there's no parent
use lsyscache: get_rel_relispartition()
---
src/backend/catalog/partition.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 239ac017fa..4dfac39adf 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -170,13 +170,14 @@ index_get_partition(Relation partition, Oid indexId)
ReleaseSysCache(tup);
if (!ispartition)
continue;
- if (get_partition_parent(lfirst_oid(l)) == indexId)
+ if (get_partition_parent(partIdx) == indexId)
{
list_free(idxlist);
return partIdx;
}
}
+ list_free(idxlist);
return InvalidOid;
}
--
2.17.0
0002-index_get_partition-use-lsyscache.patch (text/x-diff, 1.4K) download
From 768f91199b5e197759b2ead6d838a72974dca6b6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 5 Nov 2020 12:14:24 -0600
Subject: [PATCH 2/2] index_get_partition(): use lsyscache ??
The old code failed when !HeapTupleIsValid(), the new code will return false -
is that ok ??
---
src/backend/catalog/partition.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 4dfac39adf..3d78bc0872 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -28,6 +28,7 @@
#include "partitioning/partbounds.h"
#include "rewrite/rewriteManip.h"
#include "utils/fmgroids.h"
+#include "utils/lsyscache.h"
#include "utils/partcache.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -158,17 +159,8 @@ index_get_partition(Relation partition, Oid indexId)
foreach(l, idxlist)
{
Oid partIdx = lfirst_oid(l);
- HeapTuple tup;
- Form_pg_class classForm;
- bool ispartition;
-
- tup = SearchSysCache1(RELOID, ObjectIdGetDatum(partIdx));
- if (!HeapTupleIsValid(tup))
- elog(ERROR, "cache lookup failed for relation %u", partIdx);
- classForm = (Form_pg_class) GETSTRUCT(tup);
- ispartition = classForm->relispartition;
- ReleaseSysCache(tup);
- if (!ispartition)
+
+ if (!get_rel_relispartition(partIdx))
continue;
if (get_partition_parent(partIdx) == indexId)
{
--
2.17.0
* Re: list_free() in index_get_partition()
@ 2020-11-06 00:32 Michael Paquier <michael@paquier.xyz>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 3+ messages in thread
From: Michael Paquier @ 2020-11-06 00:32 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers
On Thu, Nov 05, 2020 at 02:36:06PM -0600, Justin Pryzby wrote:
> Seems to be missing.
Any code paths calling index_get_partition() is in tablecmds.c,
involving commands that are not that hot with such lookups, but that
could be an issue if this gets called by some out-of-core code if
memory context cleanup is sloppy. So I agree to fix this one and
backpatch down to 12. I'd like to apply your fix, except if Alvaro
thinks differently as that's his commit originally. So let's wait a
bit first.
> The 2nd patch does some more cleanup - Before, a failed syscache lookup would
> ERROR, but I don't think that's supposed to happen. get_rel_relispartition()
> would instead return false, and we won't call get_partition_parent().
The cache lookup error is here as a safeguard to remind that any code
path calling index_get_partition() needs a proper lock on the
relations involved before doing such lookups, so that's a defensive
move. By switching the code as you do in 0002, you could mask such
logical problems as different errors could get raised. So I don't
think that it is a good idea.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: list_free() in index_get_partition()
@ 2020-11-06 23:40 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 3+ messages in thread
From: Alvaro Herrera @ 2020-11-06 23:40 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers
On 2020-Nov-06, Michael Paquier wrote:
> On Thu, Nov 05, 2020 at 02:36:06PM -0600, Justin Pryzby wrote:
> > Seems to be missing.
>
> Any code paths calling index_get_partition() is in tablecmds.c,
> involving commands that are not that hot with such lookups, but that
> could be an issue if this gets called by some out-of-core code if
> memory context cleanup is sloppy. So I agree to fix this one and
> backpatch down to 12. I'd like to apply your fix, except if Alvaro
> thinks differently as that's his commit originally. So let's wait a
> bit first.
Agreed; I'll get this pushed now. Thanks for reporting.
> > The 2nd patch does some more cleanup - Before, a failed syscache lookup would
> > ERROR, but I don't think that's supposed to happen. get_rel_relispartition()
> > would instead return false, and we won't call get_partition_parent().
>
> The cache lookup error is here as a safeguard to remind that any code
> path calling index_get_partition() needs a proper lock on the
> relations involved before doing such lookups, so that's a defensive
> move. By switching the code as you do in 0002, you could mask such
> logical problems as different errors could get raised. So I don't
> think that it is a good idea.
Yeah, I'm not so sure either. I have memories of purposefully not using
get_rel_relispartition there, but I don't remember exactly why and
it's not in the archives.
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2020-11-06 23:40 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05 20:36 list_free() in index_get_partition() Justin Pryzby <pryzby@telsasoft.com>
2020-11-06 00:32 ` Michael Paquier <michael@paquier.xyz>
2020-11-06 23:40 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
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