[BUG] pgbench nested \if conditions incorrectly processed

Started by Michail Nikolaevabout 1 year ago2 messages
#1Michail Nikolaev
michail.nikolaev@gmail.com
1 attachment(s)

Hello,

I’ve found an issue with pgbench: it processes nested \if conditions
incorrectly.

For example:

\if false
\if true
DROP TABLE money CASCADE;
\endif
\endif

In this case, the table will be dropped, which is unexpected behavior.

Attached is a fix that addresses this issue, along with a regression test.

Best regards,
Mikhail

Attachments:

v1-0001-pgbench-Fix-nested-if-elif-else-handling-in-scrip.patchapplication/octet-stream; name=v1-0001-pgbench-Fix-nested-if-elif-else-handling-in-scrip.patchDownload
From 873028c217b4353f482d8c43a3eddd3521bfe370 Mon Sep 17 00:00:00 2001
From: nkey <michail.nikolaev@gmail.com>
Date: Sun, 15 Dec 2024 14:04:26 +0100
Subject: [PATCH v1] pgbench: Fix nested if/elif/else handling in scripts

Add support for properly handling nested conditional blocks in pgbench scripts
by correctly tracking the state of inner conditional blocks when they appear
inside skipped (false) branches. Previously, the evaluation of conditions in
nested blocks could lead to incorrect behavior.

Add a regression test to verify nested if/elif/else statements work as
expected.
---
 src/bin/pgbench/meson.build                 |  1 +
 src/bin/pgbench/pgbench.c                   | 10 +++-
 src/bin/pgbench/t/003_pgbench_nested_ifs.pl | 57 +++++++++++++++++++++
 3 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100644 src/bin/pgbench/t/003_pgbench_nested_ifs.pl

diff --git a/src/bin/pgbench/meson.build b/src/bin/pgbench/meson.build
index d330bb5eb0d..ac45cad4015 100644
--- a/src/bin/pgbench/meson.build
+++ b/src/bin/pgbench/meson.build
@@ -43,6 +43,7 @@ tests += {
     'tests': [
       't/001_pgbench_with_server.pl',
       't/002_pgbench_no_server.pl',
+      't/003_pgbench_nested_ifs.pl',
     ],
   },
 }
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index c4c38099c5b..5181629133e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3880,8 +3880,14 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
 						switch (conditional_stack_peek(st->cstack))
 						{
 							case IFSTATE_FALSE:
-								if (command->meta == META_IF ||
-									command->meta == META_ELIF)
+								if (command->meta == META_IF)
+								{
+									/* nested if in skipped branch - ignore */
+									conditional_stack_push(st->cstack,
+														   IFSTATE_IGNORED);
+									st->command++;
+								}
+								else if (command->meta == META_ELIF)
 								{
 									/* we must evaluate the condition */
 									st->state = CSTATE_START_COMMAND;
diff --git a/src/bin/pgbench/t/003_pgbench_nested_ifs.pl b/src/bin/pgbench/t/003_pgbench_nested_ifs.pl
new file mode 100644
index 00000000000..52bc1734e06
--- /dev/null
+++ b/src/bin/pgbench/t/003_pgbench_nested_ifs.pl
@@ -0,0 +1,57 @@
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Test nested if/elif/else working correctly in pgbench
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+
+use Test::More;
+
+my ($node, $result);
+
+$node = PostgreSQL::Test::Cluster->new('pgbench_nested_ifs');
+$node->init;
+$node->start;
+
+$node->pgbench(
+	'--no-vacuum --client=1 --exit-on-abort --transactions=1',
+	0,
+	[qr{actually processed}],
+	[qr{^$}],
+	'nested ifs',
+	{
+		'pgbench_nested_if' => q(
+			\if false
+				SELECT 1 / 0;
+				\if true
+					SELECT 1 / 0;
+				\elif true
+					SELECT 1 / 0;
+				\else
+					SELECT 1 / 0;
+				\endif
+				SELECT 1 / 0;
+			\elif false
+				\if true
+					SELECT 1 / 0;
+				\elif true
+					SELECT 1 / 0;
+				\else
+					SELECT 1 / 0;
+				\endif
+			\else
+				\if false
+					SELECT 1 / 0;
+				\elif false
+					SELECT 1 / 0;
+				\else
+					SELECT 'correct';
+				\endif
+			\endif
+		)
+	});
+
+$node->stop;
+done_testing();
-- 
2.43.0

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Michail Nikolaev (#1)
Re: [BUG] pgbench nested \if conditions incorrectly processed

Michail Nikolaev <michail.nikolaev@gmail.com> writes:

I’ve found an issue with pgbench: it processes nested \if conditions
incorrectly.

Right you are.

Attached is a fix that addresses this issue, along with a regression test.

Code fix looks good, but I don't agree with creating a separate
test script for this --- that entails spinning up an entire new
server instance, which is an awful lot of cycles to expend in
every check-world run forevermore for this size of problem.
I'll just add the test case to 001_pgbench_with_server.pl instead.

Looking at the code coverage report [1]https://coverage.postgresql.org/src/bin/pgbench/pgbench.c.gcov.html#3915, it seems we are also missing
any test that covers \if in the IFSTATE_IGNORED/IFSTATE_ELSE_FALSE
case. That code appears correct, but given this bug it seems prudent
to extend the test to cover that path too. I'll do that and push.

regards, tom lane

[1]: https://coverage.postgresql.org/src/bin/pgbench/pgbench.c.gcov.html#3915