public inbox for [email protected]  

help / color / mirror / Atom feed

From: Robert Haas <[email protected]>
To: Greg Burd <[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 14:54:25 -0400
Message-ID: <CA+TgmoYZfigfGgKsT2xCg6d8nhS+UFq6zrO6R6Aq+Dqz-=a5UA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>

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.

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.

-- 
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: <CA+TgmoYZfigfGgKsT2xCg6d8nhS+UFq6zrO6R6Aq+Dqz-=a5UA@mail.gmail.com>

* 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