public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Use correct macro to check compressed-ness in amcheck
5+ messages / 3 participants
[nested] [flat]
* [PATCH] Use correct macro to check compressed-ness in amcheck
@ 2022-05-17 07:19 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-17 07:19 UTC (permalink / raw)
check_tuple_attribute uses VARATT_IS_COMPRESSED() for
varatt_external. This should be VARATT_EXTERNAL_IS_COMPRESSED()
instead. Remove the following cross check for data size since the
ycondition cannot be true.
---
contrib/amcheck/verify_heapam.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..e488f5e234 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1385,19 +1385,11 @@ check_tuple_attribute(HeapCheckContext *ctx)
toast_pointer.va_rawsize,
VARLENA_SIZE_LIMIT));
- if (VARATT_IS_COMPRESSED(&toast_pointer))
+ if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer))
{
ToastCompressionId cmid;
bool valid = false;
- /* Compression should never expand the attribute */
- if (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize - VARHDRSZ)
- report_corruption(ctx,
- psprintf("toast value %u external size %u exceeds maximum expected for rawsize %d",
- toast_pointer.va_valueid,
- VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer),
- toast_pointer.va_rawsize));
-
/* Compressed attributes should have a valid compression method */
cmid = TOAST_COMPRESS_METHOD(&toast_pointer);
switch (cmid)
--
2.27.0
----Next_Part(Tue_May_17_16_27_19_2022_200)----
^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH] Use correct macro to check compressed-ness in amcheck
@ 2022-05-17 07:19 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-17 07:19 UTC (permalink / raw)
check_tuple_attribute uses VARATT_IS_COMPRESSED() for
varatt_external. This should be VARATT_EXTERNAL_IS_COMPRESSED()
instead. Remove the following cross check for data size since the
ycondition cannot be true.
---
contrib/amcheck/verify_heapam.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..e488f5e234 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1385,19 +1385,11 @@ check_tuple_attribute(HeapCheckContext *ctx)
toast_pointer.va_rawsize,
VARLENA_SIZE_LIMIT));
- if (VARATT_IS_COMPRESSED(&toast_pointer))
+ if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer))
{
ToastCompressionId cmid;
bool valid = false;
- /* Compression should never expand the attribute */
- if (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize - VARHDRSZ)
- report_corruption(ctx,
- psprintf("toast value %u external size %u exceeds maximum expected for rawsize %d",
- toast_pointer.va_valueid,
- VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer),
- toast_pointer.va_rawsize));
-
/* Compressed attributes should have a valid compression method */
cmid = TOAST_COMPRESS_METHOD(&toast_pointer);
switch (cmid)
--
2.27.0
----Next_Part(Tue_May_17_16_27_19_2022_200)----
^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH] Use correct macro to check compressed-ness in amcheck
@ 2022-05-17 07:19 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-17 07:19 UTC (permalink / raw)
check_tuple_attribute uses VARATT_IS_COMPRESSED() for
varatt_external. This should be VARATT_EXTERNAL_IS_COMPRESSED()
instead. Remove the following cross check for data size since the
ycondition cannot be true.
---
contrib/amcheck/verify_heapam.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..e488f5e234 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1385,19 +1385,11 @@ check_tuple_attribute(HeapCheckContext *ctx)
toast_pointer.va_rawsize,
VARLENA_SIZE_LIMIT));
- if (VARATT_IS_COMPRESSED(&toast_pointer))
+ if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer))
{
ToastCompressionId cmid;
bool valid = false;
- /* Compression should never expand the attribute */
- if (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize - VARHDRSZ)
- report_corruption(ctx,
- psprintf("toast value %u external size %u exceeds maximum expected for rawsize %d",
- toast_pointer.va_valueid,
- VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer),
- toast_pointer.va_rawsize));
-
/* Compressed attributes should have a valid compression method */
cmid = TOAST_COMPRESS_METHOD(&toast_pointer);
switch (cmid)
--
2.27.0
----Next_Part(Tue_May_17_16_27_19_2022_200)----
^ permalink raw reply [nested|flat] 5+ messages in thread
* An improvement on parallel DISTINCT
@ 2023-12-26 11:23 Richard Guo <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Richard Guo @ 2023-12-26 11:23 UTC (permalink / raw)
To: pgsql-hackers
While reviewing Heikki's Omit-junk-columns patchset[1], I noticed that
root->upper_targets[] is used to set target for partial_distinct_rel,
which is not great because root->upper_targets[] is not supposed to be
used by the core code. The comment in grouping_planner() says:
* Save the various upper-rel PathTargets we just computed into
* root->upper_targets[]. The core code doesn't use this, but it
* provides a convenient place for extensions to get at the info.
Then while fixing this issue, I noticed an opportunity for improvement
in how we generate Gather/GatherMerge paths for the two-phase DISTINCT.
The Gather/GatherMerge paths are added by generate_gather_paths(), which
does not consider ordering that might be useful above the GatherMerge
node. This can be improved by using generate_useful_gather_paths()
instead. With this change I can see query plan improvement from the
regression test "select_distinct.sql". For instance,
-- Test parallel DISTINCT
SET parallel_tuple_cost=0;
SET parallel_setup_cost=0;
SET min_parallel_table_scan_size=0;
SET max_parallel_workers_per_gather=2;
-- Ensure we get a parallel plan
EXPLAIN (costs off)
SELECT DISTINCT four FROM tenk1;
-- on master
EXPLAIN (costs off)
SELECT DISTINCT four FROM tenk1;
QUERY PLAN
----------------------------------------------------
Unique
-> Sort
Sort Key: four
-> Gather
Workers Planned: 2
-> HashAggregate
Group Key: four
-> Parallel Seq Scan on tenk1
(8 rows)
-- on patched
EXPLAIN (costs off)
SELECT DISTINCT four FROM tenk1;
QUERY PLAN
----------------------------------------------------
Unique
-> Gather Merge
Workers Planned: 2
-> Sort
Sort Key: four
-> HashAggregate
Group Key: four
-> Parallel Seq Scan on tenk1
(8 rows)
I believe the second plan is better.
Attached is a patch that includes this change and also eliminates the
usage of root->upper_targets[] in the core code. It also makes some
tweaks for the comment.
Any thoughts?
[1]
https://www.postgresql.org/message-id/flat/2ca5865b-4693-40e5-8f78-f3b45d5378fb%40iki.fi
Thanks
Richard
Attachments:
[application/octet-stream] v1-0001-Improve-parallel-DISTINCT.patch (4.6K, ../../CAMbWs48u9VoVOouJsys1qOaC9WVGVmBa+wT1dx8KvxF5GPzezA@mail.gmail.com/3-v1-0001-Improve-parallel-DISTINCT.patch)
download | inline diff:
From 8f7fcf0ffc12407f48c12761a3b4686239d5716d Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Tue, 26 Dec 2023 17:30:36 +0800
Subject: [PATCH v1] Improve parallel DISTINCT
Commit 22c4e88ebf introduced parallel DISTINCT, which is implemented by
adding unique/aggregate paths on top of the input rel's partial paths,
and then adding Gather/GatherMerge paths on top and adding a final
unique/aggregate path at last.
The Gather/GatherMerge paths are added by generate_gather_paths(), which
does not consider ordering that might be useful above the GatherMerge
node. This can be improved by using generate_useful_gather_paths()
instead. As the plan change in regression test shows, this change
improves query plans.
In passing, this patch also avoids the use of root->upper_targets to set
target for partial_distinct_rel, because root->upper_targets is not
supposed to be used by the core code.
---
src/backend/optimizer/path/allpaths.c | 8 ++++----
src/backend/optimizer/plan/planner.c | 13 ++++++++++---
src/test/regress/expected/select_distinct.out | 8 ++++----
3 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 67921a0826..b84bf7ee8b 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -3053,10 +3053,10 @@ set_worktable_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
*
* If we're generating paths for a scan or join relation, override_rows will
* be false, and we'll just use the relation's size estimate. When we're
- * being called for a partially-grouped path, though, we need to override
- * the rowcount estimate. (It's not clear that the particular value we're
- * using here is actually best, but the underlying rel has no estimate so
- * we must do something.)
+ * being called for a partially-grouped or partially-distinct path, though, we
+ * need to override the rowcount estimate. (It's not clear that the
+ * particular value we're using here is actually best, but the underlying rel
+ * has no estimate so we must do something.)
*/
void
generate_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_rows)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 6f45efde21..c4698eb951 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1644,8 +1644,8 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
*/
root->upper_targets[UPPERREL_FINAL] = final_target;
root->upper_targets[UPPERREL_ORDERED] = final_target;
- root->upper_targets[UPPERREL_PARTIAL_DISTINCT] = sort_input_target;
root->upper_targets[UPPERREL_DISTINCT] = sort_input_target;
+ root->upper_targets[UPPERREL_PARTIAL_DISTINCT] = sort_input_target;
root->upper_targets[UPPERREL_WINDOW] = sort_input_target;
root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;
@@ -4751,7 +4751,6 @@ create_partial_distinct_paths(PlannerInfo *root, RelOptInfo *input_rel,
partial_distinct_rel = fetch_upper_rel(root, UPPERREL_PARTIAL_DISTINCT,
NULL);
- partial_distinct_rel->reltarget = root->upper_targets[UPPERREL_PARTIAL_DISTINCT];
partial_distinct_rel->consider_parallel = input_rel->consider_parallel;
/*
@@ -4872,7 +4871,15 @@ create_partial_distinct_paths(PlannerInfo *root, RelOptInfo *input_rel,
if (partial_distinct_rel->partial_pathlist != NIL)
{
- generate_gather_paths(root, partial_distinct_rel, true);
+ /*
+ * Set target for partial_distinct_rel as generate_useful_gather_paths
+ * requires that the input rel has a valid reltarget.
+ */
+ partial_distinct_rel->reltarget = cheapest_partial_path->pathtarget;
+
+ /* Consider generating Gather or Gather Merge paths */
+ generate_useful_gather_paths(root, partial_distinct_rel, true);
+
set_cheapest(partial_distinct_rel);
/*
diff --git a/src/test/regress/expected/select_distinct.out b/src/test/regress/expected/select_distinct.out
index 9d44ea8056..f32d39f76c 100644
--- a/src/test/regress/expected/select_distinct.out
+++ b/src/test/regress/expected/select_distinct.out
@@ -235,10 +235,10 @@ SELECT DISTINCT four FROM tenk1;
QUERY PLAN
----------------------------------------------------
Unique
- -> Sort
- Sort Key: four
- -> Gather
- Workers Planned: 2
+ -> Gather Merge
+ Workers Planned: 2
+ -> Sort
+ Sort Key: four
-> HashAggregate
Group Key: four
-> Parallel Seq Scan on tenk1
--
2.31.0
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: An improvement on parallel DISTINCT
@ 2024-02-02 03:26 David Rowley <[email protected]>
parent: Richard Guo <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: David Rowley @ 2024-02-02 03:26 UTC (permalink / raw)
To: Richard Guo <[email protected]>; +Cc: pgsql-hackers
On Wed, 27 Dec 2023 at 00:23, Richard Guo <[email protected]> wrote:
> -- on master
> EXPLAIN (costs off)
> SELECT DISTINCT four FROM tenk1;
> QUERY PLAN
> ----------------------------------------------------
> Unique
> -> Sort
> Sort Key: four
> -> Gather
> Workers Planned: 2
> -> HashAggregate
> Group Key: four
> -> Parallel Seq Scan on tenk1
> (8 rows)
>
> -- on patched
> EXPLAIN (costs off)
> SELECT DISTINCT four FROM tenk1;
> QUERY PLAN
> ----------------------------------------------------
> Unique
> -> Gather Merge
> Workers Planned: 2
> -> Sort
> Sort Key: four
> -> HashAggregate
> Group Key: four
> -> Parallel Seq Scan on tenk1
> (8 rows)
>
> I believe the second plan is better.
I wonder if this change is worthwhile. The sort is only required at
all because the planner opted to HashAggregate in phase1, of which the
rows are output unordered. If phase1 was done by Group Aggregate, then
no sorting would be needed. The only reason the planner didn't Hash
Aggregate for phase2 is because of the order we generate the distinct
paths and because of STD_FUZZ_FACTOR.
Look at the costs of the above plan:
Unique (cost=397.24..397.28 rows=4 width=4)
if I enable_sort=0; then I get a cheaper plan:
HashAggregate (cost=397.14..397.18 rows=4 width=4)
If we add more rows then the cost of sorting will grow faster than the
cost of hash aggregate due to the O(N log2 N) part of our sort
costing.
If I drop the index on tenk1(hundred), I only need to go to the
"hundred" column to have it switch to Hash Aggregate on the 2nd phase.
This is because the number of distinct groups costs the paths for
Group Aggregate and Hash Aggregate more than STD_FUZZ_FACTOR apart.
Adjusting the STD_FUZZ_FACTOR with the following means Hash Aggregate
is used for both phases.
-#define STD_FUZZ_FACTOR 1.01
+#define STD_FUZZ_FACTOR 1.0000001
In light of this, do you still think it's worthwhile making this change?
For me, I think all it's going to result in is extra planner work
without any performance gains.
> Attached is a patch that includes this change and also eliminates the
> usage of root->upper_targets[] in the core code. It also makes some
> tweaks for the comment.
We should fix that. We can consider it independently from the other
change you're proposing.
David
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-02-02 03:26 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 07:19 [PATCH] Use correct macro to check compressed-ness in amcheck Kyotaro Horiguchi <[email protected]>
2022-05-17 07:19 [PATCH] Use correct macro to check compressed-ness in amcheck Kyotaro Horiguchi <[email protected]>
2022-05-17 07:19 [PATCH] Use correct macro to check compressed-ness in amcheck Kyotaro Horiguchi <[email protected]>
2023-12-26 11:23 An improvement on parallel DISTINCT Richard Guo <[email protected]>
2024-02-02 03:26 ` Re: An improvement on parallel DISTINCT David Rowley <[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