Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Allow extension functions to participate in in-place updates.

  2. Implement new optimization rule for updates of expanded variables.

  3. Detect whether plpgsql assignment targets are "local" variables.

  4. Preliminary refactoring of plpgsql expression construction.

  5. Refactor pl_funcs.c to provide a usage-independent tree walker.

  6. Generalize plpgsql's heuristic for importing expanded objects.

  1. Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-20T16:32:13Z

    Hello!

I'm working on the OneSparse Postgres extension that wraps the GraphBLAS
API with a SQL interface for doing graph analytics and other sparse linear
algebra operations:

https://onesparse.github.io/OneSparse/test_matrix_header/

OneSparse wraps the GraphBLAS opaque handles in Expanded Object Header
structs that register ExpandedObjectMethods for flattening and expanding
objects from their "live" handle that can be passed to the SuiteSparse API,
and their "flat" representations are de/serialized and get written as TOAST
values.  This works perfectly.

However during some single source shortest path (sssp) benchmarking I was
getting good numbers but not as good as I expected, and noticed some
sublinear scaling as the problems got bigger.  It seems my objects are
getting constantly flattened/expanded from plpgsql during the iterative
phases of an algorithm.  As the solution grows the result vector gets
bigger and the expand/flatten cost increases on each iteration.

I found this thread from the original path implementation from Tom Lane in
2015:

https://www.postgresql.org/message-id/E1Ysvgz-0000s0-DP%40gemulon.postgresql.org

In this initial implementation, a few heuristics have been hard-wired
> into plpgsql to improve performance for arrays that are stored in
> plpgsql variables. We would like to generalize those hacks so that
> other datatypes can obtain similar improvements, but figuring out some
> appropriate APIs is left as a task for future work.


Sure enough looking at the code I see this condition:


https://github.com/postgres/postgres/blob/master/src/pl/plpgsql/src/pl_exec.c#L549

This is a showstopper for me as I can't see a good way around it, I tried
to "fake" an array but didn't get too far down that approach but I may
still pull it off as GraphBLAS objects are very much array-like, but I
figured I'd also open the discussion on how we can fix this permanently so
that future extensions don't run into this penalty.

My first thought was to add a flag to CREATE TYPE like "EXPANDED = true" or
some other better name that indicates that the object can be safely taken
ownership of in its expanded state and not copied.  The GraphBLAS is
specific in its API in that the object handle holder is the owner of the
reference, so that would work fine for me.  Another option I guess is some
kind of whitelist or blacklist telling plpgsql which types can be kept
expanded.

And then there is just removing the existing restriction on arrays only.
Is any other expanded object out there really interested in being
flattened/expanded over and over again?

Thanks,

-Michel

  2. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-20T17:13:31Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> I found this thread from the original path implementation from Tom Lane in
> 2015:
> https://www.postgresql.org/message-id/E1Ysvgz-0000s0-DP%40gemulon.postgresql.org
>> In this initial implementation, a few heuristics have been hard-wired
>> into plpgsql to improve performance for arrays that are stored in
>> plpgsql variables. We would like to generalize those hacks so that
>> other datatypes can obtain similar improvements, but figuring out some
>> appropriate APIs is left as a task for future work.

Yeah, we thought that it wouldn't be appropriate to try to design
general APIs till we had more kinds of expandable objects.  Maybe
now is a good time to push forward on that.

> My first thought was to add a flag to CREATE TYPE like "EXPANDED = true" or
> some other better name that indicates that the object can be safely taken
> ownership of in its expanded state and not copied.

Isn't that inherent in the notion of R/W vs R/O expanded-object
pointers?

> And then there is just removing the existing restriction on arrays only.
> Is any other expanded object out there really interested in being
> flattened/expanded over and over again?

I'm not sure.  It seems certain that if the object is already expanded
(either R/W or R/O), the paths for that in plpgsql_exec_function could
be taken regardless of its specific type.  The thing that is debatable
is "if the object is in a flat state, should we forcibly expand it
here?".  That could be a win if the function later does object
accesses that would benefit --- but it might never do so.  We chose
to always expand arrays, and we've gotten little pushback on that,
but the tradeoffs might be different for other sorts of expanded
objects.

The other problem is that plpgsql only knows how to do such expansion
for arrays, and it's not obvious how to extend that part.

But it seems like we could get an easy win by adjusting
plpgsql_exec_function along the lines of

l. 549:
-                    if (!var->isnull && var->datatype->typisarray)
+                    if (!var->isnull)

l. 564:
-                        else
+                        else if (var->datatype->typisarray)

How far does that improve matters for you?

The comment above line 549 cross-references exec_assign_value,
but it looks like that's already set up to be similarly type-agnostic
about values that are already expanded.

			regards, tom lane

  3. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-20T18:25:30Z

    On Sun, Oct 20, 2024 at 10:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
> > I found this thread from the original path implementation from Tom Lane
> in
>
> >> appropriate APIs is left as a task for future work.
>
> Yeah, we thought that it wouldn't be appropriate to try to design
> general APIs till we had more kinds of expandable objects.  Maybe
> now is a good time to push forward on that.
>

Great, I'm happy to be involved!

> My first thought was to add a flag to CREATE TYPE like "EXPANDED = true"
> or
> > some other better name that indicates that the object can be safely taken
> > ownership of in its expanded state and not copied.
>
> Isn't that inherent in the notion of R/W vs R/O expanded-object
> pointers?
>

I'm not sure I'm qualified enough to agree with you but I see your point.


> > And then there is just removing the existing restriction on arrays only.
> > Is any other expanded object out there really interested in being
> > flattened/expanded over and over again?
>
> I'm not sure.  It seems certain that if the object is already expanded
> (either R/W or R/O), the paths for that in plpgsql_exec_function could
> be taken regardless of its specific type.


Agreed.


>   The thing that is debatable
> is "if the object is in a flat state, should we forcibly expand it
> here?".  That could be a win if the function later does object
> accesses that would benefit --- but it might never do so.  We chose
> to always expand arrays, and we've gotten little pushback on that,
> but the tradeoffs might be different for other sorts of expanded
> objects.
>

The OneSparse objects will always expand themselves on first use via a
DatumGetExpandedArray like macro wrapper, there is no run-time benefit to
their flat representation, so I'm fine with not forcing their expansion
ahead of time, but once I expand it I'd like it to stay expanded until the
function returns (as much as possible) the serialization costs for very
large sparse matrices is heavy.

The other problem is that plpgsql only knows how to do such expansion
> for arrays, and it's not obvious how to extend that part.
>

Perhaps a third member function for ExpandedObjectMethods that formalizes
the expansion interface like found in DatumGetExpandedArray?  I closely
follow that same pattern in my code.


>
> But it seems like we could get an easy win by adjusting
> plpgsql_exec_function along the lines of
> ...
>
> How far does that improve matters for you?
>

I'll give it a try in my local benchmarking code and get back to you, thank
you for the fast reply and the insight!

-Michel

  4. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-20T19:19:00Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> On Sun, Oct 20, 2024 at 10:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The other problem is that plpgsql only knows how to do such expansion
>> for arrays, and it's not obvious how to extend that part.

> Perhaps a third member function for ExpandedObjectMethods that formalizes
> the expansion interface like found in DatumGetExpandedArray?  I closely
> follow that same pattern in my code.

The trouble is we don't have an expanded object to consult at this
point --- only a flat Datum.  plpgsql has hard-wired knowledge that
it's okay to apply expand_array if the datatype passes the typisarray
tests, but I'm pretty unclear on how to provide similar knowledge for
extension datatypes.

			regards, tom lane

  5. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-21T03:29:13Z

    On Sun, Oct 20, 2024 at 10:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
> I'm not sure.  It seems certain that if the object is already expanded
> (either R/W or R/O), the paths for that in plpgsql_exec_function could
> be taken regardless of its specific type.
>


> But it seems like we could get an easy win by adjusting
> plpgsql_exec_function along the lines of
>
> l. 549:
> -                    if (!var->isnull && var->datatype->typisarray)
> +                    if (!var->isnull)
>
> l. 564:
> -                        else
> +                        else if (var->datatype->typisarray)
>
> How far does that improve matters for you?
>

I tried this change and couldn't get it to work, on the next line:

if (!var->isnull)
{
    if (VARATT_IS_EXTERNAL_EXPANDED_RW(DatumGetPointer(var->value)))

var->value might not be a pointer, as it seems at least from my gdb
scratching, but say an integer.  This segfaults on non-array but
non-expandable datum.

I guess this gets back into knowing if a flat thing is expandable or not.
I'm going to spend some more time looking at it, I haven't been in this
corner of Postgres before.

Another comment that caught my eye was this one:

https://github.com/postgres/postgres/blob/master/src/pl/plpgsql/src/pl_exec.c#L8304

Not sure what the implication is there.

-Michel

  6. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-21T03:46:44Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> On Sun, Oct 20, 2024 at 10:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> But it seems like we could get an easy win by adjusting
>> plpgsql_exec_function along the lines of
>> ...

> I tried this change and couldn't get it to work, on the next line:
>     if (VARATT_IS_EXTERNAL_EXPANDED_RW(DatumGetPointer(var->value)))
> var->value might not be a pointer, as it seems at least from my gdb
> scratching, but say an integer.  This segfaults on non-array but
> non-expandable datum.

Oh, duh --- the typisarray test serves to eliminate pass-by-value
types.  We need the same test that exec_assign_value makes,
!var->datatype->typbyval, before it's safe to apply DatumGetPointer.
So line 549 needs to be more like

-                    if (!var->isnull && var->datatype->typisarray)
+                    if (!var->isnull && !var->datatype->typbyval)

> Another comment that caught my eye was this one:
> https://github.com/postgres/postgres/blob/master/src/pl/plpgsql/src/pl_exec.c#L8304
> Not sure what the implication is there.

Yeah, that's some more unfinished business.  I'm not sure if it
matters to your use-case or not.

BTW, we probably should move this thread to pgsql-hackers.

			regards, tom lane

  7. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-21T17:23:33Z

    On Sun, Oct 20, 2024 at 8:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
> > On Sun, Oct 20, 2024 at 10:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>

(from thread
https://www.postgresql.org/message-id/CACxu%3DvJaKFNsYxooSnW1wEgsAO5u_v1XYBacfVJ14wgJV_PYeg%40mail.gmail.com
)


> >> But it seems like we could get an easy win by adjusting
> >> plpgsql_exec_function along the lines of
> >> ...
>
> > I tried this change and couldn't get it to work, on the next line:
> >     if (VARATT_IS_EXTERNAL_EXPANDED_RW(DatumGetPointer(var->value)))
> > var->value might not be a pointer, as it seems at least from my gdb
> > scratching, but say an integer.  This segfaults on non-array but
> > non-expandable datum.
>
>   We need the same test that exec_assign_value makes,
> !var->datatype->typbyval, before it's safe to apply DatumGetPointer.
> So line 549 needs to be more like
>
> -                    if (!var->isnull && var->datatype->typisarray)
> +                    if (!var->isnull && !var->datatype->typbyval)
>
> > Another comment that caught my eye was this one:
> >
> https://github.com/postgres/postgres/blob/master/src/pl/plpgsql/src/pl_exec.c#L8304
> > Not sure what the implication is there.
>
> Yeah, that's some more unfinished business.  I'm not sure if it
> matters to your use-case or not.
>
> BTW, we probably should move this thread to pgsql-hackers.


And here we are, thanks for your help on this Tom.  For some thread
switching context for others, I'm writing a postgres extension that wraps
the SuiteSparse:GraphBLAS API and provides new types for sparse and dense
matrices and vectors.  It's like a combination of numpy and scipy.sparse
but for Postgres with an emphasis on graph analytics as sparse adjacency
matrices using linear algebra.

I use the expandeddatum API to flatten and expand on disk compressed
representations of these objects into "live" in-memory objects managed by
SuiteSparse.  All GraphBLAS objects are opaque handles, and my expanded
objects are essentially a box around this handle.  I use memory context
callbacks to free the handles when the memory context of the box is freed.
This works very well and I've made a lot of progress on creating a very
clean algebraic API, here are the doctests for matrices, this is all
generated from live code!

https://onesparse.github.io/OneSparse/test_matrix_header/

Doing some benchmarking I noticed that when writing some simple graph
algorithms in plpgsql, my objects were being constantly flattened and
expanded.  Posting my question above to pgsql-general Tom gave me some tips
and suggested I move the thread here.

My non-expert summary: plpgsql only optimizes for expanded objects if they
are arrays.  Non array expanded objects get flattened/expanded on every
use.  For large matrices and vectors this is very expensive.  Ideally I'd
like to expand my object, use it throughout the function, return it to
another function that may use it, and only flatten it when it goes to disk
or it's completely unavoidable.  The comment in expandeddatum.h really kind
of sells this as one of the main features:

 * An expanded object is meant to survive across multiple operations, but
 * not to be enormously long-lived; for example it might be a local variable
 * in a PL/pgSQL procedure.  So its extra bulk compared to the on-disk
format
 * is a worthwhile trade-off.

In my case it's not a question of saving bulk, the on disk representation
of a matrix is not useful at compute time, it needs to be expanded (using
GraphBLAS's serialize/deserialize API) for it to be usable for matrix
operations like matmul.  In most cases algorithms using these objects
iterate in a loop, doing various algebraic operations almost always
involving a matmul until they converge on a stable solution or they exhaust
the input elements.  Here for example is a "minimum parent BFS" that takes
a graph and a starting node, and traverses the graph breadth first,
computing a vector of every node and its minimum parent id.

CREATE OR REPLACE FUNCTION bfs(graph matrix, start_node bigint)
>     RETURNS vector LANGUAGE plpgsql AS
>     $$
>     DECLARE
>     bfs_vector vector = vector('int32');
>     next_vector vector = vector('int32');
>     BEGIN
>         bfs_vector = set_element(bfs_vector, start_node, 1);
>         WHILE sssp_vector != next_vector LOOP
>             next_vector = dup(bfs_vector);
>             bfs_vector = vxm(bfs_vector, graph, 'any_secondi_int32',
>                              w=>bfs_vector, accum=>'min_int32');
>         END LOOP;
>     RETURN bfs_vector;
>     end;
>     $$;
>

(If you're wondering "Why would anyone do it this way" it's because
SuiteSparse is optimized for parallel sparse matrix multiplication and has
a JIT compiler that can target multiple architectures, at the moment CPUs
and CUDA GPUs.  Reusing the same Linear Algebra already prevalent in graph
theory means not having to think about any low level implementation issues
and having code that is completely portable from CPU to GPU or other
accelerators).

So, I made the two small changes Tom suggested above and I have them in a
side fork here:

https://github.com/postgres/postgres/compare/master...michelp:postgres-upstream:michelp-flatless#diff-0c35024d1576c347689c7abad68abd8562a0aa5f0d2c63d6d65df4b360b0e807

Good news, my code still works, but bad news is there is still a lot of
flattening/expanding/freeing going on at multiple points in each iteration
of the algorithm.  I'll note too that:

    BEGIN
        bfs_vector = set_element(sssp_vector, start_node, 1);

I'd prefer that to not be an assignment, set_element mutates the object (I
eventually plan to support subscripting syntax like bfs_vector[start_node]
= 1)

same with:

            bfs_vector = vxm(bfs_vector, graph, 'any_secondi_int32',
                             w=>bfs_vector, accum=>'min_int32');

This matmul mutates bfs_vector, I shouldn't need to reassign it back but at
the moment it seems necessary otherwise the mutations are lost but this
costs a full flatten/expand cycle.

Short term my goal is to optimize plpgsql so that my objects stay expanded
for the life of the function.  Long term there's some "unfinished business"
to use Tom's words around the expandeddatum API.  I'm not really qualified
to speak on these issue but this is my understanding of some of them:

  - plpgsql knows how to expand arrays and is hardwired for it, but how
would it know how to expand other expandable types?

  - Issues with exec_check_rw_parameter also being hardwired to only
optimize expanded objects for array append and prepend, I suspect this has
something to do with my issue above about mutating objects in place.

I may have missed something but hopefully that brings anyone up to speed
interested in this topic.

-Michel

  8. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-22T19:33:40Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> Doing some benchmarking I noticed that when writing some simple graph
> algorithms in plpgsql, my objects were being constantly flattened and
> expanded.  Posting my question above to pgsql-general Tom gave me some tips
> and suggested I move the thread here.

> My non-expert summary: plpgsql only optimizes for expanded objects if they
> are arrays.  Non array expanded objects get flattened/expanded on every
> use.  For large matrices and vectors this is very expensive.  Ideally I'd
> like to expand my object, use it throughout the function, return it to
> another function that may use it, and only flatten it when it goes to disk
> or it's completely unavoidable.

Right.  My recollection is that when we first invented expanded
objects, we only implemented expanded arrays, and so it seemed
premature to try to define generalized APIs.  So that went on the back
burner until we got some more cases to look at.  But now we also have
expanded records, and with your use-case as an example of an extension
trying to do similar things, I feel like we have enough examples to
move forward.

As far as the hack we were discussing upthread goes, I realized that
it should test for typlen == -1 not just !typbyval, since the
VARATT_IS_EXTERNAL_EXPANDED_RW test requires that there be a length
word.  With that fix and some comment updates, it looks like the
attached.  I'm inclined to just go ahead and push that.  It won't move
the needle hugely far, but it will help, and it seems simple and safe.

To make more progress, there are basically two areas that we need
to look at:

1. exec_assign_value() has hard-wired knowledge that it's a good idea
to expand an array that's being assigned to a plpgsql variable, and
likewise hard-wired knowledge that calling expand_array() is how to
do that.  The bit in plpgsql_exec_function() that we're patching
in the attached is the same thing, but for the initial assignment of
a function input parameter to a plpgsql variable.  At the time this
was written I was quite unsure that forcible expansion would be a net
win, but the code is nine years old now and there's been few or no
performance complaints.  So maybe it's okay to decide that "always
expand expandable types during assignment" is a suitable policy across
the board, and we don't need to figure out a smarter rule.  It sounds
like that'd probably be a win for your application, which gives me
more faith in the idea than I would've had before.  That leaves the
problem of how to find the right code to call to do the expansion.
Clearly, we need to attach something to data types to make that
possible.  My first thought was to invent a pg_type column (and
CREATE TYPE/ALTER TYPE options, and pg_dump support, yadda yadda)
containing the OID of a function corresponding to expand_array().
However, that work would have to be done again the next time we
think of something we'd like to know about a data type.  So now
I'm thinking that we should steal ideas from the "prosupport" API
(see src/include/nodes/supportnodes.h) and invent a concept of a
"type support" function that can handle an extensible set of
different requests.  The first one would be to pass back the
address of an expansion function comparable to expand_array(),
if the type supports being converted to an expanded object.

2. Most of the performance gold is hidden in deciding when we
can optimize operations on expanded objects that look like

	plpgsql_var := f(plpgsql_var, other-parameters);

by passing a R/W rather than R/O expanded pointer to f() and letting
it munge the expanded object in-place.  If we fail to do that then
f() has to construct a new expanded object for its result.  It's
probably still better off getting a R/O pointer than a flat object,
but nonetheless we fail to avoid a lot of data copying.

The difficulty here is that we do not want to change the normal naive
semantics of such an assignment, in particular "if f() throws an error
then the value of plpgsql_var has not been modified".  This means that
we can only optimize when the R/W parameter is to be passed to the
top-level function of the expression (else, some function called later
could throw an error and ruin things).  Furthermore, we need a
guarantee from f() that it will not throw an error after modifying
the value.

As things stand, plpgsql has hard-wired knowledge that
array_subscript_handler(), array_append(), and array_prepend()
are safe in that way, but it knows nothing about anything else.
So one route to making things better seems fairly clear: invent a new
prosupport request that asks whether the function is prepared to make
such a guarantee.  I wonder though if you can guarantee any such thing
for your functions, when you are relying on a library that's probably
not designed with such a restriction in mind.  If this won't work then
we need a new concept.

One idea I was toying with is that it doesn't matter if f()
throws an error so long as the plpgsql function is not executing
within an exception block: if the error propagates out of the plpgsql
function then we no longer care about the value of the variable.
That would very substantially weaken the requirements on how f()
is implemented.  I'm not sure that we could make this work across
multiple levels of plpgsql functions (that is, if the value of the
variable ultimately resides in some outer function) but right now
that's not an issue since no plpgsql function as such would ever
promise to be safe, thus it would never receive a R/W pointer to
some outer function's variable.

> same with:
>             bfs_vector = vxm(bfs_vector, graph, 'any_secondi_int32',
>                              w=>bfs_vector, accum=>'min_int32');
> This matmul mutates bfs_vector, I shouldn't need to reassign it back but at
> the moment it seems necessary otherwise the mutations are lost but this
> costs a full flatten/expand cycle.

I'm not hugely excited about that.  We could imagine extending
this concept to INOUT parameters of procedures, but it doesn't
seem like that buys anything except a small notational savings.
Maybe it would work to allow multiple parameters of a procedure
to be passed as R/W, whereas we're restricted to one for the
function-notation method.  So possibly there's a gain there but
I'm not sure how big.

BTW, if I understand your example correctly then bfs_vector is
being passed to vxm() twice.  This brings up an interesting
point: if we pass the first instance as R/W, and vxm() manipulates it,
then the changes would also be visible in its other parameter "w".
This is certainly not per normal semantics.  A "safe" function would
have to either not have any possibility that two parameters could
refer to the same object, or be coded in a way that made it impervious
to this issue --- in your example, it couldn't look at "w" anymore
once it'd started modifying the first parameter.  Is that an okay
requirement, and if not what shall we do about it?

Anyway that's my brain dump for today.  Thoughts?

			regards, tom lane

  9. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-23T04:15:30Z

    On Tue, Oct 22, 2024 at 12:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:



> But now we also have
> expanded records, and with your use-case as an example of an extension
> trying to do similar things, I feel like we have enough examples to
> move forward.
>

Great!

As far as the hack we were discussing upthread goes, I realized that
> it should test for typlen == -1 not just !typbyval, since the
> VARATT_IS_EXTERNAL_EXPANDED_RW test requires that there be a length
> word.  With that fix and some comment updates, it looks like the
> attached.  I'm inclined to just go ahead and push that.  It won't move
> the needle hugely far, but it will help, and it seems simple and safe.
>

I made those changes and my code works a bit faster, it looks like it takes
a couple of the top level expansions out.  I'll have more data in the
morning.

To make more progress, there are basically two areas that we need
> to look at:
>
> 1. exec_assign_value() has hard-wired knowledge that it's a good idea
> to expand an array that's being assigned to a plpgsql variable, and
> likewise hard-wired knowledge that calling expand_array() is how to
> do that.  The bit in plpgsql_exec_function() that we're patching
> in the attached is the same thing, but for the initial assignment of
> a function input parameter to a plpgsql variable.  At the time this
> was written I was quite unsure that forcible expansion would be a net
> win, but the code is nine years old now and there's been few or no
> performance complaints.  So maybe it's okay to decide that "always
> expand expandable types during assignment" is a suitable policy across
> the board, and we don't need to figure out a smarter rule.  It sounds
> like that'd probably be a win for your application, which gives me
> more faith in the idea than I would've had before.


Definitely a win, as the flattened format of my objects don't have any run
time use, so there is no chance for net loss for me.  I guess I'm using
this feature differently from how arrays are, which have a usable flattened
format so there is a need to weigh a trade off with expanding or not.   In
my case, only the expanded version is useful, and serializing the flat
version is expensive.  Formalizing something like expand_array would work
well for me, as my expand_matrix function has the identical function
signature and serves the exact same purpose.

   So now

> I'm thinking that we should steal ideas from the "prosupport" API
> (see src/include/nodes/supportnodes.h) and invent a concept of a
> "type support" function that can handle an extensible set of
> different requests.  The first one would be to pass back the
> address of an expansion function comparable to expand_array(),
> if the type supports being converted to an expanded object.
>

I'll look into the support code, I haven't seen that before.


> 2. Most of the performance gold is hidden in deciding when we
> can optimize operations on expanded objects that look like
>
>         plpgsql_var := f(plpgsql_var, other-parameters);
>
> by passing a R/W rather than R/O expanded pointer to f() and letting
> it munge the expanded object in-place.  If we fail to do that then
> f() has to construct a new expanded object for its result.  It's
> probably still better off getting a R/O pointer than a flat object,
> but nonetheless we fail to avoid a lot of data copying.
>
> The difficulty here is that we do not want to change the normal naive
> semantics of such an assignment, in particular "if f() throws an error
> then the value of plpgsql_var has not been modified".  This means that
> we can only optimize when the R/W parameter is to be passed to the
> top-level function of the expression (else, some function called later
> could throw an error and ruin things).  Furthermore, we need a
> guarantee from f() that it will not throw an error after modifying
> the value.


> As things stand, plpgsql has hard-wired knowledge that
> array_subscript_handler(), array_append(), and array_prepend()
> are safe in that way, but it knows nothing about anything else.
> So one route to making things better seems fairly clear: invent a new
> prosupport request that asks whether the function is prepared to make
> such a guarantee.  I wonder though if you can guarantee any such thing
> for your functions, when you are relying on a library that's probably
> not designed with such a restriction in mind.  If this won't work then
> we need a new concept.
>

This will work for the GraphBLAS API.  The expanded object in my case is
really just a small box struct around a GraphBLAS "handle" which is an
opaque pointer to data which I cannot mutate, only the library can change
the object behind the handle.  The API makes strong guarantees that it will
either do the operation and return a success code or not do the operation
and return an error code.  It's not possible (normally) to get a corrupt or
incomplete object back.


>
> One idea I was toying with is that it doesn't matter if f()
> throws an error so long as the plpgsql function is not executing
> within an exception block: if the error propagates out of the plpgsql
> function then we no longer care about the value of the variable.
> That would very substantially weaken the requirements on how f()
> is implemented.  I'm not sure that we could make this work across
> multiple levels of plpgsql functions (that is, if the value of the
> variable ultimately resides in some outer function) but right now
> that's not an issue since no plpgsql function as such would ever
> promise to be safe, thus it would never receive a R/W pointer to
> some outer function's variable.
>

The water here is pretty deep for me but I'm pretty sure I get what you're
saying, I'll need to do some more studying of the plpgsql code which I've
been spending the last couple of days familiarizing myself with more.


> > same with:
> >             bfs_vector = vxm(bfs_vector, graph, 'any_secondi_int32',
> >                              w=>bfs_vector, accum=>'min_int32');
> > This matmul mutates bfs_vector, I shouldn't need to reassign it back but
> at
> > the moment it seems necessary otherwise the mutations are lost but this
> > costs a full flatten/expand cycle.
>
> I'm not hugely excited about that.  We could imagine extending
> this concept to INOUT parameters of procedures, but it doesn't
> seem like that buys anything except a small notational savings.
> Maybe it would work to allow multiple parameters of a procedure
> to be passed as R/W, whereas we're restricted to one for the
> function-notation method.  So possibly there's a gain there but
> I'm not sure how big.
>
> BTW, if I understand your example correctly then bfs_vector is
> being passed to vxm() twice.


Yes, this is not unusual in this form of linear algebra, as multiple
operations often accumulate into the same object to prevent a bunch of
copying during each iteration of a given algorithm.  There is also a "mask"
parameter where another or the same object can be provided to either mask
in or out (compliment mask) values during the accumulation phase.  This is
very useful for many algorithms, and good example is the Burkhardt method
of Triangle Counting (
https://journals.sagepub.com/doi/10.1177/1473871616666393) which in
GraphBLAS boils down to:

    GrB_mxm (C, A, NULL, semiring, A, A, GrB_DESC_S) ;
    GrB_reduce (&ntri, NULL, monoid, C, NULL) ;
    ntri /= 6 ;

In this case A is three of the parameters to mxm, the left operand, right
operand, and a structural mask.  This can be summed up as "A squared,
masked by A", which when reduced returns the number of triangles in the
graph (times 6).


>   This brings up an interesting
> point: if we pass the first instance as R/W, and vxm() manipulates it,
> then the changes would also be visible in its other parameter "w".
> This is certainly not per normal semantics.  A "safe" function would
> have to either not have any possibility that two parameters could
> refer to the same object, or be coded in a way that made it impervious
> to this issue --- in your example, it couldn't look at "w" anymore
> once it'd started modifying the first parameter.  Is that an okay
> requirement, and if not what shall we do about it?
>

I *think*, if I understand you correctly, that this isn't an issue for the
GraphBLAS.  My expanded objects are just boxes around an opaque handle, I
don't actually mutate anything inside the box, and I can't see past the
opaque pointer.  SuiteSparse may be storing the matrix in one of many
different formats, or on a GPU, or who knows, all I have is a handle to "A"
which I pass to GraphBLAS methods which is the only way I can interact with
them.  Here's the definition of that vxm function:

https://github.com/OneSparse/OneSparse/blob/main/src/matrix.c#L907

It's pretty straightforward, get the arguments, and pass them to the
GraphBLAS API, there is no mutable structure inside the expanded "box",
just the handle.

I'm using the expanded object API to solve my two key problems, flatten an
object for disk storage, expand that object (through the GraphBLAS
serialize/deserialize API) and turn it into an object handle, which is
secretly just a pointer of course to the internal details of the object,
but I can't see or change that, only SuiteSparse can.

(btw sorry about the bad parameter names, "w" is the name from the API spec
which I've been sticking to, which is the optional output object to use, if
one is not passed, I create a new one, this is similar to the numpy "out"
parameter semantics) .

I added some debug instrumentation that might show a bit more of what's
going on for me, consider this function:

CREATE OR REPLACE FUNCTION test(graph matrix)
    RETURNS matrix LANGUAGE plpgsql AS
    $$
    DECLARE
      n bigint = nrows(graph);
      m bigint = ncols(graph);
    BEGIN
    RETURN graph;
    end;
    $$;

The graph passes straight through but first I call two methods to get the
number rows and columns.  When I run it on a graph:

postgres=# select pg_typeof(test(graph)) from test_graphs ;
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
DEBUG:  matrix_ncols
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
 pg_typeof
-----------
 matrix
(1 row)

THe matrix gets expanded twice, presumably because the object comes in
flat, and both nrows() and ncols() expands it, which ends up being two
separate handles and thus two separate objects to the GraphBLAS.

Here's another example:

CREATE OR REPLACE FUNCTION test2(graph matrix)
    RETURNS bigint LANGUAGE plpgsql AS
    $$
    BEGIN
    perform set_element(graph, 1, 1, 1);
    RETURN nvals(graph);
    end;
    $$;
CREATE FUNCTION
postgres=# select test2(matrix('int32'));
DEBUG:  new_matrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  scalar_int32
DEBUG:  new_scalar
DEBUG:  matrix_set_element
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  DatumGetScalar
DEBUG:  matrix_get_flat_size
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  context_callback_matrix_free
DEBUG:  context_callback_scalar_free
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
DEBUG:  context_callback_matrix_free
 test2
-------
     0
(1 row)

I would expect that to return 1.  If I do "graph = set_element(graph, 1, 1,
1)" it works.

I hope that gives a bit more information about my use cases, in general I'm
very happy with the API, it's very algebraic, I have a lot of interesting
plans for supporting more operators and subscription syntax, but this issue
is not my top priority to see if we can resolve it.  I'm sure I missed
something in your detailed plan so I'll be going over it some more this
week.  Please let me know if you have any other questions about my use case
or concerns about my expectations.

Thank you!

-Michel

  10. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-23T15:21:25Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> Here's another example:

> CREATE OR REPLACE FUNCTION test2(graph matrix)
>     RETURNS bigint LANGUAGE plpgsql AS
>     $$
>     BEGIN
>     perform set_element(graph, 1, 1, 1);
>     RETURN nvals(graph);
>     end;
>     $$;
> CREATE FUNCTION
> postgres=# select test2(matrix('int32'));
> DEBUG:  new_matrix
> DEBUG:  matrix_get_flat_size
> DEBUG:  flatten_matrix
> DEBUG:  scalar_int32
> DEBUG:  new_scalar
> DEBUG:  matrix_set_element
> DEBUG:  DatumGetMatrix
> DEBUG:  expand_matrix
> DEBUG:  new_matrix
> DEBUG:  DatumGetScalar
> DEBUG:  matrix_get_flat_size
> DEBUG:  matrix_get_flat_size
> DEBUG:  flatten_matrix
> DEBUG:  context_callback_matrix_free
> DEBUG:  context_callback_scalar_free
> DEBUG:  matrix_nvals
> DEBUG:  DatumGetMatrix
> DEBUG:  expand_matrix
> DEBUG:  new_matrix
> DEBUG:  context_callback_matrix_free
> DEBUG:  context_callback_matrix_free
>  test2
> -------
>      0
> (1 row)

I'm a little confused by your debug output.  What are "scalar_int32"
and "new_scalar", and what part of the plpgsql function is causing
them to be invoked?

Another thing that confuses me is why there's a second flatten_matrix
operation happening here.  Shouldn't set_element return its result
as a R/W expanded object?

> I would expect that to return 1.  If I do "graph = set_element(graph, 1, 1,
> 1)" it works.

I think you have a faulty understanding of PERFORM.  It's defined as
"evaluate this expression and throw away the result", so it's *not*
going to change "graph", not even if set_element declares that
argument as INOUT.  (Our interpretation of OUT arguments for functions
is that they're just an alternate notation for specifying the function
result.)  If you want to avoid the explicit assignment back to "graph"
then the thing to do would be to declare set_element as a procedure,
not a function, with an INOUT argument and then call it with CALL.

That's only cosmetically different though, in that the updated
"graph" value is still passed back much as if it were a function
result, and then the CALL infrastructure knows it has to assign that
back to the argument variable.  And, as I tried to explain earlier,
that code path currently has no mechanism for avoiding making a copy
of the graph somewhere along the line: it will pass "graph" to the
procedure as either a flat Datum or a R/O expanded object, so that
set_element will be required to copy that before modifying it.
We can imagine extending whatever we do for "x := f(x)" cases so that
it also works during "CALL p(x)".  But I think that's only going to
yield cosmetic or notational improvements so I don't want to start
with doing that.  Let's focus first on improving the existing
infrastructure for the f(x) case.

			regards, tom lane

  11. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-23T16:04:30Z

    I wrote:
> One idea I was toying with is that it doesn't matter if f()
> throws an error so long as the plpgsql function is not executing
> within an exception block: if the error propagates out of the plpgsql
> function then we no longer care about the value of the variable.
> That would very substantially weaken the requirements on how f()
> is implemented.

The more I think about this idea the better I like it.  We can
improve on the original concept a bit: the assignment can be
within an exception block so long as the target variable is too.
For example, consider

	DECLARE x float8[];
	BEGIN
	   ...
	   DECLARE y float8[];
	   BEGIN
	      x := array_append(x, 42);
	      y := array_append(y, 42);
	   END;
	EXCEPTION WHEN ...;
	END;

Currently, both calls of array_append are subject to R/W optimization,
so that array_append must provide a strong guarantee that it won't
throw an error after it's begun to change the R/W object.  If we
redefine things so that the optimization is applied only to "y",
then AFAICS we need nothing from array_append.  It only has to be
sure it doesn't corrupt the object so badly that it can't be freed
... but that requirement exists already, for anything dealing with
expanded objects.  So this would put us in a situation where we
could apply the optimization by default, which'd be a huge win.

There is an exception: if we are considering

	      x := array_cat(x, x);

then I don't think we can optimize because of the aliasing problem
I mentioned before.  So there'd have to be a restriction that the
target variable is mentioned only once in the function's arguments.
For stuff like your vxm() function, that'd be annoying.  But functions
that need that and are willing to deal with the aliasing hazard could
still provide a prosupport function that promises it's okay.  What
we'd accomplish is that a large fraction of interesting functions
could get the benefit without having to create a prosupport function,
which is a win all around.

Also worth noting: in the above example, we could optimize the
update on "x" too, if we know that "x" is not referenced in the
block's EXCEPTION handlers.  I wouldn't bother with this in the
first version, but it might be worth doing later.

So if we go this way, the universe of functions that can benefit
from the optimization enlarges considerably, and the risk of bugs
that break the optimization drops considerably.  The cost is that
some cases that were optimized before now will not be.  But I
suspect that plpgsql functions where this optimization is key
probably don't contain EXCEPTION handlers at all, so that they
won't notice any change.

Thoughts?

			regards, tom lane

  12. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-24T01:39:03Z

    On Wed, Oct 23, 2024 at 8:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
> > Here's another example:
>
> > CREATE OR REPLACE FUNCTION test2(graph matrix)
> >     RETURNS bigint LANGUAGE plpgsql AS
> >     $$
> >     BEGIN
> >     perform set_element(graph, 1, 1, 1);
> >     RETURN nvals(graph);
> >     end;
> >     $$;
> > CREATE FUNCTION
> > postgres=# select test2(matrix('int32'));
> > DEBUG:  new_matrix
> > DEBUG:  matrix_get_flat_size
> > DEBUG:  flatten_matrix
> > DEBUG:  scalar_int32
> > DEBUG:  new_scalar
> > DEBUG:  matrix_set_element
> > DEBUG:  DatumGetMatrix
> > DEBUG:  expand_matrix
> > DEBUG:  new_matrix
> > DEBUG:  DatumGetScalar
> > DEBUG:  matrix_get_flat_size
> > DEBUG:  matrix_get_flat_size
> > DEBUG:  flatten_matrix
> > DEBUG:  context_callback_matrix_free
> > DEBUG:  context_callback_scalar_free
> > DEBUG:  matrix_nvals
> > DEBUG:  DatumGetMatrix
> > DEBUG:  expand_matrix
> > DEBUG:  new_matrix
> > DEBUG:  context_callback_matrix_free
> > DEBUG:  context_callback_matrix_free
> >  test2
> > -------
> >      0
> > (1 row)
>
> I'm a little confused by your debug output.  What are "scalar_int32"
> and "new_scalar", and what part of the plpgsql function is causing
> them to be invoked?
>

GraphBLAS scalars hold a single element value for the matrix type.
Internally, they are simply 1x1 matrices (much like vectors are 1xn
matrices).  The function signature is:

set_element(a matrix, i bigint, j bigint, s scalar)

There is a "CAST (integer as scalar)" function (scalar_int32) that casts
Postgres integers to GraphBLAS GrB_INT32 scalar elements (which calls
new_scalar because like vectors and matrices, they are expanded objects
which have a GrB_Scalar "handle").  Scalars are useful for working with
individual values, for example reduce() returns a scalar.  There are way
more efficient ways to push huge C arrays of values into matrices but for
now I'm just working at the element level.

Another thing that confuses me is why there's a second flatten_matrix
> operation happening here.  Shouldn't set_element return its result
> as a R/W expanded object?
>

That confuses me too, and my default assumption is always that I'm doing it
wrong.  set_element does return a R/W object afaict, here is the return:

https://github.com/OneSparse/OneSparse/blob/main/src/matrix.c#L1726

where:

#define OS_RETURN_MATRIX(_matrix) return EOHPGetRWDatum(&(_matrix)->hdr)


> > I would expect that to return 1.  If I do "graph = set_element(graph, 1,
> 1,
> > 1)" it works.
>
> I think you have a faulty understanding of PERFORM.  It's defined as
> "evaluate this expression and throw away the result", so it's *not*
> going to change "graph", not even if set_element declares that
> argument as INOUT.


Faulty indeed, I was going from the plpgsql statement documentation:

"Sometimes it is useful to evaluate an expression or SELECT query but
discard the result, for example when calling a function that has
side-effects but no useful result value."

My understanding of "side-effects" was flawed there, but I'm fine with "x =
set_element(x...)" anyway as I was trying to follow the example of
array_append et al.


> (Our interpretation of OUT arguments for functions
> is that they're just an alternate notation for specifying the function
> result.)  If you want to avoid the explicit assignment back to "graph"
> then the thing to do would be to declare set_element as a procedure,
> not a function, with an INOUT argument and then call it with CALL.
>

I'll stick with the assignment.

That's only cosmetically different though, in that the updated
> "graph" value is still passed back much as if it were a function
> result, and then the CALL infrastructure knows it has to assign that
> back to the argument variable.  And, as I tried to explain earlier,
> that code path currently has no mechanism for avoiding making a copy
> of the graph somewhere along the line: it will pass "graph" to the
> procedure as either a flat Datum or a R/O expanded object, so that
> set_element will be required to copy that before modifying it.
>

Right, I'm still figuring out exactly what that code flow is.  This is my
first dive into these corners of the code so thank you for being patient
with me.  I promise to write up some expanded object documentation if this
works!


> We can imagine extending whatever we do for "x := f(x)" cases so that
> it also works during "CALL p(x)".  But I think that's only going to
> yield cosmetic or notational improvements so I don't want to start
> with doing that.  Let's focus first on improving the existing
> infrastructure for the f(x) case.
>

+1

-Michel

  13. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-24T02:10:31Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> On Wed, Oct 23, 2024 at 8:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Another thing that confuses me is why there's a second flatten_matrix
>> operation happening here.  Shouldn't set_element return its result
>> as a R/W expanded object?

> That confuses me too, and my default assumption is always that I'm doing it
> wrong.  set_element does return a R/W object afaict, here is the return:
> https://github.com/OneSparse/OneSparse/blob/main/src/matrix.c#L1726

Hmph.  That seems right.  Can you add errbacktrace() to your logging
ereports, in hopes of seeing how we're getting to flatten_matrix?
Or break there with gdb for a more complete/reliable stack trace.

			regards, tom lane

  14. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-24T02:10:56Z

    On Wed, Oct 23, 2024 at 9:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > One idea I was toying with is that it doesn't matter if f()
> > throws an error so long as the plpgsql function is not executing
> > within an exception block: if the error propagates out of the plpgsql
> > function then we no longer care about the value of the variable.
> > That would very substantially weaken the requirements on how f()
> > is implemented.
>
> The more I think about this idea the better I like it.  We can
> improve on the original concept a bit: the assignment can be
> within an exception block so long as the target variable is too.
> For example, consider
>
>         DECLARE x float8[];
>         BEGIN
>            ...
>            DECLARE y float8[];
>            BEGIN
>               x := array_append(x, 42);
>               y := array_append(y, 42);
>            END;
>         EXCEPTION WHEN ...;
>         END;
>
> Currently, both calls of array_append are subject to R/W optimization,
> so that array_append must provide a strong guarantee that it won't
> throw an error after it's begun to change the R/W object.  If we
> redefine things so that the optimization is applied only to "y",
> then AFAICS we need nothing from array_append.  It only has to be
> sure it doesn't corrupt the object so badly that it can't be freed
> ... but that requirement exists already, for anything dealing with
> expanded objects.  So this would put us in a situation where we
> could apply the optimization by default, which'd be a huge win.
>

Great!  I can make that same guarantee.


> There is an exception: if we are considering
>
>               x := array_cat(x, x);
>
> then I don't think we can optimize because of the aliasing problem
> I mentioned before.  So there'd have to be a restriction that the
> target variable is mentioned only once in the function's arguments.
> For stuff like your vxm() function, that'd be annoying.  But functions
> that need that and are willing to deal with the aliasing hazard could
> still provide a prosupport function that promises it's okay.  What
> we'd accomplish is that a large fraction of interesting functions
> could get the benefit without having to create a prosupport function,
> which is a win all around.
>

I see, I'm not completely clear on the prosupport code so let me repeat it
back just so I know I'm getting it right, it looks like I'll need to write
a C function, that I specify with CREATE FUNCTION ... SUPPORT, that the
planner will call asking me to tell it that it's ok to alias arguments
(which is fine for SuiteSparse so no problem).  You also mentioned a couple
emails back about a "type support" feature similar to prosupport, that
would allow me to specify an eager expansion function for my types.


> Also worth noting: in the above example, we could optimize the
> update on "x" too, if we know that "x" is not referenced in the
> block's EXCEPTION handlers.  I wouldn't bother with this in the
> first version, but it might be worth doing later.
>
> So if we go this way, the universe of functions that can benefit
> from the optimization enlarges considerably, and the risk of bugs
> that break the optimization drops considerably.  The cost is that
> some cases that were optimized before now will not be.  But I
> suspect that plpgsql functions where this optimization is key
> probably don't contain EXCEPTION handlers at all, so that they
> won't notice any change.
>

Sounds like a good tradeoff to me!  Hopefully if anyone does have concerns
with this approach they'll see this thread and comment.

Thanks,

-Michel

>
>                         regards, tom lane
>

  15. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-24T02:38:59Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> On Wed, Oct 23, 2024 at 9:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> For stuff like your vxm() function, that'd be annoying.  But functions
>> that need that and are willing to deal with the aliasing hazard could
>> still provide a prosupport function that promises it's okay.  What
>> we'd accomplish is that a large fraction of interesting functions
>> could get the benefit without having to create a prosupport function,
>> which is a win all around.

> I see, I'm not completely clear on the prosupport code so let me repeat it
> back just so I know I'm getting it right, it looks like I'll need to write
> a C function, that I specify with CREATE FUNCTION ... SUPPORT, that the
> planner will call asking me to tell it that it's ok to alias arguments
> (which is fine for SuiteSparse so no problem).  You also mentioned a couple
> emails back about a "type support" feature similar to prosupport, that
> would allow me to specify an eager expansion function for my types.

Right.  You could omit the support function for anything where
aliasing is impossible (e.g., there's only one matrix argument).
Also, very possibly multiple functions could share the same
support function.

>> Also worth noting: in the above example, we could optimize the
>> update on "x" too, if we know that "x" is not referenced in the
>> block's EXCEPTION handlers.  I wouldn't bother with this in the
>> first version, but it might be worth doing later.

Aside: I'm less excited about that than I was earlier.  It's not wrong
for the example I gave, but in more general cases it doesn't work:

         DECLARE x float8[];
         BEGIN
            ...
            BEGIN
               x := array_append(x, 42);
            EXCEPTION WHEN ...;
            END;
         // we might use x here
         END;

So for an "x" in a further-out DECLARE section, we'd have to track not
only whether x is used in the EXCEPTION clause, but whether it's used
anyplace that can be reached after the exception block ends.  That's
starting to sound overly complicated for the benefit.

>> So if we go this way, the universe of functions that can benefit
>> from the optimization enlarges considerably, and the risk of bugs
>> that break the optimization drops considerably.  The cost is that
>> some cases that were optimized before now will not be.

> Sounds like a good tradeoff to me!  Hopefully if anyone does have concerns
> with this approach they'll see this thread and comment.

I realized that we could suppress any complaints of that ilk by
creating prosupport functions for the three built-in functions that
are handled today, allowing them to operate on the same rules as now.
I might not bother with that on purely performance grounds, but it's
likely worth doing simply to provide an in-core test case for the code
path where a prosupport function can be called.  I'm still writing up
details, but right now I'm envisioning completely separate sets of
rules for the prosupport case versus the no-prosupport case.

			regards, tom lane

  16. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-24T18:32:26Z

    I wrote:
> ... I'm still writing up
> details, but right now I'm envisioning completely separate sets of
> rules for the prosupport case versus the no-prosupport case.

So here is the design I've come up with for optimizing R/W expanded
object updates in plpgsql without any special knowledge from a
prosupport function.  AFAICS this requires no assumptions at all
about the behavior of called functions, other than the bare minimum
"you can't corrupt the object to the point where it wouldn't be
cleanly free-able".  In particular that means it can work for
user-written called functions in plpgsql, SQL, or whatever, not
only for C-coded functions.

There are two requirements to apply the optimization:

* If the assignment statement is within a BEGIN ... EXCEPTION block,
its target variable must be declared inside the most-closely-nested
such block.  This ensures that if an error is thrown from within the
assignment statement's expression, we do not care about the value
of the target variable, except to the extent of being able to clean
it up.

* The target variable must be referenced exactly once within the
RHS expression.  This avoids aliasing hazards such as we discussed
upthread.  But unlike the existing rule, that reference can be
anywhere in the RHS --- it doesn't have to be an argument of the
topmost function.  So for example we can optimize
	foo := fee(fi(fo(fum(foo), other_variable), ...));

While I've not tried to write any code yet, I think both of these
conditions should be reasonably easy to verify.

Given that those conditions are met and the current value of the
assignment target variable is a R/W expanded pointer, we can
execute the assignment as follows:

* Provide the R/W expanded pointer as the value of the Param
representing the sole reference.  At the same time, apply
TransferExpandedObject to reparent the object under the transient
eval_mcontext memory context that's being used to evaluate the
RHS expression, and then set the target variable's value to NULL.
(At this point, the R/W object has exactly the same logical
status as any intermediate calculation result that's palloc'd
in the eval_mcontext.)

* At successful completion of the RHS, assign the result to the
target variable normally.  This includes, if it's an R/W expanded
object, reparenting it under the calling function's main context.

If the RHS expression winds up returning the same expanded object
(probably mutated), then the outer function regains ownership of it
after no more than a couple of TransferExpandedObject calls, which
are cheap.  If the RHS returns something different, then either the
original expanded object got discarded during RHS evaluation, or it
will be cleaned up when we reset the eval_mcontext, so that it won't
be leaked.

I didn't originally foresee the need to transfer the object
into the transient memory context.  But this design avoids any
possibility of double-free attempts, which would be likely to
happen if we allow the outer function's variable to hold onto
a reference to the object while the RHS is evaluated.  A function
receiving an R/W reference is entitled to assume that that value
is not otherwise referenced and can be freed when it's no longer
needed, so it might well get freed during RHS evaluation.  By
converting the original R/W object into (effectively) a temporary
value within the RHS evaluation, we make that assumption valid.

So, while this design greatly expands the set of cases we can
optimize, it does lose some cases that the old approach could
support.  I envision addressing that by allowing a prosupport
function attached to the RHS' topmost function to "bless"
other cases as safe, using reasoning similar to the old rules.
(Or different rules, even, but it's on the prosupport function
to be sure it's safe.)  I don't have a detailed design in mind,
but I'm thinking along the lines of just passing the whole RHS
expression to the prosupport function and letting it decide
what's safe.  In any case, we don't need to even call the
prosupport function unless there's an exception block or
multiple RHS references to the target variable.

			regards, tom lane

  17. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-31T23:41:48Z

    Here's two backtraces from gdb from this function:

CREATE OR REPLACE FUNCTION test2(graph matrix)
    RETURNS bigint LANGUAGE plpgsql AS
    $$
    BEGIN
    perform set_element(graph, 1, 1, 1);
    RETURN nvals(graph);
    end;
    $$;

https://gist.githubusercontent.com/michelp/d02e3e300710443454357222077f9ded/raw/86d9c2c3de3f9b4740065a7b8c29a3e1c54b1cac/gistfile1.txt

Both traces are in that file split on the hyphens line 44.  I'm still
puzzling it out, could it have something to do with the volatility of the
functions?  I'm not entirely clear on how to interpret function volatility
with expanded objects, nvals is STRICT, but set_element is STABLE.  I think
my logic there was because it's a mutation.  This is likely another
misunderstanding of mine.

-Michel

On Wed, Oct 23, 2024 at 7:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
> > On Wed, Oct 23, 2024 at 8:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Another thing that confuses me is why there's a second flatten_matrix
> >> operation happening here.  Shouldn't set_element return its result
> >> as a R/W expanded object?
>
> > That confuses me too, and my default assumption is always that I'm doing
> it
> > wrong.  set_element does return a R/W object afaict, here is the return:
> > https://github.com/OneSparse/OneSparse/blob/main/src/matrix.c#L1726
>
> Hmph.  That seems right.  Can you add errbacktrace() to your logging
> ereports, in hopes of seeing how we're getting to flatten_matrix?
> Or break there with gdb for a more complete/reliable stack trace.
>
>                         regards, tom lane
>

  18. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-10-31T23:51:59Z

    On Thu, Oct 24, 2024 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > ... I'm still writing up
> > details, but right now I'm envisioning completely separate sets of
> > rules for the prosupport case versus the no-prosupport case.
>
> So here is the design I've come up with for optimizing R/W expanded
> object updates in plpgsql without any special knowledge from a
> prosupport function.  AFAICS this requires no assumptions at all
> about the behavior of called functions, other than the bare minimum
> "you can't corrupt the object to the point where it wouldn't be
> cleanly free-able".  In particular that means it can work for
> user-written called functions in plpgsql, SQL, or whatever, not
> only for C-coded functions.
>

Great, I checked with the upstream library authors and they verified that
the object can't be corrupted to where it can't be freed.  Since my
expanded objects are just a box around a library handle, I use a
MemoryContext callback to call the library free function when the context
cleans up, and we can't think of a path where that will fail.


>
> There are two requirements to apply the optimization:
>
> * If the assignment statement is within a BEGIN ... EXCEPTION block,
> its target variable must be declared inside the most-closely-nested
> such block.  This ensures that if an error is thrown from within the
> assignment statement's expression, we do not care about the value
> of the target variable, except to the extent of being able to clean
> it up.
>

My users are writing algebraic expressions to be done in bulk on GPUs,
etc.  I don't think I have to worry too much about wrapping stuff in
exception blocks while handling my library objects.

<snip>

> While I've not tried to write any code yet, I think both of these
> conditions should be reasonably easy to verify.
>
> Given that those conditions are met and the current value of the
> assignment target variable is a R/W expanded pointer, we can
> execute the assignment as follows:
>
> <snip>

> So, while this design greatly expands the set of cases we can
> optimize, it does lose some cases that the old approach could
> support.  I envision addressing that by allowing a prosupport
> function attached to the RHS' topmost function to "bless"
> other cases as safe, using reasoning similar to the old rules.
> (Or different rules, even, but it's on the prosupport function
> to be sure it's safe.)  I don't have a detailed design in mind,
> but I'm thinking along the lines of just passing the whole RHS
> expression to the prosupport function and letting it decide
> what's safe.  In any case, we don't need to even call the
> prosupport function unless there's an exception block or
> multiple RHS references to the target variable.
>

That all sounds great, and it sounds like my prosupport function just needs
to return true, or set some kind of flag saying aliasing is ok.  I'd like
to help as much as possible, but some of that reparenting stuff was pretty
deep for me, other than being a quick sanity check case, is there anything
I can do to help?


>
>                         regards, tom lane
>

  19. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-01T22:20:09Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> Here's two backtraces from gdb from this function:
> CREATE OR REPLACE FUNCTION test2(graph matrix)
>     RETURNS bigint LANGUAGE plpgsql AS
>     $$
>     BEGIN
>     perform set_element(graph, 1, 1, 1);
>     RETURN nvals(graph);
>     end;
>     $$;

Well, you shouldn't be using PERFORM.  Not only does it not do the
right thing, but it's not optimized for expanded objects at all:
they'll get flattened both on the way into the statement and on
the way out.  Try it with

     graph := set_element(graph, 1, 1, 1);
     RETURN nvals(graph);

			regards, tom lane

  20. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-01T22:27:53Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> That all sounds great, and it sounds like my prosupport function just needs
> to return true, or set some kind of flag saying aliasing is ok.  I'd like
> to help as much as possible, but some of that reparenting stuff was pretty
> deep for me, other than being a quick sanity check case, is there anything
> I can do to help?

I wasn't expecting you to write the code, but if you can test it and
see how much it helps your use-case, that would be great.

Here is a v1 patch series that does the first part of what we've been
talking about, which is to implement the new optimization rule for
the case of a single RHS reference to the target variable.  I'm
curious to know if it helps you at all by itself.  You'll definitely
also need commit 534d0ea6c, so probably applying these on our git
master branch would be the place to start.

			regards, tom lane

  21. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-11-02T00:50:52Z

    On Fri, Nov 1, 2024 at 3:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
>
> Well, you shouldn't be using PERFORM.  Not only does it not do the
> right thing, but it's not optimized for expanded objects at all:
> they'll get flattened both on the way into the statement and on
> the way out.  Try it with
>
>      graph := set_element(graph, 1, 1, 1);
>      RETURN nvals(graph);
>

Ah my bad, you mentioned that already and I missed it, here's the two
backtraces with the assignment:

https://gist.githubusercontent.com/michelp/20b917686149d482be2359569845f232/raw/ca8349ae4b0469674b4b2c77f34c5a02553583a9/gistfile1.txt


>
>                         regards, tom lane
>

  22. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-11-02T00:53:14Z

    On Fri, Nov 1, 2024 at 3:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
>
> Here is a v1 patch series that does the first part of what we've been
> talking about, which is to implement the new optimization rule for
> the case of a single RHS reference to the target variable.  I'm
> curious to know if it helps you at all by itself.  You'll definitely
> also need commit 534d0ea6c, so probably applying these on our git
> master branch would be the place to start.
>

I'll apply these tonight and get back to you asap.  There are many
functions in my API that take only one expanded RHS argument, so I'll look
for some cases where your changes reduce expansions when I run my tests.

Thank you!

-Michel

  23. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-11-11T21:27:12Z

    On Fri, Nov 1, 2024 at 5:53 PM Michel Pelletier <pelletier.michel@gmail.com>
wrote:

> On Fri, Nov 1, 2024 at 3:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> Michel Pelletier <pelletier.michel@gmail.com> writes:
>>
>> Here is a v1 patch series that does the first part of what we've been
>> talking about, which is to implement the new optimization rule for
>> the case of a single RHS reference to the target variable.  I'm
>> curious to know if it helps you at all by itself.  You'll definitely
>> also need commit 534d0ea6c, so probably applying these on our git
>> master branch would be the place to start.
>>
>
> I'll apply these tonight and get back to you asap.  There are many
> functions in my API that take only one expanded RHS argument, so I'll look
> for some cases where your changes reduce expansions when I run my tests.
>

I tested these patches with my test setup and can confirm there is now one
less expansion in this function:

create or replace function test_expand(graph matrix) returns matrix
language plpgsql as
    $$
    declare
        nvals bigint = nvals(graph);
    begin
        return graph;
    end;
    $$;

postgres=# select test_expand(a) from test_fixture ;
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
DEBUG:  matrix_out
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free

The second expansion in matrix_out happens outside the function, so inside
there is only the one expansion for both matrix_nvals and the assignment.
Thank you!  All my tests continue to pass and the change seems to work well.

Looking forward to helping test the support function idea, let me know if
there's anything else I can do to validate the idea.

-Michel

>

  24. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-11-11T21:34:06Z

    >
> The second expansion in matrix_out happens outside the function, so inside
> there is only the one expansion for both matrix_nvals and the assignment.
> Thank you!  All my tests continue to pass and the change seems to work well.


Hmm, well I spoke to soon looking at the wrong function without an
assignment, I've been down in gdb a bit too much and I got mixed up, here's
another function that wraps it and does an assignment, and it looks like
there is another expansion happening after the nvals but before the
assignment:

create or replace function test_expand_expand(graph matrix) returns matrix
language plpgsql as
    $$
    declare
        nvals bigint = nvals(graph);
    begin
        graph = test_expand(graph);
        return test_expand(graph);
    end;
    $$;

postgres=# select test_expand_expand(a) from test_fixture ;
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
DEBUG:  matrix_out
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free

So going back on the assumption I'm somehow not returning the right
pointer, but digging into the array code I'm pretty sure I'm following the
same pattern and tracing my code path is calling EOHPGetRWDatum when I
return the object, I can't get it to stop on DeleteExpandedObject, so I
don't think plpgsql is deleting it, I'm going to keep investigating, sorry
for the noise.

-Michel

>

  25. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-11-19T17:50:29Z

    >
>
> So going back on the assumption I'm somehow not returning the right
> pointer, but digging into the array code I'm pretty sure I'm following the
> same pattern and tracing my code path is calling EOHPGetRWDatum when I
> return the object, I can't get it to stop on DeleteExpandedObject, so I
> don't think plpgsql is deleting it, I'm going to keep investigating, sorry
> for the noise.
>

A couple years ago I tried to compress what I learned about expanded
objects into a dummy extension that just provides the necessary
boilerplate.  It wasn't great but a start:

https://github.com/michelp/pgexpanded

Pavel Stehule indicated this might be a good example to put into contrib:

https://www.postgresql.org/message-id/CAFj8pRDnAnsm8pMosEys-TDRZpnCx1KaxePjV8HT6A1JPQtsCw@mail.gmail.com

I've updated the repo to include points from our current discussion wrt
multiple expansions in a plpgsql function, and added some test functions.
The new pgexpanded type just keeps track of the number of times it's been
expanded starting from an initialized value.  While it only stores a
flattened integer, it follows the typical pattern of pallocing the value
and pfreeing it with a memory context callback, so it should be covering
most of the boilerplate needed to start a new expanded type.

This would also be a good place to showcase and test the support function
feature you've described to me.

It occurs to me that including this example in contrib would be an easier
way for us to collaborate on my existing issue instead of punting back and
forth on the list and would benefit all future expanded object developers.
Do you think that's a good idea?  If this were in contrib you could access
the code I'm working with directly and I can just follow along in my
project.

-Michel

  26. Re: Using Expanded Objects other than Arrays from plpgsql

    Pavel Stehule <pavel.stehule@gmail.com> — 2024-11-19T18:44:49Z

    út 19. 11. 2024 v 18:51 odesílatel Michel Pelletier <
pelletier.michel@gmail.com> napsal:

>
>> So going back on the assumption I'm somehow not returning the right
>> pointer, but digging into the array code I'm pretty sure I'm following the
>> same pattern and tracing my code path is calling EOHPGetRWDatum when I
>> return the object, I can't get it to stop on DeleteExpandedObject, so I
>> don't think plpgsql is deleting it, I'm going to keep investigating, sorry
>> for the noise.
>>
>
> A couple years ago I tried to compress what I learned about expanded
> objects into a dummy extension that just provides the necessary
> boilerplate.  It wasn't great but a start:
>
> https://github.com/michelp/pgexpanded
>
> Pavel Stehule indicated this might be a good example to put into contrib:
>
>
> https://www.postgresql.org/message-id/CAFj8pRDnAnsm8pMosEys-TDRZpnCx1KaxePjV8HT6A1JPQtsCw@mail.gmail.com
>
> I've updated the repo to include points from our current discussion wrt
> multiple expansions in a plpgsql function, and added some test functions.
> The new pgexpanded type just keeps track of the number of times it's been
> expanded starting from an initialized value.  While it only stores a
> flattened integer, it follows the typical pattern of pallocing the value
> and pfreeing it with a memory context callback, so it should be covering
> most of the boilerplate needed to start a new expanded type.
>
> This would also be a good place to showcase and test the support function
> feature you've described to me.
>
> It occurs to me that including this example in contrib would be an easier
> way for us to collaborate on my existing issue instead of punting back and
> forth on the list and would benefit all future expanded object developers.
> Do you think that's a good idea?  If this were in contrib you could access
> the code I'm working with directly and I can just follow along in my
> project.
>

another position can be src/test/modules - I think so your example is
"similar" to plsample

Regards

Pavel


>
> -Michel
>

  27. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-19T19:45:16Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> út 19. 11. 2024 v 18:51 odesílatel Michel Pelletier <
> pelletier.michel@gmail.com> napsal:
>> A couple years ago I tried to compress what I learned about expanded
>> objects into a dummy extension that just provides the necessary
>> boilerplate.  It wasn't great but a start:
>> https://github.com/michelp/pgexpanded
>> Pavel Stehule indicated this might be a good example to put into contrib:

> another position can be src/test/modules - I think so your example is
> "similar" to plsample

Yeah.  I think we've largely adopted the position that contrib should
contain installable modules that do something potentially useful to
end-users.  A pure skeleton wouldn't be that, but if it's fleshed out
enough to be test code for some core features then src/test/modules
could be a reasonable home.

			regards, tom lane

  28. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-11-19T20:52:46Z

    On Tue, Nov 19, 2024 at 11:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > út 19. 11. 2024 v 18:51 odesílatel Michel Pelletier <
> > pelletier.michel@gmail.com> napsal:
> >> A couple years ago I tried to compress what I learned about expanded
> >> objects into a dummy extension that just provides the necessary
> >> boilerplate.  It wasn't great but a start:
> >> https://github.com/michelp/pgexpanded
> >> Pavel Stehule indicated this might be a good example to put into
> contrib:
>
> > another position can be src/test/modules - I think so your example is
> > "similar" to plsample
>
> Yeah.  I think we've largely adopted the position that contrib should
> contain installable modules that do something potentially useful to
> end-users.  A pure skeleton wouldn't be that, but if it's fleshed out
> enough to be test code for some core features then src/test/modules
> could be a reasonable home.
>

Great!  I'll put a patch together that adds the skeleton object to
src/test/modules and I'll write some expected tests that run the expansion
through its paces, when the support function feature happens I'll update it
to include tests for that.

Should I include Tom's patch changes on top of mine or keep those
separate?  I'm not entirely clear on the best practice to carry those
forward as well.

-Michel

  29. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-11-25T02:02:17Z

    On Tue, Nov 19, 2024 at 12:52 PM Michel Pelletier <
pelletier.michel@gmail.com> wrote:

> On Tue, Nov 19, 2024 at 11:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> > another position can be src/test/modules - I think so your example is
>> > "similar" to plsample
>>
>> Yeah.  I think we've largely adopted the position that contrib should
>> contain installable modules that do something potentially useful to
>> end-users.  A pure skeleton wouldn't be that, but if it's fleshed out
>> enough to be test code for some core features then src/test/modules
>> could be a reasonable home.
>>
>
> Great!  I'll put a patch together that adds the skeleton object to
> src/test/modules and I'll write some expected tests that run the expansion
> through its paces, when the support function feature happens I'll update it
> to include tests for that.
>

Here's a WIP patch for a pgexpanded example in src/test/modules.  The
object is very simple and starts with an integer and increments that value
every time it is expanded.  I added some regression tests that test two sql
functions that replicate the expansion issue that I'm seeing with my
extension.

I considered a more complex data type like a linked list, something that
could maybe also showcase subscripting support for expanded objects, but I
didn't want to go too far without discussion.

-Michel

>

  30. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-04T00:42:03Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> Here's a WIP patch for a pgexpanded example in src/test/modules.

I didn't look at your patch yet, but in the meantime here's an update
that takes the next step towards what I promised.

0001-0003 are the same as before, with a couple of trivial changes
to rebase them up to current HEAD.  0004 adds a support function
request to allow extension functions to perform in-place updates.
You should be able to use that to improve what your extension
is doing.  The new comments in supportnodes.h explain how to
use it (plus see the built-in examples, though they are quite
simple).

			regards, tom lane

  31. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-05T20:34:13Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> Here's a WIP patch for a pgexpanded example in src/test/modules.  The
> object is very simple and starts with an integer and increments that value
> every time it is expanded.  I added some regression tests that test two sql
> functions that replicate the expansion issue that I'm seeing with my
> extension.

I took a look through this and have a few comments:

* I really don't like the "increments when expanded" behavior.  That's
just insane from a semantic viewpoint: expanding and then flattening
a value should not change it, at least not in any user-visible way.
I get that you want the tests to exhibit how many times expansion
happened, but the LOGF() debug printouts seem to serve that need just
fine.  How about we just make the objects store a palloc'd string,
or some such?

* context_callback_exobj_free() seems bizarre.  I can see the
usefulness of the LOGF() printout for the test's purposes, but there's
no need for the pfree() because the whole context is about to go away.
I'm concerned that people using this as a skeleton might think they
need to do something equivalent, when they don't.

* Maybe the answer to the above objection is to make the expanded
object hold a malloc'd not palloc'd string, so that it's actually
necessary to have an explicit free() to avoid a permanent memory
leak.  This'd be silly in a real use-case, and should be
documented as such; but it seems good to have a callback function
to demonstrate how to clean up resources outside the expanded
object itself.

* BTW, it might be good to make LOGF() print more than just the
function's name; some indication of what it's been called on
could be very valuable.

* If we believe that this is a skeleton other people might
use as a starting point, it'd be good to have more comments
explaining what each bit is doing and why.  For instance,
I think it's important to note that a context callback function
isn't required if deleting the memory context is sufficient
to clean up the object.

* It seems like test_expand() and test_expand_expand() belong
to the pgexpanded.sql test script and should be defined there,
rather than being part of the extension.

* As the patch stands, the module would never be invoked by
"make check", other than by manually invoking that in the
new subdirectory.  You need to hook it into the parent
directory's Makefile.  And you need meson infrastructure
too, ie a meson.build file.  (Should be able to mostly crib
that from one of the sibling test modules.)

* This does not seem like a great idea to me:
   +SET client_min_messages = 'debug1';
It's pure luck if the test output doesn't get messed up by
somebody else's unrelated debug output, because there's
elog(DEBUG1) in a lot of places and more could show up any day.
I'd be inclined to make LOGF() emit messages at NOTICE level,
and then the test doesn't have to mess with client_min_messages
at all.

* There's a whole bunch of minor ways in which this doesn't
conform to project style:

- We don't use markdown (.md) for per-directory README files.
  (There's been discussion of that, but we're not there today.)
  I would not use a URL to cross-reference our docs, either.

- "create or replace function" isn't great style in test
  scripts, much less extension scripts.  We're not expecting
  conflicting functions to exist, and if one does it's better
  to error out than silently overwrite it.

- We put '#include "postgres.h"' in .c files, never in .h files.

- At least the .c and .h files should carry standard header
  comments with a PGDG copyright notice.

- Code layout and comments are frequently not per style.
  You can mostly fix this by running the .c and .h
  files through pgindent, but it might mangle your comments;
  usually best to make those look like project standard first.
  See https://www.postgresql.org/docs/devel/source-format.html

- Declaring static functions in a .h file doesn't seem like
  a great plan.  If the thing is only meant to be included in
  one .c file, why bother with a separate .h file at all?
  "static const ExpandedObjectMethods exobj_methods" belongs
  there even less, since you'd end with one copy per
  including file.

- We don't put "/* Local Variables: */" comments into code
  files; we generally expect the source files to be agnostic
  about what editors people use on them.


To move forward, I'd suggest dealing with these concerns first,
and then as a separate patch start adding things like
in-place-modification support.  It'd be cool to see the
results of that optimization manifest as fewer expansions
visible in the test's output.

			regards, tom lane

  32. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-12-07T00:51:50Z

    On Tue, Dec 3, 2024 at 4:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
> > Here's a WIP patch for a pgexpanded example in src/test/modules.
>
> I didn't look at your patch yet, but in the meantime here's an update
> that takes the next step towards what I promised.
>

Awesome!  I made a support function for my set_element(matrix, i, j, value)
function and it works great, but I do have a couple questions about how to
move forward with some of my other methods.  For reference, here's the
set_element test function, and a trace of function calls, after
implementing the support function for set_element, the expand_matrix
function is called twice, first by nvals(), and later by the first
set_element, but not the subsequent sets, so the true clause of the
PLPGSQL_RWOPT_INPLACE case in plpgsql_param_eval_var_check works great and
as expected:

postgres=# create or replace function test_se(graph matrix) returns matrix
language plpgsql as
    $$
    declare
        nvals bigint;
    begin
        graph = wait(graph);
        nvals = nvals(graph);
        raise notice 'nvals: %', nvals;
        graph = set_element(graph, 4, 2, 42);
        graph = set_element(graph, 4, 3, 43);
        graph = set_element(graph, 4, 4, 44);
        return graph;
    end;
    $$;
CREATE FUNCTION
postgres=# select nvals(test_se('int32'::matrix));
DEBUG:  new_matrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  matrix_wait
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix            <- wait expands with support function
DEBUG:  new_matrix
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  expand_matrix            <- nvals() reexpands
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
NOTICE:  nvals: 0
DEBUG:  scalar_int32
DEBUG:  new_scalar
DEBUG:  matrix_set_element
DEBUG:  DatumGetMatrix            < set_element does not reexpand, yay!
DEBUG:  DatumGetScalar
DEBUG:  context_callback_scalar_free
DEBUG:  scalar_int32
DEBUG:  new_scalar
DEBUG:  matrix_set_element
DEBUG:  DatumGetMatrix
DEBUG:  DatumGetScalar
DEBUG:  context_callback_scalar_free
DEBUG:  scalar_int32
DEBUG:  new_scalar
DEBUG:  matrix_set_element
DEBUG:  DatumGetMatrix
DEBUG:  DatumGetScalar
DEBUG:  context_callback_scalar_free
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  context_callback_matrix_free
DEBUG:  context_callback_matrix_free
┌───────┐
│ nvals │
├───────┤
│     3 │
└───────┘

 My question is about nvals = nvals(graph) in that function above, the
object is flattened and then rexpanded, even after the object was expanded
and returned by wait() [1] into a r/w pointer.  set_element honors the
expanded object from wait(), but nvals does not.  It seems like I want to
be able to pass the argument as a RO pointer, but I'm not sure how to
trigger the "else" clause in the PLPGSQL_RWOPT_INPLACE case.  I can see how
the support function triggers the if, but I don't see a similar way to
trigger the else.  I'm almost certainly missing something.

[1](Due to the asynchronous nature of the GraphBLAS API, there is a
GrB_wait() function to wait until an object is "complete" computationally,
but since this object was just expanded, there is no pending work, so the
wait() is essentially a noop but a handy way for me to return a r/w pointer
for subsequent operations).

set_element and wait are the simple case where there is only one reference
to the r/w object in the argument list.  As we've discussed I have many
functions that can possibly have multiple references to the same object.
The core operation of the library is matrix multiplication, and all other
complex functions follow a very similar pattern, so I'll just focus on that
one here:

CREATE FUNCTION mxm(
    a matrix,
    b matrix,
    op semiring default null,
    inout c matrix default null,
    mask matrix default null,
    accum binaryop default null,
    descr descriptor default null
    )
RETURNS matrix

The order of arguments mostly follows the order in the C API.  a and b are
the left and right matrix operands and the matrix product is the return
value.  If c is not null, then it is a pre-created return value which may
contain partial results already from some previous operations, otherwise
mxm creates a new matrix of the correct dimensions and returns that.  I
think the inout is meaningless as it doesn't seem to change anything, but
I'm using it as a visual indication in code that c can be the return value
if it's not null.

Here's an example of doing Triangle Counting using Burkhardt's method [2],
where a, b, and the mask are all the same adjacency matrix (here the
Newman/karate graph.  The 'plus_pair' semiring is optimized for structural
counting, and the descriptor 's' tells suitesparse to only use the
structure of the mask and not to consider the values):

[2] https://doi.org/10.1177/1473871616666393

CREATE OR REPLACE FUNCTION public.tcount_burkhardt(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;
    $$;

postgres=# select tcount_burkhardt(graph) from karateg;
DEBUG:  matrix_wait
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix                    <- wait expands and returns r/w
pointer with support function
DEBUG:  new_matrix
DEBUG:  matrix_mxm                        <- mxm starts here
DEBUG:  DatumGetMatrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  expand_matrix                    <- expanding left operand again
DEBUG:  new_matrix
DEBUG:  DatumGetMatrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  expand_matrix                    <- expanding right operand again
DEBUG:  new_matrix
DEBUG:  DatumGetSemiring
DEBUG:  expand_semiring
DEBUG:  new_semiring
DEBUG:  new_matrix
DEBUG:  DatumGetMatrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  expand_matrix                    <- expanding mask argument again
DEBUG:  new_matrix
DEBUG:  DatumGetDescriptor
DEBUG:  expand_descriptor
DEBUG:  new_descriptor
DEBUG:  context_callback_matrix_free
DEBUG:  context_callback_descriptor_free
DEBUG:  context_callback_matrix_free
DEBUG:  context_callback_semiring_free
DEBUG:  context_callback_matrix_free
DEBUG:  context_callback_matrix_free
DEBUG:  matrix_reduce_scalar
DEBUG:  DatumGetMatrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  expand_matrix                                        <- reduce also
re-expands matrix
DEBUG:  new_matrix
DEBUG:  new_scalar
DEBUG:  scalar_div_int32
DEBUG:  DatumGetScalar
DEBUG:  new_scalar
DEBUG:  cast_scalar_int64
DEBUG:  DatumGetScalar
DEBUG:  context_callback_scalar_free
DEBUG:  context_callback_scalar_free
DEBUG:  context_callback_matrix_free
DEBUG:  context_callback_matrix_free
┌──────────────────┐
│ tcount_burkhardt                  │
├──────────────────┤
│               45                           │
└──────────────────┘

mxm calls expand_matrix three times for each of the three arguments.
Ideally I'd like the already expanded rw pointer from wait() to be honored
by mxm so that it doesn't re-expand the object three times but, like
set_element, not at all.

Hope that question makes sense, still going on the main theory that I'm not
understanding the support function, and maybe the c argument thing being
optional throws a wrench in the plan, and I'm happy to try and find a
workaround for that.  Maybe always requiring the result to be
pre-constructed and then making c required and reassigning back to the same
input argument is the right approach?

Some good news I always like seeing is that 45 is the right answer.  Very
close to having optimal sparse linear algebra in Postgres!

Thanks for your help!  I'll move onto your comments on the test module next.

-Michel

  33. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-12-08T07:05:13Z

    On Fri, Dec 6, 2024 at 4:51 PM Michel Pelletier <pelletier.michel@gmail.com>
wrote:

>
>  My question is about nvals = nvals(graph) in that function above, the
> object is flattened and then rexpanded, even after the object was expanded
> and returned by wait() [1] into a r/w pointer.
>
...

>
> mxm calls expand_matrix three times for each of the three arguments.
> Ideally I'd like the already expanded rw pointer from wait() to be honored
> by mxm so that it doesn't re-expand the object three times but, like
> set_element, not at all.
>

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.

Thanks for your help Tom, I'm looking forward to the changes being released
soon!  In the meanwhile I'll keep a locally patched version for ongoing
testing purposes.

-Michel

>

  34. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-18T20:22:09Z

    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?

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.

			regards, tom lane

  35. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-12-23T03:52:08Z

    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

  36. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-23T16:26:41Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> On Wed, Dec 18, 2024 at 12:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So, just to clarify where we're at: you are satisfied that the current
>> patch-set does what you need?

> 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;
>     $$;

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

Hmm.  I agree that the wait() call is a bit ugly, but there are at
least two things that seem worth looking into before we go so far
as inventing type-support infrastructure:

1. Why isn't the incoming "graph" object already expanded?  It
often would be read-only, but that seems like it might be enough
given your description of GraphBLAS' behavior.

2. If the problem is primarily with passing the same object to
multiple parameters of a function, couldn't you detect and optimize
that within the function?  It would be messier than just blindly
applying DatumGetWhatever() to each parameter position; but with a
bit of thought I bet you could create some support logic that would
hide most of the mess.

			regards, tom lane

  37. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2024-12-25T20:25:18Z

    On Mon, Dec 23, 2024 at 8:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
> > ...
> > 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.
>
> Hmm.  I agree that the wait() call is a bit ugly, but there are at
> least two things that seem worth looking into before we go so far
> as inventing type-support infrastructure:
>

2. If the problem is primarily with passing the same object to
> multiple parameters of a function, couldn't you detect and optimize
> that within the function?  It would be messier than just blindly
> applying DatumGetWhatever() to each parameter position; but with a
> bit of thought I bet you could create some support logic that would
> hide most of the mess.
>

Ah that's a great idea, and it works beautifully!  Now I can do an
efficient triangle count without even needing a function, see below
expand_matrix is only called once:

postgres=# select reduce_scalar(mxm(graph, graph, mask=>graph, c=>graph)) /
6 as tcount from vlivejournals ;
DEBUG:  matrix_mxm
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix   -- only called once!
DEBUG:  new_matrix
DEBUG:  DatumGetMatrixMaybeA
DEBUG:  DatumGetMatrixMaybeAB
DEBUG:  DatumGetMatrixMaybeABC
DEBUG:  matrix_reduce_scalar
DEBUG:  DatumGetMatrix
DEBUG:  new_scalar
DEBUG:  scalar_div_int32
DEBUG:  new_scalar
DEBUG:  scalar_out
┌─────────────────┐
│     tcount      │
├─────────────────┤
│ int32:177820130 │
└─────────────────┘

What a wonderful Christmas present to me, thank you Tom!

That pretty much resolves my main issues.  I'm still in an exploratory
phase but I think this gets me pretty far.  Is this something that has to
wait for 18 to be released?  Also do you need any further testing or code
reviewing from me?

-Michel

  38. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-27T00:32:11Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> On Mon, Dec 23, 2024 at 8:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2. If the problem is primarily with passing the same object to
>> multiple parameters of a function, couldn't you detect and optimize
>> that within the function?

> Ah that's a great idea, and it works beautifully!  Now I can do an
> efficient triangle count without even needing a function, see below
> expand_matrix is only called once:

Nice!

> That pretty much resolves my main issues.  I'm still in an exploratory
> phase but I think this gets me pretty far.  Is this something that has to
> wait for 18 to be released?  Also do you need any further testing or code
> reviewing from me?

Yeah, this is not the kind of change we would back-patch, so it'll
have to wait for v18 (or later if people are slow to review it :-( ).

I don't think there's anything more we need from you, though of course
you should keep working on your code and report back if you hit
anything that needs further improvement on our end.

			regards, tom lane

  39. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2025-01-04T16:37:39Z

    On Tue, Nov 19, 2024 at 11:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > út 19. 11. 2024 v 18:51 odesílatel Michel Pelletier <
> > pelletier.michel@gmail.com> napsal:
> >> A couple years ago I tried to compress what I learned about expanded
> >> objects into a dummy extension that just provides the necessary
> >> boilerplate.  It wasn't great but a start:
> >> https://github.com/michelp/pgexpanded
> >> Pavel Stehule indicated this might be a good example to put into
> contrib:
>
> > another position can be src/test/modules - I think so your example is
> > "similar" to plsample
>
> Yeah.  I think we've largely adopted the position that contrib should
> contain installable modules that do something potentially useful to
> end-users.  A pure skeleton wouldn't be that, but if it's fleshed out
> enough to be test code for some core features then src/test/modules
> could be a reasonable home.
>

I've circled back on this task to do some work improving the skeleton code,
but going back through our thread I landed on this point Tom made about
usefulness vs pure skeleton and my natural desire is to make a simple
expanded object that is also useful, so I brainstormed a bit and decided to
try something relatively simple but also (IMO) quite useful, an expanded
datum that wraps sqlite's serialize/derserialize API:

https://github.com/michelp/postgres-sqlite

As crazy as this sounds there are some good use cases here, very easy to
stuff relational data into a completely isolated box without having to
worry about things like very granular RLS policies or other issues of
traditional postgres multi-tenancy.  Being wire compatible with sqlite-wasm
also means databases can be slurped right from postgres into a browser and
synced with no need to transform data back and forth.  Large chunks of
complex structured relational data can be wiped out with a simple row
deletion, and since sqlite can't escape from its box and has no scripting
ability, it makes a nice secure sandbox that even if users could corrupt
it, it would have minimal impact on Postgres.

It's only a bit more complicated than the pgexpanded skeleton and the
expanded datum bits are is their own separate C file so they can be studied
in isolation.  Based on the above comments, this seems something more
appropriate for contrib than test/modules, although I can see there may be
some understandable pushback about something so weird that also has an
external library dependency.

Any thoughts?  I want to nail down the core functionality before I go back
and clean up either case based on Tom review comments on the skeleton
module (most of which still apply since I used the skeleton to make it!)

-Michel

  40. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-04T19:35:47Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> I've circled back on this task to do some work improving the skeleton code,
> but going back through our thread I landed on this point Tom made about
> usefulness vs pure skeleton and my natural desire is to make a simple
> expanded object that is also useful, so I brainstormed a bit and decided to
> try something relatively simple but also (IMO) quite useful, an expanded
> datum that wraps sqlite's serialize/derserialize API:
> https://github.com/michelp/postgres-sqlite

I think the odds that we'd accept a module with a dependency on sqlite
are negligible.  It's too big of a build dependency for too little
return.  Also, I'm sure that a module defined like that would be a
pretty poor example/starting point for other expanded-object
applications: there'd be too many aspects that have only to do with
interfacing to sqlite, making it hard to see the expanded-object
forest for the sqlite trees.

I have to admit though that the forest-v-trees aspect makes it fairly
hard to think of any suitable example module that would serve much
real-world purpose.  Likely scenarios for expanded objects just have
a lot of functionality in them.  For instance, I thought for a moment
of suggesting that teaching contrib/hstore to work with expanded
representations of hstores could be useful.  But I'd forgotten how
much functionality that type has.  It'd be a big project and would
still have a lot of baggage.

			regards, tom lane

  41. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2025-01-04T20:34:40Z

    On Sat, Jan 4, 2025 at 11:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michel Pelletier <pelletier.michel@gmail.com> writes:
> > I've circled back on this task to do some work improving the skeleton
> code,
> > but going back through our thread I landed on this point Tom made about
> > usefulness vs pure skeleton and my natural desire is to make a simple
> > expanded object that is also useful, so I brainstormed a bit and decided
> to
> > try something relatively simple but also (IMO) quite useful, an expanded
> > datum that wraps sqlite's serialize/derserialize API:
> > https://github.com/michelp/postgres-sqlite
>
> I think the odds that we'd accept a module with a dependency on sqlite
> are negligible.  It's too big of a build dependency for too little
> return.


That's fair, I wasn't sure if contrib modules could have optional build
dependencies that just skip building that module if they are not
installed.  If this were the case for users who want to study the approach
they can look at the code, and users who want to use the feature can
install the dependency or maybe require a configuration flag like
--with-sqlite.


> Also, I'm sure that a module defined like that would be a
> pretty poor example/starting point for other expanded-object
> applications: there'd be too many aspects that have only to do with
> interfacing to sqlite, making it hard to see the expanded-object
> forest for the sqlite trees.
>

I don't agree it would be a poor example, there are really only two touch
points with sqlite that matter, the call to sqlite3_serialize in the
flattening function and sqlite3_deserialize in the expander and consist of
a simple pointer exchange and memcpy.  That code is in its own file,
separate from the exec/query/dump code which can be effectively ignored by
someone looking to understand the expansion life cycle.


> I have to admit though that the forest-v-trees aspect makes it fairly
> hard to think of any suitable example module that would serve much
> real-world purpose.  Likely scenarios for expanded objects just have
> a lot of functionality in them.


Agree that an expanded object is only useful if it provides functionality.
My original pgexpanded extension from way back provided a dumb dense matrix
to do matrix multiplication, but I trimmed it out to just be the counter as
posted earlier in this thread, and you in turn mentioned maybe it should
just be a malloc'ed string, but in either case the pointlessness of it
bothers me a bit so I was hoping to find something that just crosses the
line into useful while still being a really simply expanded example.

-Michel

  42. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-15T18:09:03Z

    I noticed that v2 of this patch series failed to apply after
7b27f5fd3, so here's v3.  No non-trivial changes.

			regards, tom lane

  43. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2025-01-21T18:05:48Z

    On Wed, Jan 15, 2025 at 10:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I noticed that v2 of this patch series failed to apply after
> 7b27f5fd3, so here's v3.  No non-trivial changes.
>

Thanks Tom!  These applied cleanly to my test env and actually increased my
livejournal graph benchmark by a million edges per second, so I guess I
didn't have all the previous changes in my last build as you noted.

-Michel

  44. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-21T18:12:05Z

    Michel Pelletier <pelletier.michel@gmail.com> writes:
> Thanks Tom!  These applied cleanly to my test env and actually increased my
> livejournal graph benchmark by a million edges per second, so I guess I
> didn't have all the previous changes in my last build as you noted.

Nice!  I hope somebody will review this, because I'd really like to
get it into v18, and feature freeze is getting closer.

			regards, tom lane

  45. Re: Using Expanded Objects other than Arrays from plpgsql

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-01-26T10:07:22Z

    Hello everyone in this thread.

> On 21 Jan 2025, at 23:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> somebody will review this

I'm trying to dig into the patch set. My knowledge of the module is shallow and I hope to improve it by reading more patches in this area.

This patch set provides a new test, which runs just fine without the patch. But it's somewhat expected, such optimizations must be transparent for user...

And the coverage of newly invented mark_stmt() 42.37%. Some of branches are easy noops, but some are not.
I assume as a granted that we will not every get into infinite loop in a recursive call of mark_stmt().

expr_is_assignment_source() is named like if it should return nool, but it's void.

I could not grasp from reading the code one generic question about new optimization rule. What cost does checking for possible in-place update incurs to code cannot have this optimization? Is it O(numer_of_arguments) of for every assignment execution?

Thanks!


Best regards, Andrey Borodin.

  46. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-26T15:37:05Z

    Andrey Borodin <x4mmm@yandex-team.ru> writes:
>> On 21 Jan 2025, at 23:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> somebody will review this

> I'm trying to dig into the patch set. My knowledge of the module is shallow and I hope to improve it by reading more patches in this area.

Thanks for looking!

> And the coverage of newly invented mark_stmt() 42.37%. Some of branches are easy noops, but some are not.

Yeah.  I'm not too concerned about that because it's pretty much a
copy-and-paste of the adjacent code.  Maybe we should think about
some way of refactoring pl_funcs.c to reduce duplication, but I
don't have any great ideas about how.

> expr_is_assignment_source() is named like if it should return nool, but it's void.

I've been less than satisfied with that name too.  I intended it
as a statement of fact, "this expression has been found to be 
the source of an assignment".  But it does seem confusing.
Maybe we should recast it as an action.  What do you think of
"mark_expr_as_assignment_source"?

> I could not grasp from reading the code one generic question about new optimization rule. What cost does checking for possible in-place update incurs to code cannot have this optimization? Is it O(numer_of_arguments) of for every assignment execution?

No, the extra effort is incurred at most once per assignment statement
per session.  (Unless the plpgsql function's cache entry gets
invalidated, in which case we'd rebuild all of the function's data
structures and have to redo this work too.)  We set up the evaluation
function "paramfunc" as plpgsql_param_eval_var_check if we think we
might be able to apply this optimization, or plpgsql_param_eval_var_ro
if we don't think so but the variable is of varlena type.  At runtime,
if the variable's current value is not actually expanded, then
plpgsql_param_eval_var_check falls through doing essentially the same
work as plpgsql_param_eval_var_ro, so there should be no added cost.
The first time we observe that the value *is* expanded, we incur the
cost to detect whether an optimization is really possible, and then
we change the "paramfunc" pointer to be the appropriate function
so as to apply the optimization or not without rechecking.  So
generally speaking, if we're considering a variable of a type that
doesn't support expansion, there should be zero extra per-execution
cost.  There is some extra cost at function compilation time to
determine which expressions are assignment sources (but we were doing
that already) and to discover whether those assignments are to
nonlocal variables (which is new work, but only needs to be done in
functions with exception blocks).

			regards, tom lane

  47. Re: Using Expanded Objects other than Arrays from plpgsql

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-01-26T17:04:15Z

    
> On 26 Jan 2025, at 20:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
>> And the coverage of newly invented mark_stmt() 42.37%. Some of branches are easy noops, but some are not.
> 
> Yeah.  I'm not too concerned about that because it's pretty much a
> copy-and-paste of the adjacent code.  Maybe we should think about
> some way of refactoring pl_funcs.c to reduce duplication, but I
> don't have any great ideas about how.

OK, now I got it. The whole purpose of 2nd patch is to do
if (expr->target_param >= 0)	expr->target_is_local = bms_is_member(expr->target_param, local_dnos);
to local variables. 

> 
>> expr_is_assignment_source() is named like if it should return nool, but it's void.
> 
> I've been less than satisfied with that name too.  I intended it
> as a statement of fact, "this expression has been found to be 
> the source of an assignment".  But it does seem confusing.
> Maybe we should recast it as an action.  What do you think of
> "mark_expr_as_assignment_source"?

Sounds better to me. I found no examples of similar functions nether in pl_gram.y, nor in gram.y, so IMO mark_expr_as_assignment_source() is the best candidate.

> 
>> I could not grasp from reading the code one generic question about new optimization rule. What cost does checking for possible in-place update incurs to code cannot have this optimization? Is it O(numer_of_arguments) of for every assignment execution?
> 
> No, the extra effort is incurred at most once per assignment statement
> per session.  (Unless the plpgsql function's cache entry gets
> invalidated, in which case we'd rebuild all of the function's data
> structures and have to redo this work too.)

OK, I think execution benefits justify this preparatory costs.

>  We set up the evaluation
> function "paramfunc" as plpgsql_param_eval_var_check if we think we
> might be able to apply this optimization, or plpgsql_param_eval_var_ro
> if we don't think so but the variable is of varlena type.  At runtime,
> if the variable's current value is not actually expanded, then
> plpgsql_param_eval_var_check falls through doing essentially the same
> work as plpgsql_param_eval_var_ro, so there should be no added cost.
> The first time we observe that the value *is* expanded, we incur the
> cost to detect whether an optimization is really possible, and then
> we change the "paramfunc" pointer to be the appropriate function
> so as to apply the optimization or not without rechecking.  So
> generally speaking, if we're considering a variable of a type that
> doesn't support expansion, there should be zero extra per-execution
> cost.  There is some extra cost at function compilation time to
> determine which expressions are assignment sources (but we were doing
> that already) and to discover whether those assignments are to
> nonlocal variables (which is new work, but only needs to be done in
> functions with exception blocks).

Got it, many thanks for the explanation.

But I've got some new questions:

I'm lost in internals of ExprEvalStep. But void *paramarg and his friend void *paramarg2 are cryptic. They always have same type and same meaning, but have very generic names.

I wonder if you plan similar optimizations for array_cat(), array_remove() etc?

+   a := a || a; -- not optimizable

Why is it not optimizable? Because there is no support function, because array_cat() has no support function, or something else?

Besides this, the patch looks good to me.


Best regards, Andrey Borodin.

  48. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-26T19:04:41Z

    Andrey Borodin <x4mmm@yandex-team.ru> writes:
> On 26 Jan 2025, at 20:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Maybe we should recast it as an action.  What do you think of
>> "mark_expr_as_assignment_source"?

> Sounds better to me. I found no examples of similar functions nether in pl_gram.y, nor in gram.y, so IMO mark_expr_as_assignment_source() is the best candidate.

WFM, I'll make it so in next version.

> Got it, many thanks for the explanation.

I'll see about incorporating more of that in the comments, too.

> I wonder if you plan similar optimizations for array_cat(), array_remove() etc?
> +   a := a || a; -- not optimizable
> Why is it not optimizable? Because there is no support function, because array_cat() has no support function, or something else?

plpgsql won't attempt to optimize it because "a" is referenced twice
and there is no support function that might say it's safe anyway.

array_cat doesn't currently have any special smarts about expanded
arrays, so it's all moot because the arrays would get flattened
on the way into it.  If we did improve it to be able to cope with
expanded arrays, I'm not real sure that it could safely manage an
in-place update where the two inputs are the same array --- at
the least, some extreme care would be needed to get the right
answers.

I'm not real excited about optimizing additional array operations
anyway.  Maybe some more will get done at some point, but I don't
see that as part of this work.

			regards, tom lane

  49. Re: Using Expanded Objects other than Arrays from plpgsql

    Pavel Borisov <pashkin.elfe@gmail.com> — 2025-01-30T20:32:03Z

    Hi, Michel and Tom!

On Sun, 26 Jan 2025 at 23:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrey Borodin <x4mmm@yandex-team.ru> writes:
> > On 26 Jan 2025, at 20:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Maybe we should recast it as an action.  What do you think of
> >> "mark_expr_as_assignment_source"?
>
> > Sounds better to me. I found no examples of similar functions nether in pl_gram.y, nor in gram.y, so IMO mark_expr_as_assignment_source() is the best candidate.
>
> WFM, I'll make it so in next version.
>
> > Got it, many thanks for the explanation.
>
> I'll see about incorporating more of that in the comments, too.
>
> > I wonder if you plan similar optimizations for array_cat(), array_remove() etc?
> > +   a := a || a; -- not optimizable
> > Why is it not optimizable? Because there is no support function, because array_cat() has no support function, or something else?
>
> plpgsql won't attempt to optimize it because "a" is referenced twice
> and there is no support function that might say it's safe anyway.
>
> array_cat doesn't currently have any special smarts about expanded
> arrays, so it's all moot because the arrays would get flattened
> on the way into it.  If we did improve it to be able to cope with
> expanded arrays, I'm not real sure that it could safely manage an
> in-place update where the two inputs are the same array --- at
> the least, some extreme care would be needed to get the right
> answers.
>
> I'm not real excited about optimizing additional array operations
> anyway.  Maybe some more will get done at some point, but I don't
> see that as part of this work.
>
>                         regards, tom lane

I started looking at the patchset.
Recently it got conflicts with changes to yyparse (473a575e05979b4db).
Could you rebase it and also do naming changes proposed by Andrew
Borodin, which I definitely agree with?

Regards,
Pavel Borisov

  50. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-31T01:53:41Z

    Pavel Borisov <pashkin.elfe@gmail.com> writes:
> I started looking at the patchset.
> Recently it got conflicts with changes to yyparse (473a575e05979b4db).
> Could you rebase it and also do naming changes proposed by Andrew
> Borodin, which I definitely agree with?

Hmm, it seemed to still apply for me.  But anyway, I needed to make
the other changes, so here's v4.

			regards, tom lane

  51. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-02T21:56:59Z

    I wrote:
> Hmm, it seemed to still apply for me.  But anyway, I needed to make
> the other changes, so here's v4.

I decided to see what would happen if we tried to avoid the code
duplication in pl_funcs.c by making some "walker" infrastructure
akin to expression_tree_walker.  While that doesn't seem useful
for the dump_xxx functions, it works very nicely for the free_xxx
functions and now for the mark_xxx ones as well.  pl_funcs.c
nets out about 400 lines shorter than in the v4 patch.  The
code coverage score for the file is still awful :-(, but that's
because we're not testing the dump_xxx functions at all.

PFA v5.  The new 0001 patch refactors the free_xxx infrastructure
to create plpgsql_statement_tree_walker(), and then in what's now
0003 we can use that instead of writing a lot of duplicate code.

			regards, tom lane

  52. Re: Using Expanded Objects other than Arrays from plpgsql

    Michel Pelletier <pelletier.michel@gmail.com> — 2025-02-03T03:02:16Z

    On Sun, Feb 2, 2025 at 1:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > Hmm, it seemed to still apply for me.  But anyway, I needed to make
> > the other changes, so here's v4.
>
> PFA v5.  The new 0001 patch refactors the free_xxx infrastructure
> to create plpgsql_statement_tree_walker(), and then in what's now
> 0003 we can use that instead of writing a lot of duplicate code.
>

Thanks Tom!  These patches apply for me and all my tests and benchmarks are
still good.

-Michel

  53. Re: Using Expanded Objects other than Arrays from plpgsql

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-02-03T09:19:09Z

    
> On 3 Feb 2025, at 02:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I decided to see what would happen if we tried to avoid the code
> duplication in pl_funcs.c by making some "walker" infrastructure
> akin to expression_tree_walker.  While that doesn't seem useful
> for the dump_xxx functions, it works very nicely for the free_xxx
> functions and now for the mark_xxx ones as well.  pl_funcs.c
> nets out about 400 lines shorter than in the v4 patch.  The
> code coverage score for the file is still awful :-(, but that's
> because we're not testing the dump_xxx functions at all.
> 
> PFA v5.  The new 0001 patch refactors the free_xxx infrastructure
> to create plpgsql_statement_tree_walker(), and then in what's now
> 0003 we can use that instead of writing a lot of duplicate code.


Pre-preliminary refactoring looks good to me, as the rest of the patch set.

(Well, maybe paramarg2 resonates a bit, just from similarity with varchar2)

ecpg tests seem to fail on Windows[0], but looks like it's not related to this thread.


Best regards, Andrey Borodin.

[0] https://cirrus-ci.com/task/4835794898124800

  54. Re: Using Expanded Objects other than Arrays from plpgsql

    Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-03T11:42:04Z

    Hi, Tom

On Mon, 3 Feb 2025 at 07:02, Michel Pelletier
<pelletier.michel@gmail.com> wrote:
>
> On Sun, Feb 2, 2025 at 1:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> I wrote:
>> > Hmm, it seemed to still apply for me.  But anyway, I needed to make
>> > the other changes, so here's v4.
>>
>> PFA v5.  The new 0001 patch refactors the free_xxx infrastructure
>> to create plpgsql_statement_tree_walker(), and then in what's now
>> 0003 we can use that instead of writing a lot of duplicate code.
>
>
> Thanks Tom!  These patches apply for me and all my tests and benchmarks are still good.

I've looked at the patches v4 and v5.
v5 logic of patch 0003 is much more understandable with refactoring
free_* functions to a walker. So I think it's much better than v4 even
regarding the principle have not changed.

Using support functions in 0005 complicates things. But I think it's
justified by extensibility and benchmarks demonstrated by Michel above
in the thread.

Overall patch to me looks well-structured, beneficial for performance
and extensibility and it looks good to me.

Minor notes on the patches:

If dump_* functions could use the newly added walker, the code would
look better. I suppose the main complication is that dump_* functions
contain a lot of per-statement prints/formatting. So maybe a way to
implement this is to put these statements into the existing tree
walker i.e. plpgsql_statement_tree_walker_impl() and add an argument
bool dump_debug into it. So in effect called with dump_debug=false
plpgsql_statement_tree_walker_impl() would walk silent, and with
dump_debug=false it would walk and print what is supposed to be
printed currently in dump_* functions. Maybe there are other problems
besides this?

For exec_check_rw_parameter():

I think renaming expr->expr_simple_expr to sexpr saves few bytes but
doesn't makes anything simpler, so if possible I'd prefer using just
expr->expr_simple_expr with necessary casts. Furtermore in this
function mostly we use cast results fexpr, opexpr and sbsref of
expr->expr_simple_expr that already has separate names.

Transferring target param as int paramid looks completely ok. But we
have unconditional checking Assert(paramid == expr->target_param + 1),
so it looks as a redundant split as of now. Do we plan a true split
and removal of this assert in the future?

Thanks for creating and working on this patch!
Regards,
Pavel Borisov
Supabase

  55. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-03T17:36:48Z

    Andrey Borodin <x4mmm@yandex-team.ru> writes:
> (Well, maybe paramarg2 resonates a bit, just from similarity with varchar2)

I'm not wedded to that name; do you have a better idea?

Another idea could be to make it an array:

-            void       *paramarg;    /* private data for same */
+            void       *paramarg[2]; /* private data for same */

That would require touching other code using that field, but there
probably isn't much --- at least within our own tree, plpgsql itself
is the only user of paramarg.  Still, possibly breaking code that
didn't need to be broken doesn't seem like an improvement.

> ecpg tests seem to fail on Windows[0], but looks like it's not related to this thread.

Yeah, known problem with Meson dependencies[1]; it's breaking
pretty much all the cfbot's windows builds right now, although
maybe there's a timing issue that allows some to pass.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CAGECzQSvM3iSDmjF%2B%3DKof5an6jN8UbkP_4cKKT9w6GZavmb5yQ%40mail.gmail.com

  56. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-03T17:53:06Z

    Pavel Borisov <pashkin.elfe@gmail.com> writes:
> Minor notes on the patches:

> If dump_* functions could use the newly added walker, the code would
> look better. I suppose the main complication is that dump_* functions
> contain a lot of per-statement prints/formatting. So maybe a way to
> implement this is to put these statements into the existing tree
> walker i.e. plpgsql_statement_tree_walker_impl() and add an argument
> bool dump_debug into it. So in effect called with dump_debug=false
> plpgsql_statement_tree_walker_impl() would walk silent, and with
> dump_debug=false it would walk and print what is supposed to be
> printed currently in dump_* functions. Maybe there are other problems
> besides this?

I'm not thrilled with that idea, mainly because it would add overhead
to the performance-relevant cases (mark and free) to benefit a rarely
used debugging feature.  I'm content to leave the debug code out of
this for now --- it seems to me that it's serving a different master
and doesn't have to be unified with the other routines.

> For exec_check_rw_parameter():

> I think renaming expr->expr_simple_expr to sexpr saves few bytes but
> doesn't makes anything simpler, so if possible I'd prefer using just
> expr->expr_simple_expr with necessary casts. Furtermore in this
> function mostly we use cast results fexpr, opexpr and sbsref of
> expr->expr_simple_expr that already has separate names.

Hmm, I thought it looked cleaner like this, but I agree beauty
is in the eye of the beholder.  Anybody else have a preference?

> Transferring target param as int paramid looks completely ok. But we
> have unconditional checking Assert(paramid == expr->target_param + 1),
> so it looks as a redundant split as of now. Do we plan a true split
> and removal of this assert in the future?

We've already fetched the target variable using the paramid (cf
plpgsql_param_eval_var_check), so I think checking that the
expression does match it seems like a useful sanity check.
Agreed, it shouldn't ever not match, but that's why that's just
an Assert.

> Thanks for creating and working on this patch!

Thanks for reviewing it!

			regards, tom lane

  57. Re: Using Expanded Objects other than Arrays from plpgsql

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-02-03T17:57:27Z

    
> On 3 Feb 2025, at 22:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I'm not wedded to that name; do you have a better idea?

I'd propose something like attached. But feel free to ignore my suggestion: I do not understand context of these structure members.


Best regards, Andrey Borodin.

  58. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-03T18:18:10Z

    Andrey Borodin <x4mmm@yandex-team.ru> writes:
>> On 3 Feb 2025, at 22:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm not wedded to that name; do you have a better idea?

> I'd propose something like attached. But feel free to ignore my suggestion: I do not understand context of these structure members.

Hmm, you're suggesting naming those field members after PL/pgSQL's
specific use of them.  But the intent was that they are generic
workspace for anything that provides a EEOP_PARAM_CALLBACK
callback --- that is, the "param" in the field name refers to the
fact that this is an expression step for some kind of Param, and
not to what PL/pgSQL happens to do with the field.

Admittedly this is all moot unless some other extension starts
using EEOP_PARAM_CALLBACK, and I didn't find any evidence of that
using Debian Code Search.  But I don't want to think of
EEOP_PARAM_CALLBACK as being specifically tied to PL/pgSQL.

			regards, tom lane

  59. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-03T18:48:56Z

    I wrote:
> Admittedly this is all moot unless some other extension starts
> using EEOP_PARAM_CALLBACK, and I didn't find any evidence of that
> using Debian Code Search.  But I don't want to think of
> EEOP_PARAM_CALLBACK as being specifically tied to PL/pgSQL.

However ... given that I failed to find any outside users today,
I'm warming to the idea of "void *paramarg[2]".  That does look
less random than two separate fields.  There are probably not
any extensions that would need to change their code, and even
if there are, we impose bigger API breaks than this one in
every major release.

So I'm willing to do that if it satisfies your concern.

			regards, tom lane

  60. Re: Using Expanded Objects other than Arrays from plpgsql

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-02-03T19:15:02Z

    
> On 3 Feb 2025, at 23:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> But I don't want to think of
> EEOP_PARAM_CALLBACK as being specifically tied to PL/pgSQL.

I've found 1 users on Github [0]. And a lot of copied code like [1]. And there might be some unindexed cases.
Let's keep paramarg2.


Best regards, Andrey Borodin.
[0] https://github.com/wanglinn/xadb/blob/7695b7edcb0a89f3173b648c0da5b953538f2aa9/src/backend/pgxc/pool/execRemote.c#L835
[1] https://github.com/babelfish-for-postgresql/babelfish_extensions/blob/376cf488804fa02f9b1db5bbfbe74e98627fe96c/contrib/babelfishpg_tsql/src/pl_exec.c#L8030

  61. Re: Using Expanded Objects other than Arrays from plpgsql

    Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-04T11:54:52Z

    Hi, Tom!

On Mon, 3 Feb 2025 at 21:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Pavel Borisov <pashkin.elfe@gmail.com> writes:
> > Minor notes on the patches:
>
> > If dump_* functions could use the newly added walker, the code would
> > look better. I suppose the main complication is that dump_* functions
> > contain a lot of per-statement prints/formatting. So maybe a way to
> > implement this is to put these statements into the existing tree
> > walker i.e. plpgsql_statement_tree_walker_impl() and add an argument
> > bool dump_debug into it. So in effect called with dump_debug=false
> > plpgsql_statement_tree_walker_impl() would walk silent, and with
> > dump_debug=false it would walk and print what is supposed to be
> > printed currently in dump_* functions. Maybe there are other problems
> > besides this?
>
> I'm not thrilled with that idea, mainly because it would add overhead
> to the performance-relevant cases (mark and free) to benefit a rarely
> used debugging feature.  I'm content to leave the debug code out of
> this for now --- it seems to me that it's serving a different master
> and doesn't have to be unified with the other routines.
Makes sense.

> > For exec_check_rw_parameter():
>
> > I think renaming expr->expr_simple_expr to sexpr saves few bytes but
> > doesn't makes anything simpler, so if possible I'd prefer using just
> > expr->expr_simple_expr with necessary casts. Furtermore in this
> > function mostly we use cast results fexpr, opexpr and sbsref of
> > expr->expr_simple_expr that already has separate names.
>
> Hmm, I thought it looked cleaner like this, but I agree beauty
> is in the eye of the beholder.  Anybody else have a preference?
>
> > Transferring target param as int paramid looks completely ok. But we
> > have unconditional checking Assert(paramid == expr->target_param + 1),
> > so it looks as a redundant split as of now. Do we plan a true split
> > and removal of this assert in the future?
>
> We've already fetched the target variable using the paramid (cf
> plpgsql_param_eval_var_check), so I think checking that the
> expression does match it seems like a useful sanity check.
> Agreed, it shouldn't ever not match, but that's why that's just
> an Assert.
There are no problems with these.

Regards,
Pavel Borisov

  62. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-06T21:05:02Z

    Andrey Borodin <x4mmm@yandex-team.ru> writes:
> I've found 1 users on Github [0]. And a lot of copied code like [1]. And there might be some unindexed cases.
> Let's keep paramarg2.

Yeah, sounds like the best path if there are already outside users
of paramarg.

Do you have any further comments on this patch?

			regards, tom lane

  63. Re: Using Expanded Objects other than Arrays from plpgsql

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-02-07T10:40:44Z

    
> On 7 Feb 2025, at 02:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Do you have any further comments on this patch?

No, all steps of the patch set look good to me.


Best regards, Andrey Borodin.

  64. Re: Using Expanded Objects other than Arrays from plpgsql

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-11T17:50:14Z

    Andrey Borodin <x4mmm@yandex-team.ru> writes:
> On 7 Feb 2025, at 02:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Do you have any further comments on this patch?

> No, all steps of the patch set look good to me.

Pushed then.  Thanks for reviewing!

			regards, tom lane

  65. Re: Using Expanded Objects other than Arrays from plpgsql

    Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-13T13:02:18Z

    On Tue, 11 Feb 2025 at 21:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrey Borodin <x4mmm@yandex-team.ru> writes:
> > On 7 Feb 2025, at 02:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Do you have any further comments on this patch?
>
> > No, all steps of the patch set look good to me.
>
> Pushed then.  Thanks for reviewing!
Thanks to Michel and everyone working on the patch!

Regards,
Pavel Borisov