Re: ResourceOwner refactoring

On 08/09/2021 13:19, Aleksander Alekseev wrote:
> 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.

Thanks!

Attached is a newly rebased version. It includes your tweaks, and a few 
more comment and indentation tweaks.

> 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.

ReleaseResource() must call ResourceOwnerForget(), which removes the 
item from the or hash table. This works the same as the code in 'master':

> 		/*
> 		 * Release buffer pins.  Note that ReleaseBuffer will remove the
> 		 * buffer entry from our array, so we just have to iterate till there
> 		 * are none.
> 		 *
> 		 * During a commit, there shouldn't be any remaining pins --- that
> 		 * would indicate failure to clean up the executor correctly --- so
> 		 * issue warnings.  In the abort case, just clean up quietly.
> 		 */
> 		while (ResourceArrayGetAny(&(owner->bufferarr), &foundres))
> 		{
> 			Buffer		res = DatumGetBuffer(foundres);
> 
> 			if (isCommit)
> 				PrintBufferLeakWarning(res);
> 			ReleaseBuffer(res);
> 		}

That comment explains how it works. I added a comment like that in this 
patch, too.

> 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.

Hmm, the hash table uses open addressing, I think we want to also 
rearrange any existing entries that might not be at their right place 
because of collisions. We don't actually do that currently when an 
element is removed (even before this patch), but enlarging the array is 
a good opportunity for it. In any case, I haven't seen 
ResourceOwnerEnlarge() show up while profiling, so I'm going to leave it 
as it is.

- Heikki