Asymmetry in opening and closing indices for partition routing
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t42567psql -h localhost -U postgresBuilt from patchset v3 (message #3), October 23, 2026 at 01:04 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t42567_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t42567_3 && git checkout t42567_3Patchset v3 (message #3) is on t42567_3
Hi All,
ExecInitPartitionInfo() has code
536 /*
537 * Open partition indices. The user may have asked to check for
conflicts
538 * within this leaf partition and do "nothing" instead of throwing
an
539 * error. Be prepared in that case by initializing the index
information
540 * needed by ExecInsert() to perform speculative insertions.
541 */
542 if (partrel->rd_rel->relhasindex &&
543 leaf_part_rri->ri_IndexRelationDescs == NULL)
544 ExecOpenIndices(leaf_part_rri,
545 (node != NULL &&
546 node->onConflictAction != ONCONFLICT_NONE));
This calls ExecOpenIndices only when ri_indexRelationDescs has not been
initialized which is fine. I think this is done so that we don't open
indices again and again when multiple tuples are routed to the same
partition. But as part of opening indices, we also open corresponding index
relations using index_open.
The indices opened here are closed in ExecCleanupTupleRouting(), but it
does this unconditionally. This means that for any reason we had called
ExecOpenIndices on a partition before ExecInitPartitionInfo() following
things will happen
1. ExecOpenIndices will overwrite the old arrays for index descriptors
leaking memory.
2. ExecCleanupTupleRouting will close index relations that were opened
second time cleaning up memory. But the relcache references corresponding
to the first call to ExecOpenIndices will leak.
Similar situation can happen if ExecOpenIndices is called on a partition
after it has been called in ExecInitPartitionInfo.
I couldn't find code where this can happen but I don't see any code which
prevents this. This looks like a recipe for memory and reference leaks.
We could fix this by
1. Make ExecOpenIndices and ExecCloseIndices so that they can be called
multiple times on the same relation similar to heap_open. The second time
onwards ExecOpenIndices doesn't allocate memory and open indexes but
increases a refcount. ExecCloseIndices releases memory and closes the index
relations when the refcount drops to 0. Then we don't need to
check leaf_part_rri->ri_IndexRelationDescs == NULL in
ExecInitPartitionInfo().
2. Throw an error in ExecOpenIndices if all the arrays are present. We will
need to check leaf_part_rri->ri_IndexRelationDescs == NULL in
ExecInitPartitionInfo().
--
Best Wishes,
Ashutosh
Hi Ashutosh,
On 2020-Jun-22, Ashutosh Bapat wrote:
I couldn't find code where this can happen but I don't see any code which
prevents this. This looks like a recipe for memory and reference leaks.We could fix this by
1. Make ExecOpenIndices and ExecCloseIndices so that they can be called
multiple times on the same relation similar to heap_open. The second time
onwards ExecOpenIndices doesn't allocate memory and open indexes but
increases a refcount. ExecCloseIndices releases memory and closes the index
relations when the refcount drops to 0. Then we don't need to
check leaf_part_rri->ri_IndexRelationDescs == NULL in
ExecInitPartitionInfo().
I think there are a couple of places in executor related to partition
tuple routing where code is a bit weirdly structured. It might be nice
to improve on that if you either find inefficiencies that can be fixed,
or clear code structure improvements, as long as they don't make
performance worse. Feel free to have a look around and see if you can
propose some concrete proposals.
I'm not sure that expecting the relcache entry's refcount drops to zero
at the right time is a good approach; that may cause leaks some other
place might have refcounts you're not expecting (say, an open cursor
that's not fully read).
(I'm not terribly worried about refcount leakage as a theoretical
concern, since the ResourceOwner mechanism will warn us about that if it
happens.)
2. Throw an error in ExecOpenIndices if all the arrays are present. We will
need to check leaf_part_rri->ri_IndexRelationDescs == NULL in
ExecInitPartitionInfo().
This sounds like a job for an assert rather than an error.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, 22 Jun 2020 at 23:22, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
I'm not sure that expecting the relcache entry's refcount drops to zero
at the right time is a good approach; that may cause leaks some other
place might have refcounts you're not expecting (say, an open cursor
that's not fully read).
My proposal was to maintain a refcount counting the number of times an
index is opened in ResultRelInfo itself, not to rely on the relcache ref
count. But I think that would be an overkill. Please read ahead
(I'm not terribly worried about refcount leakage as a theoretical
concern, since the ResourceOwner mechanism will warn us about that if it
happens.)2. Throw an error in ExecOpenIndices if all the arrays are present. We
will
need to check leaf_part_rri->ri_IndexRelationDescs == NULL in
ExecInitPartitionInfo().This sounds like a job for an assert rather than an error.
I agree. Here's a patch to fix to add Assert'ion in ExecOpenIndices(). I
ran make check with this patch and the assertion didn't trip. I think this
will be a good step forward.
--
Best Wishes,
Ashutosh