On Wed, Dec 18, 2024 at 12:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michel Pelletier <pelletier.michel@gmail.com> writes:
> My bad, sorry for the long confusing email, I figured out that I was
> calling the wrong macro when getting my matrix datum and inadvertently
> expanding RO pointers as well, I've fixed that issue, and everything is
> working great!  No extra expansions and my support functions are working
> well, I need to go through a few more places in the API to add more support
> but otherwise the fixes Tom has put into plpgsql have worked perfectly and
> the library now appears to be behaving optimally!  I can get down to doing
> some benchmarks and head-to-head with the C and Python bindings to compare
> against.

So, just to clarify where we're at: you are satisfied that the current
patch-set does what you need?

I have some updates on this thread based on some graph algorithms I've ported from the Python/C graphblas libraries.

All of the plpgsql expanded object optimizations so far are working well, I can minimize object expansion in most cases, there are a couple I haven't been able to work around but I'm still getting excellent benchmarking numbers on some large test graphs:

                LiveJournal         Orkut
Nodes           3,997,962           3,072,441
Edges           34,681,185          117,185,037
Triangles       177,820,130         627,583,972

                Seconds Edges/S     Seconds Edges/S
Tri Count LL    2.80s   12,386,138  32.03s  3,658,602
Tri Count LU    1.91s   18,157,688  16.38s  7,156,338
Tri Centrality  1.55s   22,374,958  12.22s  9,589,610
Page Rank       8.10s   4,281,628   23.14s  5,064,176

That's on a 2020 era 4 core economy laptop and is in line with what the C/Python/Julia bindings get on similar hardware.

There are a few cases where I have to force an expansion, I work around this by calling a `wait()` function, which expands the datum, calls GrB_wait() on it (a nop in this case) and returns a r/w pointer.  You can see this in the following Triangle Counting function which is a matrix multiplication of a graph to itself, using itself as a mask.  This matrix reduces to the triangle count (times six):

create or replace function tcount_b(graph matrix) returns bigint language plpgsql as
    $$
    begin
        graph = wait(graph);
        graph = mxm(graph, graph, 'plus_pair_int32', mask=>graph, descr=>'s');
        return reduce_scalar(graph) / 6;
    end;
    $$;

DEBUG:  new_matrix
DEBUG:  flatten_matrix
DEBUG:  matrix_wait
DEBUG:  expand_matrix  -- expansion happens here in wait()
DEBUG:  new_matrix
DEBUG:  matrix_mxm      -- mxm does not re-expand the object, good!
DEBUG:  expand_semiring
DEBUG:  new_semiring
DEBUG:  new_matrix
DEBUG:  expand_descriptor
DEBUG:  new_descriptor
DEBUG:  matrix_reduce_scalar  -- neither does reduce, good!
DEBUG:  new_scalar
DEBUG:  scalar_div_int32
DEBUG:  new_scalar
DEBUG:  cast_scalar_int64

If I take out the call to wait(), then mxm calls expand_matrix 3 times as it did before your optimizations.

The other task we'd talked about was generalizing the existing
heuristics in exec_assign_value() and plpgsql_exec_function() that
say that array-type values should be forced into expanded R/W form
when being assigned to an array-type PL/pgSQL variable.  The argument
for that is that the PL/pgSQL function might subsequently do a lot of
subscripted accesses to the array (which'd benefit from working with
an expanded array) while never doing another assignment and thus not
having any opportunity to revisit the decision.  The counter-argument
is that it might *not* do such accesses, so that the expansion was
just a waste of cycles.  So this is squishy enough that I'd prefer to
have some solid use-cases to look at before trying to generalize it.

It's sounding to me like you're going to end up in a place where all
your values are passed around in expanded form already and so you have
little need for that optimization.
  If so, I'd prefer not to go any
further than the present patch-set for now.  Adding "type support"
hooks as discussed would be a substantial amount of work, so I'd
like to have a more compelling case for it before doing that.

I agree it makes sense to have more use cases before making deeper changes.  I only work with expanded forms,  but need to call wait() to pre-expand the object to avoid multiple expansions in functions that can take the same object in multiple parameters.  This is a pretty common pattern in GraphBLAS (and linear algebra in general) where (many) matrices are commutable to themselves in several ways like multiplication, element-wise operations, and element masking.

I'm not sure if eliminating wait() is a good enough use case, it would definitely be nice to get rid of but I can document it pretty thoroughly and it's relatively easy to catch. 
 

-Michel