Re: proposal: enhancing plpgsql debug API - returns text value of variable content - Mailing list pgsql-hackers

Greg Stark <stark@mit.edu> writes:
> It looks like this is -- like a lot of plpgsql patches -- having
> difficulty catching the attention of reviewers and committers.

I was hoping that someone with more familiarity with pldebugger
would comment on the suitableness of this patch for their desires.
But nobody's stepped up, so I took a look through this.  It looks
like there are several different things mashed into this patch:

1. Expose exec_eval_datum() to plugins.  OK; I see that pldebugger
has code that duplicates that functionality (and not terribly well).

2. Expose do_cast_value() to plugins.  Mostly OK, but shouldn't we
expose exec_cast_value() instead?  Otherwise it's on the caller
to make sure it doesn't ask for a no-op cast, which seems like a
bad idea; not least because the example usage in get_string_value()
fails to do so.

3. Store relevant PLpgSQL_nsitem chain link in each PLpgSQL_stmt.
This makes me itch, for a number of reasons:
* I was a bit astonished that it even works; I'd thought that the
nsitem data structure is transient data thrown away when we finish
compiling.  I see now that that's not so, but do we really want to
nail down that that can't ever be improved?
* This ties us forevermore to the present, very inefficient, nsitem
list data structure.  Sooner or later somebody is going to want to
improve that linear search, and what then?
* The space overhead seems nontrivial; many PLpgSQL_stmt nodes are
not very big.
* The code implications are way more subtle than you would think
from inspecting this totally-comment-free patch implementation.
In particular, the fact that the nsitem chain pointed to by a
plpgsql_block is the right thing depends heavily on exactly where
in the parse sequence we capture the value of plpgsql_ns_top().
That could be improved with a comment, perhaps.

I think that using the PLpgSQL_nsitem chains to look up variables
in a debugger is just the wrong thing.  The right thing is to
crawl up the statement tree, and when you see a PLpgSQL_stmt_block
or loop construct, examine the associated datums.  I'll concede
that crawling *up* the tree is hard, as we only store down-links.
Now a plugin could fix that by itself, by recursively traversing the
statement tree one time and recording parent relationships in its own
data structure (say, an array of parent-statement pointers indexed by
stmtid).  Or we could add parent links in the statement tree, though
I remain concerned about the space cost.  On the whole I prefer the
first way, because (a) we don't pay the overhead when it's not needed,
and (b) a plugin could use it even in existing release branches.

BTW, crawling up the statement tree would also be a far better answer
than what's shown in the patch for locating surrounding for-loops.

So my inclination is to accept the additional function pointers
(modulo pointing to exec_cast_value) but reject the nsitem additions.

Not sure what to do with test_dbgapi.  There's some value in exercising
the find_rendezvous_variable mechanism, but I'm dubious that that
justifies a whole test module.

            regards, tom lane