Обсуждение: list_free() in index_get_partition()

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

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;
 }

commit 0a01cb7561d6ec74aa5829040bd1478e7b113d89
Author: Justin Pryzby <pryzbyj@telsasoft.com>
Date:   Thu Nov 5 12:14:24 2020 -0600

    index_get_partition(): use lsyscache ??
    
    The old code failed when !HeapTupleIsValid(), the new code will return false -
    is that ok ??  Two of the existing callers error anyway.

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)
         {

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

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.