Re: hyrax vs. RelationBuildPartitionDesc

Hi,

On 2019/04/10 15:42, Michael Paquier wrote:
> On Mon, Apr 08, 2019 at 10:40:41AM -0400, Robert Haas wrote:
>> On Mon, Apr 8, 2019 at 9:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>>> Yeah, it's an open issue IMO.  I think we've been focusing on getting
>>> as many feature patches done as we could during the CF, but now it's
>>> time to start mopping up problems like this one.
> 
> Please note that it is registered as an older bug and not an open
> item.

The problem lies in all branches that have partitioning, so it should be
listed under Older Bugs, right?  You may have noticed that I posted
patches for all branches down to 10.

>> Do you have any further thoughts based on my last response?
> 
> So your last response is that:
> https://www.postgresql.org/message-id/CA+Tgmoa5rT+ZR+Vv+q1XLwQtDMCqkNL6B4PjR4V6YAC9K_LBxw@mail.gmail.com
> And what are you proposing as patch?  Perhaps something among those
> lines?
> https://www.postgresql.org/message-id/036852f2-ba7f-7a1f-21c6-00bc3515eda3@lab.ntt.co.jp

AFAIK, the patch there isn't meant to solve problems discussed at the 1st
link.  It's meant to fix poor cache memory management of partition
constraint expression trees, which seems to be a separate issue from the
PartitionDesc memory management issue (the latter is the main topic of
this thread.)  Problem with partition constraint expression trees was only
mentioned in passing in this thread [1], although it had also come up in
the past, as I said when posting the patch.

>> Does anyone else wish to offer opinions?
> 
> It seems to me that Tom's argument to push in the way relcache
> information is handled by copying its contents sounds sensible to me.
> That's not perfect, but it is consistent with what exists (note
> PublicationActions for a rather-still-not-much-complex example of
> structure which gets copied from the relcache).

I gave my vote for direct access of unchangeable relcache substructures
(TupleDesc, PartitionDesc, etc.), because they're accessed during the
planning of every user query and fairly expensive to copy compared to list
of indexes or PublicationActions that you're citing.  To further my point
a bit, I wonder why PublicationActions needs to be copied out of relcache
at all?  Looking at its usage in execReplication.c, it seems we can do
fine without copying, because we are holding both a lock and a relcache
reference on the replication target relation during the entirety of
apply_handle_insert(), which means that information can't change under us,
neither logically nor physically.

Also, to reiterate what I think was one of Robert's points upthread [2],
the reason for requiring some code to copy the relcache substructures out
of relcache should be that the caller might want change its content; for
example, planner or its hooks may want to add/remove an index to/from the
list of indexes copied from the relcache.  The reason for copying should
not be that relcache content may change under us despite holding a lock
and relcache reference.

Thanks,
Amit

[1] https://www.postgresql.org/message-id/7961.1552498252%40sss.pgh.pa.us

[2]
https://www.postgresql.org/message-id/CA%2BTgmoa5rT%2BZR%2BVv%2Bq1XLwQtDMCqkNL6B4PjR4V6YAC9K_LBxw%40mail.gmail.com