Date: Fri, 6 Nov 2020 20:40:05 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@postgresql.org
Subject: Re: list_free() in index_get_partition()
Message-ID: <20201106234005.GA23227@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201106003235.GC21123@paquier.xyz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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.