> I've been thinking since the beginning of this thread that there
> was no coherent, defensible rationale being offered for jumbling
> some utility statements and not others.
+1 to the idea, as there are other utility statement cases
that should be Jumbled. Here is a recent thread for jumbling
cu
Thank you for the feedback!
>While it seems to be a good idea to have the atomic counter for the
>number of indexes completed, I think we should not use the global
>variable referencing the counter as follow:
>+static pg_atomic_uint32 *index_vacuum_completed = NULL;
>:
>+v
Attached is v13-0001--Show-progress-for-index-vacuums.patch which addresses
the latest comments. The main changes are:
1/ Call the parallel_vacuum_progress_report inside the AMs rather than
vacuum_delay_point.
2/ A Boolean when set to True in vacuumparallel.c will be used to determine if
callin
Resubmitting patches with proper format.
Thanks
Sami Imseih
Amazon Web Services (AWS)
v14-0002-Function-to-return-currently-vacuumed-or-cleaned.patch
Description: v14-0002-Function-to-return-currently-vacuumed-or-cleaned.patch
v14-0001-Add-2-new-columns-to-pg_stat_progress_vacuum.-Th.pat
Thank you for the feedback!
>I think we can pass the progress update function to
> WaitForParallelWorkersToFinish(), which seems simpler. And we can call
Directly passing the callback to WaitForParallelWorkersToFinish
will require us to modify the function signature.
To me, it seemed simpl
We see another occurrence of this bug with the last patch applied in 13.7.
After a promotion we observe the following in the logs:
2022-05-25 00:35:38 UTC::@:[371]:PANIC: xlog flush request 10/B1FA3D88 is not
satisfied --- flushed only to 7/A860
2022-05-25 00:35:38
UTC:172.31.26.238(38610):
>Another idea I came up with is that we can wait for all index vacuums
>to finish while checking and updating the progress information, and
>then calls WaitForParallelWorkersToFinish after confirming all index
>status became COMPLETED. That way, we don’t need to change the
>para
>AFAICS you're proposing to add an identifier for a specific plan, but no
> way to
>know what that plan was? How are users supposed to use the information if
> they
>know something changed but don't know what changed exactly?
I see this as a start to do more useful things with plans
>Thus, I still don't see what have happened at Imseih's hand, but I can
>cause PANIC with a bit tricky steps, which I don't think valid. This
>is what I wanted to know the exact steps to cause the PANIC.
>The attached 1 is the PoC of the TAP test (it uses system()..), and
>the
> Would you mind trying the second attached to abtain detailed log on
> your testing environment? With the patch, the modified TAP test yields
> the log lines like below.
Thanks for this. I will apply this to the testing environment and
will share the output.
Regards,
Sami Imseih
Amazon Web Serv
> Any luck with this?
Apologies for the delay, as I have been away.
I will test this next week and report back my findings.
Thanks
Sami Imseih
Amazon Web Services (AWS)
On 6/28/22, 2:10 AM, "Kyotaro Horiguchi" wrote:
CAUTION: This email originated from outside of the organization. Do n
> The server seem to have started as a standby after crashing a
> primary. Is it correct?
Yes, that is correct.
2022-08-05 17:18:51 UTC::@:[359]:LOG: database system was interrupted; last
known up at 2022-08-05 17:08:52 UTC
2022-08-05 17:18:51 UTC::@:[359]:LOG: creating missing WAL directory
>> This sounds like useful information to me.
> Thanks for looking at it!
The VacuumDelay is the only visibility available to
gauge the cost_delay. Having this information
advertised by pg_stat_progress_vacuum as is being proposed
is much better. However, I also think that the
"number of times"
> I'm not convinced that reporting the number of waits is useful. If we
> were going to report a possibly-inaccurate amount of actual waiting,
> then also reporting the number of waits might make it easier to figure
> out when the possibly-inaccurate number was in fact inaccurate. But I
> think it'
>> I'm struggling to think of a scenario in which the number of waits would be
>> useful, assuming you already know the amount of time spent waiting. Even
>> if the number of waits is huge, it doesn't tell you much else AFAICT. I'd
>> be much more likely to adjust the cost settings based on the per
>> 2. the leader being interrupted while waiting is also already happening on
>> master
>> due to the pgstat_progress_parallel_incr_param() calls in
>> parallel_vacuum_process_one_index() (that have been added in
>> 46ebdfe164). It has been the case "only" 36 times during my test case.
46ebdfe164
Attachment protected by Amazon:
[0001-Handle-Sleep-interrupts.patch]
https://us-east-1.secure-attach.amazon.com/fcdc82ce-7887-4aa1-af9e-c1161a6b1d6f/bc81fa24-41de-48f9-a767-a6d15801754b
Amazon has replaced attachment with download link. Downloads will be available
until July 28, 2024, 19:59 (UTC
> 46ebdfe164 will interrupt the leaders sleep every time a parallel workers
> reports
> progress, and we currently don't handle interrupts by restarting the sleep
> with
> the remaining time. nanosleep does provide the ability to restart with the
> remaining
> time [1], but I don't think it's wo
> I think you need to find a way to disable this "Attachment protected
> by Amazon" thing:
Yes, I am looking into that. I only noticed after sending the email.
Sorry about that.
Sami
Hi,
While recently looking into partition maintenance, I found a case in which
DETACH PARTITION FINALIZE could case deadlocks. This occurs when a
ALTER TABLE DETACH CONCURRENTLY, followed by a cancel, the followed by
an ALTER TABLE DETACH FINALIZE, and this sequence of steps occur in the
middle of
Hi,
Sorry for the delay in response and thanks for the feedback!
> I've reviewed and built the documentation for the updated patch. As it stands
> right now I think the documentation for this section is quite clear.
Sorry, I am not understanding. What is clear? The current documentation -or-
t
Hi,
I Recently encountered a situation on the field in which the message
“could not truncate directory "pg_serial": apparent wraparound”
was logged even through there was no danger of wraparound. This
was on a brand new cluster and only took a few minutes to see
the message in the logs.
Reading o
blob/master/src/backend/access/transam/slru.c#L306
Regards,
Sami
From: "Imseih (AWS), Sami"
Date: Tuesday, August 22, 2023 at 7:56 PM
To: "pgsql-hack...@postgresql.org"
Subject: False "pg_serial": apparent wraparound” in logs
Hi,
I Recently encountered a situation
Attached a patch with a new CF entry: https://commitfest.postgresql.org/44/4516/
Regards,
Sami Imseih
Amazon Web Services (AWS)
0001-v1-Fix-false-report-of-wraparound-in-pg_serial.patch
Description: 0001-v1-Fix-false-report-of-wraparound-in-pg_serial.patch
I tested the patch and it does the correct thing.
I have a few comments:
1/ cast the return of bsearch. This was done previously and is the common
convention in the code.
So
+ return bsearch(&key, localBackendStatusTable, localNumBackends,
+ sizeof(LocalPg
> Here is a new version of the patch that avoids changing the names of the
> existing functions. I'm not thrilled about the name
> (pgstat_fetch_stat_local_beentry_by_backend_id), so I am open to
> suggestions. In any case, I'd like to rename all three of the>
> pgstat_fetch_stat_* functions in v17
> I frequently hear about scenarios where users with thousands upon thousands
> of tables realize that autovacuum is struggling to keep up. When they
> inevitably go to bump up autovacuum_max_workers, they discover that it
> requires a server restart (i.e., downtime) to take effect, causing furthe
> My concern with this approach is that other background workers could use up
> all the slots and prevent autovacuum workers from starting
That's a good point, the current settings do not guarantee that you
get a worker for the purpose if none are available,
i.e. max_parallel_workers_per_gather,
I spent sometime reviewing/testing the POC. It is relatively simple with a lot
of obvious value.
I tested with 16 tables that constantly reach the autovac threashold and the
patch did the right thing. I observed concurrent autovacuum workers matching
the setting as I was adjusting it dynamically.
>> 1/ We should emit a log when autovacuum_workers is set higher than the max.
>> Hm. Maybe the autovacuum launcher could do that.
Would it be better to use a GUC check_hook that compares the
new value with the max allowed values and emits a WARNING ?
autovacuum_max_workers already has a check
> IIRC using GUC hooks to handle dependencies like this is generally frowned
> upon because it tends to not work very well [0]. We could probably get it
> to work for this particular case, but IMHO we should still try to avoid
> this approach.
Thanks for pointing this out. I agree, this could lea
> Another option could be to just remove the restart-only GUC and hard-code
> the upper limit of autovacuum_max_workers to 64 or 128 or something. While
> that would simplify matters, I suspect it would be hard to choose an
> appropriate limit that won't quickly become outdated.
Hardcoded values a
> Here is a first attempt at a proper patch set based on the discussion thus
> far. I've split it up into several small patches for ease of review, which
> is probably a bit excessive. If this ever makes it to commit, they could
> likely be combined.
I looked at the patch set. With the help of DEB
> Part of the underlying problem here is that, AFAIK, neither PostgreSQL
> as a piece of software nor we as human beings who operate PostgreSQL
> databases have much understanding of how autovacuum_max_workers should
> be set. It's relatively easy to hose yourself by raising
> autovacuum_max_worker
> FWIW, I'd like to think that we could improve the situation, requiring
> a mix of calling pgstat_report_query_id() while feeding on some query
> IDs retrieved from CachedPlanSource->query_list. I have not in
> details looked at how much could be achieved, TBH.
I was dealing with this today and f
> I am also a bit surprised with the choice of using the first Query
> available in the list for the ID, FWIW.
IIUC, the query trees returned from QueryRewrite
will all have the same queryId, so it appears valid to
use the queryId from the first tree in the list. Right?
Here is an example I was
> But simplistic case with a prepared statement shows how the value of
> queryId can be changed if you don't acquire all the objects needed for
> the execution:
> CREATE TABLE test();
> PREPARE name AS SELECT * FROM test;
> EXPLAIN (ANALYSE, VERBOSE, COSTS OFF) EXECUTE name;
> DROP TABLE test;
>
> We choose a arguably more user-friendly option:
> https://www.postgresql.org/docs/current/sql-prepare.html
Thanks for pointing this out!
Regards,
Sami
>> But simplistic case with a prepared statement shows how the value of
>> queryId can be changed if you don't acquire all the objects needed for
>> the execution:
>> CREATE TABLE test();
>> PREPARE name AS SELECT * FROM test;
>> EXPLAIN (ANALYSE, VERBOSE, COSTS OFF) EXECUTE name;
>> DROP TABLE
Here is a new rev of the patch which deals with the scenario
mentioned by Andrei [1] in which the queryId may change
due to a cached query invalidation.
[1]
https://www.postgresql.org/message-id/724348C9-8023-41BC-895E-80634E79A538%40amazon.com
Regards,
Sami
0001-v2-Fix-Extended-Query-Proto
I've been following this discussion and would like to add my
2 cents.
> Unless I'm missing something major, that's completely bonkers. It
> might be true that it would be a good idea to vacuum such a table more
> often than we do at present, but there's no shot that we want to do it
> that much mo
> And as far as that goes, I'd like you - and others - to spell out more
> precisely why you think 100 or 200 million tuples is too much. It
> might be, or maybe it is in some cases but not in others. To me,
> that's not a terribly large amount of data. Unless your tuples are
> very wide, it's a fe
> That's true, but using a hard-coded limit means we no longer need to add a
> new GUC. Always allocating, say, 256 slots might require a few additional
> kilobytes of shared memory, most of which will go unused, but that seems
> unlikely to be a problem for the systems that will run Postgres v18.
> This is about how I feel, too. In any case, I +1'd a higher default
> because I think we need to be pretty conservative with these changes, at
> least until we have a better prioritization strategy. While folks may opt
> to set this value super low, I think that's more likely to lead to some
> in
> I discovered the current state of queryId reporting and found that it
> may be unlogical: Postgres resets queryId right before query execution
> in simple protocol and doesn't reset it at all in extended protocol and
> other ways to execute queries.
In exec_parse_message, exec_bind_message and
> Okay, that's what I precisely wanted to understand: queryId doesn't have
> semantics to show the job that consumes resources right now—it is mostly
> about convenience to know that the backend processes nothing except
> (probably) this query.
It may be a good idea to expose in pg_stat_activity o
>>> That's true, but using a hard-coded limit means we no longer need to add a
>>> new GUC. Always allocating, say, 256 slots might require a few additional
>>> kilobytes of shared memory, most of which will go unused, but that seems
>>> unlikely to be a problem for the systems that will run Postgr
> I'm unsure if it needs to call ExecutorStart in the bind code. But if we
> don't change the current logic, would it make more sense to move
> pgstat_report_query_id to the ExecutorRun routine?
I initially thought about that, but for utility statements (CTAS, etc.) being
executed with extended q
>>> If the exact values
>>> are important, maybe we could introduce more GUCs like
>>> shared_memory_size_in_huge_pages that can be consulted (instead of
>>> requiring users to break out their calculators).
>>
>> I don't especially like shared_memory_size_in_huge_pages, and I don't
>> want to intro
> postgres -D pgdev-dev -c shared_buffers=16MB -C
> shared_memory_size_in_huge_pages
> 13
> postgres -D pgdev-dev -c shared_buffers=16MB -c huge_page_size=1GB -C
> shared_memory_size_in_huge_pages
> 1
> Which is very useful to be able to actually configure that number of huge
> pages. I don't t
> Any concerns with doing something like this [0] for the back-branches? The
> constant would be 6 instead of 7 on v14 through v16.
As far as backpatching the present inconsistencies in the docs,
[0] looks good to me.
[0]
https://postgr.es/m/attachment/160360/v1-0001-fix-kernel-resources-docs-on
>I agree. Testing StandbyMode here seems bogus. I thought initially
>that the test should perhaps be for InArchiveRecovery rather than
>ArchiveRecoveryRequested, but I see that the code which switches to a
>new timeline cares about ArchiveRecoveryRequested, so I think that is
>t
>I think, however, that your fix is wrong and this one is right.
>Fundamentally, the server is either in normal running, or crash
>recovery, or archive recovery. Standby mode is just an optional
>behavior of archive recovery
Good point. Thanks for clearing my understanding.
Thanks
>> For the benefit of anyone who may be looking at this thread in the
>> archive later, I believe this commit will have fixed this issue:
I can also confirm that indeed the commit
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=6672d79
does fix this issue.
Thanks!
--
>The utility commands for cursor like DECLARE CURSOR seem to have the same
> issue
>and can cause lots of pgss entries. For example, when we use postgres_fdw
> and
>execute "SELECT * FROM WHERE id = 10" five times in the
> same
>transaction, the following commands are executed i
Hi,
What appears to be a pg_dump/pg_restore bug was observed with the new
BEGIN ATOMIC function body syntax introduced in Postgres 14.
Dependencies inside a BEGIN ATOMIC function cannot be resolved
if those dependencies are dumped after the function body. The repro
case is when a primary key cons
> So it's lacking a rule to tell it what to do in this case, and the
> default is the wrong way around. I think we need to fix it in
> about the same way as the equivalent case for matviews, which
> leads to the attached barely-tested patch.
Thanks for the patch! A test on the initially reported u
Hi,
[1] is a ready-for-committer enhancement to pg_stat_progress_vacuum which
exposes
the total number of indexes to vacuum and how many indexes have been vacuumed in
the current vacuum cycle.
To even further improve visibility into index vacuuming, it would be beneficial
to have a
function cal
> I'm sorry for not having read (and not reading) the other thread yet,
> but what was the reason we couldn't store that oid in a column in the
> pg_s_p_vacuum-view?
> Could you summarize the other solutions that were considered for this issue?
Thanks for your feedback!
The reason we cannot sti
As cfbot points out [0], this is missing a Windows/frontend implementation.
[0] https://cirrus-ci.com/task/6393555868975104
Looks like the pgsleep implementation is missing the
#ifndef WIN32
I followed what is done in pg_usleep.
v11 should now build on Windows, hopefully.
Strangely, v10 buil
Skimming the last few messages of that thread [0], it looks like Bertrand
is exploring ways to avoid so many interrupts. I guess the unavoidable
question is whether this work is still worthwhile given that improvement.
The way the instrumentation in [0] dealt with interrupts was too complex,
On 8/13/24 10:09 AM, Nathan Bossart wrote:
On Mon, Aug 12, 2024 at 05:35:08PM -0500, Imseih (AWS), Sami wrote:
Skimming the last few messages of that thread [0], it looks like Bertrand
is exploring ways to avoid so many interrupts. I guess the unavoidable
question is whether this work is
On 8/13/24 10:57 AM, Nathan Bossart wrote:
On Tue, Aug 13, 2024 at 10:47:51AM -0500, Imseih (AWS), Sami wrote:
On 8/13/24 10:09 AM, Nathan Bossart wrote:
On Mon, Aug 12, 2024 at 05:35:08PM -0500, Imseih (AWS), Sami wrote:
Skimming the last few messages of that thread [0], it looks like
Please disregards this point from the last reply:
"""
2/ What if there are other callers in the future that wish
to instrument parallel vacuum workers? they will need to implement
similar logic.
"""
I misspoke about this and this point does not matter since only
vacuum sleep matters for this
None of this seems intractable to me. 1 Hz seems like an entirely
reasonable place to start, and it is very easy to change (or to even make
configurable). pg_stat_progress_vacuum might show slightly old values in
this column, but that should be easy enough to explain in the docs if we
are really
Sounds fine by me (still need to check all three patterns).
+ if (list_length(psrc->query_list) > 0)
+ pgstat_report_query_id(linitial_node(Query, psrc->query_list)->queryId,
false);
Something that slightly worries me is to assume that the first Query
in the query_list is fetched. Usin
I think the testing discussion should be moved to a different thread.
What do you think?
See v4.
0001 deals with reporting queryId in exec_execute_message and
exec_bind_message.
0002 deals with reporting queryId after a cache invalidation.
There are no tests as this requires more discussion i
> Attached is the patch I am finishing with, with some new tests for
> BRIN and btree to force parallel builds with immutable expressions
> through functions.
Sorry about my last reply. Not sure what happened with my email client.
Here it is again.
glad to see the asserts are working as expected
101 - 168 of 168 matches
Mail list logo
