Horton

Thread

Commits

  1. Protect against hypothetical memory leaks in RelationGetPartitionKey

  2. Get rid of copy_partition_key

  1. pgsql: Get rid of copy_partition_key

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-12-21T17:43:14Z

    Get rid of copy_partition_key

That function currently exists to avoid leaking memory in
CacheMemoryContext in case of trouble while the partition key is being
built, but there's a better way: allocate everything in a memcxt that
goes away if the current (sub)transaction fails, and once the partition
key is built and no further errors can occur, make the memcxt permanent
by making it a child of CacheMemoryContext.

Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/20171027172730.eh2domlkpn4ja62m@alvherre.pgsql

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/8a0596cb656e357c391cccf12854beb2e05f3901

Modified Files
--------------
src/backend/utils/cache/relcache.c | 71 +++++---------------------------------
1 file changed, 9 insertions(+), 62 deletions(-)

  2. Re: pgsql: Get rid of copy_partition_key

    Robert Haas <robertmhaas@gmail.com> — 2017-12-21T21:22:40Z

    On Thu, Dec 21, 2017 at 12:43 PM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:
> Get rid of copy_partition_key
>
> That function currently exists to avoid leaking memory in
> CacheMemoryContext in case of trouble while the partition key is being
> built, but there's a better way: allocate everything in a memcxt that
> goes away if the current (sub)transaction fails, and once the partition
> key is built and no further errors can occur, make the memcxt permanent
> by making it a child of CacheMemoryContext.

I'm not saying this commit is wrong, but it's a weaker form of
protection than we had before.  For instance, suppose that while
building the partition key we do a bunch of random palloc's that are
only used transiently when constructing the key.  In the old coding,
those go into the temp context and get blown up immediately; only what
we copy to the new context ends up there.  In the new way, they stick
around for as long as the relcache entry does.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  3. Re: pgsql: Get rid of copy_partition_key

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-12-21T21:36:57Z

    Robert Haas wrote:
> On Thu, Dec 21, 2017 at 12:43 PM, Alvaro Herrera
> <alvherre@alvh.no-ip.org> wrote:
> > Get rid of copy_partition_key
> >
> > That function currently exists to avoid leaking memory in
> > CacheMemoryContext in case of trouble while the partition key is being
> > built, but there's a better way: allocate everything in a memcxt that
> > goes away if the current (sub)transaction fails, and once the partition
> > key is built and no further errors can occur, make the memcxt permanent
> > by making it a child of CacheMemoryContext.
> 
> I'm not saying this commit is wrong, but it's a weaker form of
> protection than we had before.  For instance, suppose that while
> building the partition key we do a bunch of random palloc's that are
> only used transiently when constructing the key.  In the old coding,
> those go into the temp context and get blown up immediately; only what
> we copy to the new context ends up there.  In the new way, they stick
> around for as long as the relcache entry does.

True.  We're careful to avoid leaks, so I'm not terribly worried TBH.
Example: in the original coding we were pfree'ing exprString, even
though that was mostly pointless since (I think) it would just be freed
with some short-lived context deletion shortly afterwards.

Compare RelationBuildRuleLock.  It doesn't have the problem you describe
because it uses MemoryContextAlloc instead of palloc for the things it
wants to persist.  Maybe we should do that in RelationBuildPartitionKey
too.  Best of both worlds I think.

(At the same time, we should copy in the other direction the idea
of reparenting a context initially made temporary.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: pgsql: Get rid of copy_partition_key

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-12-22T16:24:19Z

    I believe this patch (which also fixes a comment I neglected to fix in
the previous one) should satisfy your concerns.

It's still running a few relevant tests (create_function_1 create_type
point polygon circle create_table copy create_misc create_index
alter_table partition_join partition_prune hash_part) under
CLOBBER_FREED_MEMORY + CLOBBER_CACHE_ALWAYS, but so far it's looking
good.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  5. Re: pgsql: Get rid of copy_partition_key

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-12-22T16:47:56Z

    Seems I misremembered the whole opfuncid getting reset thing (it applies
to reading a node from string, not copying) and it was undone by
9f1255ac8593 anyway.  I don't think it makes much of a difference, but I
mention this in case you're wondering why I changed the fix_opfuncids()
call.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: pgsql: Get rid of copy_partition_key

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-12-27T21:31:12Z

    Pushed, thanks.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services