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

Hi

I am working on tracing support to plpgsql_check

https://github.com/okbob/plpgsql_check

I would like to print content of variables - and now, I have to go some deeper than I would like. I need to separate between scalar, row, and record variables. PLpgSQL has code for it - but it is private.

Now plpgsql debug API has an API for expression evaluation - and it is working fine, but there is a need to know the necessary namespace. Unfortunately, the plpgsql variables have not assigned any info about related namespaces. It increases the necessary work for implementing conditional breakpoints or just printing all variables (and maintaining a lot of plpgsql code outside plpgsql core).

So my proposals:

1. enhancing debug api about method

char *get_cstring_valule(PLpgSQL_variable *var, bool *isnull)

2. enhancing PLpgSQL_var structure about related namespace "struct PLpgSQL_nsitem *ns",

PLpgSQL_stmt *scope statement (statement that limits scope of variable's visibility). For usage in debuggers, tracers can be nice to have a info about kind of variable (function argument, local variable, automatic custom variable (FORC), automatic internal variable (SQLERRM, FOUND, TG_OP, ...).

Comments, notes?

Regards

Pavel

Hi

fresh rebase

Regards

Pavel

Hi Pavel,

> I would like to print content of variables - and now, I have to go some
> deeper than I would like. I need to separate between scalar, row, and
> record variables. PLpgSQL has code for it - but it is private.
> [...]

The patch seems OK, but I wonder - would it be possible to write a test on it?

--
Best regards,
Aleksander Alekseev

Hi

pá 16. 7. 2021 v 15:05 odesílatel Aleksander Alekseev <aleksander@timescale.com> napsal:

Hi Pavel,

> I would like to print content of variables - and now, I have to go some
> deeper than I would like. I need to separate between scalar, row, and
> record variables. PLpgSQL has code for it - but it is private.
> [...]

The patch seems OK, but I wonder - would it be possible to write a test on it?

 Sure, it is possible - unfortunately - the size of this test will be significantly bigger than patch self.

I'll try to write it some simply tracer, where this API can be used

--
Best regards,
Aleksander Alekseev

Hi

pá 16. 7. 2021 v 18:40 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

Hi

pá 16. 7. 2021 v 15:05 odesílatel Aleksander Alekseev <aleksander@timescale.com> napsal:

Hi Pavel,

> I would like to print content of variables - and now, I have to go some
> deeper than I would like. I need to separate between scalar, row, and
> record variables. PLpgSQL has code for it - but it is private.
> [...]

The patch seems OK, but I wonder - would it be possible to write a test on it?

 Sure, it is possible - unfortunately - the size of this test will be significantly bigger than patch self.

I'll try to write it some simply tracer, where this API can be used

I am sending an enhanced patch about the regress test for plpgsql's debug API.

Regards

Pavel

--
Best regards,
Aleksander Alekseev

Hi Pavel,

>> I am sending an enhanced patch about the regress test for plpgsql's debug API.

Thanks for the test! I noticed some little issues with formatting and typos. The corrected patch is attached.

> override CPPFLAGS := $(CPPFLAGS) -I$(top_srcdir)/src/pl/plpgsql/src

You probably already noticed, but for the record - AppVeyor doesn't seem to be happy still [1]:

```

src/test/modules/test_dbgapi/test_dbgapi.c(17): fatal error C1083: Cannot open include file: 'plpgsql.h': No such file or directory [C:\projects\postgresql\test_dbgapi.vcxproj]

```

[1]: https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.141500

--
Best regards,
Aleksander Alekseev

Hi Pavel,

> I know it. Attached patch try to fix this issue
>
> I merged you patch (thank you)

Thanks! I did some more minor changes, mostly in the comments. See the attached patch. Other than that it looks OK. I think it's Ready for Committer now.

--
Best regards,
Aleksander Alekseev

pá 23. 7. 2021 v 10:30 odesílatel Aleksander Alekseev <aleksander@timescale.com> napsal:

Hi Pavel,

> I know it. Attached patch try to fix this issue
>
> I merged you patch (thank you)

Thanks! I did some more minor changes, mostly in the comments. See the attached patch. Other than that it looks OK. I think it's Ready for Committer now.

looks well,

thank you very much

Pavel

--
Best regards,
Aleksander Alekseev

pá 23. 7. 2021 v 10:47 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

pá 23. 7. 2021 v 10:30 odesílatel Aleksander Alekseev <aleksander@timescale.com> napsal:

Hi Pavel,

> I know it. Attached patch try to fix this issue
>
> I merged you patch (thank you)

Thanks! I did some more minor changes, mostly in the comments. See the attached patch. Other than that it looks OK. I think it's Ready for Committer now.

looks well,

thank you very much

Pavel

rebase

Pavel

--
Best regards,
Aleksander Alekseev

Hi

st 28. 7. 2021 v 11:01 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

pá 23. 7. 2021 v 10:47 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

pá 23. 7. 2021 v 10:30 odesílatel Aleksander Alekseev <aleksander@timescale.com> napsal:

Hi Pavel,

> I know it. Attached patch try to fix this issue
>
> I merged you patch (thank you)

Thanks! I did some more minor changes, mostly in the comments. See the attached patch. Other than that it looks OK. I think it's Ready for Committer now.

looks well,

thank you very much

Pavel

rebase

unfortunately, previous patch that I sent was broken, so I am sending fixed patch and fresh rebase

Regards

Pavel

Pavel

--
Best regards,
Aleksander Alekseev

It looks like this is -- like a lot of plpgsql patches -- having
difficulty catching the attention of reviewers and committers.
Aleksander asked for a test and Pavel put quite a bit of work into
adding a good test case. I actually like that there's a test because
it shows the API can be used effectively.

From my quick skim of this code it does indeed look like it's ready to
commit. It's mostly pretty mechanical code to expose a couple fields
so that a debugger can see them.

Pavel, are you planning to add a debugger to contrib using this? The
test example code looks like it would already be kind of useful even
in this form.

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

Pavel Stehule <pavel.stehule@gmail.com> writes:
> I am sending updated patch

After studying the list of exposed functions for awhile, it seemed
to me that we should also expose exec_assign_value.  The new pointers
allow a plugin to compute a value in Datum+isnull format, but then it
can't do much of anything with it: exec_assign_expr is a completely
inconvenient API if what you want to do is put a specific Datum
value into a variable.  Adding exec_assign_value provides "store"
and "fetch" APIs that are more or less inverses, which should be
easier to work with.

So I did that and pushed it.

                        regards, tom lane