Hi! Great you continue to work on this patch! I'm checking out the newest changes.
On Fri, Sep 15, 2023 at 2:32 PM Aleksander Alekseev < aleksan...@timescale.com> wrote: > Hi, > > > Renaming/Refactoring: > > - All spans are now tracked in the palloced current_trace_spans buffer > > compared to top_span and parse_span being kept in a static variable > > before. > > - I've renamed query_spans to top_span. A top_span serves as the > > parent for all spans in a specific nested level. > > - More debugging information and assertions. Spans now track their > > nested level, if they've been ended and if they are a top_span. > > > > Changes: > > - I've added the subxact_count to the span's metadata. This can help > > identify the moment a subtransaction was started. > > - I've reworked nested queries and utility statements. Previously, I > > made the assumptions that we could only have one top_span per nested > > level which is not the case. Some utility statements can execute > > multiple queries in the same nested level. Tracing utility statement > > now works better (see image of tracing a create extension). > > Many thanks for the updated patch. > > If it's not too much trouble, please use `git format-patch`. This > makes applying and testing the patch much easier. Also you can provide > a commit message which 1. will simplify the work for the committer and > 2. can be reviewed as well. > > The tests fail on CI [1]. I tried to run them locally and got the same > results. I'm using full-build.sh from this repository [2] for > Autotools and the following one-liner for Meson: > > ``` > time CPPFLAGS="-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS" sh -c 'git > clean -dfx && meson setup --buildtype debug -Dcassert=true > -DPG_TEST_EXTRA="kerberos ldap ssl load_balance" -Dldap=disabled > -Dssl=openssl -Dtap_tests=enabled > -Dprefix=/home/eax/projects/pginstall build && ninja -C build && ninja > -C build docs && PG_TEST_EXTRA=1 meson test -C build' > ``` > > The comments for pg_tracing.c don't seem to be formatted properly. > Please make sure to run pg_indent. See src/tools/pgindent/README > > The interface pg_tracing_spans(true | false) doesn't strike me as > particularly readable. Maybe this function should be private and the > interface be like pg_tracing_spans() and pg_trancing_consume_spans(). > Alternatively you could use named arguments in the tests, but I don't > think this is a preferred solution. > > Speaking of the tests I suggest adding a bit more comments before > every (or most) of the queries. Figuring out what they test could be > not particularly straightforward for somebody who will make changes > after the patch will be accepted. > > [1]: http://cfbot.cputube.org/ > [2]: https://github.com/afiskon/pgscripts/ > > -- > Best regards, > Aleksander Alekseev > -- Regards, Nikita Malakhov Postgres Professional The Russian Postgres Company https://postgrespro.ru/
