public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/2] Add extra statistics to explain for Nested Loop
8+ messages / 4 participants
[nested] [flat]
* [PATCH 2/2] Add extra statistics to explain for Nested Loop
@ 2022-04-02 02:36 Ekaterina Sokolova <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Ekaterina Sokolova @ 2022-04-02 02:36 UTC (permalink / raw)
For some distributions of data in tables, different loops in nested loop
joins can take different time and process different amounts of entries.
It makes average statistics returned by explain analyze not very useful for DBA.
This patch add collecting of min, max and total statistics for time and rows
across all loops to EXPLAIN ANALYSE. You need to set the VERBOSE flag to display
this information. The patch contains regression tests.
Example of results in TEXT format:
-> Append (actual rows=5 loops=5)
Loop Min Rows: 2 Max Rows: 6 Total Rows: 23
Reviewed-by: Lukas Fittl, Justin Pryzby, Yugo Nagata, Julien Rouhaud.
---
src/backend/commands/explain.c | 42 ++++++-
src/backend/executor/instrument.c | 38 +++++-
src/include/executor/instrument.h | 10 ++
src/test/regress/expected/explain.out | 10 ++
src/test/regress/expected/partition_prune.out | 117 ++++++++++++++++++
src/test/regress/sql/partition_prune.sql | 32 +++++
6 files changed, 247 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 06e089a1220..df14803ff06 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -119,7 +119,7 @@ static void show_instrumentation_count(const char *qlabel, int which,
static void show_foreignscan_info(ForeignScanState *fsstate, ExplainState *es);
static void show_eval_params(Bitmapset *bms_params, ExplainState *es);
static void show_loop_info(Instrumentation *instrument, bool isworker,
- ExplainState *es);
+ ExplainState *es);
static const char *explain_get_index_name(Oid indexId);
static void show_buffer_usage(ExplainState *es, const BufferUsage *usage,
bool planning);
@@ -541,6 +541,9 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
if (es->wal)
instrument_option |= INSTRUMENT_WAL;
+ if (es->verbose)
+ instrument_option |= INSTRUMENT_EXTRA;
+
/*
* We always collect timing for the entire statement, even when node-level
* timing is off, so we don't look at es->timing here. (We could skip
@@ -1624,6 +1627,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
appendStringInfoString(es->str, " (never executed)");
else
{
+ /* without min and max values because actual result is 0 */
if (es->timing)
{
ExplainPropertyFloat("Actual Startup Time", "ms", 0.0, 3, es);
@@ -3989,6 +3993,11 @@ show_loop_info(Instrumentation *instrument, bool isworker, ExplainState *es)
double startup_ms = 1000.0 * instrument->startup / nloops;
double total_ms = 1000.0 * instrument->total / nloops;
double rows = instrument->ntuples / nloops;
+ double loop_total_rows = instrument->ntuples;
+ double loop_min_r = instrument->min_tuples;
+ double loop_max_r = instrument->max_tuples;
+ double loop_min_t_ms = 1000.0 * instrument->min_t;
+ double loop_max_t_ms = 1000.0 * instrument->max_t;
if (es->format == EXPLAIN_FORMAT_TEXT)
{
@@ -4008,6 +4017,21 @@ show_loop_info(Instrumentation *instrument, bool isworker, ExplainState *es)
if (!isworker)
appendStringInfoChar(es->str, ')');
+
+ if (nloops > 1 && es->verbose)
+ {
+ appendStringInfoChar(es->str, '\n');
+ ExplainIndentText(es);
+
+ if (es->timing)
+ appendStringInfo(es->str,
+ "Loop Min Rows: %.0f Max Rows: %.0f Total Rows: %.0f Min Time: %.3f Max Time: %.3f",
+ loop_min_r, loop_max_r, loop_total_rows, loop_min_t_ms, loop_max_t_ms);
+ else
+ appendStringInfo(es->str,
+ "Loop Min Rows: %.0f Max Rows: %.0f Total Rows: %.0f",
+ loop_min_r, loop_max_r, loop_total_rows);
+ }
}
else
{
@@ -4017,8 +4041,24 @@ show_loop_info(Instrumentation *instrument, bool isworker, ExplainState *es)
3, es);
ExplainPropertyFloat("Actual Total Time", "ms", total_ms,
3, es);
+ if (nloops > 1 && es->verbose)
+ {
+ ExplainPropertyFloat("Loop Min Time", "ms", loop_min_t_ms,
+ 3, es);
+ ExplainPropertyFloat("Loop Max Time", "ms", loop_max_t_ms,
+ 3, es);
+ }
}
+
ExplainPropertyFloat("Actual Rows", NULL, rows, 0, es);
+
+ if (nloops > 1 && es->verbose)
+ {
+ ExplainPropertyFloat("Loop Min Rows", NULL, loop_min_r, 0, es);
+ ExplainPropertyFloat("Loop Max Rows", NULL, loop_max_r, 0, es);
+ ExplainPropertyFloat("Loop Total Rows", NULL, loop_total_rows, 0, es);
+ }
+
ExplainPropertyFloat("Actual Loops", NULL, nloops, 0, es);
}
}
diff --git a/src/backend/executor/instrument.c b/src/backend/executor/instrument.c
index c5ff02a8424..ac46011e516 100644
--- a/src/backend/executor/instrument.c
+++ b/src/backend/executor/instrument.c
@@ -34,11 +34,12 @@ InstrAlloc(int n, int instrument_options, bool async_mode)
/* initialize all fields to zeroes, then modify as needed */
instr = palloc0(n * sizeof(Instrumentation));
- if (instrument_options & (INSTRUMENT_BUFFERS | INSTRUMENT_TIMER | INSTRUMENT_WAL))
+ if (instrument_options & (INSTRUMENT_BUFFERS | INSTRUMENT_TIMER | INSTRUMENT_WAL | INSTRUMENT_EXTRA))
{
bool need_buffers = (instrument_options & INSTRUMENT_BUFFERS) != 0;
bool need_wal = (instrument_options & INSTRUMENT_WAL) != 0;
bool need_timer = (instrument_options & INSTRUMENT_TIMER) != 0;
+ bool need_extra = (instrument_options & INSTRUMENT_EXTRA) != 0;
int i;
for (i = 0; i < n; i++)
@@ -46,6 +47,7 @@ InstrAlloc(int n, int instrument_options, bool async_mode)
instr[i].need_bufusage = need_buffers;
instr[i].need_walusage = need_wal;
instr[i].need_timer = need_timer;
+ instr[i].need_extra = need_extra;
instr[i].async_mode = async_mode;
}
}
@@ -61,6 +63,7 @@ InstrInit(Instrumentation *instr, int instrument_options)
instr->need_bufusage = (instrument_options & INSTRUMENT_BUFFERS) != 0;
instr->need_walusage = (instrument_options & INSTRUMENT_WAL) != 0;
instr->need_timer = (instrument_options & INSTRUMENT_TIMER) != 0;
+ instr->need_extra = (instrument_options & INSTRUMENT_EXTRA) != 0;
}
/* Entry to a plan node */
@@ -154,6 +157,35 @@ InstrEndLoop(Instrumentation *instr)
instr->startup += instr->firsttuple;
instr->total += totaltime;
instr->ntuples += instr->tuplecount;
+
+ /*
+ * this is first loop
+ *
+ * We only initialize the min values. We don't need to bother with the
+ * max, because those are 0 and the non-zero values will get updated a
+ * couple lines later.
+ */
+ if (instr->need_extra)
+ {
+ if (instr->nloops == 0)
+ {
+ instr->min_t = totaltime;
+ instr->min_tuples = instr->tuplecount;
+ }
+
+ if (instr->min_t > totaltime)
+ instr->min_t = totaltime;
+
+ if (instr->max_t < totaltime)
+ instr->max_t = totaltime;
+
+ if (instr->min_tuples > instr->tuplecount)
+ instr->min_tuples = instr->tuplecount;
+
+ if (instr->max_tuples < instr->tuplecount)
+ instr->max_tuples = instr->tuplecount;
+ }
+
instr->nloops += 1;
/* Reset for next cycle (if any) */
@@ -186,6 +218,10 @@ InstrAggNode(Instrumentation *dst, Instrumentation *add)
dst->nloops += add->nloops;
dst->nfiltered1 += add->nfiltered1;
dst->nfiltered2 += add->nfiltered2;
+ dst->min_t = Min(dst->min_t, add->min_t);
+ dst->max_t = Max(dst->max_t, add->max_t);
+ dst->min_tuples = Min(dst->min_tuples, add->min_tuples);
+ dst->max_tuples = Max(dst->max_tuples, add->max_tuples);
/* Add delta of buffer usage since entry to node's totals */
if (dst->need_bufusage)
diff --git a/src/include/executor/instrument.h b/src/include/executor/instrument.h
index 1b7157bdd15..e6178e248dc 100644
--- a/src/include/executor/instrument.h
+++ b/src/include/executor/instrument.h
@@ -58,6 +58,8 @@ typedef enum InstrumentOption
INSTRUMENT_BUFFERS = 1 << 1, /* needs buffer usage */
INSTRUMENT_ROWS = 1 << 2, /* needs row count */
INSTRUMENT_WAL = 1 << 3, /* needs WAL usage */
+ INSTRUMENT_EXTRA = 1 << 4, /* needs counters for min,
+ * max and total values */
INSTRUMENT_ALL = PG_INT32_MAX
} InstrumentOption;
@@ -67,6 +69,8 @@ typedef struct Instrumentation
bool need_timer; /* true if we need timer data */
bool need_bufusage; /* true if we need buffer usage data */
bool need_walusage; /* true if we need WAL usage data */
+ bool need_extra; /* true if we need min, max and total
+ * statistics for loops */
bool async_mode; /* true if node is in async mode */
/* Info about current plan cycle: */
bool running; /* true if we've completed first tuple */
@@ -79,7 +83,13 @@ typedef struct Instrumentation
/* Accumulated statistics across all completed cycles: */
double startup; /* total startup time (in seconds) */
double total; /* total time (in seconds) */
+ double min_t; /* time of fastest loop (in seconds) */
+ double max_t; /* time of slowest loop (in seconds) */
double ntuples; /* total tuples produced */
+ double min_tuples; /* min counter of produced tuples for all
+ * loops */
+ double max_tuples; /* max counter of produced tuples for all
+ * loops */
double ntuples2; /* secondary node-specific tuple counter */
double nloops; /* # of run cycles for this node */
double nfiltered1; /* # of tuples removed by scanqual or joinqual */
diff --git a/src/test/regress/expected/explain.out b/src/test/regress/expected/explain.out
index bc361759219..c70d0e288da 100644
--- a/src/test/regress/expected/explain.out
+++ b/src/test/regress/expected/explain.out
@@ -357,8 +357,13 @@ select jsonb_pretty(
"Actual Loops": 0, +
"Startup Cost": 0.0, +
"Async Capable": false, +
+ "Loop Max Rows": 0, +
+ "Loop Max Time": 0.0, +
+ "Loop Min Rows": 0, +
+ "Loop Min Time": 0.0, +
"Relation Name": "tenk1", +
"Parallel Aware": true, +
+ "Loop Total Rows": 0, +
"Local Hit Blocks": 0, +
"Temp Read Blocks": 0, +
"Actual Total Time": 0.0, +
@@ -403,7 +408,12 @@ select jsonb_pretty(
"Actual Loops": 0, +
"Startup Cost": 0.0, +
"Async Capable": false, +
+ "Loop Max Rows": 0, +
+ "Loop Max Time": 0.0, +
+ "Loop Min Rows": 0, +
+ "Loop Min Time": 0.0, +
"Parallel Aware": false, +
+ "Loop Total Rows": 0, +
"Sort Space Used": 0, +
"Local Hit Blocks": 0, +
"Temp Read Blocks": 0, +
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index 7555764c779..18617b9e206 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1962,6 +1962,24 @@ begin
end loop;
end;
$$;
+create function explain_verbose_parallel_append(text) returns setof text
+language plpgsql as
+$$
+declare
+ ln text;
+begin
+ for ln in
+ execute format('explain (analyze, verbose, costs off, summary off, timing off) %s',
+ $1)
+ loop
+ ln := regexp_replace(ln, 'Workers Launched: \d+', 'Workers Launched: N');
+ ln := regexp_replace(ln, 'actual rows=\d+ loops=\d+', 'actual rows=N loops=N');
+ ln := regexp_replace(ln, 'Loop Min Rows: \d+ Max Rows: \d+ Total Rows: \d+',
+ 'Loop Min Rows: N Max Rows: N Total Rows: N');
+ return next ln;
+ end loop;
+end;
+$$;
prepare ab_q4 (int, int) as
select avg(a) from ab where a between $1 and $2 and b < 4;
-- Encourage use of parallel plans
@@ -2218,6 +2236,39 @@ select explain_parallel_append('select avg(ab.a) from ab inner join lprt_a a on
Index Cond: (a = a.a)
(28 rows)
+-- Tests for extra statistics
+create table lprt_b (b int not null);
+insert into lprt_b select generate_series(1,20);
+select explain_verbose_parallel_append('select * from lprt_a join lprt_b on a != b');
+ explain_verbose_parallel_append
+------------------------------------------------------------------------------
+ Nested Loop (actual rows=N loops=N)
+ Output: lprt_a.a, lprt_b.b
+ Join Filter: (lprt_a.a <> lprt_b.b)
+ Rows Removed by Join Filter: 4
+ -> Gather (actual rows=N loops=N)
+ Output: lprt_b.b
+ Workers Planned: 2
+ Workers Launched: N
+ -> Parallel Seq Scan on public.lprt_b (actual rows=N loops=N)
+ Loop Min Rows: N Max Rows: N Total Rows: N
+ Output: lprt_b.b
+ Worker 0: actual rows=N loops=N
+ Worker 1: actual rows=N loops=N
+ -> Materialize (actual rows=N loops=N)
+ Loop Min Rows: N Max Rows: N Total Rows: N
+ Output: lprt_a.a
+ -> Gather (actual rows=N loops=N)
+ Output: lprt_a.a
+ Workers Planned: 1
+ Workers Launched: N
+ -> Parallel Seq Scan on public.lprt_a (actual rows=N loops=N)
+ Loop Min Rows: N Max Rows: N Total Rows: N
+ Output: lprt_a.a
+ Worker 0: actual rows=N loops=N
+(24 rows)
+
+drop table lprt_b;
delete from lprt_a where a = 1;
select explain_parallel_append('select avg(ab.a) from ab inner join lprt_a a on ab.a = a.a where a.a in(1, 0, 0)');
explain_parallel_append
@@ -2715,6 +2766,72 @@ order by tbl1.col1, tprt.col1;
1001 | 1001
(3 rows)
+-- Tests for extra statistics
+explain (analyze, verbose, costs off, summary off, timing off)
+select * from tbl1 inner join tprt on tbl1.col1 > tprt.col1;
+ QUERY PLAN
+---------------------------------------------------------------------------------
+ Nested Loop (actual rows=23 loops=1)
+ Output: tbl1.col1, tprt.col1
+ -> Seq Scan on public.tbl1 (actual rows=5 loops=1)
+ Output: tbl1.col1
+ -> Append (actual rows=5 loops=5)
+ Loop Min Rows: 2 Max Rows: 6 Total Rows: 23
+ -> Index Scan using tprt1_idx on public.tprt_1 (actual rows=2 loops=5)
+ Loop Min Rows: 2 Max Rows: 2 Total Rows: 10
+ Output: tprt_1.col1
+ Index Cond: (tprt_1.col1 < tbl1.col1)
+ -> Index Scan using tprt2_idx on public.tprt_2 (actual rows=3 loops=4)
+ Loop Min Rows: 2 Max Rows: 3 Total Rows: 11
+ Output: tprt_2.col1
+ Index Cond: (tprt_2.col1 < tbl1.col1)
+ -> Index Scan using tprt3_idx on public.tprt_3 (actual rows=1 loops=2)
+ Loop Min Rows: 1 Max Rows: 1 Total Rows: 2
+ Output: tprt_3.col1
+ Index Cond: (tprt_3.col1 < tbl1.col1)
+ -> Index Scan using tprt4_idx on public.tprt_4 (never executed)
+ Output: tprt_4.col1
+ Index Cond: (tprt_4.col1 < tbl1.col1)
+ -> Index Scan using tprt5_idx on public.tprt_5 (never executed)
+ Output: tprt_5.col1
+ Index Cond: (tprt_5.col1 < tbl1.col1)
+ -> Index Scan using tprt6_idx on public.tprt_6 (never executed)
+ Output: tprt_6.col1
+ Index Cond: (tprt_6.col1 < tbl1.col1)
+(27 rows)
+
+explain (analyze, verbose, costs off, summary off, timing off)
+select * from tbl1 inner join tprt on tbl1.col1 = tprt.col1;
+ QUERY PLAN
+---------------------------------------------------------------------------------
+ Nested Loop (actual rows=3 loops=1)
+ Output: tbl1.col1, tprt.col1
+ -> Seq Scan on public.tbl1 (actual rows=5 loops=1)
+ Output: tbl1.col1
+ -> Append (actual rows=1 loops=5)
+ Loop Min Rows: 0 Max Rows: 1 Total Rows: 3
+ -> Index Scan using tprt1_idx on public.tprt_1 (never executed)
+ Output: tprt_1.col1
+ Index Cond: (tprt_1.col1 = tbl1.col1)
+ -> Index Scan using tprt2_idx on public.tprt_2 (actual rows=1 loops=2)
+ Loop Min Rows: 1 Max Rows: 1 Total Rows: 2
+ Output: tprt_2.col1
+ Index Cond: (tprt_2.col1 = tbl1.col1)
+ -> Index Scan using tprt3_idx on public.tprt_3 (actual rows=0 loops=3)
+ Loop Min Rows: 0 Max Rows: 1 Total Rows: 1
+ Output: tprt_3.col1
+ Index Cond: (tprt_3.col1 = tbl1.col1)
+ -> Index Scan using tprt4_idx on public.tprt_4 (never executed)
+ Output: tprt_4.col1
+ Index Cond: (tprt_4.col1 = tbl1.col1)
+ -> Index Scan using tprt5_idx on public.tprt_5 (never executed)
+ Output: tprt_5.col1
+ Index Cond: (tprt_5.col1 = tbl1.col1)
+ -> Index Scan using tprt6_idx on public.tprt_6 (never executed)
+ Output: tprt_6.col1
+ Index Cond: (tprt_6.col1 = tbl1.col1)
+(26 rows)
+
-- Last partition
delete from tbl1;
insert into tbl1 values (4400);
diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index d70bd8610cb..68110e20eeb 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -469,6 +469,25 @@ begin
end;
$$;
+create function explain_verbose_parallel_append(text) returns setof text
+language plpgsql as
+$$
+declare
+ ln text;
+begin
+ for ln in
+ execute format('explain (analyze, verbose, costs off, summary off, timing off) %s',
+ $1)
+ loop
+ ln := regexp_replace(ln, 'Workers Launched: \d+', 'Workers Launched: N');
+ ln := regexp_replace(ln, 'actual rows=\d+ loops=\d+', 'actual rows=N loops=N');
+ ln := regexp_replace(ln, 'Loop Min Rows: \d+ Max Rows: \d+ Total Rows: \d+',
+ 'Loop Min Rows: N Max Rows: N Total Rows: N');
+ return next ln;
+ end loop;
+end;
+$$;
+
prepare ab_q4 (int, int) as
select avg(a) from ab where a between $1 and $2 and b < 4;
@@ -528,6 +547,12 @@ insert into lprt_a values(3),(3);
select explain_parallel_append('select avg(ab.a) from ab inner join lprt_a a on ab.a = a.a where a.a in(1, 0, 3)');
select explain_parallel_append('select avg(ab.a) from ab inner join lprt_a a on ab.a = a.a where a.a in(1, 0, 0)');
+-- Tests for extra statistics
+create table lprt_b (b int not null);
+insert into lprt_b select generate_series(1,20);
+select explain_verbose_parallel_append('select * from lprt_a join lprt_b on a != b');
+drop table lprt_b;
+
delete from lprt_a where a = 1;
select explain_parallel_append('select avg(ab.a) from ab inner join lprt_a a on ab.a = a.a where a.a in(1, 0, 0)');
@@ -654,6 +679,13 @@ select tbl1.col1, tprt.col1 from tbl1
inner join tprt on tbl1.col1 = tprt.col1
order by tbl1.col1, tprt.col1;
+-- Tests for extra statistics
+explain (analyze, verbose, costs off, summary off, timing off)
+select * from tbl1 inner join tprt on tbl1.col1 > tprt.col1;
+
+explain (analyze, verbose, costs off, summary off, timing off)
+select * from tbl1 inner join tprt on tbl1.col1 = tprt.col1;
+
-- Last partition
delete from tbl1;
insert into tbl1 values (4400);
--
2.17.1
--LWVQOr/QoF/fPPTS--
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: add non-option reordering to in-tree getopt_long
@ 2023-06-15 05:30 Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Kyotaro Horiguchi @ 2023-06-15 05:30 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers
At Wed, 14 Jun 2023 15:46:08 -0700, Nathan Bossart <[email protected]> wrote in
> On Wed, Jun 14, 2023 at 03:11:54PM -0700, Noah Misch wrote:
> > Here's some output from this program (on AIX 7.1, same output when compiled
> > 32-bit or 64-bit):
> >
> > $ ./a.out a b c d e f
> > f e d c b a ./a.out
>
> Thanks again.
>
> > Interesting discussion here, too:
> > https://github.com/libuv/libuv/pull/1187
>
> Hm. IIUC modifying the argv pointers on AIX will modify the process title,
> which could cause 'ps' to temporarily show duplicate/missing arguments
> during option parsing. That doesn't seem too terrible, but if pointer
> assignments aren't atomic, maybe 'ps' could be sent off to another part of
> memory, which does seem terrible.
Hmm, the discussion seems to be based on the assumption that argv[0]
can be safely redirected to a different memory location. If that's the
case, we can prpbably rearrange the array, even if there's a small
window where ps might display a confusing command line, right?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: add non-option reordering to in-tree getopt_long
@ 2023-06-16 00:09 Nathan Bossart <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2023-06-16 00:09 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers
On Thu, Jun 15, 2023 at 02:30:34PM +0900, Kyotaro Horiguchi wrote:
> At Wed, 14 Jun 2023 15:46:08 -0700, Nathan Bossart <[email protected]> wrote in
>> Hm. IIUC modifying the argv pointers on AIX will modify the process title,
>> which could cause 'ps' to temporarily show duplicate/missing arguments
>> during option parsing. That doesn't seem too terrible, but if pointer
>> assignments aren't atomic, maybe 'ps' could be sent off to another part of
>> memory, which does seem terrible.
>
> Hmm, the discussion seems to be based on the assumption that argv[0]
> can be safely redirected to a different memory location. If that's the
> case, we can prpbably rearrange the array, even if there's a small
> window where ps might display a confusing command line, right?
If that's the extent of the breakage, then it seems alright to me. I've
attached a new version of the patch that omits the POSIXLY_CORRECT stuff.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v2-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch (3.8K, ../../20230616000959.GA922959@nathanxps13/2-v2-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch)
download | inline diff:
From 89ec098d2515d7cf03b630b787e8f53ca25916b9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 9 Jun 2023 15:51:58 -0700
Subject: [PATCH v2 1/1] Teach in-tree getopt_long() to move non-options to the
end of argv.
---
src/bin/scripts/t/040_createuser.pl | 10 +++++----
src/port/getopt_long.c | 34 +++++++++++++++++++++++++----
2 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 9ca282181d..ba40ab11a3 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -42,9 +42,9 @@ $node->issues_sql_like(
'add a role as a member with admin option of the newly created role');
$node->issues_sql_like(
[
- 'createuser', '-m',
- 'regress_user3', '-m',
- 'regress user #4', 'REGRESS_USER5'
+ 'createuser', 'REGRESS_USER5',
+ '-m', 'regress_user3',
+ '-m', 'regress user #4'
],
qr/statement: CREATE ROLE "REGRESS_USER5" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user3,"regress user #4";/,
'add a role as a member of the newly created role');
@@ -73,11 +73,13 @@ $node->issues_sql_like(
qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
'--role');
$node->issues_sql_like(
- [ 'createuser', '--member-of', 'regress_user1', 'regress_user12' ],
+ [ 'createuser', 'regress_user12', '--member-of', 'regress_user1' ],
qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
'--member-of');
$node->command_fails([ 'createuser', 'regress_user1' ],
'fails if role already exists');
+$node->command_fails([ 'createuser', 'regress_user1', '-m', 'regress_user2', 'regress_user3' ],
+ 'fails for too many non-options');
done_testing();
diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index c989276988..4bbd8e0b85 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -50,8 +50,8 @@
* This implementation does not use optreset. Instead, we guarantee that
* it can be restarted on a new argv array after a previous call returned -1,
* if the caller resets optind to 1 before the first call of the new series.
- * (Internally, this means we must be sure to reset "place" to EMSG before
- * returning -1.)
+ * (Internally, this means we must be sure to reset "place" to EMSG and
+ * nonopt_start to -1 before returning -1.)
*/
int
getopt_long(int argc, char *const argv[],
@@ -60,21 +60,45 @@ getopt_long(int argc, char *const argv[],
{
static char *place = EMSG; /* option letter processing */
char *oli; /* option letter list index */
+ static int nonopt_start = -1;
if (!*place)
{ /* update scanning pointer */
if (optind >= argc)
{
place = EMSG;
+ nonopt_start = -1;
return -1;
}
+retry:
place = argv[optind];
if (place[0] != '-')
{
- place = EMSG;
- return -1;
+ char **args = (char **) argv;
+
+ /*
+ * If only non-options remain, return -1. Else, move the
+ * non-option to the end and try again.
+ */
+ if (optind == nonopt_start)
+ {
+ place = EMSG;
+ nonopt_start = -1;
+ return -1;
+ }
+
+ for (int i = optind; i < argc - 1; i++)
+ args[i] = args[i + 1];
+ args[argc - 1] = place;
+
+ if (nonopt_start == -1)
+ nonopt_start = argc - 1;
+ else
+ nonopt_start--;
+
+ goto retry;
}
place++;
@@ -83,6 +107,7 @@ getopt_long(int argc, char *const argv[],
{
/* treat "-" as not being an option */
place = EMSG;
+ nonopt_start = -1;
return -1;
}
@@ -91,6 +116,7 @@ getopt_long(int argc, char *const argv[],
/* found "--", treat it as end of options */
++optind;
place = EMSG;
+ nonopt_start = -1;
return -1;
}
--
2.25.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: add non-option reordering to in-tree getopt_long
@ 2023-06-16 01:30 Michael Paquier <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2023-06-16 01:30 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
On Thu, Jun 15, 2023 at 05:09:59PM -0700, Nathan Bossart wrote:
> On Thu, Jun 15, 2023 at 02:30:34PM +0900, Kyotaro Horiguchi wrote:
>> Hmm, the discussion seems to be based on the assumption that argv[0]
>> can be safely redirected to a different memory location. If that's the
>> case, we can prpbably rearrange the array, even if there's a small
>> window where ps might display a confusing command line, right?
>
> If that's the extent of the breakage, then it seems alright to me.
Okay by me to live with this burden.
> I've attached a new version of the patch that omits the
> POSIXLY_CORRECT stuff.
This looks OK at quick glance, though you may want to document at the
top of getopt_long() the reordering business with the non-options?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: add non-option reordering to in-tree getopt_long
@ 2023-06-16 04:58 Nathan Bossart <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2023-06-16 04:58 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
On Fri, Jun 16, 2023 at 10:30:09AM +0900, Michael Paquier wrote:
> On Thu, Jun 15, 2023 at 05:09:59PM -0700, Nathan Bossart wrote:
>> I've attached a new version of the patch that omits the
>> POSIXLY_CORRECT stuff.
>
> This looks OK at quick glance, though you may want to document at the
> top of getopt_long() the reordering business with the non-options?
I added a comment to this effect in v3. I also noticed that '-' wasn't
properly handled as a non-option, so I've tried to fix that as well.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v3-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch (4.0K, ../../20230616045828.GA972663@nathanxps13/2-v3-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch)
download | inline diff:
From e31fa0ab5237d3ad35bdb44404fd5b5eeea3f5c6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 9 Jun 2023 15:51:58 -0700
Subject: [PATCH v3 1/1] Teach in-tree getopt_long() to move non-options to the
end of argv.
---
src/bin/scripts/t/040_createuser.pl | 10 ++++---
src/port/getopt_long.c | 45 +++++++++++++++++++++--------
2 files changed, 39 insertions(+), 16 deletions(-)
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 9ca282181d..ba40ab11a3 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -42,9 +42,9 @@ $node->issues_sql_like(
'add a role as a member with admin option of the newly created role');
$node->issues_sql_like(
[
- 'createuser', '-m',
- 'regress_user3', '-m',
- 'regress user #4', 'REGRESS_USER5'
+ 'createuser', 'REGRESS_USER5',
+ '-m', 'regress_user3',
+ '-m', 'regress user #4'
],
qr/statement: CREATE ROLE "REGRESS_USER5" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user3,"regress user #4";/,
'add a role as a member of the newly created role');
@@ -73,11 +73,13 @@ $node->issues_sql_like(
qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
'--role');
$node->issues_sql_like(
- [ 'createuser', '--member-of', 'regress_user1', 'regress_user12' ],
+ [ 'createuser', 'regress_user12', '--member-of', 'regress_user1' ],
qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
'--member-of');
$node->command_fails([ 'createuser', 'regress_user1' ],
'fails if role already exists');
+$node->command_fails([ 'createuser', 'regress_user1', '-m', 'regress_user2', 'regress_user3' ],
+ 'fails for too many non-options');
done_testing();
diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index c989276988..da233728e1 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -50,8 +50,11 @@
* This implementation does not use optreset. Instead, we guarantee that
* it can be restarted on a new argv array after a previous call returned -1,
* if the caller resets optind to 1 before the first call of the new series.
- * (Internally, this means we must be sure to reset "place" to EMSG before
- * returning -1.)
+ * (Internally, this means we must be sure to reset "place" to EMSG and
+ * nonopt_start to -1 before returning -1.)
+ *
+ * Note that this routine reorders the pointers in argv (despite the const
+ * qualifier) so that all non-options will be at the end when -1 is returned.
*/
int
getopt_long(int argc, char *const argv[],
@@ -60,37 +63,55 @@ getopt_long(int argc, char *const argv[],
{
static char *place = EMSG; /* option letter processing */
char *oli; /* option letter list index */
+ static int nonopt_start = -1;
if (!*place)
{ /* update scanning pointer */
if (optind >= argc)
{
place = EMSG;
+ nonopt_start = -1;
return -1;
}
+retry:
place = argv[optind];
- if (place[0] != '-')
+ if (place[0] != '-' || place[1] == '\0')
{
- place = EMSG;
- return -1;
- }
+ char **args = (char **) argv;
- place++;
+ /*
+ * If only non-options remain, return -1. Else, move the
+ * non-option to the end and try again.
+ */
+ if (optind == nonopt_start)
+ {
+ place = EMSG;
+ nonopt_start = -1;
+ return -1;
+ }
- if (!*place)
- {
- /* treat "-" as not being an option */
- place = EMSG;
- return -1;
+ for (int i = optind; i < argc - 1; i++)
+ args[i] = args[i + 1];
+ args[argc - 1] = place;
+
+ if (nonopt_start == -1)
+ nonopt_start = argc - 1;
+ else
+ nonopt_start--;
+
+ goto retry;
}
+ place++;
+
if (place[0] == '-' && place[1] == '\0')
{
/* found "--", treat it as end of options */
++optind;
place = EMSG;
+ nonopt_start = -1;
return -1;
}
--
2.25.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: add non-option reordering to in-tree getopt_long
@ 2023-06-16 07:51 Kyotaro Horiguchi <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Kyotaro Horiguchi @ 2023-06-16 07:51 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers
At Thu, 15 Jun 2023 21:58:28 -0700, Nathan Bossart <[email protected]> wrote in
> On Fri, Jun 16, 2023 at 10:30:09AM +0900, Michael Paquier wrote:
> > On Thu, Jun 15, 2023 at 05:09:59PM -0700, Nathan Bossart wrote:
> >> I've attached a new version of the patch that omits the
> >> POSIXLY_CORRECT stuff.
> >
> > This looks OK at quick glance, though you may want to document at the
> > top of getopt_long() the reordering business with the non-options?
>
> I added a comment to this effect in v3. I also noticed that '-' wasn't
> properly handled as a non-option, so I've tried to fix that as well.
(Honestly, the rearrangement code looks somewhat tricky to grasp..)
It doesn't work properly if '--' is involved.
For example, consider the following options (even though they don't
work for the command).
psql -t -T hoho -a hoge -- -1 hage hige huge
After the function returns -1, the argv array looks like this. The
"[..]" indicates the position of optind.
psql -t -T hoho -a -- [-1] hage hige huge hoge
This is clearly incorrect since "hoge" gets moved to the end. The
rearrangement logic needs to take into account the '--'. For your
information, the glibc version yields the following result for the
same options.
psql -t -T hoho -a -- [hoge] -1 hage hige huge
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: add non-option reordering to in-tree getopt_long
@ 2023-06-16 18:28 Nathan Bossart <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2023-06-16 18:28 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers
On Fri, Jun 16, 2023 at 04:51:38PM +0900, Kyotaro Horiguchi wrote:
> (Honestly, the rearrangement code looks somewhat tricky to grasp..)
Yeah, I think there's still some room for improvement here.
> It doesn't work properly if '--' is involved.
>
> For example, consider the following options (even though they don't
> work for the command).
>
> psql -t -T hoho -a hoge -- -1 hage hige huge
>
> After the function returns -1, the argv array looks like this. The
> "[..]" indicates the position of optind.
>
> psql -t -T hoho -a -- [-1] hage hige huge hoge
>
> This is clearly incorrect since "hoge" gets moved to the end. The
> rearrangement logic needs to take into account the '--'. For your
> information, the glibc version yields the following result for the
> same options.
>
> psql -t -T hoho -a -- [hoge] -1 hage hige huge
Ah, so it effectively retains the non-option ordering, even if there is a
'--'. I think this behavior is worth keeping. I attempted to fix this in
the attached patch.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v4-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch (4.1K, ../../20230616182847.GA71216@nathanxps13/2-v4-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch)
download | inline diff:
From 51e1033374c464ed90484f641d31b0ab705672f2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 9 Jun 2023 15:51:58 -0700
Subject: [PATCH v4 1/1] Teach in-tree getopt_long() to move non-options to the
end of argv.
---
src/bin/scripts/t/040_createuser.pl | 10 +++---
src/port/getopt_long.c | 50 +++++++++++++++++++++--------
2 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 9ca282181d..ba40ab11a3 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -42,9 +42,9 @@ $node->issues_sql_like(
'add a role as a member with admin option of the newly created role');
$node->issues_sql_like(
[
- 'createuser', '-m',
- 'regress_user3', '-m',
- 'regress user #4', 'REGRESS_USER5'
+ 'createuser', 'REGRESS_USER5',
+ '-m', 'regress_user3',
+ '-m', 'regress user #4'
],
qr/statement: CREATE ROLE "REGRESS_USER5" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user3,"regress user #4";/,
'add a role as a member of the newly created role');
@@ -73,11 +73,13 @@ $node->issues_sql_like(
qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
'--role');
$node->issues_sql_like(
- [ 'createuser', '--member-of', 'regress_user1', 'regress_user12' ],
+ [ 'createuser', 'regress_user12', '--member-of', 'regress_user1' ],
qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
'--member-of');
$node->command_fails([ 'createuser', 'regress_user1' ],
'fails if role already exists');
+$node->command_fails([ 'createuser', 'regress_user1', '-m', 'regress_user2', 'regress_user3' ],
+ 'fails for too many non-options');
done_testing();
diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index c989276988..a4bdc6c8f0 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -50,8 +50,9 @@
* This implementation does not use optreset. Instead, we guarantee that
* it can be restarted on a new argv array after a previous call returned -1,
* if the caller resets optind to 1 before the first call of the new series.
- * (Internally, this means we must be sure to reset "place" to EMSG before
- * returning -1.)
+ *
+ * Note that this routine reorders the pointers in argv (despite the const
+ * qualifier) so that all non-options will be at the end when -1 is returned.
*/
int
getopt_long(int argc, char *const argv[],
@@ -60,38 +61,59 @@ getopt_long(int argc, char *const argv[],
{
static char *place = EMSG; /* option letter processing */
char *oli; /* option letter list index */
+ static int nonopt_start = -1;
+ static bool force_nonopt = false;
if (!*place)
{ /* update scanning pointer */
+retry:
if (optind >= argc)
{
place = EMSG;
+ nonopt_start = -1;
+ force_nonopt = false;
return -1;
}
place = argv[optind];
- if (place[0] != '-')
+ /* non-options, including '-' and anything after '--' */
+ if (place[0] != '-' || place[1] == '\0' || force_nonopt)
{
- place = EMSG;
- return -1;
- }
+ char **args = (char **) argv;
- place++;
+ /*
+ * If only non-options remain, return -1. Else, move the
+ * non-option to the end and try again.
+ */
+ if (optind == nonopt_start)
+ {
+ place = EMSG;
+ nonopt_start = -1;
+ force_nonopt = false;
+ return -1;
+ }
- if (!*place)
- {
- /* treat "-" as not being an option */
- place = EMSG;
- return -1;
+ for (int i = optind; i < argc - 1; i++)
+ args[i] = args[i + 1];
+ args[argc - 1] = place;
+
+ if (nonopt_start == -1)
+ nonopt_start = argc - 1;
+ else
+ nonopt_start--;
+
+ goto retry;
}
+ place++;
+
if (place[0] == '-' && place[1] == '\0')
{
/* found "--", treat it as end of options */
++optind;
- place = EMSG;
- return -1;
+ force_nonopt = true;
+ goto retry;
}
if (place[0] == '-' && place[1])
--
2.25.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: add non-option reordering to in-tree getopt_long
@ 2023-06-20 05:12 Kyotaro Horiguchi <[email protected]>
parent: Nathan Bossart <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Kyotaro Horiguchi @ 2023-06-20 05:12 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers
At Fri, 16 Jun 2023 11:28:47 -0700, Nathan Bossart <[email protected]> wrote in
> On Fri, Jun 16, 2023 at 04:51:38PM +0900, Kyotaro Horiguchi wrote:
> > (Honestly, the rearrangement code looks somewhat tricky to grasp..)
>
> Yeah, I think there's still some room for improvement here.
The argv elements get shuffled around many times with the
patch. However, I couldn't find a way to decrease the count without
resorting to a forward scan. So I've concluded the current approach
is them most effeicient, considering the complexity.
> Ah, so it effectively retains the non-option ordering, even if there is a
> '--'. I think this behavior is worth keeping. I attempted to fix this in
> the attached patch.
I tried some patterns with the patch and it generates the same results
with the glibc version.
The TAP test looks fine and it catches the change.
Everything looks fine to me.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-06-20 05:12 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-02 02:36 [PATCH 2/2] Add extra statistics to explain for Nested Loop Ekaterina Sokolova <[email protected]>
2023-06-15 05:30 Re: add non-option reordering to in-tree getopt_long Kyotaro Horiguchi <[email protected]>
2023-06-16 00:09 ` Re: add non-option reordering to in-tree getopt_long Nathan Bossart <[email protected]>
2023-06-16 01:30 ` Re: add non-option reordering to in-tree getopt_long Michael Paquier <[email protected]>
2023-06-16 04:58 ` Re: add non-option reordering to in-tree getopt_long Nathan Bossart <[email protected]>
2023-06-16 07:51 ` Re: add non-option reordering to in-tree getopt_long Kyotaro Horiguchi <[email protected]>
2023-06-16 18:28 ` Re: add non-option reordering to in-tree getopt_long Nathan Bossart <[email protected]>
2023-06-20 05:12 ` Re: add non-option reordering to in-tree getopt_long Kyotaro Horiguchi <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox