agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feed[PATCH 7/8] A couple more places for incremental sort
8+ messages / 4 participants
[nested] [flat]
* [PATCH 7/8] A couple more places for incremental sort
@ 2019-07-28 14:03 Tomas Vondra <tomas@2ndquadrant.com>
0 siblings, 0 replies; 8+ messages in thread
From: Tomas Vondra @ 2019-07-28 14:03 UTC (permalink / raw)
---
src/backend/optimizer/geqo/geqo_eval.c | 2 +-
src/backend/optimizer/plan/planner.c | 220 ++++++++++++++++++++++++-
2 files changed, 217 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/geqo/geqo_eval.c b/src/backend/optimizer/geqo/geqo_eval.c
index 6d897936d7..ff33acc7b6 100644
--- a/src/backend/optimizer/geqo/geqo_eval.c
+++ b/src/backend/optimizer/geqo/geqo_eval.c
@@ -274,7 +274,7 @@ merge_clump(PlannerInfo *root, List *clumps, Clump *new_clump, int num_gene,
* grouping_planner).
*/
if (old_clump->size + new_clump->size < num_gene)
- generate_gather_paths(root, joinrel, false);
+ generate_useful_gather_paths(root, joinrel, false);
/* Find and save the cheapest paths for this joinrel */
set_cheapest(joinrel);
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 46dc355af3..2880fcabe8 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5080,6 +5080,67 @@ create_ordered_paths(PlannerInfo *root,
add_path(ordered_rel, path);
}
+
+ /*
+ * Consider incremental sort with a gather merge on partial paths.
+ *
+ * XXX This is probably duplicate with the paths we already generate
+ * in generate_useful_gather_paths in apply_scanjoin_target_to_paths.
+ */
+ if (enable_incrementalsort)
+ {
+ ListCell *lc;
+
+ foreach(lc, input_rel->partial_pathlist)
+ {
+ Path *input_path = (Path *) lfirst(lc);
+ Path *sorted_path = input_path;
+ bool is_sorted;
+ int presorted_keys;
+ double total_groups;
+
+ /*
+ * We don't care if this is the cheapest partial path - we
+ * can't simply skip it, because it may be partially sorted in
+ * which case we want to consider incremental sort on top of
+ * it (instead of full sort, which is what happens above).
+ */
+
+ is_sorted = pathkeys_common_contained_in(root->sort_pathkeys,
+ input_path->pathkeys,
+ &presorted_keys);
+
+ /* Ignore already sorted paths */
+ if (is_sorted)
+ continue;
+
+ if (presorted_keys == 0)
+ continue;
+
+ /* Since we have presorted keys, consider incremental sort. */
+ sorted_path = (Path *) create_incremental_sort_path(root,
+ ordered_rel,
+ input_path,
+ root->sort_pathkeys,
+ presorted_keys,
+ limit_tuples);
+ total_groups = input_path->rows *
+ input_path->parallel_workers;
+ sorted_path = (Path *)
+ create_gather_merge_path(root, ordered_rel,
+ sorted_path,
+ sorted_path->pathtarget,
+ root->sort_pathkeys, NULL,
+ &total_groups);
+
+ /* Add projection step if needed */
+ if (sorted_path->pathtarget != target)
+ sorted_path = apply_projection_to_path(root, ordered_rel,
+ sorted_path, target);
+
+ add_path(ordered_rel, sorted_path);
+ }
+ }
}
/*
@@ -6580,12 +6641,18 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
foreach(lc, partially_grouped_rel->pathlist)
{
Path *path = (Path *) lfirst(lc);
+ Path *path_original = path;
+ bool is_sorted;
+ int presorted_keys;
+
+ is_sorted = pathkeys_contained_in(root->group_pathkeys,
+ path->pathkeys);
/*
* Insert a Sort node, if required. But there's no point in
* sorting anything but the cheapest path.
*/
- if (!pathkeys_contained_in(root->group_pathkeys, path->pathkeys))
+ if (!is_sorted)
{
if (path != partially_grouped_rel->cheapest_total_path)
continue;
@@ -6616,6 +6683,56 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
parse->groupClause,
havingQual,
dNumGroups));
+
+ /*
+ * Now we may consider incremental sort on this path, but only
+ * when the path is not already sorted and when incremental
+ * sort is enabled.
+ */
+ if (is_sorted || !enable_incrementalsort)
+ continue;
+
+ /* Restore the input path (we might have added Sort on top). */
+ path = path_original;
+
+ is_sorted = pathkeys_common_contained_in(root->group_pathkeys,
+ path->pathkeys,
+ &presorted_keys);
+
+ /* We've already skipped fully sorted paths above. */
+ Assert(!is_sorted);
+
+ /* no shared prefix, not point in building incremental sort */
+ if (presorted_keys == 0)
+ continue;
+
+ path = (Path *) create_incremental_sort_path(root,
+ grouped_rel,
+ path,
+ root->group_pathkeys,
+ presorted_keys,
+ -1.0);
+
+ if (parse->hasAggs)
+ add_path(grouped_rel, (Path *)
+ create_agg_path(root,
+ grouped_rel,
+ path,
+ grouped_rel->reltarget,
+ parse->groupClause ? AGG_SORTED : AGG_PLAIN,
+ AGGSPLIT_FINAL_DESERIAL,
+ parse->groupClause,
+ havingQual,
+ agg_final_costs,
+ dNumGroups));
+ else
+ add_path(grouped_rel, (Path *)
+ create_group_path(root,
+ grouped_rel,
+ path,
+ parse->groupClause,
+ havingQual,
+ dNumGroups));
}
}
}
@@ -6887,6 +7004,60 @@ create_partial_grouping_paths(PlannerInfo *root,
dNumPartialGroups));
}
}
+
+ /*
+ * Also consider incremental sort on all partially sorted paths.
+ */
+ if (enable_incrementalsort)
+ {
+ foreach(lc, input_rel->pathlist)
+ {
+ Path *path = (Path *) lfirst(lc);
+ bool is_sorted;
+ int presorted_keys;
+
+ is_sorted = pathkeys_common_contained_in(root->group_pathkeys,
+ path->pathkeys,
+ &presorted_keys);
+
+ /* Ignore already sorted paths */
+ if (is_sorted)
+ continue;
+
+ if (presorted_keys == 0)
+ continue;
+
+ /* Since we have presorted keys, consider incremental sort. */
+ path = (Path *) create_incremental_sort_path(root,
+ partially_grouped_rel,
+ path,
+ root->group_pathkeys,
+ presorted_keys,
+ -1.0);
+
+ if (parse->hasAggs)
+ add_path(partially_grouped_rel, (Path *)
+ create_agg_path(root,
+ partially_grouped_rel,
+ path,
+ partially_grouped_rel->reltarget,
+ parse->groupClause ? AGG_SORTED : AGG_PLAIN,
+ AGGSPLIT_INITIAL_SERIAL,
+ parse->groupClause,
+ NIL,
+ agg_partial_costs,
+ dNumPartialGroups));
+ else
+ add_path(partially_grouped_rel, (Path *)
+ create_group_path(root,
+ partially_grouped_rel,
+ path,
+ parse->groupClause,
+ NIL,
+ dNumPartialGroups));
+ }
+ }
+
}
if (can_sort && cheapest_partial_path != NULL)
@@ -6951,10 +7122,10 @@ create_partial_grouping_paths(PlannerInfo *root,
/* We've already skipped fully sorted paths above. */
Assert(!is_sorted);
- /* no shared prefix, not point in building incremental sort */
if (presorted_keys == 0)
continue;
+ /* Since we have presorted keys, consider incremental sort. */
path = (Path *) create_incremental_sort_path(root,
partially_grouped_rel,
path,
@@ -7079,10 +7250,11 @@ create_partial_grouping_paths(PlannerInfo *root,
static void
gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
{
+ ListCell *lc;
Path *cheapest_partial_path;
/* Try Gather for unordered paths and Gather Merge for ordered ones. */
- generate_gather_paths(root, rel, true);
+ generate_useful_gather_paths(root, rel, true);
/* Try cheapest partial path + explicit Sort + Gather Merge. */
cheapest_partial_path = linitial(rel->partial_pathlist);
@@ -7108,6 +7280,46 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
add_path(rel, path);
}
+
+ if (!enable_incrementalsort)
+ return;
+
+ /* also consider incremental sort on partial paths, if enabled */
+ foreach(lc, rel->partial_pathlist)
+ {
+ Path *path = (Path *) lfirst(lc);
+ bool is_sorted;
+ int presorted_keys;
+ double total_groups;
+
+ is_sorted = pathkeys_common_contained_in(root->group_pathkeys,
+ path->pathkeys,
+ &presorted_keys);
+
+ if (is_sorted)
+ continue;
+
+ if (presorted_keys == 0)
+ continue;
+
+ path = (Path *) create_incremental_sort_path(root,
+ rel,
+ path,
+ root->group_pathkeys,
+ presorted_keys,
+ -1.0);
+
+ path = (Path *)
+ create_gather_merge_path(root,
+ rel,
+ path,
+ rel->reltarget,
+ root->group_pathkeys,
+ NULL,
+ &total_groups);
+
+ add_path(rel, path);
+ }
}
/*
@@ -7209,7 +7421,7 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
* paths by doing it after the final scan/join target has been
* applied.
*/
- generate_gather_paths(root, rel, false);
+ generate_useful_gather_paths(root, rel, false);
/* Can't use parallel query above this level. */
rel->partial_pathlist = NIL;
--
2.21.1
--dfcjsgdukgytabqd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="v39-0008-fix.patch"
^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH 5/5] A couple more places for incremental sort
@ 2019-07-28 14:03 Tomas Vondra <tomas@2ndquadrant.com>
0 siblings, 0 replies; 8+ messages in thread
From: Tomas Vondra @ 2019-07-28 14:03 UTC (permalink / raw)
---
src/backend/optimizer/geqo/geqo_eval.c | 2 +-
src/backend/optimizer/plan/planner.c | 218 ++++++++++++++++++++++++-
2 files changed, 216 insertions(+), 4 deletions(-)
diff --git a/src/backend/optimizer/geqo/geqo_eval.c b/src/backend/optimizer/geqo/geqo_eval.c
index 6d897936d7..ff33acc7b6 100644
--- a/src/backend/optimizer/geqo/geqo_eval.c
+++ b/src/backend/optimizer/geqo/geqo_eval.c
@@ -274,7 +274,7 @@ merge_clump(PlannerInfo *root, List *clumps, Clump *new_clump, int num_gene,
* grouping_planner).
*/
if (old_clump->size + new_clump->size < num_gene)
- generate_gather_paths(root, joinrel, false);
+ generate_useful_gather_paths(root, joinrel, false);
/* Find and save the cheapest paths for this joinrel */
set_cheapest(joinrel);
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 84ed69ec5e..15223017c0 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5070,6 +5070,67 @@ create_ordered_paths(PlannerInfo *root,
add_path(ordered_rel, path);
}
+
+ /*
+ * Consider incremental sort with a gather merge on partial paths.
+ *
+ * XXX This is probably duplicate with the paths we already generate
+ * in generate_useful_gather_paths in apply_scanjoin_target_to_paths.
+ */
+ if (enable_incrementalsort)
+ {
+ ListCell *lc;
+
+ foreach (lc, input_rel->partial_pathlist)
+ {
+ Path *input_path = (Path *) lfirst(lc);
+ Path *sorted_path = input_path;
+ bool is_sorted;
+ int presorted_keys;
+ double total_groups;
+
+ /*
+ * We don't care if this is the cheapest partial path - we
+ * can't simply skip it, because it may be partially sorted
+ * in which case we want to consider incremental sort on top
+ * of it (instead of full sort, which is what happens above).
+ */
+
+ is_sorted = pathkeys_common_contained_in(root->sort_pathkeys,
+ input_path->pathkeys,
+ &presorted_keys);
+
+ /* also ignore already sorted paths */
+ if (is_sorted)
+ continue;
+
+ if (presorted_keys == 0)
+ continue;
+
+ /* Also consider incremental sort. */
+ sorted_path = (Path *) create_incremental_sort_path(root,
+ ordered_rel,
+ input_path,
+ root->sort_pathkeys,
+ presorted_keys,
+ limit_tuples);
+ total_groups = input_path->rows *
+ input_path->parallel_workers;
+ sorted_path = (Path *)
+ create_gather_merge_path(root, ordered_rel,
+ sorted_path,
+ sorted_path->pathtarget,
+ root->sort_pathkeys, NULL,
+ &total_groups);
+
+ /* Add projection step if needed */
+ if (sorted_path->pathtarget != target)
+ sorted_path = apply_projection_to_path(root, ordered_rel,
+ sorted_path, target);
+
+ add_path(ordered_rel, sorted_path);
+ }
+ }
}
/*
@@ -6570,12 +6631,18 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
foreach(lc, partially_grouped_rel->pathlist)
{
Path *path = (Path *) lfirst(lc);
+ Path *path_original = path;
+ bool is_sorted;
+ int presorted_keys;
+
+ is_sorted = pathkeys_contained_in(root->group_pathkeys,
+ path->pathkeys);
/*
* Insert a Sort node, if required. But there's no point in
* sorting anything but the cheapest path.
*/
- if (!pathkeys_contained_in(root->group_pathkeys, path->pathkeys))
+ if (!is_sorted)
{
if (path != partially_grouped_rel->cheapest_total_path)
continue;
@@ -6606,6 +6673,56 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
parse->groupClause,
havingQual,
dNumGroups));
+
+ /*
+ * Now we may consider incremental sort on this path, but only
+ * when the path is not already sorted and when incremental sort
+ * is enabled.
+ */
+ if (is_sorted || !enable_incrementalsort)
+ continue;
+
+ /* Restore the input path (we might have addes Sort on top). */
+ path = path_original;
+
+ is_sorted = pathkeys_common_contained_in(root->group_pathkeys,
+ path->pathkeys,
+ &presorted_keys);
+
+ /* We've already skipped fully sorted paths above. */
+ Assert(!is_sorted);
+
+ /* no shared prefix, not point in building incremental sort */
+ if (presorted_keys == 0)
+ continue;
+
+ path = (Path *) create_incremental_sort_path(root,
+ grouped_rel,
+ path,
+ root->group_pathkeys,
+ presorted_keys,
+ -1.0);
+
+ if (parse->hasAggs)
+ add_path(grouped_rel, (Path *)
+ create_agg_path(root,
+ grouped_rel,
+ path,
+ grouped_rel->reltarget,
+ parse->groupClause ? AGG_SORTED : AGG_PLAIN,
+ AGGSPLIT_FINAL_DESERIAL,
+ parse->groupClause,
+ havingQual,
+ agg_final_costs,
+ dNumGroups));
+ else
+ add_path(grouped_rel, (Path *)
+ create_group_path(root,
+ grouped_rel,
+ path,
+ parse->groupClause,
+ havingQual,
+ dNumGroups));
}
}
}
@@ -6875,6 +6992,60 @@ create_partial_grouping_paths(PlannerInfo *root,
dNumPartialGroups));
}
}
+
+ /*
+ * Also consider incremental sort on all partially sorted paths.
+ */
+ if (enable_incrementalsort)
+ {
+ foreach(lc, input_rel->pathlist)
+ {
+ Path *path = (Path *) lfirst(lc);
+ bool is_sorted;
+ int presorted_keys;
+
+ is_sorted = pathkeys_common_contained_in(root->group_pathkeys,
+ path->pathkeys,
+ &presorted_keys);
+
+ /* also ignore already sorted paths */
+ if (is_sorted)
+ continue;
+
+ if (presorted_keys == 0)
+ continue;
+
+ /* add incremental sort */
+ path = (Path *) create_incremental_sort_path(root,
+ partially_grouped_rel,
+ path,
+ root->group_pathkeys,
+ presorted_keys,
+ -1.0);
+
+ if (parse->hasAggs)
+ add_path(partially_grouped_rel, (Path *)
+ create_agg_path(root,
+ partially_grouped_rel,
+ path,
+ partially_grouped_rel->reltarget,
+ parse->groupClause ? AGG_SORTED : AGG_PLAIN,
+ AGGSPLIT_INITIAL_SERIAL,
+ parse->groupClause,
+ NIL,
+ agg_partial_costs,
+ dNumPartialGroups));
+ else
+ add_path(partially_grouped_rel, (Path *)
+ create_group_path(root,
+ partially_grouped_rel,
+ path,
+ parse->groupClause,
+ NIL,
+ dNumPartialGroups));
+ }
+ }
+
}
if (can_sort && cheapest_partial_path != NULL)
@@ -7067,10 +7238,11 @@ create_partial_grouping_paths(PlannerInfo *root,
static void
gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
{
+ ListCell *lc;
Path *cheapest_partial_path;
/* Try Gather for unordered paths and Gather Merge for ordered ones. */
- generate_gather_paths(root, rel, true);
+ generate_useful_gather_paths(root, rel, true);
/* Try cheapest partial path + explicit Sort + Gather Merge. */
cheapest_partial_path = linitial(rel->partial_pathlist);
@@ -7096,6 +7268,46 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
add_path(rel, path);
}
+
+ if (!enable_incrementalsort)
+ return;
+
+ /* also consider incremental sort on partial paths, if enabled */
+ foreach (lc, rel->partial_pathlist)
+ {
+ Path *path = (Path *) lfirst(lc);
+ bool is_sorted;
+ int presorted_keys;
+ double total_groups;
+
+ is_sorted = pathkeys_common_contained_in(root->group_pathkeys,
+ path->pathkeys,
+ &presorted_keys);
+
+ if (is_sorted)
+ continue;
+
+ if (presorted_keys == 0)
+ continue;
+
+ path = (Path *) create_incremental_sort_path(root,
+ rel,
+ path,
+ root->group_pathkeys,
+ presorted_keys,
+ -1.0);
+
+ path = (Path *)
+ create_gather_merge_path(root,
+ rel,
+ path,
+ rel->reltarget,
+ root->group_pathkeys,
+ NULL,
+ &total_groups);
+
+ add_path(rel, path);
+ }
}
/*
@@ -7197,7 +7409,7 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
* paths by doing it after the final scan/join target has been
* applied.
*/
- generate_gather_paths(root, rel, false);
+ generate_useful_gather_paths(root, rel, false);
/* Can't use parallel query above this level. */
rel->partial_pathlist = NIL;
--
2.21.1
--xvcuvgto6w2bcqjv--
^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v4] Allow pgbnech to cancel queries during benchmark
@ 2023-07-24 12:53 Yugo Nagata <nagata@sraoss.co.jp>
0 siblings, 0 replies; 8+ messages in thread
From: Yugo Nagata @ 2023-07-24 12:53 UTC (permalink / raw)
Previously, Ctrl+C during benchmark killed pgbench immediately,
but queries running at that time were not cancelled. The commit
fixes this so that cancel requests are sent for all connections
before pgbench exits.
In thread #0, setup_cancel_handler is called before the benchmark
so that CancelRequested is set when SIGINT is sent. When SIGINT
is sent during the benchmark, on non-Windows, thread #0 will be
interrupted, return from I/O wait, and send cancel requests to
all connections. After queries are cancelled, other threads also
be interrupted and pgbench will exit at the end. On Windows, cancel
requests are sent in the callback function specified by
setup_cancel_hander.
---
src/bin/pgbench/pgbench.c | 89 ++++++++++++++++++++
src/bin/pgbench/t/001_pgbench_with_server.pl | 42 +++++++++
2 files changed, 131 insertions(+)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 713e8a06bb..5adf099b76 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -596,6 +596,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* query cancel */
int id; /* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
@@ -638,6 +639,8 @@ typedef struct
* here */
} CState;
+CState *client_states; /* status of all clients */
+
/*
* Thread state
*/
@@ -837,6 +840,10 @@ static void add_socket_to_set(socket_set *sa, int fd, int idx);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
static bool socket_has_input(socket_set *sa, int fd, int idx);
+#ifdef WIN32
+static void pgbench_cancel_callback(void);
+#endif
+
/* callback used to build rows for COPY during data loading */
typedef void (*initRowMethod) (PQExpBufferData *sql, int64 curr);
@@ -3639,6 +3646,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_ABORTED;
break;
}
+ st->cancel = PQgetCancel(st->con);
/* reset now after connection */
now = pg_time_now();
@@ -4670,6 +4678,18 @@ disconnect_all(CState *state, int length)
finishCon(&state[i]);
}
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[1];
+ if (client_states[i].cancel != NULL)
+ (void) PQcancel(client_states[i].cancel, errbuf, sizeof(errbuf));
+ }
+}
+
/*
* Remove old pgbench tables, if any exist
*/
@@ -7146,6 +7166,9 @@ main(int argc, char **argv)
}
}
+ /* enable threads to access the status of all clients */
+ client_states = state;
+
/* other CState initializations */
for (i = 0; i < nclients; i++)
{
@@ -7358,6 +7381,37 @@ threadRun(void *arg)
StatsData last,
aggs;
+ /*
+ * Query cancellation is handled only in thread #0.
+ *
+ * On Windows, a callback function is set in which query cancel requests
+ * are sent to all benchmark queries running in the backend.
+ *
+ * On non-Windows, any callback function is not set. When SIGINT is
+ * received, CancelRequested is just set, and only thread #0 is interrupted
+ * and returns from waiting input from the backend. After that, the thread
+ * sends cancel requests to all benchmark queries.
+ */
+ if (thread->tid == 0)
+#ifdef WIN32
+ setup_cancel_handler(pgbench_cancel_callback);
+#else
+ setup_cancel_handler(NULL);
+#endif
+
+#ifndef WIN32
+ if (thread->tid > 0)
+ {
+ sigset_t sigint_sigset;
+ sigset_t osigset;
+ sigemptyset(&sigint_sigset);
+ sigaddset(&sigint_sigset, SIGINT);
+
+ /* Block SIGINT in all threads except one. */
+ pthread_sigmask(SIG_BLOCK, &sigint_sigset, &osigset);
+ }
+#endif
+
/* open log file if requested */
if (use_log)
{
@@ -7400,6 +7454,7 @@ threadRun(void *arg)
pg_fatal("could not create connection for client %d",
state[i].id);
}
+ state[i].cancel = PQgetCancel(state[i].con);
}
}
@@ -7427,6 +7482,26 @@ threadRun(void *arg)
pg_time_usec_t min_usec;
pg_time_usec_t now = 0; /* set this only if needed */
+ /*
+ * If pgbench is cancelled, send cancel requests to all connections
+ * and exit the benchmark.
+ *
+ * Note that only thread #0 can be interrupted by SIGINT while waiting
+ * the result from the backend. Other threads will return from waiting
+ * just after queries they running are cancelled by thread #0.
+ *
+ * On Windows, cancel requests are sent in the callback function, so
+ * do nothing but exit the benchmark.
+ */
+ if (CancelRequested)
+ {
+#ifndef WIN32
+ if (thread->tid == 0)
+ cancel_all();
+#endif
+ goto done;
+ }
+
/*
* identify which client sockets should be checked for input, and
* compute the nearest time (if any) at which we need to wake up.
@@ -7650,6 +7725,8 @@ finishCon(CState *st)
{
PQfinish(st->con);
st->con = NULL;
+ PQfreeCancel(st->cancel);
+ st->cancel = NULL;
}
}
@@ -7867,3 +7944,15 @@ socket_has_input(socket_set *sa, int fd, int idx)
}
#endif /* POLL_USING_SELECT */
+
+#ifdef WIN32
+/*
+ * query cancellation callback for Windows
+ */
+static void
+pgbench_cancel_callback(void)
+{
+ /* send cancel requests to all connections */
+ cancel_all();
+}
+#endif
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 96be529d6b..43eadfbe9f 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -7,6 +7,7 @@ use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+use Time::HiRes qw(usleep);
# Check the initial state of the data generated. Tables for tellers and
# branches use NULL for their filler attribute. The table accounts uses
@@ -1502,6 +1503,47 @@ update counter set i = i+1 returning i \gset
# Clean up
$node->safe_psql('postgres', 'DROP TABLE counter;');
+# Test query canceling by sending SIGINT to a running pgbench
+SKIP:
+{
+ skip "sending SIGINT on Windows terminates the test itself", 3
+ if $windows_os;
+
+ my ($stdin, $stdout, $stderr, @file);
+
+ @file = $node->_pgbench_make_files(
+ {
+ '003_pgbench_cancel' => qq{
+select pg_sleep($PostgreSQL::Test::Utils::timeout_default);
+ }});
+
+ local %ENV = $node->_get_env();
+
+ my $h = IPC::Run::start(
+ [ 'pgbench', '-c', '2', '-j', '2',
+ '-T', "$PostgreSQL::Test::Utils::timeout_default", @file ],
+ \$stdin, \$stdout, \$stderr);
+
+ $node->poll_query_until('postgres',
+ q{SELECT (SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep') = 2;}
+ ) or die "timed out";
+
+ # Send cancel request
+ $h->signal('INT');
+
+ my $result = finish $h;
+
+ ok(!$result, 'pgbench failed as expected');
+ like(
+ $stderr,
+ qr/Run was aborted; the above results are incomplete/,
+ 'pgbench was canceled');
+
+ is($node->safe_psql('postgres',
+ q{SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep'}),
+ '0', 'all queries were canceled');
+}
+
# done
$node->safe_psql('postgres', 'DROP TABLESPACE regress_pgbench_tap_1_ts');
$node->stop;
--
2.25.1
--Multipart=_Tue__19_Sep_2023_17_30_11_+0900_JATr+KlP9+ALhH+Y--
^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v4] Allow pgbnech to cancel queries during benchmark
@ 2023-07-24 12:53 Yugo Nagata <nagata@sraoss.co.jp>
0 siblings, 0 replies; 8+ messages in thread
From: Yugo Nagata @ 2023-07-24 12:53 UTC (permalink / raw)
Previously, Ctrl+C during benchmark killed pgbench immediately,
but queries running at that time were not cancelled. The commit
fixes this so that cancel requests are sent for all connections
before pgbench exits.
In thread #0, setup_cancel_handler is called before the benchmark
so that CancelRequested is set when SIGINT is sent. When SIGINT
is sent during the benchmark, on non-Windows, thread #0 will be
interrupted, return from I/O wait, and send cancel requests to
all connections. After queries are cancelled, other threads also
be interrupted and pgbench will exit at the end. On Windows, cancel
requests are sent in the callback function specified by
setup_cancel_hander.
---
src/bin/pgbench/pgbench.c | 89 ++++++++++++++++++++
src/bin/pgbench/t/001_pgbench_with_server.pl | 42 +++++++++
2 files changed, 131 insertions(+)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 713e8a06bb..5adf099b76 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -596,6 +596,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* query cancel */
int id; /* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
@@ -638,6 +639,8 @@ typedef struct
* here */
} CState;
+CState *client_states; /* status of all clients */
+
/*
* Thread state
*/
@@ -837,6 +840,10 @@ static void add_socket_to_set(socket_set *sa, int fd, int idx);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
static bool socket_has_input(socket_set *sa, int fd, int idx);
+#ifdef WIN32
+static void pgbench_cancel_callback(void);
+#endif
+
/* callback used to build rows for COPY during data loading */
typedef void (*initRowMethod) (PQExpBufferData *sql, int64 curr);
@@ -3639,6 +3646,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_ABORTED;
break;
}
+ st->cancel = PQgetCancel(st->con);
/* reset now after connection */
now = pg_time_now();
@@ -4670,6 +4678,18 @@ disconnect_all(CState *state, int length)
finishCon(&state[i]);
}
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[1];
+ if (client_states[i].cancel != NULL)
+ (void) PQcancel(client_states[i].cancel, errbuf, sizeof(errbuf));
+ }
+}
+
/*
* Remove old pgbench tables, if any exist
*/
@@ -7146,6 +7166,9 @@ main(int argc, char **argv)
}
}
+ /* enable threads to access the status of all clients */
+ client_states = state;
+
/* other CState initializations */
for (i = 0; i < nclients; i++)
{
@@ -7358,6 +7381,37 @@ threadRun(void *arg)
StatsData last,
aggs;
+ /*
+ * Query cancellation is handled only in thread #0.
+ *
+ * On Windows, a callback function is set in which query cancel requests
+ * are sent to all benchmark queries running in the backend.
+ *
+ * On non-Windows, any callback function is not set. When SIGINT is
+ * received, CancelRequested is just set, and only thread #0 is interrupted
+ * and returns from waiting input from the backend. After that, the thread
+ * sends cancel requests to all benchmark queries.
+ */
+ if (thread->tid == 0)
+#ifdef WIN32
+ setup_cancel_handler(pgbench_cancel_callback);
+#else
+ setup_cancel_handler(NULL);
+#endif
+
+#ifndef WIN32
+ if (thread->tid > 0)
+ {
+ sigset_t sigint_sigset;
+ sigset_t osigset;
+ sigemptyset(&sigint_sigset);
+ sigaddset(&sigint_sigset, SIGINT);
+
+ /* Block SIGINT in all threads except one. */
+ pthread_sigmask(SIG_BLOCK, &sigint_sigset, &osigset);
+ }
+#endif
+
/* open log file if requested */
if (use_log)
{
@@ -7400,6 +7454,7 @@ threadRun(void *arg)
pg_fatal("could not create connection for client %d",
state[i].id);
}
+ state[i].cancel = PQgetCancel(state[i].con);
}
}
@@ -7427,6 +7482,26 @@ threadRun(void *arg)
pg_time_usec_t min_usec;
pg_time_usec_t now = 0; /* set this only if needed */
+ /*
+ * If pgbench is cancelled, send cancel requests to all connections
+ * and exit the benchmark.
+ *
+ * Note that only thread #0 can be interrupted by SIGINT while waiting
+ * the result from the backend. Other threads will return from waiting
+ * just after queries they running are cancelled by thread #0.
+ *
+ * On Windows, cancel requests are sent in the callback function, so
+ * do nothing but exit the benchmark.
+ */
+ if (CancelRequested)
+ {
+#ifndef WIN32
+ if (thread->tid == 0)
+ cancel_all();
+#endif
+ goto done;
+ }
+
/*
* identify which client sockets should be checked for input, and
* compute the nearest time (if any) at which we need to wake up.
@@ -7650,6 +7725,8 @@ finishCon(CState *st)
{
PQfinish(st->con);
st->con = NULL;
+ PQfreeCancel(st->cancel);
+ st->cancel = NULL;
}
}
@@ -7867,3 +7944,15 @@ socket_has_input(socket_set *sa, int fd, int idx)
}
#endif /* POLL_USING_SELECT */
+
+#ifdef WIN32
+/*
+ * query cancellation callback for Windows
+ */
+static void
+pgbench_cancel_callback(void)
+{
+ /* send cancel requests to all connections */
+ cancel_all();
+}
+#endif
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 96be529d6b..43eadfbe9f 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -7,6 +7,7 @@ use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+use Time::HiRes qw(usleep);
# Check the initial state of the data generated. Tables for tellers and
# branches use NULL for their filler attribute. The table accounts uses
@@ -1502,6 +1503,47 @@ update counter set i = i+1 returning i \gset
# Clean up
$node->safe_psql('postgres', 'DROP TABLE counter;');
+# Test query canceling by sending SIGINT to a running pgbench
+SKIP:
+{
+ skip "sending SIGINT on Windows terminates the test itself", 3
+ if $windows_os;
+
+ my ($stdin, $stdout, $stderr, @file);
+
+ @file = $node->_pgbench_make_files(
+ {
+ '003_pgbench_cancel' => qq{
+select pg_sleep($PostgreSQL::Test::Utils::timeout_default);
+ }});
+
+ local %ENV = $node->_get_env();
+
+ my $h = IPC::Run::start(
+ [ 'pgbench', '-c', '2', '-j', '2',
+ '-T', "$PostgreSQL::Test::Utils::timeout_default", @file ],
+ \$stdin, \$stdout, \$stderr);
+
+ $node->poll_query_until('postgres',
+ q{SELECT (SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep') = 2;}
+ ) or die "timed out";
+
+ # Send cancel request
+ $h->signal('INT');
+
+ my $result = finish $h;
+
+ ok(!$result, 'pgbench failed as expected');
+ like(
+ $stderr,
+ qr/Run was aborted; the above results are incomplete/,
+ 'pgbench was canceled');
+
+ is($node->safe_psql('postgres',
+ q{SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep'}),
+ '0', 'all queries were canceled');
+}
+
# done
$node->safe_psql('postgres', 'DROP TABLESPACE regress_pgbench_tap_1_ts');
$node->stop;
--
2.25.1
--Multipart=_Tue__19_Sep_2023_17_30_11_+0900_JATr+KlP9+ALhH+Y--
^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v4] Allow pgbnech to cancel queries during benchmark
@ 2023-07-24 12:53 Yugo Nagata <nagata@sraoss.co.jp>
0 siblings, 0 replies; 8+ messages in thread
From: Yugo Nagata @ 2023-07-24 12:53 UTC (permalink / raw)
Previously, Ctrl+C during benchmark killed pgbench immediately,
but queries running at that time were not cancelled. The commit
fixes this so that cancel requests are sent for all connections
before pgbench exits.
In thread #0, setup_cancel_handler is called before the benchmark
so that CancelRequested is set when SIGINT is sent. When SIGINT
is sent during the benchmark, on non-Windows, thread #0 will be
interrupted, return from I/O wait, and send cancel requests to
all connections. After queries are cancelled, other threads also
be interrupted and pgbench will exit at the end. On Windows, cancel
requests are sent in the callback function specified by
setup_cancel_hander.
---
src/bin/pgbench/pgbench.c | 89 ++++++++++++++++++++
src/bin/pgbench/t/001_pgbench_with_server.pl | 42 +++++++++
2 files changed, 131 insertions(+)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 713e8a06bb..5adf099b76 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -596,6 +596,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* query cancel */
int id; /* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
@@ -638,6 +639,8 @@ typedef struct
* here */
} CState;
+CState *client_states; /* status of all clients */
+
/*
* Thread state
*/
@@ -837,6 +840,10 @@ static void add_socket_to_set(socket_set *sa, int fd, int idx);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
static bool socket_has_input(socket_set *sa, int fd, int idx);
+#ifdef WIN32
+static void pgbench_cancel_callback(void);
+#endif
+
/* callback used to build rows for COPY during data loading */
typedef void (*initRowMethod) (PQExpBufferData *sql, int64 curr);
@@ -3639,6 +3646,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_ABORTED;
break;
}
+ st->cancel = PQgetCancel(st->con);
/* reset now after connection */
now = pg_time_now();
@@ -4670,6 +4678,18 @@ disconnect_all(CState *state, int length)
finishCon(&state[i]);
}
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[1];
+ if (client_states[i].cancel != NULL)
+ (void) PQcancel(client_states[i].cancel, errbuf, sizeof(errbuf));
+ }
+}
+
/*
* Remove old pgbench tables, if any exist
*/
@@ -7146,6 +7166,9 @@ main(int argc, char **argv)
}
}
+ /* enable threads to access the status of all clients */
+ client_states = state;
+
/* other CState initializations */
for (i = 0; i < nclients; i++)
{
@@ -7358,6 +7381,37 @@ threadRun(void *arg)
StatsData last,
aggs;
+ /*
+ * Query cancellation is handled only in thread #0.
+ *
+ * On Windows, a callback function is set in which query cancel requests
+ * are sent to all benchmark queries running in the backend.
+ *
+ * On non-Windows, any callback function is not set. When SIGINT is
+ * received, CancelRequested is just set, and only thread #0 is interrupted
+ * and returns from waiting input from the backend. After that, the thread
+ * sends cancel requests to all benchmark queries.
+ */
+ if (thread->tid == 0)
+#ifdef WIN32
+ setup_cancel_handler(pgbench_cancel_callback);
+#else
+ setup_cancel_handler(NULL);
+#endif
+
+#ifndef WIN32
+ if (thread->tid > 0)
+ {
+ sigset_t sigint_sigset;
+ sigset_t osigset;
+ sigemptyset(&sigint_sigset);
+ sigaddset(&sigint_sigset, SIGINT);
+
+ /* Block SIGINT in all threads except one. */
+ pthread_sigmask(SIG_BLOCK, &sigint_sigset, &osigset);
+ }
+#endif
+
/* open log file if requested */
if (use_log)
{
@@ -7400,6 +7454,7 @@ threadRun(void *arg)
pg_fatal("could not create connection for client %d",
state[i].id);
}
+ state[i].cancel = PQgetCancel(state[i].con);
}
}
@@ -7427,6 +7482,26 @@ threadRun(void *arg)
pg_time_usec_t min_usec;
pg_time_usec_t now = 0; /* set this only if needed */
+ /*
+ * If pgbench is cancelled, send cancel requests to all connections
+ * and exit the benchmark.
+ *
+ * Note that only thread #0 can be interrupted by SIGINT while waiting
+ * the result from the backend. Other threads will return from waiting
+ * just after queries they running are cancelled by thread #0.
+ *
+ * On Windows, cancel requests are sent in the callback function, so
+ * do nothing but exit the benchmark.
+ */
+ if (CancelRequested)
+ {
+#ifndef WIN32
+ if (thread->tid == 0)
+ cancel_all();
+#endif
+ goto done;
+ }
+
/*
* identify which client sockets should be checked for input, and
* compute the nearest time (if any) at which we need to wake up.
@@ -7650,6 +7725,8 @@ finishCon(CState *st)
{
PQfinish(st->con);
st->con = NULL;
+ PQfreeCancel(st->cancel);
+ st->cancel = NULL;
}
}
@@ -7867,3 +7944,15 @@ socket_has_input(socket_set *sa, int fd, int idx)
}
#endif /* POLL_USING_SELECT */
+
+#ifdef WIN32
+/*
+ * query cancellation callback for Windows
+ */
+static void
+pgbench_cancel_callback(void)
+{
+ /* send cancel requests to all connections */
+ cancel_all();
+}
+#endif
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 96be529d6b..43eadfbe9f 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -7,6 +7,7 @@ use warnings;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+use Time::HiRes qw(usleep);
# Check the initial state of the data generated. Tables for tellers and
# branches use NULL for their filler attribute. The table accounts uses
@@ -1502,6 +1503,47 @@ update counter set i = i+1 returning i \gset
# Clean up
$node->safe_psql('postgres', 'DROP TABLE counter;');
+# Test query canceling by sending SIGINT to a running pgbench
+SKIP:
+{
+ skip "sending SIGINT on Windows terminates the test itself", 3
+ if $windows_os;
+
+ my ($stdin, $stdout, $stderr, @file);
+
+ @file = $node->_pgbench_make_files(
+ {
+ '003_pgbench_cancel' => qq{
+select pg_sleep($PostgreSQL::Test::Utils::timeout_default);
+ }});
+
+ local %ENV = $node->_get_env();
+
+ my $h = IPC::Run::start(
+ [ 'pgbench', '-c', '2', '-j', '2',
+ '-T', "$PostgreSQL::Test::Utils::timeout_default", @file ],
+ \$stdin, \$stdout, \$stderr);
+
+ $node->poll_query_until('postgres',
+ q{SELECT (SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep') = 2;}
+ ) or die "timed out";
+
+ # Send cancel request
+ $h->signal('INT');
+
+ my $result = finish $h;
+
+ ok(!$result, 'pgbench failed as expected');
+ like(
+ $stderr,
+ qr/Run was aborted; the above results are incomplete/,
+ 'pgbench was canceled');
+
+ is($node->safe_psql('postgres',
+ q{SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep'}),
+ '0', 'all queries were canceled');
+}
+
# done
$node->safe_psql('postgres', 'DROP TABLESPACE regress_pgbench_tap_1_ts');
$node->stop;
--
2.25.1
--Multipart=_Tue__19_Sep_2023_17_30_11_+0900_JATr+KlP9+ALhH+Y--
^ permalink raw reply [nested|flat] 8+ messages in thread
* Planing edge case for sorts with limit on non null column
@ 2026-02-05 11:23 Mayrom Rabinovich <mayromrabinovich@gmail.com>
2026-02-05 15:46 ` Re: Planing edge case for sorts with limit on non null column Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 8+ messages in thread
From: Mayrom Rabinovich @ 2026-02-05 11:23 UTC (permalink / raw)
To: pgsql-hackers
Hi,
I am not very familiar with mailing lists so forgive me if I am committing
some sort of cardinal sin.
I found a weird edge case within this simple query:
```
-- setup table with an non null column and index on it
create table t(i serial primary key);
-- query by the reverse order of the index
explain select * from t order by i desc limit 1;
-- works as expected with the following plan:
-- "Limit (cost=0.15..0.19 rows=1 width=4)"
-- " -> Index Only Scan Backward using t_pkey on t (cost=0.15..82.41
rows=2550 width=4)"
-- same deal query by the reverse order of the index, but also specify the
wrong null order
-- from my understanding this should not matter because we don't have any
nulls on the table
-- due to the constraint.
explain select * from t order by i desc nulls last limit 1;
-- here is the issue, when I ran the following query I get this plan:
-- "Limit (cost=48.25..48.25 rows=1 width=4)"
-- " -> Sort (cost=48.25..54.63 rows=2550 width=4)"
-- " Sort Key: i DESC NULLS LAST"
-- " -> Seq Scan on t (cost=0.00..35.50 rows=2550 width=4)"
```
It seems that the planner ignores the fact that the column does not contain
nulls, and looks for a match between order of the index nulls and the order
of the nulls specified in the query, even though the nulls order is
irrelevant in this case.
I think that patching `build_index_pathkeys` would lead to the smallest
amount of changes, my concern with the patch is the fact that
`list_member_ptr` iterates over all of the pathkeys in the planner info.
I did this weird step creating an alternative pathkey and testing if its
relevant because the call sites to that function does some sort of
deduplication of useless pathkey and when I tried adding both directions of
`nulls_first` to the `retval` the last one was deduplicated (or at
least that is what I think that is happening).
I am not very familiar with the Postgres codebase but I hacked a simple
patch that from my testing, fixes the issue. But I don't know if it's the
correct place to apply that sort of logic, and I haven't written any tests
yet. The patch is very much work in progress, it's basically a toy example.
I would like to contribute if possible but I wanted to hear your opinion
before digging further into it.
Thanks,
Mayrom Rabinovich
Attachments:
[application/octet-stream] try_sort_with_reverse_null_order_on_non_null_columns.patch (1.8K, ../../CAH-Ro_22oaP1B8oYsaiukY3QzC3yDueB-rGGG_Rtb-QtG+hb+w@mail.gmail.com/3-try_sort_with_reverse_null_order_on_non_null_columns.patch)
download | inline diff:
From 2e436e636b083169b716e47e155542fad3fe0d99 Mon Sep 17 00:00:00 2001
From: Mayrom Rabinovich <mayromrabinovich@gmail.com>
Date: Thu, 5 Feb 2026 11:55:15 +0200
Subject: [PATCH] perf(optimizer): try to create reverse pathkey for non null
indexes
This is done in order to prevent full table scan and sorts on queries
that that do "...order by x desc nulls last limit 1" where the the index
nulls order is not the same as the query BUT the column is non null.
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 5eb7163..6b0b7e4 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -778,6 +778,29 @@ build_index_pathkeys(PlannerInfo *root,
nulls_first = index->nulls_first[i];
}
+ /*
+ * For not null columns nulls_first order is irrelevant since there are no nulls,
+ * We try to create an alternative pathkey with the reverse nulls_first direction and search
+ * if its present in our query pathkeys, if so we should use it as it's a prefect match.
+ */
+ if (index->indexkeys[i] > 0 &&
+ bms_is_member(index->indexkeys[i], index->rel->notnullattnums))
+ {
+ cpathkey = make_pathkey_from_sortinfo(root,
+ indexkey,
+ index->sortopfamily[i],
+ index->opcintype[i],
+ index->indexcollations[i],
+ reverse_sort,
+ !nulls_first,
+ 0,
+ index->rel->relids,
+ false);
+
+ if (cpathkey != NULL && list_member_ptr(root->sort_pathkeys, cpathkey))
+ goto reverse_pathkey_found;
+ }
+
/*
* OK, try to make a canonical pathkey for this sort key.
*/
@@ -791,7 +814,7 @@ build_index_pathkeys(PlannerInfo *root,
0,
index->rel->relids,
false);
-
+reverse_pathkey_found:
if (cpathkey)
{
/*
--
2.52.0
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Planing edge case for sorts with limit on non null column
2026-02-05 11:23 Planing edge case for sorts with limit on non null column Mayrom Rabinovich <mayromrabinovich@gmail.com>
@ 2026-02-05 15:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-02-10 09:29 ` Re: Planing edge case for sorts with limit on non null column Mayrom Rabinovich <mayromrabinovich@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2026-02-05 15:46 UTC (permalink / raw)
To: Mayrom Rabinovich <mayromrabinovich@gmail.com>; +Cc: pgsql-hackers
Mayrom Rabinovich <mayromrabinovich@gmail.com> writes:
> -- same deal query by the reverse order of the index, but also specify the
> wrong null order
> -- from my understanding this should not matter because we don't have any
> nulls on the table
> -- due to the constraint.
No, that is not taken into account. The planner's notion of a
concrete sort order always includes a nulls first/last flag, and this
index doesn't match what the query asks for. If you want this query
to use an index you'll need to make an index that puts nulls at the
other end (either ASC NULLS FIRST or DESC NULLS LAST will do).
I'm not really excited about poking holes in the PathKey concept to
make this work the way you want. I think the odds of introducing bugs
would be high. Also, the question could be turned around: if you know
that the table contains no nulls, why are you going out of your way to
specify the "wrong" null order?
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Planing edge case for sorts with limit on non null column
2026-02-05 11:23 Planing edge case for sorts with limit on non null column Mayrom Rabinovich <mayromrabinovich@gmail.com>
2026-02-05 15:46 ` Re: Planing edge case for sorts with limit on non null column Tom Lane <tgl@sss.pgh.pa.us>
@ 2026-02-10 09:29 ` Mayrom Rabinovich <mayromrabinovich@gmail.com>
0 siblings, 0 replies; 8+ messages in thread
From: Mayrom Rabinovich @ 2026-02-10 09:29 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
Thanks for the quick response,
On Thu, Feb 5, 2026 at 5:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Also, the question could be turned around: if you know
> that the table contains no nulls, why are you going out of your way to
> specify the "wrong" null order?
That query was generated by an ORM, and I didn't want to create a new
index on my table just for that query because of the overhead
associated with it.
So I ended up patching the ORM library I used in order to drop the
null ordering if the column is non null. But still, that caught me off
guard. I was expecting Postgres to build a better plan for the query.
Here is a simple example that shows how I stumbled into that edge case:
```
-- Create a table to query using created_at as a pagination cursor
CREATE TABLE d (i INT PRIMARY KEY, created_at TIMESTAMP NOT NULL DEFAULT NOW());
CREATE INDEX a ON d (created_at);
-- Get the next 10 records using the "a" index, this select is called
repeatedly with decreasing created_at value based on the smallest
value returned by the previous query.
-- this query is generated by my ORM and the ORM was programmed to
always return nulls last when working with pagination, so it builds a
query similar to this one:
SELECT * FROM d WHERE created_at < $0 ORDER BY created_at DESC NULLS
LAST LIMIT 10;
```
> I'm not really excited about poking holes in the PathKey concept to
> make this work the way you want. I think the odds of introducing bugs
> would be high.
Do you have anything in mind that would be acceptable or safe?
Unless you feel like the risk outweighs the benefit here, I do think
that this edge case could catch other people off guard, especially
users that interact with the database using some sort of ORM.
Thanks again,
Mayrom Rabinovich
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-02-10 09:29 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-07-28 14:03 [PATCH 7/8] A couple more places for incremental sort Tomas Vondra <tomas@2ndquadrant.com>
2019-07-28 14:03 [PATCH 5/5] A couple more places for incremental sort Tomas Vondra <tomas@2ndquadrant.com>
2023-07-24 12:53 [PATCH v4] Allow pgbnech to cancel queries during benchmark Yugo Nagata <nagata@sraoss.co.jp>
2023-07-24 12:53 [PATCH v4] Allow pgbnech to cancel queries during benchmark Yugo Nagata <nagata@sraoss.co.jp>
2023-07-24 12:53 [PATCH v4] Allow pgbnech to cancel queries during benchmark Yugo Nagata <nagata@sraoss.co.jp>
2026-02-05 11:23 Planing edge case for sorts with limit on non null column Mayrom Rabinovich <mayromrabinovich@gmail.com>
2026-02-05 15:46 ` Re: Planing edge case for sorts with limit on non null column Tom Lane <tgl@sss.pgh.pa.us>
2026-02-10 09:29 ` Re: Planing edge case for sorts with limit on non null column Mayrom Rabinovich <mayromrabinovich@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox