From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Greg Stark <stark(at)mit(dot)edu> |
Cc: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Aleksander Alekseev <aleksander(at)timescale(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: proposal: enhancing plpgsql debug API - returns text value of variable content |
Date: | 2022-03-30 19:09:01 |
Message-ID: | 279241.1648667341@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
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
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2022-03-30 19:12:51 | Re: pgsql: Add 'basebackup_to_shell' contrib module. |
Previous Message | Robert Haas | 2022-03-30 18:57:25 | Re: pg_stat_reset_single_*_counters vs pg_stat_database.stats_reset |