public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Burd <[email protected]>
To: Robert Haas <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Mon, 15 Sep 2025 15:03:21 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoYZfigfGgKsT2xCg6d8nhS+UFq6zrO6R6Aq+Dqz-=a5UA@mail.gmail.com>
References: <CA+TgmoYZfigfGgKsT2xCg6d8nhS+UFq6zrO6R6Aq+Dqz-=a5UA@mail.gmail.com>
On Sep 15 2025, at 2:54 pm, Robert Haas <[email protected]> wrote:
> On Mon, Sep 15, 2025 at 2:00 PM Greg Burd <[email protected]> wrote:
>> For reference radixtree has:
>>
>> coverage: HEAD
>> lines......: 98.3
>> functions..: 97.2
>> branches...: 89.4
>
> + /* Test negative member in bms_make_singleton */
> + error_caught = false;
> + PG_TRY();
> + {
> + bms_make_singleton(-1);
> + }
> + PG_CATCH();
> + {
> + error_caught = true;
> + FlushErrorState();
> + }
> + PG_END_TRY();
> + EXPECT_TRUE(error_caught);
>
> This is an anti-pattern for PostgreSQL code. You can't just flush an
> error without aborting a transaction or subtransaction to recover.
> Even if it could be shown that this were harmless here, I think it's a
> terrible idea to have code like this in the tree, as it encourages
> people to do exactly the wrong thing.
Fair enough, I'll rework it.
> But backing up a step, this also doesn't really seem like the right
> way to test the error conditions. It deliberately throws away the
> error message. All this verifies is that you caught an error. If you
> let the error escape to the client you could have the expected output
> test that you got the expected message.
>
> I think it would be a better idea to structure this as a set of
> SQL-callable functions and move a bunch of the logic into SQL.
I'll give that approach a try, thanks for the suggestion.
-greg
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
view thread (81+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Add tests for Bitmapset
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox