st 30. 3. 2022 v 21:09 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
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.
good idea, changed
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.
I removed this part
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.
