Horton

Re: ResourceOwner refactoring

Aleksander Alekseev <aleksander@timescale.com>

View thread View on postgr.es
From: Aleksander Alekseev <aleksander@timescale.com>
To: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Date: 2021-09-08T10:19:19Z
Lists: pgsql-hackers

Attachments

Hi Heikki,

> Yeah, needed some manual fixing, but here you go.

Thanks for working on this!

v8-0002 didn't apply to the current master, so I rebased it. See
attached v9-* patches. I also included v9-0004 with some minor tweaks
from me. I have several notes regarding the code.

1. Not sure if I understand this code of ResourceOwnerReleaseAll():
```
    /* First handle all the entries in the array. */
    do
    {
        found = false;
        for (int i = 0; i < owner->narr; i++)
        {
            if (owner->arr[i].kind->phase == phase)
            {
                Datum        value = owner->arr[i].item;
                ResourceOwnerFuncs *kind = owner->arr[i].kind;

                if (printLeakWarnings)
                    kind->PrintLeakWarning(value);
                kind->ReleaseResource(value);
                found = true;
            }
        }

        /*
         * If any resources were released, check again because some of the
         * elements might have been moved by the callbacks. We don't want to
         * miss them.
         */
    } while (found && owner->narr > 0);
```

Shouldn't we mark the resource as released and/or decrease narr and/or
save the last processed i? Why this will not call ReleaseResource()
endlessly on the same resource (array item)? Same question for the
following code that iterates over the hash table.

2. Just an idea/observation. It's possible that the performance of
ResourceOwnerEnlarge() can be slightly improved. Since the size of the
hash table is always a power of 2 and the code always doubles the size
of the hash table, (idx & mask) value will get one extra bit, which
can be 0 or 1. If it's 0, the value is already in its place,
otherwise, it should be moved on the known distance. In other words,
it's possible to copy `oldhash` to `newhash` and then move only half
of the items. I don't claim that this code necessarily should be
faster, or that this should be checked in the scope of this work.

-- 
Best regards,
Aleksander Alekseev

Commits

  1. Make RelationFlushRelation() work without ResourceOwner during abort

  2. Fix bug in bulk extending temp relation after failure

  3. Add missing PGDLLIMPORT markings

  4. Add test_dsa module.

  5. Clear CurrentResourceOwner earlier in CommitTransaction.

  6. Fix dsa.c with different resource owners.

  7. Fix bug in the new ResourceOwner implementation.

  8. Change pgcrypto to use the new ResourceOwner mechanism.

  9. Use a faster hash function in resource owners.

  10. Make ResourceOwners more easily extensible.

  11. Move a few ResourceOwnerEnlarge() calls for safety and clarity.