Re: proposal: enhancing plpgsql debug API - returns text value of variable content

Hi

st 30. 3. 2022 v 21:09 odesílatel Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> napsal:

> Greg Stark <stark(at)mit(dot)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.
>

I removed this test

I am sending updated patch

Regards

Pavel

>
> regards, tom lane
>