Re: pgsql: Test pg_atomic_fetch_add_ with variable addend and 16-bit edge c
Hi,
On 2019-09-15 15:14:50 -0700, Noah Misch wrote:
> On Sun, Sep 15, 2019 at 01:00:21PM -0300, Alvaro Herrera wrote:
> > On 2019-Sep-14, Noah Misch wrote:
> > > Test pg_atomic_fetch_add_ with variable addend and 16-bit edge cases.
> >
> > I don't understand this.
> >
> > if (pg_atomic_fetch_add_u32(, INT_MAX) != INT_MAX)
> > elog(ERROR, "pg_atomic_add_fetch_u32() #3 wrong");
> >
> > pg_atomic_fetch_add_u32(, 2); /* wrap to 0 */
> >
> > if (pg_atomic_fetch_add_u32(, PG_INT16_MAX) != 0)
> > elog(ERROR, "pg_atomic_fetch_add_u32() #3 wrong");
> >
> > The existing one seems to be naming the wrong function in the error
> > message, and if you fix that then you have two "#3 wrong" cases.
> > Isn't this confusing? Am I just too sensitive? Is this pointless to
> > fix?
>
> It's a typo-class defect. Would you like me to fix it, or would you like to
> fix it? I'd consider wrapping the tests in a macro (may be overkill, since
> these tests don't change much):
>
> --- a/src/test/regress/regress.c
> +++ b/src/test/regress/regress.c
> @@ -670,6 +670,16 @@ test_atomic_flag(void)
> pg_atomic_clear_flag();
> }
>
> +#define EXPECT(result_expr, expected_expr) \
> + do { \
> + uint32 result = (result_expr); \
> + uint32 expected = (expected_expr); \
> + if (result != expected) \
> + elog(ERROR, \
> + "%s yielded %u, expected %s in file \"%s\"
> line %u", \
> + #result_expr, result, #expected_expr,
> __FILE__, __LINE__); \
> + } while (0)
> +
While it might be overkill on its own, it seems like it'd be a good
utility to have for these kinds of tests.
Unfortunately we can't easily make this type independent. The local
variables are needed to avoid multiple evaluation. While we could infer
their type using compiler specific magic (__typeof__() or C++), we'd
still need to print them. We could however remove the local variables,
and purely rely on stringification of the arguments for printing the
error.
> static void
> test_atomic_uint32(void)
> {
> @@ -678,17 +688,10 @@ test_atomic_uint32(void)
> int i;
>
> pg_atomic_init_u32(, 0);
> -
> - if (pg_atomic_read_u32() != 0)
> - elog(ERROR, "atomic_read_u32() #1 wrong");
> -
> + EXPECT(pg_atomic_read_u32(), 0);
> pg_atomic_write_u32(, 3);
> -
> - if (pg_atomic_read_u32() != 3)
> - elog(ERROR, "atomic_read_u32() #2 wrong");
> -
> - if (pg_atomic_fetch_add_u32(, pg_atomic_read_u32() - 2) != 3)
> - elog(ERROR, "atomic_fetch_add_u32() #1 wrong");
> + EXPECT(pg_atomic_read_u32(), 3);
> + EXPECT(pg_atomic_fetch_add_u32(, pg_atomic_read_u32() - 2), 3);
>
> if (pg_atomic_fetch_sub_u32(, 1) != 4)
> elog(ERROR, "atomic_fetch_sub_u32() #1 wrong");
I'd name it EXPECT_EQ_U32 or such, but otherwise I think this is a clear
improvement.
Greetings,
Andres Freund
Re: pgsql: Test pg_atomic_fetch_add_ with variable addend and 16-bit edge c
On Sun, Sep 15, 2019 at 01:00:21PM -0300, Alvaro Herrera wrote:
> On 2019-Sep-14, Noah Misch wrote:
> > Test pg_atomic_fetch_add_ with variable addend and 16-bit edge cases.
>
> I don't understand this.
>
> if (pg_atomic_fetch_add_u32(, INT_MAX) != INT_MAX)
> elog(ERROR, "pg_atomic_add_fetch_u32() #3 wrong");
>
> pg_atomic_fetch_add_u32(, 2); /* wrap to 0 */
>
> if (pg_atomic_fetch_add_u32(, PG_INT16_MAX) != 0)
> elog(ERROR, "pg_atomic_fetch_add_u32() #3 wrong");
>
> The existing one seems to be naming the wrong function in the error
> message, and if you fix that then you have two "#3 wrong" cases.
> Isn't this confusing? Am I just too sensitive? Is this pointless to
> fix?
It's a typo-class defect. Would you like me to fix it, or would you like to
fix it? I'd consider wrapping the tests in a macro (may be overkill, since
these tests don't change much):
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -670,6 +670,16 @@ test_atomic_flag(void)
pg_atomic_clear_flag();
}
+#define EXPECT(result_expr, expected_expr) \
+ do { \
+ uint32 result = (result_expr); \
+ uint32 expected = (expected_expr); \
+ if (result != expected) \
+ elog(ERROR, \
+"%s yielded %u, expected %s in file \"%s\"
line %u", \
+#result_expr, result, #expected_expr,
__FILE__, __LINE__); \
+ } while (0)
+
static void
test_atomic_uint32(void)
{
@@ -678,17 +688,10 @@ test_atomic_uint32(void)
int i;
pg_atomic_init_u32(, 0);
-
- if (pg_atomic_read_u32() != 0)
- elog(ERROR, "atomic_read_u32() #1 wrong");
-
+ EXPECT(pg_atomic_read_u32(), 0);
pg_atomic_write_u32(, 3);
-
- if (pg_atomic_read_u32() != 3)
- elog(ERROR, "atomic_read_u32() #2 wrong");
-
- if (pg_atomic_fetch_add_u32(, pg_atomic_read_u32() - 2) != 3)
- elog(ERROR, "atomic_fetch_add_u32() #1 wrong");
+ EXPECT(pg_atomic_read_u32(), 3);
+ EXPECT(pg_atomic_fetch_add_u32(, pg_atomic_read_u32() - 2), 3);
if (pg_atomic_fetch_sub_u32(, 1) != 4)
elog(ERROR, "atomic_fetch_sub_u32() #1 wrong");
Re: pgsql: Test pg_atomic_fetch_add_ with variable addend and 16-bit edge c
On 2019-Sep-14, Noah Misch wrote: > Test pg_atomic_fetch_add_ with variable addend and 16-bit edge cases. I don't understand this. if (pg_atomic_fetch_add_u32(, INT_MAX) != INT_MAX) elog(ERROR, "pg_atomic_add_fetch_u32() #3 wrong"); pg_atomic_fetch_add_u32(, 2); /* wrap to 0 */ if (pg_atomic_fetch_add_u32(, PG_INT16_MAX) != 0) elog(ERROR, "pg_atomic_fetch_add_u32() #3 wrong"); The existing one seems to be naming the wrong function in the error message, and if you fix that then you have two "#3 wrong" cases. Isn't this confusing? Am I just too sensitive? Is this pointless to fix? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
pgsql: Fix bogus sizeof calculations.
Fix bogus sizeof calculations. Noted by Coverity. Typo in 27cc7cd2b, so back-patch to v12 as that was. Branch -- REL_12_STABLE Details --- https://git.postgresql.org/pg/commitdiff/27bc87985c4135f27bdcddd905baad3b62a3f03a Modified Files -- src/backend/executor/execMain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
pgsql: Fix bogus sizeof calculations.
Fix bogus sizeof calculations. Noted by Coverity. Typo in 27cc7cd2b, so back-patch to v12 as that was. Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/d8122578098d3ff20a9a12d25807e56cecac673c Modified Files -- src/backend/executor/execMain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
pgsql: Fix intermittent self-test failures caused by the stats_ext test
Fix intermittent self-test failures caused by the stats_ext test. Commit d7f8d26d9 added new tests to the stats_ext regression test that included creating a view in the public schema, without realising that the stats_ext test runs in the same parallel group as the rules test, which makes doing that unsafe. This led to intermittent failures of the rules test on the buildfarm, although I wasn't able to reproduce that locally. Fix by creating the view in a different schema. Tomas Vondra and Dean Rasheed, report and diagnosis by Thomas Munro. Discussion: https://postgr.es/m/ca+hukgkx9hfzrya7rqzamre07l4hzicc-no_b3tajpiukyl...@mail.gmail.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/3d9a3ef5cbfc70bd2802c3f0da3fbc4e5abdf377 Modified Files -- src/test/regress/expected/stats_ext.out | 42 ++--- src/test/regress/sql/stats_ext.sql | 40 --- 2 files changed, 44 insertions(+), 38 deletions(-)
pgsql: Fix intermittent self-test failures caused by the stats_ext test
Fix intermittent self-test failures caused by the stats_ext test. Commit d7f8d26d9 added new tests to the stats_ext regression test that included creating a view in the public schema, without realising that the stats_ext test runs in the same parallel group as the rules test, which makes doing that unsafe. This led to intermittent failures of the rules test on the buildfarm, although I wasn't able to reproduce that locally. Fix by creating the view in a different schema. Tomas Vondra and Dean Rasheed, report and diagnosis by Thomas Munro. Discussion: https://postgr.es/m/ca+hukgkx9hfzrya7rqzamre07l4hzicc-no_b3tajpiukyl...@mail.gmail.com Branch -- REL_12_STABLE Details --- https://git.postgresql.org/pg/commitdiff/5576cbc8ff1f8b34e519e54ef43de68ed1b2f93e Modified Files -- src/test/regress/expected/stats_ext.out | 42 ++--- src/test/regress/sql/stats_ext.sql | 40 --- 2 files changed, 44 insertions(+), 38 deletions(-)
