Hi
st 30. 3. 2022 v 21:09 odesÃlatel Tom Lane <t...@sss.pgh.pa.us> napsal:
> Greg Stark <st...@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.
>
I removed this test
I am sending updated patch
Regards
Pavel
>
> regards, tom lane
>
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 00328fddcf..1a647c396f 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4086,6 +4086,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
{
(*plpgsql_plugin_ptr)->error_callback = plpgsql_exec_error_callback;
(*plpgsql_plugin_ptr)->assign_expr = exec_assign_expr;
+ (*plpgsql_plugin_ptr)->eval_datum = exec_eval_datum;
+ (*plpgsql_plugin_ptr)->cast_value = exec_cast_value;
if ((*plpgsql_plugin_ptr)->func_setup)
((*plpgsql_plugin_ptr)->func_setup) (estate, func);
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 813c32c70f..e49fd02ccf 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1121,8 +1121,11 @@ typedef struct PLpgSQL_execstate
*
* Also, immediately before any call to func_setup, PL/pgSQL fills in the
* error_callback and assign_expr fields with pointers to its own
- * plpgsql_exec_error_callback and exec_assign_expr functions. This is
- * a somewhat ad-hoc expedient to simplify life for debugger plugins.
+ * plpgsql_exec_error_callback and exec_assign_expr functions. eval_datum
+ * is assigned to function exec_eval_datum, and cast_value to function
+ * do_cast_value. With last two functions is easy to get content of
+ * any PLpgSQL_datum in any expected type. This is a somewhat ad-hoc
+ * expedient to simplify life for debugger plugins.
*/
typedef struct PLpgSQL_plugin
{
@@ -1137,6 +1140,13 @@ typedef struct PLpgSQL_plugin
void (*error_callback) (void *arg);
void (*assign_expr) (PLpgSQL_execstate *estate, PLpgSQL_datum *target,
PLpgSQL_expr *expr);
+ void (*eval_datum) (PLpgSQL_execstate *estate, PLpgSQL_datum *datum,
+ Oid *typeid, int32 *typetypmod, Datum *value,
+ bool *isnull);
+ Datum (*cast_value) (PLpgSQL_execstate *estate,
+ Datum value, bool *isnull,
+ Oid valtype, int32 valtypmod,
+ Oid reqtype, int32 reqtypmod);
} PLpgSQL_plugin;
/*
