Обсуждение: Add test module for verifying backtrace functionality

Hi,

Postgres has a good amount of code for dealing with backtraces - two
GUCs backtrace_functions and backtrace_on_internal_error,
errbacktrace; all of which use core function set_backtrace from
elog.c. I've not seen this code being tested at all, see code coverage
report - https://coverage.postgresql.org/src/backend/utils/error/elog.c.gcov.html.

I think adding a simple test module (containing no .c files) with only
TAP tests will help cover this code. I ended up having it as a
separate module under src/test/modules/test_backtrace as I was not
able to find an existing TAP file in src/test to add these tests.  I'm
able to verify the backtrace related code with the attached patch
consistently. The TAP tests rely on the fact that the server emits
text "BACKTRACE: " to server logs before logging the backtrace, and
the backtrace contains the function name in which the error occurs.
I've turned off query statement logging (set log_statement = none,
log_min_error_statement = fatal) so that the tests get to see the
functions only in the backtrace. Although the CF bot is happy with the
attached patch https://github.com/BRupireddy2/postgres/tree/add_test_module_for_bcktrace_functionality_v1,
there might be some more flakiness to it.

Thoughts?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Tue, Feb 13, 2024 at 2:11 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> Postgres has a good amount of code for dealing with backtraces - two
> GUCs backtrace_functions and backtrace_on_internal_error,
> errbacktrace; all of which use core function set_backtrace from
> elog.c. I've not seen this code being tested at all, see code coverage
> report - https://coverage.postgresql.org/src/backend/utils/error/elog.c.gcov.html.
>
> I think adding a simple test module (containing no .c files) with only
> TAP tests will help cover this code. I ended up having it as a
> separate module under src/test/modules/test_backtrace as I was not
> able to find an existing TAP file in src/test to add these tests.  I'm
> able to verify the backtrace related code with the attached patch
> consistently. The TAP tests rely on the fact that the server emits
> text "BACKTRACE: " to server logs before logging the backtrace, and
> the backtrace contains the function name in which the error occurs.
> I've turned off query statement logging (set log_statement = none,
> log_min_error_statement = fatal) so that the tests get to see the
> functions only in the backtrace. Although the CF bot is happy with the
> attached patch https://github.com/BRupireddy2/postgres/tree/add_test_module_for_bcktrace_functionality_v1,
> there might be some more flakiness to it.
>
> Thoughts?

Ran pgperltidy on the new TAP test file added. Please see the attached v2 patch.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Tue, Feb 20, 2024 at 11:30 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Feb 13, 2024 at 2:11 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Hi,
> >
> > Postgres has a good amount of code for dealing with backtraces - two
> > GUCs backtrace_functions and backtrace_on_internal_error,
> > errbacktrace; all of which use core function set_backtrace from
> > elog.c. I've not seen this code being tested at all, see code coverage
> > report - https://coverage.postgresql.org/src/backend/utils/error/elog.c.gcov.html.
> >
> > I think adding a simple test module (containing no .c files) with only
> > TAP tests will help cover this code. I ended up having it as a
> > separate module under src/test/modules/test_backtrace as I was not
> > able to find an existing TAP file in src/test to add these tests.  I'm
> > able to verify the backtrace related code with the attached patch
> > consistently. The TAP tests rely on the fact that the server emits
> > text "BACKTRACE: " to server logs before logging the backtrace, and
> > the backtrace contains the function name in which the error occurs.
> > I've turned off query statement logging (set log_statement = none,
> > log_min_error_statement = fatal) so that the tests get to see the
> > functions only in the backtrace. Although the CF bot is happy with the
> > attached patch https://github.com/BRupireddy2/postgres/tree/add_test_module_for_bcktrace_functionality_v1,
> > there might be some more flakiness to it.
> >
> > Thoughts?
>
> Ran pgperltidy on the new TAP test file added. Please see the attached v2 patch.

I've now moved the new TAP test file to src/test/modules/test_misc/t
as opposed to a new test module to keep it simple. I was not sure why
I hadn't done that in the first place.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 16.03.24 05:25, Bharath Rupireddy wrote:
>>> Postgres has a good amount of code for dealing with backtraces - two
>>> GUCs backtrace_functions and backtrace_on_internal_error,
>>> errbacktrace; all of which use core function set_backtrace from
>>> elog.c. I've not seen this code being tested at all, see code coverage
>>> report - https://coverage.postgresql.org/src/backend/utils/error/elog.c.gcov.html.
>>>
>>> I think adding a simple test module (containing no .c files) with only
>>> TAP tests will help cover this code. I ended up having it as a
>>> separate module under src/test/modules/test_backtrace as I was not
>>> able to find an existing TAP file in src/test to add these tests.  I'm
>>> able to verify the backtrace related code with the attached patch
>>> consistently. The TAP tests rely on the fact that the server emits
>>> text "BACKTRACE: " to server logs before logging the backtrace, and
>>> the backtrace contains the function name in which the error occurs.
>>> I've turned off query statement logging (set log_statement = none,
>>> log_min_error_statement = fatal) so that the tests get to see the
>>> functions only in the backtrace. Although the CF bot is happy with the
>>> attached patch https://github.com/BRupireddy2/postgres/tree/add_test_module_for_bcktrace_functionality_v1,
>>> there might be some more flakiness to it.
>>>
>>> Thoughts?
>>
>> Ran pgperltidy on the new TAP test file added. Please see the attached v2 patch.
> 
> I've now moved the new TAP test file to src/test/modules/test_misc/t
> as opposed to a new test module to keep it simple. I was not sure why
> I hadn't done that in the first place.

Note that backtrace_on_internal_error has been removed, so this patch 
will need to be adjusted for that.

I suggest you consider joining forces with thread [0] where a 
replacement for backtrace_on_internal_error would be discussed.  Having 
some test coverage for whatever is being developed there might be useful.


[0]: 
https://www.postgresql.org/message-id/flat/CAGECzQTpdujCEt2SH4DBwRLoDq4HJArGDaxJSsWX0G=tNnzaVA@mail.gmail.com