of pg_strnxfrm() are not allowing for that possibility.
Patch attached, which should be backported to 17.
Regards,
Jeff Davis
From 9a9680455cab0cf747ef65cf11cf2cb06d5f500e Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Tue, 30 Jul 2024 12:16:45 -0700
Subject: [PATCH] Relax check for ret
On Tue, 2024-07-30 at 23:01 +0300, Heikki Linnakangas wrote:
> +1. A comment in the pg_strnxfrm() function itself would be good too.
Committed, thank you. Backported to 16 (original email said 17, but
that was a mistake).
Regards,
Jeff Davis
that inherently
maintains a connection between a prefix and a range? Does it remain
true even when strange rules are added to a collation?
Regards,
Jeff Davis
, too?
Comments?
Regards,
Jeff Davis
eAs() but does go
through CreateIntoRelDestReceiver().
See:
https://postgr.es/m/20444c382e6cb5e21e93c94d679d0198b0dba4dd.ca...@j-davis.com
Should we refactor a bit and try to make EXPLAIN use the same code
paths?
Regards,
Jeff Davis
On Fri, 2024-08-02 at 00:13 +0500, Kirill Reshke wrote:
> On Thu, 1 Aug 2024 at 23:27, Jeff Davis wrote:
> >
> > EXPLAIN ANALYZE CREATE MATERIALIZED VIEW doesn't go through
> > ExecCreateTableAs(), but does use CreateIntoRelDestReceiver().
>
> EXPLAIN ANALYZ
row)
I think I like this, as well, except for the return value, which seems
like too much information and a bit over-engineered. Can we simplify it
to what's actually going to be used by pg_upgrade and other tools?
> Attached is v25.
I believe 0001 and 0002 are in good shape API-wise, and I can start
getting those committed. I will try to clean up the code in the
process.
Regards,
Jeff Davis
;s
> design this as an independent hook, separate from rmgrs.
That's a good way to look at it, agreed.
Regards,
Jeff Davis
not easy to make that all
work with REFRESH ... CONCURRENTLY, but perhaps it could work with
CREATE MATERIALIZED VIEW and REFRESH (without CONCURRENTLY).
Regards,
Jeff Davis
REFRESH, but not CONCURRENTLY.
4. Do #3 but also make it work for REFRESH ... CONCURRENTLY and provide
new information that's not available by only explaining the query.
And also figure out if any of this should be back-patched.
Regards,
Jeff Davis
On Tue, 2024-07-30 at 12:13 -0700, Jeff Davis wrote:
> I found a couple issues with the later patches:
>
> * There was still some confusion about the default collation vs.
> datcollate/datctype for callers of wchar2char() and char2wchar()
> (those
> functions only work for lib
le(), which uses the result of setlocale().
Thoughts?
Regards,
Jeff Davis
iswspace(), but behavior can be
different at least in theory.
So I guess we're stuck with setlocale()/uselocale() for a while, unless
we're able to move most of those call sites over to an ascii-only
variant.
Regards,
Jeff Davis
o each of those call sites,
but I'm not sure that's a huge advantage over just using uselocale().
Regards,
Jeff Davis
ted branches, but the fixes are
somewhat invasive so I'm not inclined to backport them unless someone
thinks the problems are serious enough.
Regards,
Jeff Davis
From 7a007d3573c6bda67729ab45bd58931bfcdaf702 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Tue, 6 Aug 2024 12:44:14 -0
s, because "locale" is not even an
argument of the MATCH_LOWER(t) or GETCHAR(t) macros, it's taken
implicitly from the outer scope.
I don't think your patches cause this confusion, but is there a way you
can clarify some of this along the way?
Regards,
Jeff Davis
e here, or just add
that to the release notes for 18?
Regards,
Jeff Davis
rn different results for things like
isspace(), but it sounds plausible -- toupper() can return different
results for 'i' in tr_TR.
Also, what about the values outside 128-255, which are still valid
input to isspace()?
Regards,
Jeff Davis
.@neon.tech
Sounds good, sorry I missed that.
Can you please rebase and we can discuss in that thread?
Regards,
Jeff Davis
On Mon, 2024-08-12 at 09:00 +0200, Peter Eisentraut wrote:
> > I assume it would now be okay to take out "locale &&" here?
>
> Correct, that check is no longer necessary.
Thank you, fixed.
Regards,
Jeff Davis
place where we expect lc_collate_is_c() to work
without catalog access at all: libpq/hba.c uses regexes with
C_COLLATION_OID.
But I don't think that's a major problem -- we can just move the
hardcoded test into pg_newlocale_from_collation() and return a
predefined struct with collate_is_
gt; somewhere to feed to _l functions...
If we're going to do that, why not just have ascii-only variants of our
own? pg_ascii_isspace(...) is at least as readable as isspace_l(...,
LC_C_LOCALE).
Regards,
Jeff Davis
ary code it could be the
> singleton mechanism I showed in CF#5166.
+1 to this approach. It makes things more consistent across platforms
and avoids surprising dependencies on the global setting.
We'll have to be careful that each call site is either OK with C, or
that it gets changed to an _l() variant. We also have to be careful
about extensions.
Regards,
Jeff Davis
field makes it seem like there is more to it so I suggest that we
> should
> just remove it.
+1. When I added that, there was also a NULL check, but that was
removed so we might as well just read the field.
> 0006-Slightly-refactor-varstr_sortsupport-to-improve-read.patch
>
> Small refactor to make a hard to read function a bit easier to read.
Looks good to me.
Regards,
Jeff Davis
global locale), and also query the "" categories and make a copy of
> them in case anyone wants them later, and then never call setlocale()
> again.
+1.
Regards,
Jeff Davis
On Thu, 2024-08-08 at 12:24 -0700, Jeff Davis wrote:
> The collation cache, which maps collation oids to pg_locale_t
> objects,
> has a few longstanding issues:
Here's a patch set v2.
I changed it so that the pg_locale_t itself a resource kind, rather
than having separate locale_t
sample code that implements late locking for a FDW? I'm
not quite clear on how it's supposed to work.
Regards,
Jeff Davis
ound this would disappear, but I think somebody building from
> source
> can figure that out more easily than the subtle issue that pkg-config
> is
> not installed.
That looks good to me. Does anyone have a different opinion? If not,
I'll go ahead and commit (but not backport) this change.
Regards,
Jeff Davis
he report. Fixed and backported to version 17, where the
function was introduced.
Regards,
Jeff Davis
On Thu, 2024-08-15 at 01:57 -0700, Jeff Davis wrote:
> On Sun, 2024-08-04 at 01:09 -0400, Corey Huinker wrote:
> >
> > > I believe 0001 and 0002 are in good shape API-wise, and I can
> > > start
> > > getting those committed. I will try to clean up the code
eaching, but it occurs to me that MemSetAligned() is itself
> concerned about the alignment of data returned from palloc(). Could
> be
> a similar issue here, too.
Perhaps, but if so, what remedy would that suggest?
Regards,
Jeff Davis
e else would
have a better suggestion.
How about if I wait another week, and if we still don't have a better
fix, I will commit this one.
Regards,
Jeff Davis
sponse to a review comment (point #5):
https://www.postgresql.org/message-id/20200219191636.gvdywx32kwbix6kd@development
Tomas suggested a min of 1024, and I thought I was being more
conservative choosing 256. Still too high, I guess?
I can lower it. What do you think is a reasonable minimum?
Regards,
Jeff Davis
On Thu, 2020-06-04 at 21:41 -0400, Tom Lane wrote:
> I think what'd make more sense is to file this as a gcc bug ("why
> doesn't
> it remove the useless object size check?")
Filed:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95556
Regards,
Jeff Davis
libc versions did you encounter this?
gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
gcc-9 (Ubuntu 9.2.1-19ubuntu1~18.04.york0) 9.2.1 20191109
libc-dev-bin/bionic,now 2.27-3ubuntu1 amd64 [installed]
Regards,
Jeff Davis
On Thu, 2020-06-04 at 16:35 -0400, Alvaro Herrera wrote:
> If it is something worth worrying about, let's discuss what's a good
> fix for it.
While making a minimal test case for the GCC bug report, I found
another surprisingly-small workaround. Patch attached.
Regards,
haven't tested end-to-end that it solves the problem, but I'm pretty
sure it will.
Regards,
Jeff Davis
gt; 3) entry = LookupTupleHashEntry(&hash); if (!entry)
> hashagg_spill_tuple();
> 4) InsertTupleHashEntry(&hash, &isnew); if (isnew) initialize(entry)
I'll work up a patch to refactor this. I'd still like to see if we can
preserve the calculate-hash-once behavior somehow.
Regards,
Jeff Davis
leHashEntry(&hash, &isnew); if (isnew) initialize(entry)
I see, you are suggesting that I change around the execGrouping.c
signatures to return the hash, which will avoid the extra call. That
makes more sense.
Regards,
Jeff Davis
ystem set synchronous_standby_names='';
select pg_reload_conf();
it will release the backends waiting on sync rep.
Regards,
Jeff Davis
Perhaps I should just get rid of that GUC and use the stats trick?
Regards,
Jeff Davis
word
"reject" is too definite for the planner, which is working with
imperfect information.
In master, there is no explicit way to get 2(b), but you can just set
work_mem higher in a lot of cases. If enough people want 2(b), I can
add it easily. Perhaps hashagg_overflow=on|off, which would control
execution time behavior?
Regards,
Jeff Davis
On Tue, 2020-04-07 at 11:20 -0700, Jeff Davis wrote:
> Now that we have Disk-based Hash Aggregation, there are a lot more
> situations where the planner can choose HashAgg. The
> enable_hashagg_disk GUC, if set to true, chooses HashAgg based on
> costing. If false, it only generates a
which to read the hash value).
Regards,
Jeff Davis
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index 8be36ca7634..27dbf264766 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -24,9 +24,10 @@
static in
On Thu, 2020-06-11 at 10:45 -0700, Andres Freund wrote:
> Did you run any performance tests?
Yes, I reproduced your ~12% regression from V12, and this patch nearly
eliminated it for me.
Regards,
Jeff Davis
On Wed, 2020-06-10 at 11:39 -0700, Jeff Davis wrote:
> 1. Testing the spilling of hashed grouping sets: I'm inclined to just
> get rid of enable_groupingsets_hash_disk and use Melanie's stats-
> hacking approach instead.
Fixed in 92c58fd9.
> think the names you suggested q
e similar to
Melanie's patch from a while ago:
https://www.postgresql.org/message-id/caakru_aefesv+ukqwqu+ioenoil2lju9diuh9br8mbyxuz0...@mail.gmail.com
Which makes sense, but it's also more code.
Regards,
Jeff Davis
nt before we revert 4cad2534da. I added Robert because
he also seemed to think it was a reasonable idea.
Regards,
Jeff Davis
; estimated groups is large, and not otherwise.
That seems like a nice compromise that would be non-invasive, at least
for create_agg_plan().
Regards,
Jeff Davis
in all around. It could
use some review/cleanup though.
Regards,
Jeff Davis
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 331acee2814..5b5aa96c517 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -358,6 +358,14 @@ ty
nother. After I investigated, I think everything is
fine -- it seems that one worker was getting more tuples from the
underlying parallel scan. So everything looks good to me.
Regards,
Jeff Davis
es
that disk-based hashagg is half-baked.
Regards,
Jeff Davis
ing wrong?
Regards,
Jeff Davis
t all? That would be a tough
sell for such a common plan type.
Regards,
Jeff Davis
of them. We've penalized plans at risk of
spilling to disk, but what's the point? The planner needs to choose one
of them, and both are at risk of spilling to disk.
Regards,
Jeff Davis
o you are saying
this query would get to use all 64MB of memory, right?
But then you run ANALYZE. Now the query is (correctly) expected to use
64MB of memory. Are you saying this query, executed again with better
stats, would only get to use 32MB of memory, and therefore run slower?
Regards,
Jeff Davis
is an example where an underestimate can give an
advantage, but I don't think we want to extend the concept into other
areas.
Regards,
Jeff Davis
in a
crude form. Just have a shared counter in memory representing GB. If a
node is about to spill, it could try to decrement the counter by N, and
if it succeeds, it gets to exceed work_mem by N more GB.
Regards,
Jeff Davis
ely to
expect the other to spill. If one spills in the executor, then the
other is likely to spill, too. (I'm ignoring the case with a lot of
tuples and few groups because that doesn't seem relevant.)
Imagine that there was only one path available to choose. Would you
suggest the same thing, that unexpected spills can exceed work_mem but
expected spills can't?
Regards,
Jeff Davis
we don't have to
support them forever, and we are more likely to hear problem reports.
Regards,
Jeff Davis
more and optimize more, we can reduce or eliminate the
penalty in a future release. I'm not sure exactly what the penalty
would be, though.
Regards,
Jeff Davis
That resulted in getting the prealloc and projection patches in.
Regards,
Jeff Davis
On Tue, Jun 30, 2020, 7:04 PM David Rowley wrote:
> Does anyone have any objections to that being changed?
>
That's OK with me. By the way, I'm on vacation and will catch up on these
HashAgg threads next week.
Regards,
Jeff Davis
rger overhall of
work_mem/hash_mem (which might happen fairly soon, given the interest
in this thread), or if we change something about HashAgg that makes the
GUCs harder to maintain.
Regards,
Jeff Davis
t; or for some other reasons?
Perhaps "can't" was too strong of a word, but I think it would be
unprecedented to introduce a GUC in a minor version. It could be a
source of confusion.
If others think that adding a GUC in a minor version would be
acceptable, please let me know.
Regards,
Jeff Davis
to cooperate in processing
> their inputs.
Cool! It would certainly be nice to share the partitioning work between
a HashAgg and a HJ.
Regards,
Jeff Davis
Agg to ignore the memory limit
just like v12).
Regards,
Jeff Davis
e of adding a GUC that I missed
in this thread, please let me know. Otherwise, I intend to commit a new
GUC shortly that will enable users to bypass work_mem for HashAgg, just
as in v12.
Regards,
Jeff Davis
hold off on my "escape-hatch" GUC.
Regards,
Jeff Davis
plan too [25])
I am not on board with these options.
Regards,
Jeff Davis
mplicity, but I'm fine with this approach
as well.
The patch itself looks reasonable to me. I don't see a lot of obvious
dangers, but perhaps someone would like to take a closer look at the
planner changes as you suggest.
Regards,
Jeff Davis
s for controlling parallism,
JIT, etc.
Regards,
Jeff Davis
ay in the cost
model.
That would lessen the number of changed plans, but we could easily
remove the pessimization without controversy later if it turned out to
be unnecessary, or if we further optimize HashAgg IO.
Regards,
Jeff Davis
On Sat, 2020-07-18 at 21:15 -0400, Tom Lane wrote:
> Jeff Davis writes:
> > What is your opinion about pessimizing the HashAgg disk costs (not
> > affecting HashAgg plans expected to stay in memory)? Tomas Vondra
> > presented some evidence that Sort had some better IO
lt; 2% in my tests). I was using an
OFFSET 1 instead of EXPLAIN ANALYZE in my test measurements
because it was less noisy, and I focused on the 1GB test for the reason
you mention.
I also addressed Andres's comments:
* changed the name of the flags from MCXT_STAT to MCXT_COUNT
*
ed slightly verbose for
me.
> > +#define MCXT_STAT_ALL ((1 << 6) - 1)
>
> Hm, why not go for ~0 or such?
Done.
Regards,
Jeff Davis
.
Regards,
Jeff Davis
On Mon, 2020-03-16 at 11:45 -0700, Jeff Davis wrote:
> AllocSet allocates memory for itself in blocks, which double in size
> up
> to maxBlockSize. So, the current block (the last one malloc'd) may
> represent half of the total memory allocated for the context itself.
Narro
VERBOSE"
or maybe invent a new explain option to provide this level of detail,
though.
Regards,
Jeff Davis
work_mem) might now run poorly after spilling to
> disk (because
> of overflowing work_mem).
It's probably worth a mention in the release notes, but I wouldn't word
it too strongly. Typically the performance difference is not a lot if
the workload still fits in system memory.
Regards,
Jeff Davis
stimates
correctly, no hashed grouping sets will spill, and the spilling code
won't be exercised. This GUC makes the planner disregard which grouping
sets' hash tables will fit, making it much easier to exercise the
spilling code. Is there a better way I should be testing this code
path?
Regards,
Jeff Davis
t, should we still fix it?
4. Does the attached fix have any dangers of regressing on other
compilers/platforms?
5. Does anyone have a suggestion for a better fix?
Regards,
Jeff Davis
[1]
https://postgr.es/m/91ca648cfd1f99bf07981487a7d81a1ec926caad.ca...@j-davis.com
[2]
https://fedor
https://news.ycombinator.com/item?id=12050579
That raises another consideration: perhaps this is not uniformly a
regression, but actually faster in some situations? If so, what
situations?
Regards,
Jeff Davis
or counters) rather than appending tuples.
Is it possible to achieve this in other ways? The recursive CTE
implementation is a very direct implementation of the standard, perhaps
there are smarter approaches?
Regards,
Jeff Davis
thms inline in SQL?
Regards,
Jeff Davis
[1] http://www.vldb.org/conf/1999/P47.pdf
change slightly.
Regards,
Jeff Davis
th a lower max partitions and rerun
the query? How much would we have to lower it for either the cost to
approach reality or the OS readahead to become effective?
Regards,
Jeff Davis
need to change the API. It seems fine for sort to do
the same thing, even though there's not any benefit.
Regards,
Jeff Davis
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index bc8d56807e2..f44594191e1 100644
--- a/src/backend/utils/sort/logta
why it's doing that, can you verify that
it is a reasonable thing to do?
Aside from that, feel free to commit it.
Regards,
Jeff Davis
prealloc change, as well.
I'm tweaking the patch to be a bit more flexible. I'm thinking we
should start the preallocation list size ~8 and then double it up to
~128 (depending on your results). That would reduce the waste in case
we have a large number of small partitions.
Regards,
Jeff Davis
ite know what you mean. Can you elaborate?
Regards,
Jeff Davis
ignificant amount of wasted IO
effort if we aren't careful.
Regards,
Jeff Davis
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index bc8d56807e2..84f9d6d2358 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -110,6
e whole input in each
worker isn't much worse than processing all of the groups in each
worker.
Regards,
Jeff Davis
dling reads vs. writes in the kernel? Or do you think that 128
blocks is not enough to amortize the cost of a seek for that device?
Regards,
Jeff Davis
nt prealloc patch for v13,
or is it about ready for commit?
Regards,
Jeff Davis
t it. Perhaps we should get an
acknowledgement from one of them that your one-line change is the right
approach?
Regards,
Jeff Davis
On Tue, 2020-05-26 at 17:40 -0700, Jeff Davis wrote:
> On Tue, 2020-05-26 at 21:15 +0200, Tomas Vondra wrote:
> > Yeah. I agree prefetching is definitely out of v13 scope. It might
> > be
> > interesting to try how useful would it be, if you're willing to
> > sp
y happen in create_groupingsets_path?
Regards,
Jeff Davis
On Fri, 2020-05-29 at 15:04 +0200, Tomas Vondra wrote:
> Ah, right. Yeah, we only need to check for AGG_HASH here. Moreover,
> AGG_MIXED probably does not need the tlist tweak, because the input
> should be pre-sorted as with AGG_SORTED.
>
> And we should probably do similar check in the
> create_
herefore likely to require repeated re-sorting to get
back to descending order. Wouldn't a minheap or something make more
sense?
Regards,
Jeff Davis
1101 - 1200 of 1506 matches
Mail list logo
