Re: brininsert optimization opportunity
Thanks a lot for reviewing and pushing! :) Regards, Soumyadeep (VMware)
Re: brininsert optimization opportunity
On Sat, Nov 25, 2023 at 12:06 PM Tomas Vondra wrote: > I've done a bit more cleanup on the last version of the patch (renamed > the fields to start with bis_ as agreed, rephrased the comments / docs / > commit message a bit) and pushed. Thanks a lot Tomas for helping to drive the patch to completion iteratively and realizing the benefits. - Ashwin
Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements
On Sat, Nov 25, 2023 at 10:45 PM Andrei Zubkov wrote: > On Sat, 2023-11-25 at 02:45 +0200, Alexander Korotkov wrote: > > > I've reviewed this patch. > > Thank you very much for your review. > > > I think the design was well-discussed in this thread. Implementation > > also looks good to me. I've just slightly revised the commit > > messages. > > I've noted a strange space in a commit message of 0001 patch: > "I t is needed for upcoming patch..." > It looks like a typo. Thank you for catching it. I'll fix this before commit. -- Regards, Alexander Korotkov
Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements
Hi Alexander! On Sat, 2023-11-25 at 02:45 +0200, Alexander Korotkov wrote: > I've reviewed this patch. Thank you very much for your review. > I think the design was well-discussed in this thread. Implementation > also looks good to me. I've just slightly revised the commit > messages. I've noted a strange space in a commit message of 0001 patch: "I t is needed for upcoming patch..." It looks like a typo. -- regards, Andrei Zubkov Postgres Professional
strange para/programlisting pattern in sgml docs
Hi, while working on a patch I noticed we do this in the SGML docs (for example in indexam.sgml and a bunch of other files): ... some text ... some code ... description of the code. That is, the program listing is in a paragraph that starts immediately before it. I just noticed this ends up like this in the HTML: ... some text ... some code ... description of the code. That is, there's an empty before , which seems a bit weird, but it seems to render fine (at least in Firefox), so maybe it looks weird but is not a problem in practice ... I did search for what (X)HTML says about this, and the only thing I found is HTML5 flow control section [1], which says ... elements whose content model allows any flow content should have either at least one descendant text node ... Ofc, we're rendering into XHTML, not HTML5. However, W3 advises against this in the HTML4.0 document too [2]: We discourage authors from using empty P elements. User agents should ignore empty P elements. So it might be "OK" because browsers ignore those elements, but maybe we should stop doing that anyway? regards [1] https://www.w3.org/TR/2011/WD-html5-20110525/content-models.html#flow-content-0 [2] https://www.w3.org/TR/1998/REC-html40-19980424/struct/text.html#h-9.3.1 -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: brininsert optimization opportunity
I've done a bit more cleanup on the last version of the patch (renamed the fields to start with bis_ as agreed, rephrased the comments / docs / commit message a bit) and pushed. Thanks for the patch! -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Test 002_pg_upgrade fails with olddump on Windows
Hello,
When trying to use a custom dump with the test pg_upgrade/002_pg_upgrade,
I observe the following test failure on Windows:
>meson test --suite setup
>echo create database regression>...\dump.sql
>set olddump=...\dump.sql& set oldinstall=.../tmp_install/usr/local/pgsql&
meson test pg_upgrade/002_pg_upgrade
1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade ERROR 11.38s
exit status 1
regress_log_002_pg_upgrade.txt contains:
...
[09:07:06.704](3.793s) ok 11 - run of pg_upgrade for new instance
...
[09:07:07.301](0.001s) not ok 15 - old and new dumps match after pg_upgrade
[09:07:07.301](0.000s) # Failed test 'old and new dumps match after
pg_upgrade'
# at .../src/bin/pg_upgrade/t/002_pg_upgrade.pl line 452.
[09:07:07.301](0.000s) # got: '1'
# expected: '0'
=== diff of ...\build\testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8/dump1.sql and
...\build\testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8/dump2.sql
=== stdout ===
=== stderr ===
=== EOF ===
>dir "testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8/"
11/25/2023 09:06 AM 2,729 dump1.sql
11/25/2023 09:07 AM 2,590 dump2.sql
>diff -s "testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8\dump1.sql"
"testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8\dump2.sql"
Files testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8\dump1.sql and
testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8\dump2.sql are identical
As I can see, dump1.sql contains line endings 0d 0a, while dump2.sql — 0a.
The attached patch fixes the issue for me.
Best regards,
Alexanderdiff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index c6d83d3c21..d34b45e346 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -293,6 +293,7 @@ if (defined($ENV{oldinstall}))
}
open my $fh, ">", $dump1_file or die "could not open dump file";
+ binmode $fh;
print $fh $dump_data;
close $fh;
Re: New instability in stats regression test
I wrote:
> I'm a bit mystified by this. This test was introduced in Andres'
> commit 10a082bf7 of 2023-02-11, and it seems to have been stable
> since then. I trawled the buildfarm logs going back three months
> and found no similar failures. So why's it failing now? The
> most plausible theory seems to be that Michael's recent commits
> adding pg_stat_reset_xxx features destabilized the test somehow ...
> but I sure don't see how/why.
After a bit more looking around, I have part of a theory.
Commit 23c8c0c8f of 2023-11-12 added this, a little ways before
the problematic test:
-- Test that reset_shared with no argument resets all the stats types
-- supported (providing NULL as argument has the same effect).
SELECT pg_stat_reset_shared();
The test that is failing is of course
-- Test IO stats reset
SELECT pg_stat_have_stats('io', 0, 0);
SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) +
sum(writes) + sum(writebacks) + sum(hits) AS io_stats_pre_reset
FROM pg_stat_io \gset
SELECT pg_stat_reset_shared('io');
SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads) +
sum(writes) + sum(writebacks) + sum(hits) AS io_stats_post_reset
FROM pg_stat_io \gset
SELECT :io_stats_post_reset < :io_stats_pre_reset;
So the observed failure could be explained if, between the
"pg_stat_reset_shared('io')" call and the subsequent scan of
pg_stat_io, concurrent sessions had done more I/O operations
than happened since that new pg_stat_reset_shared() call.
Previously, the "pre_reset" counts would be large enough to
make that a pretty ridiculous theory, but after 23c8c0c8f maybe
it's not.
To test this idea, I made the test print out the actual values
of the counts, like this:
@@ -1585,10 +1585,10 @@
SELECT sum(evictions) + sum(reuses) + sum(extends) + sum(fsyncs) + sum(reads)
+ sum(writes) + sum(writebacks) + sum(hits) AS io_stats_post_reset
FROM pg_stat_io \gset
-SELECT :io_stats_post_reset < :io_stats_pre_reset;
- ?column?
---
- t
+SELECT :io_stats_post_reset, :io_stats_pre_reset;
+ ?column? | ?column?
+--+--
+10452 | 190087
(1 row)
Of course, this makes it fail every time, but the idea is to get
a sense of the magnitude of the counts; and what I'm seeing is
that the "pre reset" counts are typically 10x more than the
"post reset" ones, even after 23c8c0c8f. If I remove the
suspicious pg_stat_reset_shared() call, there's about 3 orders
of magnitude difference; but still you'd think a 10x safety
margin would be enough. So this theory doesn't seem to quite
work as-is. Perhaps there's some additional contributing factor
I didn't think to control.
Nonetheless, it seems like a really bad idea that this test
of I/O stats reset happens after the newly-added test. It
is clearly now dependent on timing and the amount of concurrent
activity whether it will pass or not. We should probably
re-order the tests to do the old test first; or else abandon
this test methodology and just test I/O reset the same way
we test the other cases (checking only for timestamp advance).
Or maybe we don't really need the pg_stat_reset_shared() test?
regards, tom lane
New instability in stats regression test
In the past few days we've had two buildfarm failures[1][2] in the stats regression test that look like @@ -1582,7 +1582,7 @@ SELECT :io_stats_post_reset < :io_stats_pre_reset; ?column? -- - t + f (1 row) -- test BRIN index doesn't block HOT update I'm a bit mystified by this. This test was introduced in Andres' commit 10a082bf7 of 2023-02-11, and it seems to have been stable since then. I trawled the buildfarm logs going back three months and found no similar failures. So why's it failing now? The most plausible theory seems to be that Michael's recent commits adding pg_stat_reset_xxx features destabilized the test somehow ... but I sure don't see how/why. Failure [1] was on my own animal longfin, so I tried to reproduce it on that animal's host, but no luck so far. Thoughts? regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin&dt=2023-11-21%2001%3A55%3A00 [2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=guaibasaurus&dt=2023-11-25%2016%3A20%3A04
Re: Table AM Interface Enhancements
On Fri, Nov 24, 2023 at 5:18 PM Mark Dilger wrote: > > On Nov 23, 2023, at 4:42 AM, Alexander Korotkov > > wrote: > > > > 0006-Generalize-table-AM-API-for-INSERT-.-ON-CONFLICT-v1.patch > > > > Provides a new table AM API method to encapsulate the whole INSERT ... > > ON CONFLICT ... algorithm rather than just implementation of > > speculative tokens. > > I *think* I understand that you are taking the part of INSERT..ON CONFLICT > that lives outside the table AM and pulling it inside so that table AM > authors are free to come up with whatever implementation is more suited for > them. The most straightforward way of doing so results in an EState > parameter in the interface definition. That seems not so good, as the EState > is a fairly complicated structure, and future changes in the executor might > want to rearrange what EState tracks, which would change which values > tuple_insert_with_arbiter() can depend on. I think this is the correct understanding. > Should the patch at least document which parts of the EState are expected to > be in which states, and which parts should be viewed as undefined? If the > implementors of table AMs rely on any/all aspects of EState, doesn't that > prevent future changes to how that structure is used? New tuple tuple_insert_with_arbiter() table AM API method needs EState argument to call executor functions: ExecCheckIndexConstraints(), ExecUpdateLockMode(), and ExecInsertIndexTuples(). I think we probably need to invent some opaque way to call this executor function without revealing EState to table AM. Do you think this could work? > > 0008-Let-table-AM-insertion-methods-control-index-inse-v1.patch > > > > Allows table AM to skip index insertions in the executor and handle > > those insertions itself. > > The new parameter could use more documentation. > > > 0009-Custom-reloptions-for-table-AM-v1.patch > > > > Enables table AMs to define and override reloptions for tables and indexes. > > This could use some regression tests to exercise the custom reloptions. Thank you for these notes. I'll take this into account for the next patchset version. -- Regards, Alexander Korotkov
Improve rowcount estimate for UNNEST(column)
Hello,
Here is a patch to improve rowcount estimates for
`UNNEST(some_array_column)`. Today we hard code this to 10, but we
have statistics about array size, so it's easy to use them.
I've seen plans where this would make a difference. If the array has
only 1 or 2 elements, then overestimating the rowcount by 10 leads to
unnecessary seqscans downstream. I can see how an underestimate would
cause issues too.
This patch builds on a391ff3c3d41 which allowed set-returning
functions like UNNEST to include a support function to estimate their
result count. (There is a nice writeup at
https://www.cybertec-postgresql.com/en/optimizer-support-functions/)
But that patch only changes UNNEST if it has a Const or ArrayExpr
argument.
The statistic I'm using is the last value in the DECHIST array, which
is the average number of distinct elements in the array. Using the
plain (non-distinct) element count would be more accurate, but we
don't have that, and using distinct elements is still better than a
hardcoded 10.
The real change is in estimate_array_length, which has several callers
besides array_unnest_support, but I think this change should give more
accurate estimates for all of them.
There is a comment that estimate_array_length must agree with
scalararraysel. I don't think this commit introduces any
discrepancies. The most relevant case there is `scalar = ANY/ALL
(array)`, which also consults DECHIST (and/or MCELEM).
I wasn't sure where to put a test. I finally settled on arrays.sql
since (1) that has other UNNEST tests (2) array_unnest_support is in
util/adt/arrayfuncs.c (3) I couldn't find a place devoted to
rowcount/selectivity estimates. I'm happy to move it if someone has a
better idea!
Based on 712dc2338b23.
Yours,
--
Paul ~{:-)
p...@illuminatedcomputing.com
v1-0001-Use-statitics-for-estimating-UNNEST-column-rows.patch
Description: Binary data
Re: pg_stats and range statistics
On Sat, Nov 25, 2023 at 11:14 AM Egor Rogov wrote: > > Hi Alexander, > > On 25.11.2023 02:06, Alexander Korotkov wrote: > > > > In conclusion of all of the above, I decided to revise the patch and > > show the bounds histogram as it's stored in pg_statistic. I revised > > the docs correspondingly. > > > So basically we returned to what it all has started from? I guess it's > better than nothing, although I have to admit that two-array > representation is much more readable. Unfortunately it brings in a > surprising amount of complexity. Yep, it is. > Anyway, thanks for looking into this! And thank you for the feedback! -- Regards, Alexander Korotkov
Re: pg_stats and range statistics
On Sat, Nov 25, 2023 at 10:58 AM jian he wrote: > On Sat, Nov 25, 2023 at 7:06 AM Alexander Korotkov > wrote: > > > > Hi! > > Additionally, I found that the current patch can't handle infinite > > range bounds and discards information about inclusiveness of range > > bounds. The infinite bounds could be represented as NULL (while I'm > > not sure how good this representation is). Regarding inclusiveness, I > > don't see the possibility to represent them in a reasonable way within > > an array of base types. I also don't feel good about discarding the > > accuracy in the pg_stats view. > > > > in range_length_histogram, maybe we can document that when calculating > the length of a range, inclusiveness will be true. I've revised the patchset. Edited comment in pg_statistic.h as you proposed. And I've added to the documentation a short note on how the range length histogram is calculated. -- Regards, Alexander Korotkov 0001-Update-comments-for-pg_statistic-catalog--20231125-2.patch Description: Binary data 0002-Display-length-and-bounds-histograms-in-p-20231125-2.patch Description: Binary data
Re: [HACKERS] Changing references of password encryption to hashing
On Wed, Nov 22, 2023 at 08:23:42PM -0500, Bruce Momjian wrote: > Let me also add that I apologize for brining up these old threads. I > feel badly I didn't address them years ago, I feel bad for the original > posters, and I do think there is some value in addressing some of them, > which I think is validated by the many useful doc patches I have made > over the past few weeks. I am over half done and hope everyone can be > patient with me while I do my best to finish this long-overdue job. I just finished reviewing 6k emails I kept for re-review back to 2014. I only have the open items left to close. I am not sure why stopped regularly reviewing such emails. Hopefully I will be better going forward. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Re: POC, WIP: OR-clause support for indexes
On Sat, Nov 25, 2023 at 1:10 PM Alena Rybakina wrote: > On 25.11.2023 04:13, Alexander Korotkov wrote: > > It seems to me there is a confusion. I didn't mean we need to move > conversion of OR-expressions to ANY into choose_bitmap_and() function > or anything like this. My idea was to avoid degradation of plans, > which I've seen in [1]. Current code for generation of bitmap paths > considers the possibility to split OR-expressions into distinct bitmap > index scans. But it doesn't consider this possibility for > ANY-expressions. So, my idea was to enhance our bitmap scan > generation to consider split values of ANY-expressions into distinct > bitmap index scans. So, in the example [1] and similar queries > conversion of OR-expressions to ANY wouldn't affect the generation of > bitmap paths. > > Thanks for the explanation, yes, I did not understand the idea correctly at > first. I will try to implement something similar. Alena, great, thank you. I'm looking forward to the updated patch. -- Regards, Alexander Korotkov
Re: [HACKERS] Bogus WAL segments archived after promotion
On Thu, Apr 23, 2015 at 02:57:59PM +0900, Michael Paquier wrote: > Finally looking at that... The commit log of b2a5545 is a bit > misleading. Segment files that were recycled during archive recovery > are not necessarily removed, they could be recycled as well during > promotion on the new timeline in line with what RemoveOldXlogFiles() > does. Hence I think that the comment on top of > RemoveNonParentXlogFiles() should be updated to reflect that like in > the patch attached. > > Something minor: perhaps we could refactor xlogarchive.c to have > XLogArchiveCheckDone() and XLogArchiveIsBusy() use the new > XLogArchiveIsReady(). > Regards, Old patch, but still valid, so applied to master, thanks. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
RE: Random pg_upgrade test failure on drongo
Dear Alexander, > > Please look at the simple test program attached. It demonstrates the > failure for me when running in two sessions as follows: > unlink-open test 150 1000 > unlink-open test2 150 1000 Thanks for attaching a program. This helps us to understand the issue. I wanted to confirm your env - this failure was occurred on windows server , right? > > That is, my idea was to try removing a file through renaming it as a fast > path (thus avoiding that troublesome state DELETE PENDING), and if that > fails, to perform removal as before. May be the whole function might be > simplified, but I'm not sure about special cases yet. I felt that your result showed pgrename() would be more rarely delayed than unlink(). That's why a file which has original name would not exist when subsequent open() was called. About special cases, I wanted seniors to check. > > * IIUC, the important points is the latter part, which waits until the > > status is > >changed. Based on that, can we remove a double rmtree() from > cleanup_output_dirs()? > >They seems to be add for the similar motivation. > > I couldn't yet reproduce a failure, which motivated that doubling (IIUC, it > was observed in [1]), with c28911750 reverted, so I need more time to > research that issue to answer this question. Yeah, as the first place, this failure seldom occurred Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: pg_upgrade and logical replication
On Sat, Nov 25, 2023 at 7:21 AM vignesh C wrote: > Few comments on v19: == 1. + + The subscriptions will be migrated to the new cluster in a disabled state. + After migration, do this: + + + + + + Enable the subscriptions by executing + ALTER SUBSCRIPTION ... ENABLE. The reason for this restriction is not very clear to me. Is it because we are using pg_dump for subscription and the existing functionality is doing it? If so, I think currently even connect is false. 2. + * b) SUBREL_STATE_SYNCDONE: A relation upgraded while in this state + * would retain the replication origin in certain cases. I think this is vague. Can we briefly describe cases where the origins would be retained? 3. I think the cases where the publisher is also upgraded restoring the origin's LSN is of no use. Currently, I can't see a problem with restoring stale originLSN in such cases as we won't be able to distinguish during the upgrade but I think we should document it in the comments somewhere in the patch. -- With Regards, Amit Kapila.
Re: POC, WIP: OR-clause support for indexes
On 25.11.2023 04:13, Alexander Korotkov wrote: It seems to me there is a confusion. I didn't mean we need to move conversion of OR-expressions to ANY into choose_bitmap_and() function or anything like this. My idea was to avoid degradation of plans, which I've seen in [1]. Current code for generation of bitmap paths considers the possibility to split OR-expressions into distinct bitmap index scans. But it doesn't consider this possibility for ANY-expressions. So, my idea was to enhance our bitmap scan generation to consider split values of ANY-expressions into distinct bitmap index scans. So, in the example [1] and similar queries conversion of OR-expressions to ANY wouldn't affect the generation of bitmap paths. Thanks for the explanation, yes, I did not understand the idea correctly at first. I will try to implement something similar. -- Regards, Alena Rybakina Postgres Professional
Re: undetected deadlock in ALTER SUBSCRIPTION ... REFRESH PUBLICATION
On Fri, Nov 24, 2023 at 5:05 PM Shlok Kyal wrote: > > > I tried to reproduce the issue and was able to reproduce it with > > scripts shared by Tomas. > > I tried testing it from PG17 to PG 11. This issue is reproducible for > > each version. > > > > Next I would try to test with the patch in the thread shared by Amit. > > I have created the v1 patch to resolve the issue. Have tested the > patch on HEAD to PG12. > The same patch applies to all the versions. The changes are similar to > the one posted in the thread > https://www.postgresql.org/message-id/1412708.1674417574%40sss.pgh.pa.us > (it's quite likely we hold lock on + * pg_replication_origin, which the sync worker will need + * to update). This part of the comment is stale and doesn't hold true. You need to update the reason based on the latest problem discovered in this thread. I think you can compare the timing of regression tests in subscription, with and without the patch to show there is no regression. And probably some tests with a large number of tables for sync with very little data. -- With Regards, Amit Kapila.
Re: pg_stats and range statistics
Hi Alexander, On 25.11.2023 02:06, Alexander Korotkov wrote: In conclusion of all of the above, I decided to revise the patch and show the bounds histogram as it's stored in pg_statistic. I revised the docs correspondingly. So basically we returned to what it all has started from? I guess it's better than nothing, although I have to admit that two-array representation is much more readable. Unfortunately it brings in a surprising amount of complexity. Anyway, thanks for looking into this!
Re: pg_stats and range statistics
On Sat, Nov 25, 2023 at 7:06 AM Alexander Korotkov wrote: > > Hi! > Additionally, I found that the current patch can't handle infinite > range bounds and discards information about inclusiveness of range > bounds. The infinite bounds could be represented as NULL (while I'm > not sure how good this representation is). Regarding inclusiveness, I > don't see the possibility to represent them in a reasonable way within > an array of base types. I also don't feel good about discarding the > accuracy in the pg_stats view. > in range_length_histogram, maybe we can document that when calculating the length of a range, inclusiveness will be true.
Re: pg_stats and range statistics
On Sat, Nov 25, 2023 at 7:06 AM Alexander Korotkov wrote: > > Hi! > > I'm going to push this if there are no objections. > > -- > Regards, > Alexander Korotkov src/include/catalog/pg_statistic.h 268: * range type's subdiff function. Only non-null rows are considered. should it be: * range type's subdiff function. Only non-null, non-empty rows are considered. Other than that, it looks fine to me.
