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

po 17. 8. 2020 v 8:40 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

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?

There are two patches

The first patch enhances dbg api by two functions - eval_datum and
cast_value - it is an interface for functions exec_eval_datum and
do_cast_value. With this API it is easy to take a value of any PLpgSQL
variable (without the necessity to duplicate a lot of plpgsql's code), and
it easy to transform this value to any expected type - usually it should
provide the cast to the text type.

Second patch injects pointer to related namespace to any plpgsql statement.
Reference to namespace is required for building custom expressions that can
be evaluated by assign_expr function. I would like to use it for
conditional breakpoints or conditional tracing. Without this patch it is
difficult to detect the correct namespace and ensure the correct variable's
visibility.

Regards

Pavel

Hi

rebase

Regards

Pavel

Hi

fresh rebase

Regards

Pavel

ne 7. 2. 2021 v 19:09 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

Hi

fresh rebase

only 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

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

st 21. 7. 2021 v 22:23 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

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.

with modified Makefile to force use option
-I$(top_srcdir)/src/pl/plpgsql/src

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

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]https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.141500:

```
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
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.141500

--
Best regards,
Aleksander Alekseev

čt 22. 7. 2021 v 14:54 odesílatel Aleksander Alekseev <
aleksander@timescale.com> napsal:

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

I know it. Attached patch try to fix this issue

I merged you patch (thank you)

Regards

Pavel

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

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

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

ne 22. 8. 2021 v 19:38 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

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

This version set $contrib_extraincludes to fix windows build

Regards

Pavel

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.

Hi

čt 17. 3. 2022 v 20:58 odesílatel Greg Stark <stark@mit.edu> napsal:

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.

I had a plan to use the new API in plpgsql_check
https://github.com/okbob/plpgsql_check for integrated tracer

There is the pldebugger https://github.com/EnterpriseDB/pldebugger and I
think this extension can use this API well too. You can see - the PL
debugger has about 6000 lines, and more, there are some extensions for EDB.
So, unfortunately, It doesn't looks like good candidate for contrib.
Writing new debugger from zero doesn't looks like effective work. I am open
any discussion about it. Maybe some form of more sophisticated tracer can
be invited, but I think so better is enhancing widely used pldebugger
extension.

Regards

Pavel

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

Hi

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.

I removed this test

I am sending updated patch

Regards

Pavel