On Wed, 2024-05-22 at 18:39 -0400, Bruce Momjian wrote:
> On Mon, May 20, 2024 at 11:48:09AM -0700, Jeff Davis wrote:
> > On Sat, 2024-05-18 at 17:51 -0400, Bruce Momjian wrote:
> > > Okay, I went with the attached applied patch. Adjustments?
> >
> > I thin
s that early locking is happening. That assumption is true for
postgres_fdw, but might not be for other FDWs. What if an FDW doesn't
do early locking and also doesn't define RefetchForeignRow?
Regards,
Jeff Davis
don't believe that empty ranges represent a design flaw. If they
don't make sense for temporal constraints, then temporal constraints
should forbid them.
Regards,
Jeff Davis
ion for comparison with libc.
Regards,
Jeff Davis
er similar to libc's C
locale (Jeff Davis)
It uses a "C" locale which is identical but independent of
libc, but it allows the use of non-"C" collations like "en_US"
and "C.UTF-8" with the "C" locale, which libc does not. MORE?
I suggest
is in the first place, but that's also a big ask and we'd
probably still not have it.
Regarding this particular change: the checkpointing hook seems more
like a table AM feature, so I agree with you that we should have a good
idea how a real table AM might use this, rather than only
pg_stat_statements.
Regards,
Jeff Davis
and
didn't get an answer:
https://www.postgresql.org/message-id/CA%2BTgmoY%2BdagCyrMKau7UQeQU6w4LuVEu%2ByjsmJBoXKAo6XbUUA%40mail.gmail.com
And if that does affect performance, what about following the same
protocol as XLogSaveBufferForHint()?
Regards,
Jeff Davis
d you consider connecting once to each database and running
many queries? Most of those seem like just checks.
Regards,
Jeff Davis
y domains zero doesn't make any sense, either.
Consider receiving an email saying "thank you for purchasing 0
widgets!". Check constraints seem like a reasonable way to prevent
those kinds of problems.
Regards,
Jeff Davis
e9d750d.camel%40j-davis.com
https://www.postgresql.org/message-id/CA%2BTgmoY%2BdagCyrMKau7UQeQU6w4LuVEu%2ByjsmJBoXKAo6XbUUA%40mail.gmail.com
In other words, we added REGBUF_NO_CHANGE for the call sites (only hash
indexes) that don't follow the rules and where it wasn't easy to make
them follow the rules. Now that we know of a concrete problem with the
design, there's more upside to fixing it properly.
Regards,
Jeff Davis
On Wed, 2024-05-15 at 16:31 -0700, Jeff Davis wrote:
> Even better would be if we could take into account partitioning. That
> might be out of scope for your current work, but it would be very
> useful. We could have a couple new GUCs like modify_tab
I don't understand the gravity of the choice here: what am I missing?
To be clear: I'm not arguing against it, but I'd like to understand it
better. Perhaps it has to do with the relationship between the sections
and the dependencies?
Regards,
Jeff Davis
parameter values.
Can you explain what you did with the
SECTION_NONE/SECTION_DATA/SECTION_POST_DATA over v19-v21 and why?
Regards,
Jeff Davis
* Why are table_modify_state and insert_modify_buffer_flush_context
globals? What if there are multiple modify nodes in a plan?
* Can you explain the design in logical rep?
Regards,
Jeff Davis
006.
That's OK with me. Let's leave those functions out for now.
>
> If the design, code and benefits that these new Table AMs bring to
> the table look good, I hope to see it for PG 18.
Sounds good.
Regards,
Jeff Davis
Regards,
Jeff Davis
idering stats to be data is so important in pg_dump.
Practically, I want to dump stats XOR data. That's because, if I dump
the data, it's so costly to reload and rebuild indexes that it's not
very important to avoid a re-ANALYZE.
Regards,
Jeff Davis
variables that we aren't considering?
And if you think this is right after all, where could we add some
clarification?
Regards,
Jeff Davis
t a bit more coverage than we have.
OK, I'll submit a test module or something.
Regards,
Jeff Davis
d arrays. I
agree: code coverage is a good goal by itself, and having a few more
platforms exercising that C code can't hurt. I think we should just
address that concern directly by spot-checking the results for a few
code points rather than trying to make the exhaustive ICU tests run on
more hosts.
Regards,
Jeff Davis
so?
Regards,
Jeff Davis
and in 17.
Regards,
Jeff Davis
t all, and I like that
idea. Perhaps the suggestions above are a step in that direction, or
perhaps we can skip ahead?
I intend to submit a patch for the July CF. Thoughts?
Regards,
Jeff Davis
On Wed, 2024-04-10 at 15:19 +0300, Alexander Korotkov wrote:
> 1) 9bd99f4c26 comprises the reworked patch after working with notes
> from Jeff Davis. I agree it would be better to wait for him to
> express explicit agreement. Before reverting this, I would prefer to
> hear
t also has the advantage that we don't change the API for binaryheap
in 17.
Regards,
Jeff Davis
k it's already
been implemented here in approach 2:
https://www.postgresql.org/message-id/CAD21AoAtf12e9Z9NLBuaO1GjHMMo16_8R-yBu9Q9jrk2QLqMEA%40mail.gmail.com
but it does feel wrong to introduce an unnecessary hash table in 17
when we know it's not the right solution.
Regards,
Jeff Davis
table is being used or trying to extend
the binaryheap API.
Of course, we should measure to be sure there aren't bad cases around
updating/removing a key, but I don't see a fundamental reason that it
should be worse.
Regards,
Jeff Davis
obsoleted uses of simplehash.
That's a good idea that preserves some utility for the warnings.
Should I go ahead and commit something like that now, or hold it until
the other thread concludes, or hold it until the July CF?
Regards,
Jeff Davis
perhaps in a more complex scenario, but it would
seem strange to me.
Regards,
Jeff Davis
e considered for v18.
Regards,
Jeff Davis
From 198235448a009a46812eac4854d760af76c85139 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Tue, 9 Apr 2024 10:42:05 -0700
Subject: [PATCH v1 1/2] simplehash.h: declare with pg_attribute_unused().
If SH_SCOPE is static, avoid warnings for unused f
to inline.
Also probably needs comment updates, etc.
Regards,
Jeff Davis
From 08dcf21646e4ded22b10fd0ed536d3bbf6fc1328 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Tue, 9 Apr 2024 10:45:00 -0700
Subject: [PATCH v1] binaryheap: move hash table out of header into
binaryheap.c.
Commit
but also inline the
comparator (which probably matters more).
Regards,
Jeff Davis
ot credits and discussion link in
> that
> commit message. I would have attributed authorship to Bharath
> though,
> because I had not realized that this patch had actually been written
> by
> you initially[1]. My apologies.
No worries. Thank you for reviewing and committing it.
Regards,
Jeff Davis
ogRecordToWAL() (and may
or may not have been evicted to the OS file yet).
Other than that, looks good, thank you.
Regards,
Jeff Davis
On Sat, 2024-04-06 at 18:13 +0200, Alvaro Herrera wrote:
> my understanding of pg_atomic_compare_exchange_u64 is that
> *expected is set to the value that's stored in *ptr prior to the
> exchange.
Sorry, my mistake. Your version looks good.
Regards,
Jeff Davis
e, as well as the machine-readability concern that Magnus
raised.
Additionally, a lot of people are simply very busy around this time,
and may not have had a chance to opine on all the recent changes yet.
Regards,
Jeff Davis
In hashfn_unstable.h, fasthash32() is declared as:
/* like fasthash64, but returns a 32-bit hashcode */
static inline uint64
fasthash32(const char *k, size_t len, uint64 seed)
Is the return type of uint64 a typo?
Regards,
Jeff Davis
x, and
use that index (rather than the pointer) for updating the heap key
(transaction size) or removing elements from the heap.
2. Provide callback for hashing, so that binaryheap.c can hash the xid
value rather than the pointer.
I don't have a strong opinion about which one to use. I prefer
something closer to #1 for v18, but for v17 I suggest whichever one
comes out simpler.
Regards,
Jeff Davis
e that the Max() call in
pg_atomic_monotonic_advance_u64() is just for clarity? Surely the
currval cannot be less than _target when it returns. I'd probably just
do Assert(currval >= _target) and then return currval.
Regards,
Jeff Davis
and the collation to the builtin C.UTF-8.
In passing, I noticed some unrelated regression test failures when I
set LANG=tr_TR: tsearch, tsdict, json, and jsonb. There's an additional
failure in the updatable_views test when LANG=tr_TR.utf8. I haven't
looked into the details yet.
Regards,
Jeff Da
lar to what I
> posted in [2]'s 0002.
I agree that it should be a separate patch. I haven't thought about the
consequences of making them fully independent -- I think that means we
give up the invariant that Copy >= Write >= Flush?
Regarding the patches themselves, 0001 looks good t
On Thu, 2024-04-04 at 10:55 -0700, Jeff Davis wrote:
> * Make a proper indexed binary heap API that accepts a hash
> function
> and provides both heap methods and HT methods that operate based on
> values (transaction size and transaction ID, respectively).
> * Get rid of Reorde
ze and transaction ID, respectively).
* Get rid of ReorderBuffer->by_txn and use the indexed binary heap
instead.
This will be a net simplification in reorderbuffer.c, which is good,
because that file makes use of a *lot* of data strucutres.
Regards
Jeff Davis
nd
fields in the structure.
Perhaps it's not worth the effort though, if performance is already
good enough?
Regards,
Jeff Davis
es exactly this pattern?
In that example, UPPER is used in the target list -- the WHERE clause
might be indexable. The UPPER is just used for display purposes, and
may depend on some locale settings stored in another table associated
with a particular user.
Regards,
Jeff Davis
ems slightly wasteful.
Other than that, it looks good to me.
> Finally, 0003 adds the Copy pointer to XLogCtl alongside Write and
> Flush, and updates WALReadFromBuffers to test that instead of the
> Write
> pointer, and adds in XLogWrite the invariant checks that involve the
> Copy pointer.
Looks good to me.
Regards,
Jeff Davis
On Wed, 2024-04-03 at 01:45 -0700, Jeff Davis wrote:
> I suggest that you add a "heap_index" field to ReorderBufferTXN that
> would point to the index into the heap's array (the same as
> bh_nodeidx_entry.index in your patch). Each time an element moves
> within the h
at's not easy to do with the current binaryheap API. But a binary
heap is not a terribly complex structure, so you can just do an inline
implementation of it where sift_{up|down} know to update the heap_index
field of the ReorderBufferTXN.
Regards,
Jeff Davis
need to derive it reliably
from the table schema on the destination side, right?
Regards,
Jeff Davis
lement type (which may be scalar or not).
Regards,
Jeff Davis
ether in
v18?
Also, a sample AM code would be a huge benefit here. Writing a real AM
is hard, but perhaps we can at least have an example one to demonstrate
how to use these APIs?
Regards,
Jeff Davis
e idea is to store StdRdOptions like normal, but if an unrecognized
option is found, ask the table AM if it understands the option. In that
case I think we'd just use a different field in pg_class so that it can
use whatever format it wants to represent its options.
Regards,
Jeff Davis
during pg_upgrade before
the data is there will store the wrong stats.
It could use a brief comment, though.
Regards,
Jeff Davis
rds,
Jeff Davis
here the variadic part consists of alternating
> parameter labels and values:
I didn't consider this and I think it has a lot of advantages. It's
slightly unfortunate that we can't make them explicitly name/value
pairs, but pg_dump can use whitespace or even SQL comments to make it
more readable.
Regards,
Jeff Davis
ats
It's possible that not all of them make it, but let's not dismiss the
entire feature yet.
Regards,
Jeff Davis
could be resolved --
I don't think it's an inherent problem.
Regards,
Jeff Davis
n systems.
(That could be a separate feature, though, and doesn't need to be a
part of this patch set.)
Regards,
Jeff Davis
e a significant cost to just not doing that? Or are you
suggesting that we special-case the behavior, or turn it off during
restore with a GUC?
Regards,
Jeff Davis
tion of whether stats are part of a schema-only
dump or not. Did we settle conclusively that they are?
Regards,
Jeff Davis
domness than block sampling. At this point that's
just an idea, I haven't looked into it seriously.
Regards,
Jeff Davis
to run analyze anyway.
What do you think about starting off with it as non-default, and then
switching it to default in 18?
Regards,
Jeff Davis
m. It's very late in
the cycle so I'm not sure we'll get more feedback in time, though.
Regards,
Jeff Davis
the before/after dumps to
see if you can reproduce a smaller version of the problem.
Regards,
Jeff Davis
and perhaps
some things should be moved around. I only tested very basic
dump/reload in SQL format.
Regards,
Jeff Davis
From 7ca575e5a02bf380af92b6144622468a501f7636 Mon Sep 17 00:00:00 2001
From: Corey Huinker
Date: Sat, 16 Mar 2024 17:21:10 -0400
Subject: [PATCH vjeff] Enable dumping of t
mp understanding the old
statistics data, and dumping it out in a form that the new server will
understand.
> I agree with the idea that an explicit representation
> of the source table attribute's type would be wise, too.
That's not in the current patch set, either.
Regards,
Jeff Davis
c for convenience functions
> and converts all the duplicate frontend uses of hash_string_pointer.
Why not make hash_string() inline, too? I'm fine with it either way,
I'm just curious why you went to the trouble to create a new .c file so
it didn't have to be inlined.
Regards,
Jeff Davis
t;POSIX", and UCS_BASIC; so it's not clear
why someone would create even more of them. And even if they did, there
would be no reason to give them a warning because we haven't
incremented the version, so there's no chance of a mismatch.
I'm inclined toward (b). Thoughts?
Regards,
Jeff Davis
ing a change to PG_C_UTF8 if it makes sense, as long as
we can point to something other than "glibc version 2.35 does it this
way".
Regards,
Jeff Davis
those other areas (if they
exist) aside from just having more eyes on the code.
>
> So on reflection, rather than trying to rush to get this into v17, I
> think it would be better to leave it to v18.
Thank you for letting me know. That allows some time for others to have
a look.
Regards,
Jeff Davis
pport index insertions for COPY/IIS, so as-
is the API feels incomplete. Thoughts?
Regards,
Jeff Davis
s the primary sources of
complexity came from rules, partitioning, and updatable views, is that
right?
Regards,
Jeff Davis
you had in mind in the
email here:
https://www.postgresql.org/message-id/20230603223824.o7iyochli2dww...@alap3.anarazel.de
?
Regards,
Jeff Davis
the target list, because different
customers might be from different locales and therefore want different
treatment of the dotted-vs-dotless-i.
Thoughts? Should we use the collation by default but then allow
parameters to override? Or should we just consider this a new set of
functions?
(All of this is v18 material, of course.)
Regards,
Jeff Davis
roblem in my setup, so I added a unit test
in case_test.c where it's easier to see the valgrind problem.
Regards,
Jeff Davis
t not" or something else), and it still
exists in master. Do we care about the calculation being wrong if
there's an unfinished write? If not, I'll just clarify that the
calculation doesn't take into account still-buffered data. If we do
care, then something might need to be fixed.
Regards,
Jeff Davis
icated things,
and that you don't mind getting constraint violations at the wrong
time. So I recommend that you punt on this problem.
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/20230603223824.o7iyochli2dwwi7k%40alap3.anarazel.de
[2]
https://www.postgresql.org/message-id/
ate from initcap.
If we create a new function, that also gives us the opportunity to
accept optional arguments to control the behavior rather than relying
on collation for every decision.
Regards,
Jeff Davis
oad stats on your own
tables, and then not worry about it.
> Not off hand, no.
To me it seems like inconsistent data to have most_common_freqs in
anything but descending order, and we should prevent it.
> >
Regards,
Jeff Davis
eplica? If the answer is
yes, should they be differentiated somehow so that you can know where
the slow queries are running?
Regards,
Jeff Davis
ow it works for at least one thing, and we can always improve it
later. For instance, we might call the hook several times and pass it a
"phase" argument.
Regards,
Jeff Davis
re it's not?
>
> Maybe we could have the functions restricted to a role or roles:
>
> 1. pg_write_all_stats (can modify stats on ANY table)
> 2. pg_write_own_stats (can modify stats on tables owned by user)
If we go that route, we are giving up on the ability for users to
restore stats on their own tables. Let's just be careful about
validating data to mitigate this risk.
Regards,
Jeff Davis
ome extra validation for consistency, like
checking that the arrays are properly sorted, check for negative
numbers in the wrong place, or fractions larger than 1.0, etc.
Regards,
Jeff Davis
ng[1] in favor of 'no_sanitize("address")'. It doesn't appear to be
deprecated in gcc[2].
Aside from that, +1.
Regards,
Jeff Davis
[1]
https://clang.llvm.org/docs/AddressSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-address
[2] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
also checking?
I haven't looked in a lot of detail.
Regards,
Jeff Davis
o choose a default.
Regards,
Jeff Davis
On Sun, 2024-03-17 at 17:46 -0400, Tom Lane wrote:
> Jeff Davis writes:
> > New series attached.
>
> Coverity thinks there's something wrong with builtin_validate_locale,
> and as far as I can tell it's right: the last ereport is unreachable,
> because required_encoding is n
sentially a complete feature itself.
I think it would also make the changes in pg_dump simpler, and the
tests in 0001 a lot simpler.
Regards,
Jeff Davis
On Fri, 2024-03-15 at 15:30 -0700, Jeff Davis wrote:
> Still looking, but one quick comment is that the third argument of
> dumpRelationStats() should be const, which eliminates a warning.
A few other comments:
* pg_set_relation_stats() needs to do an ACL check so you can't set the
t(), which returns NULL, and
then the caller segfaults.
> I'm looking at those, but in the mean time I'm seeking feedback on
> the progress so far.
Still looking, but one quick comment is that the third argument of
dumpRelationStats() should be const, which eliminates a warning.
Regards,
Jeff Davis
ike this feature from a user perspective. So +1 from me.
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/7db39b45-821f-4894-ada9-c19570b11...@postgresfriends.org
lternatives discussed? Do we have
consensus that this is the right thing to do?
And if we use this approach, is there extra validation or testing that
can be done?
Regards,
Jeff Davis
On Thu, 2024-03-14 at 15:38 +0100, Peter Eisentraut wrote:
> On 14.03.24 09:08, Jeff Davis wrote:
> > 0001 (the C.UTF-8 locale) is also close...
>
> If have tested this against the libc locale C.utf8 that was available
> on
> the OS, and the behavior is consistent.
That wa
On Thu, 2024-02-29 at 17:02 -0800, Jeff Davis wrote:
> Attached is an implementation of a per-database option STRICT_UNICODE
> which enforces the use of assigned code points only.
The CF app doesn't seem to point at the latest patch:
https://www.postgresql.org/mess
we hold a session lock on the main relation in this case.
> I've
> done this in the attached v2.
Looks good to me. Thank you for expanding on the comment, as well.
Regards,
Jeff Davis
Grepping for regexes in perl code is an "interesting" exercise.
Regards,
Jeff Davis
s to fail
on older versions of perl (like 5.16.3) and succeed on newer versions
(5.38.2). However, it may also depend on when the qr// is [re]compiled,
or regex flags, or locale, or may just be undefined.
Regards,
Jeff Davis
From be5aa677e37180a8c1b0faebcceab5506b1c8130 Mon Sep 17 00:00:0
ing that; just an idea.
Regards,
Jeff Davis
On Fri, 2024-03-08 at 12:39 -0800, Jeff Davis wrote:
> Making a better secondary structure seems doable to me. Just to
> brainstorm:
We can also keep the group_lsns, and then just update both the CLOG
page LSN and the group_lsn when setting the transaction status. The
former would be used f
ut that doesn't seem like a
big problem.
Regards,
Jeff Davis
1 - 100 of 1145 matches
Mail list logo
