Re: Disabling Heap-Only Tuples
On Thu, 2023-07-06 at 22:18 +0200, Matthias van de Meent wrote: > On Wed, 5 Jul 2023 at 19:55, Thom Brown wrote: > > > > On Wed, 5 Jul 2023 at 18:05, Matthias van de Meent > > wrote: > > > So what were you thinking of? A session GUC? A table option? > > > > Both. > > Here's a small patch implementing a new table option max_local_update > (name very much bikesheddable). Value is -1 (default, disabled) or the > size of the table in MiB that you still want to allow to update on the > same page. I didn't yet go for a GUC as I think that has too little > control on the impact on the system. > > I decided that max_local_update would be in MB because there is no > reloption value that can contain MaxBlockNumber and -1/disabled; and 1 > MiB seems like enough granularity for essentially all use cases. > > The added regression tests show how this feature works, that the new > feature works, and validate that lock levels are acceptable > (ShareUpdateExclusiveLock, same as for updating fillfactor). I have looked at your patch, and I must say that I like it. Having a size limit is better than my original idea of just "on" or "off". Essentially, it is "try to shrink the table if it grows above a limit". The patch builds fine and passes all regression tests. Documentation is missing. I agree that the name "max_local_update" could be improved. Perhaps "avoid_hot_above_size_mb". --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -342,6 +342,7 @@ typedef struct StdRdOptions int parallel_workers; /* max number of parallel workers */ StdRdOptIndexCleanup vacuum_index_cleanup; /* controls index vacuuming */ boolvacuum_truncate;/* enables vacuum to truncate a relation */ + int max_local_update; /* Updates to pages after this block must go through the VM */ } StdRdOptions; #define HEAP_MIN_FILLFACTOR10 In the comment, it should be FSM, not VM, right? Other than that, I see nothing wrong. Yours, Laurenz Albe
Re: PG 16 draft release notes ready
On Thu, 2023-05-18 at 16:49 -0400, Bruce Momjian wrote: > I have completed the first draft of the PG 16 release notes. The release notes still have: - Have initdb use ICU by default if ICU is enabled in the binary (Jeff Davis) Option --locale-provider=libc can be used to disable ICU. But this was reverted in 2535c74b1a6190cc42e13f6b6b55d94bff4b7dd6. Yours, Laurenz Albe
Re: PG 16 draft release notes ready
On Thu, 2023-05-18 at 16:49 -0400, Bruce Momjian wrote: > I have completed the first draft of the PG 16 release notes. The release notes say: - Prevent \df+ from showing function source code (Isaac Morland) Function bodies are more easily viewed with \ev and \ef. That should be \sf, not \ev or \ef, right? Yours, Laurenz Albe
Re: [PATCH] Add support function for containment operators
I wrote: > You implement both "SupportRequestIndexCondition" and > "SupportRequestSimplify", > but when I experimented, the former was never called. That does not > surprise me, since any expression of the shape "expr <@ range constant" > can be simplified. Is the "SupportRequestIndexCondition" branch dead code? > If not, do you have an example that triggers it? I had an idea about this: So far, you only consider constant ranges. But if we have a STABLE range expression, you could use an index scan for "expr <@ range", for example Index Cond (expr >= lower(range) AND expr < upper(range)). Yours, Laurenz Albe
Re: Disabling Heap-Only Tuples
On Fri, 2023-07-07 at 16:27 +0530, Dilip Kumar wrote: > On Fri, Jul 7, 2023 at 3:48 PM Tomas Vondra > wrote: > > I'm imagining either a table option with a couple possible values > > (default, non-hot, first-page, ...) or maybe something even more > > elaborate (perhaps even a callback?). > > > > Now, it's not my intention to hijack this thread, but this discussion > > reminds me one of the ideas from my "BRIN improvements" talk, about > > maybe using BRIN indexes for routing. UPDATEs may be a major issue for > > BRIN, making them gradually worse over time. If we could "tell" > > RelationGetBufferForTuple() which buffers are more suitable (by looking > > at an index, histogram or some approximate mapping), that might help. > > IMHO that seems like the right direction for this feature to be > useful. Right, I agree. A GUC/storage parameter like "update_strategy" that is an enum (try-hot | first-page | ...). To preserve BRIN indexes or CLUSTERed tables, there could be an additional "insert_strategy", but that would somehow have to be tied to a certain index. I think that is out of scope for this effort. Yours, Laurenz Albe
Re: [PATCH] Add support function for containment operators
> On Sat, 2023-04-29 at 17:07 +0200, Kim Johan Andersson wrote:
> > I had noticed that performance wasn't great when using the @> or <@
> > operators when examining if an element is contained in a range.
> > Based on the discussion in [1] I would like to suggest the following
> > changes:
> >
> > This patch attempts to improve the row estimation, as well as opening
> > the possibility of using a btree index scan when using the containment
> > operators.
I managed to break the patch:
CREATE DATABASE czech ICU_LOCALE "cs-CZ" LOCALE "cs_CZ.utf8" TEMPLATE template0;
\c czech
CREATE TYPE textrange AS RANGE (SUBTYPE = text, SUBTYPE_OPCLASS =
text_pattern_ops);
CREATE TABLE tx (t text);
INSERT INTO tx VALUES ('a'), ('c'), ('d'), ('ch');
EXPLAIN SELECT * FROM tx WHERE t <@ textrange('a', 'd');
QUERY PLAN
Seq Scan on tx (cost=0.00..30.40 rows=7 width=32)
Filter: ((t >= 'a'::text) AND (t < 'd'::text))
(2 rows)
SELECT * FROM tx WHERE t <@ textrange('a', 'd');
ERROR: could not determine which collation to use for string comparison
HINT: Use the COLLATE clause to set the collation explicitly.
The replacement operators are wrong; it should be ~>=~ and ~<~ .
Also, there should be no error message.
The result should be 'a', 'c' and 'ch'.
Yours,
Laurenz Albe
Re: [PATCH] Add support function for containment operators
On Sat, 2023-04-29 at 17:07 +0200, Kim Johan Andersson wrote:
> I had noticed that performance wasn't great when using the @> or <@
> operators when examining if an element is contained in a range.
> Based on the discussion in [1] I would like to suggest the following
> changes:
>
> This patch attempts to improve the row estimation, as well as opening
> the possibility of using a btree index scan when using the containment
> operators.
>
> This is done via a new support function handling the following 2 requests:
>
> * SupportRequestIndexCondition
> find_index_quals will build an operator clause, given at least one
> finite RangeBound.
>
> * SupportRequestSimplify
> find_simplified_clause will rewrite the containment operator into a
> clause using inequality operators from the btree family (if available
> for the element type).
>
> A boolean constant is returned if the range is either empty or has no
> bounds.
>
> Performing the rewrite here lets the clausesel machinery provide the
> same estimates as for normal scalar inequalities.
>
> In both cases build_bound_expr is used to build the operator clauses
> from RangeBounds.
I think that this is a small, but useful improvement.
The patch applies and builds without warning and passes "make
installcheck-world"
with the (ample) new regression tests.
Some comments:
- About the regression tests:
You are using EXPLAIN (ANALYZE, SUMMARY OFF, TIMING OFF, COSTS OFF).
While that returns stable results, I don't see the added value.
I think you should use EXPLAIN (COSTS OFF). You don't need to test the
actual number of result rows; we can trust that the executor processes
>= and < correctly.
Plain EXPLAIN would speed up the regression tests, which is a good thing.
- About the implementation:
You implement both "SupportRequestIndexCondition" and
"SupportRequestSimplify",
but when I experimented, the former was never called. That does not
surprise me, since any expression of the shape "expr <@ range constant"
can be simplified. Is the "SupportRequestIndexCondition" branch dead code?
If not, do you have an example that triggers it?
- About the code:
+static Node *
+find_index_quals(Const *rangeConst, Expr *otherExpr, Oid opfamily)
+{
[...]
+
+ if (!(lower.infinite && upper.infinite))
+ {
[...]
+ }
+
+ return NULL;
To avoid deep indentation and to make the code more readable, I think
it would be better to write
if (!(lower.infinite && upper.infinite))
return NULL;
and unindent the rest of the code
+static Node *
+match_support_request(Node *rawreq)
+{
[...]
+ switch (req->funcid)
+ {
+ case F_ELEM_CONTAINED_BY_RANGE:
[...]
+ case F_RANGE_CONTAINS_ELEM:
[...]
+ default:
+ return NULL;
+ }
(This code appears twice.)
The default clause should not be reachable, right?
I think that there should be an Assert() to verify that.
Perhaps something like
Assert(req->funcid == F_ELEM_CONTAINED_BY_RANGE ||
req->funcid == F_RANGE_CONTAINS_ELEM);
if (req->funcid == F_ELEM_CONTAINED_BY_RANGE)
{
[...]
}
else if (req->funcid == F_RANGE_CONTAINS_ELEM)
{
[...]
}
Yours,
Laurenz Albe
Re: Disabling Heap-Only Tuples
On Wed, 2023-07-05 at 12:02 +0100, Thom Brown wrote: > On Wed, 5 Jul 2023 at 11:57, Matthias van de Meent > wrote: > > On Wed, 5 Jul 2023 at 12:45, Thom Brown wrote: > > > Heap-Only Tuple (HOT) updates are a significant performance > > > enhancement, as they prevent unnecessary page writes. However, HOT > > > comes with a caveat: it means that if we have lots of available space > > > earlier on in the relation, it can only be used for new tuples or in > > > cases where there's insufficient space on a page for an UPDATE to use > > > HOT. > > > > > > Considering these trade-offs, I'd like to propose an option to allow > > > superusers to disable HOT on tables. The intent is to trade some > > > performance benefits for the ability to reduce the size of a table > > > without the typical locking associated with it. > > > > Interesting use case, but I think that disabling HOT would be missing > > the forest for the trees. I think that a feature that disables > > block-local updates for pages > some offset would be a better solution > > to your issue: Normal updates also prefer the new tuple to be stored > > in the same pages as the old tuple if at all possible, so disabling > > HOT wouldn't solve the issue of tuples residing in the tail of your > > table - at least not while there is still empty space in those pages. > > Hmm... I see your point. It's when an UPDATE isn't going to land on > the same page that it relocates to the earlier available page. So I > guess I'm after whatever mechanism would allow that to happen reliably > and predictably. > > So $subject should really be "Allow forcing UPDATEs off the same page". I've been thinking about the same thing - an option that changes the update strategy to always use the lowest block with enough free space. That would allow to consolidate bloated tables with no down time. Yours, Laurenz Albe
Re: Memory leak in incremental sort re-scan
On Sun, 2023-07-02 at 20:13 +0200, Tomas Vondra wrote: > FWIW I've pushed the fix prepared by James a couple days ago. Thanks for > the report! Thanks, and sorry for being pushy. Yours, Laurenz Albe
Re: Memory leak in incremental sort re-scan
On Fri, 2023-06-16 at 00:34 +0200, Tomas Vondra wrote: > On 6/15/23 22:36, Tom Lane wrote: > > Tomas Vondra writes: > > > On 6/15/23 22:11, Tom Lane wrote: > > > > I see zero leakage in that example after applying the attached quick > > > > hack. (It might be better to make the check in the caller, or to just > > > > move the call to ExecInitIncrementalSort.) > > > > > Thanks for looking. Are you planning to work on this and push the fix, > > > or do you want me to finish this up? > > > > I'm happy to let you take it -- got lots of other stuff on my plate. > > OK, will do. It would be cool if we could get that into the next minor release in August. Yours, Laurenz Albe
Re: Assistance Needed: Issue with pg_upgrade and --link option
On Wed, 2023-06-28 at 15:40 +0530, Pradeep Kumar wrote: > > > I was under the impression that the --link option would create hard links > > > between the > > > old and new cluster's data files, but it appears that the entire old > > > cluster data was > > > copied to the new cluster, resulting in a significant increase in the new > > > cluster's size. > > > > Please provide some numbers, ideally > > > > du -sk > > du -sk ~/pradeep_test/pg_upgrade_testing/postgres_11.4/master > ~/pradeep_test/pg_upgrade_testing/postgres_14/new_pg > 11224524 /home/test/pradeep_test/pg_upgrade_testing/postgres_11.4/master > 41952 /home/test/pradeep_test/pg_upgrade_testing/postgres_14/new_pg That looks fine. The files exist only once, and the 41MB that only exist in the new data directory are catalog data and other stuff that is different on the new cluster. Yours, Laurenz Albe
Re: Assistance Needed: Issue with pg_upgrade and --link option
On Wed, 2023-06-28 at 11:49 +0530, Pradeep Kumar wrote: > I was under the impression that the --link option would create hard links > between the > old and new cluster's data files, but it appears that the entire old cluster > data was > copied to the new cluster, resulting in a significant increase in the new > cluster's size. Please provide some numbers, ideally du -sk Yours, Laurenz Albe
Re: Stampede of the JIT compilers
On Sun, 2023-06-25 at 11:10 +0200, Michael Banck wrote: > On Sat, Jun 24, 2023 at 01:54:53PM -0400, Tom Lane wrote: > > I don't know whether raising the default would be enough to fix that > > in a nice way, and I certainly don't pretend to have a specific value > > to offer. But it's undeniable that we have a serious problem here, > > to the point where JIT is a net negative for quite a few people. > > Some further data: to my knowledge, most major managed postgres > providers disable jit for their users. I have also started recommending jit=off for all but analytic workloads. Yours, Laurenz Albe
Re: patch: improve "user mapping not found" error message
On Fri, 2023-06-23 at 16:45 +0900, Ian Lawrence Barwick wrote: > Mild corner-case annoyance while doing Random Experimental Things: > > postgres=# SELECT * FROM parttest; > ERROR: user mapping not found for "postgres" > > Oky, but which server? > > postgres=# \det > List of foreign tables > Schema | Table | Server > +---+--- > public | parttest_10_1 | fdw_node2 > public | parttest_10_3 | fdw_node3 > public | parttest_10_5 | fdw_node4 > public | parttest_10_7 | fdw_node5 > public | parttest_10_9 | fdw_node6 > (5 rows) > > (Muffled sound of small patch hatching) aha: > > postgres=# SELECT * FROM parttest; > ERROR: user mapping not found for user "postgres", server "fdw_node5" +1 Yours, Laurenz Albe
Re: When IMMUTABLE is not.
On Thu, 2023-06-15 at 13:22 +0300, Yura Sokolov wrote:
> Good day, hackers.
>
> I found, than declaration of function as IMMUTABLE/STABLE is not enough to be
> sure
> function doesn't manipulate data.
>
> [...]
>
> + errmsg("Damn1! Update were done
> in a non-volatile function")));
I think it is project policy to start error messages with a lower case
character.
Yours,
Laurenz Albe
Memory leak in incremental sort re-scan
ExecIncrementalSort() calls tuplesort_begin_common(), which creates the
"TupleSort main"
and "TupleSort sort" memory contexts, and ExecEndIncrementalSort() calls
tuplesort_end(),
which destroys them.
But ExecReScanIncrementalSort() only resets the memory contexts. Since the
next call to
ExecIncrementalSort() will create them again, we end up leaking these contexts
for every
re-scan.
Here is a reproducer with the regression test database:
SET enable_sort = off;
SET enable_hashjoin = off;
SET enable_mergejoin = off;
SET enable_material = off;
SELECT t.unique2, t2.r
FROM tenk1 AS t
JOIN (SELECT unique1,
row_number() OVER (ORDER BY hundred, thousand) AS r
FROM tenk1
OFFSET 0) AS t2
ON t.unique1 + 0 = t2.unique1
WHERE t.unique1 < 1000;
The execution plan:
Nested Loop
Join Filter: ((t.unique1 + 0) = tenk1.unique1)
-> Bitmap Heap Scan on tenk1 t
Recheck Cond: (unique1 < 1000)
-> Bitmap Index Scan on tenk1_unique1
Index Cond: (unique1 < 1000)
-> WindowAgg
-> Incremental Sort
Sort Key: tenk1.hundred, tenk1.thousand
Presorted Key: tenk1.hundred
-> Index Scan using tenk1_hundred on tenk1
A memory context dump at the end of the execution looks like this:
ExecutorState: 262144 total in 6 blocks; 74136 free (29 chunks); 188008
used
TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 25512
used
TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0
chunks); 208 used
TupleSort main: 32832 total in 2 blocks; 7256 free (0 chunks); 25576
used
TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0
chunks); 208 used
TupleSort main: 32832 total in 2 blocks; 7320 free (0 chunks); 25512
used
TupleSort sort: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
Caller tuples: 8192 total in 1 blocks (0 chunks); 7984 free (0
chunks); 208 used
[many more]
1903 more child contexts containing 93452928 total in 7597 blocks;
44073240 free (0 chunks); 49379688 used
The following patch fixes the problem for me:
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -1145,21 +1145,16 @@ ExecReScanIncrementalSort(IncrementalSortState *node)
node->execution_status = INCSORT_LOADFULLSORT;
/*
-* If we've set up either of the sort states yet, we need to reset them.
-* We could end them and null out the pointers, but there's no reason to
-* repay the setup cost, and because ExecIncrementalSort guards presorted
-* column functions by checking to see if the full sort state has been
-* initialized yet, setting the sort states to null here might actually
-* cause a leak.
+* Release tuplesort resources.
*/
if (node->fullsort_state != NULL)
{
- tuplesort_reset(node->fullsort_state);
+ tuplesort_end(node->fullsort_state);
node->fullsort_state = NULL;
}
if (node->prefixsort_state != NULL)
{
- tuplesort_reset(node->prefixsort_state);
+ tuplesort_end(node->prefixsort_state);
node->prefixsort_state = NULL;
}
The original comment hints that this might mot be the correct thing to do...
Yours,
Laurenz Albe
Re: Add support for AT LOCAL
On Tue, 2023-06-06 at 04:24 -0400, Vik Fearing wrote: > > At a quick glance, it looks like you resolve "timezone" at the time > > the query is parsed. Shouldn't the resolution happen at query > > execution time? > > current_setting(text) is stable, and my tests show that it is calculated > at execution time. Ah, ok, then sorry for the noise. I misread the code then. Yours, Laurenz Albe
Re: Return value of pg_promote()
On Tue, 2023-06-06 at 16:35 +0530, Ashutosh Sharma wrote: > At present, pg_promote() returns true to the caller on successful > promotion of standby, however it returns false in multiple scenarios > which includes: > > 1) The SIGUSR1 signal could not be sent to the postmaster process. > 2) The postmaster died during standby promotion. > 3) Standby couldn't be promoted within the specified wait time. > > For an application calling this function, if pg_promote returns false, > it is hard to interpret the reason behind it. So I think we should > *only* allow pg_promote to return false when the server could not be > promoted in the given wait time and in other scenarios it should just > throw an error (FATAL, ERROR ... depending on the type of failure that > occurred). Please let me know your thoughts on this change. thanks.! As the original author, I'd say that that sounds reasonable, particularly in case #1. If the postmaster dies, we are going to die too, so it probably doesn't matter much. But I think an error is certainly also correct in that case. Yours, Laurenz Albe
Re: ERROR: could not determine which collation to use for string comparison
On Tue, 2023-06-06 at 11:42 -0300, Rômulo Coutinho wrote: > We recently upgraded to Postgres 15.3. When running ANALYZE, we get the > following message: > ERROR: could not determine which collation to use for string comparison > HINT: Use the COLLATE clause to set the collation explicitly. > > We have never seen this before. Could this be a bug? Impossible to say without a way to reproduce. Yours, Laurenz Albe
Re: Add support for AT LOCAL
On Mon, 2023-06-05 at 23:13 -0400, Vik Fearing wrote: > The Standard defines time zone conversion as follows: > > ::= > [ ] > > ::= > AT > > ::= > LOCAL > | TIME ZONE > > > While looking at something else, I noticed we do not support AT LOCAL. > The local time zone is defined as that of *the session*, not the server, > which can make this quite interesting in views where the view will > automatically adjust to the session's time zone. > > Patch against 3f1180 attached. +1 on the idea; it should be faily trivial, if not very useful. At a quick glance, it looks like you resolve "timezone" at the time the query is parsed. Shouldn't the resolution happen at query execution time? Yours, Laurenz Albe
Re: Mark a transaction uncommittable
On Mon, 2023-06-05 at 00:22 -0700, Gurjeet Singh wrote: > On Sat, Apr 22, 2023 at 8:01 AM Gurjeet Singh wrote: > > > > This is a proposal for a new transaction characteristic. I haven't > > written any code, yet, and am interested in hearing if others may find > > this feature useful. > > Please see attached the patch that introduces this new feature. Can you explain why *you* would find this feature useful? Yours, Laurenz Albe
Re: Support edit order of the fields in table
On Thu, 2023-06-01 at 00:31 +0800, Chang Wei 昌維 wrote: > Hi Postgres community: I think support editing order of the fields in > table is a useful feature. I have known that the order of fields will > effect the data structure of rows data, but I think we could add a extra > information to identify the display order of fields but not effect the > rows data, and the order identification is only used to display in order > while execute `SELECT * FROM [table_name]` and display the table > structure on GUI tools like pgAdmin. > > Now, we must create a new view and define the order of fields if we need > to display the fields of table in a order of our demand, it is not a > good way. But PostgreSQL tables are not spreadsheets. When, except in the display of the result of interactive queries, would the order matter? Yours, Laurenz Albe
Re: PG 16 draft release notes ready
On Thu, 2023-05-18 at 16:49 -0400, Bruce Momjian wrote: > I have completed the first draft of the PG 16 release notes. I found two typos. Yours, Laurenz Albe diff --git a/doc/src/sgml/release-16.sgml b/doc/src/sgml/release-16.sgml index faecae7c42..7dad0b8550 100644 --- a/doc/src/sgml/release-16.sgml +++ b/doc/src/sgml/release-16.sgml @@ -1294,7 +1294,7 @@ Determine the ICU default locale from the environment (Jeff Davis) -However, ICU doesn't support the C local so UTF-8 is used in such cases. Previously the default was always UTF-8. +However, ICU doesn't support the C locale so UTF-8 is used in such cases. Previously the default was always UTF-8. @@ -1335,7 +1335,7 @@ Author: Peter Eisentraut -Add Windows process the system collations (Jose Santamaria Flecha) +Add Windows to process the system collations (Jose Santamaria Flecha) ADD THIS?
Re: Adding SHOW CREATE TABLE
On Fri, 2023-05-19 at 13:08 -0400, Andrew Dunstan wrote: > On 2023-05-18 Th 19:53, Stephen Frost wrote: > > * Kirk Wolak (wol...@gmail.com) wrote: > > > My approach for now is to develop this as the \st command. > > > After reviewing the code/output from the 3 sources (psql, fdw, and > > > pg_dump). > > > > Having this only available via psql seems like the least desirable > > option as then it wouldn't be available to any other callers.. > > > > Having it in libpq, on the other hand, would make it available to psql > > as well as any other utility, library, or language / driver which uses > > libpq, including pg_dump.. > > I think the ONLY place we should have this is in server side functions. +1 A function "pg_get_tabledef" would blend nicely into the following list: \df pg_get_*def List of functions Schema │ Name │ Result data type │ Argument data types │ Type ╪╪══╪═══╪══ pg_catalog │ pg_get_constraintdef │ text │ oid │ func pg_catalog │ pg_get_constraintdef │ text │ oid, boolean │ func pg_catalog │ pg_get_functiondef │ text │ oid │ func pg_catalog │ pg_get_indexdef│ text │ oid │ func pg_catalog │ pg_get_indexdef│ text │ oid, integer, boolean │ func pg_catalog │ pg_get_partition_constraintdef │ text │ oid │ func pg_catalog │ pg_get_partkeydef │ text │ oid │ func pg_catalog │ pg_get_ruledef │ text │ oid │ func pg_catalog │ pg_get_ruledef │ text │ oid, boolean │ func pg_catalog │ pg_get_statisticsobjdef│ text │ oid │ func pg_catalog │ pg_get_triggerdef │ text │ oid │ func pg_catalog │ pg_get_triggerdef │ text │ oid, boolean │ func pg_catalog │ pg_get_viewdef │ text │ oid │ func pg_catalog │ pg_get_viewdef │ text │ oid, boolean │ func pg_catalog │ pg_get_viewdef │ text │ oid, integer │ func pg_catalog │ pg_get_viewdef │ text │ text │ func pg_catalog │ pg_get_viewdef │ text │ text, boolean │ func (17 rows) A server function can be conveniently called from any client code. Yours, Laurenz Albe
Re: psql: Could we get "-- " prefixing on the **** QUERY **** outputs? (ECHO_HIDDEN)
On Wed, 2023-05-17 at 13:39 -0400, Kirk Wolak wrote: > Here's the patch. You removed the QUERY at the end of the query. I think we should keep that (as comments, of course). People are used to the current output, and it is nice to have a clear visual marker at the end of what isn't normally part of "psql" output. "okbob" should be "Pavel Stehule". Yours, Laurenz Albe
Re: psql: Could we get "-- " prefixing on the **** QUERY **** outputs? (ECHO_HIDDEN)
On Mon, 2023-05-15 at 08:37 +0200, Pavel Stehule wrote:
> Dne po 15. 5. 2023 8:01 uživatel Kirk Wolak napsal:
> > This would be a trivial change. Willing to do it, and push it.
> >
> > In effect, we have this GREAT feature:
> > \set ECHO_HIDDON on
> >
> > Which outputs a bunch of queries (as you all know).
> > But somehow nobody thought that a user might want to paste ALL of the
> > queries into their query editor, or even into another psql session, via (\e)
> > and NOT get a ton of syntax errors?
> >
> > As an example: (added -- and a space)
> >
> > -- * QUERY **
> > SELECT c2.relname, i.indisprimary, i.indisunique, i.indisclustered,
> > i.indisvalid, pg_catalog.pg_get_indexdef(i.indexrelid, 0, true),
> > pg_catalog.pg_get_constraintdef(con.oid, true), contype, condeferrable,
> > condeferred, i.indisreplident, c2.reltablespace
> > FROM pg_catalog.pg_class c, pg_catalog.pg_class c2, pg_catalog.pg_index i
> > LEFT JOIN pg_catalog.pg_constraint con ON (conrelid = i.indrelid AND
> > conindid = i.indexrelid AND contype IN ('p','u','x'))
> > WHERE c.oid = '21949943' AND c.oid = i.indrelid AND i.indexrelid = c2.oid
> > ORDER BY i.indisprimary DESC, c2.relname;
> > -- **
>
> This looks little bit strange
>
> What about /* comments
>
> Like
>
> /*** Query /
>
> Or just
>
> Query
+1 for either of Pavel's suggestions.
Yours,
Laurenz Albe
Re: Should we remove vacuum_defer_cleanup_age?
On Fri, 2023-04-14 at 18:43 +0200, Alvaro Herrera wrote: > On 2023-Apr-14, Greg Stark wrote: > > I assume people would use hot_standby_feedback if they have streaming > > replication. > > Yes, either that or a replication slot. A replication slot doesn't do anything against snapshot conflicts, which is what we are discussing here. Or are we not? > > I find it very hard to believe that people are doing stuff with > vacuum_defer_cleanup_age that cannot be done with either of the other > newer mechanisms, which have also seen much wider usage and so bugs > fixed, etc. vacuum_defer_cleanup_age offers a more fine-grained approach. With hot_standby_feedback you can only say "don't ever remove any dead tuples that the standby still needs". But perhaps you'd prefer "don't remove dead tuples unless they are quite old", so that you can get your shorter queries on the standby to complete, without delaying replay and without the danger that a long running query on the standby bloats your primary. How about this: Let's remove vacuum_defer_cleanup_age, and put a note in the release notes that recommends using statement_timeout and hot_standby_feedback = on on the standby instead. That should have pretty much the same effect, and it is measured in time and not in the number of transactions. Yours, Laurenz Albe
Re: Should we remove vacuum_defer_cleanup_age?
On Thu, 2023-04-13 at 12:16 -0400, Jonathan S. Katz wrote: > On 4/13/23 11:32 AM, Jonathan S. Katz wrote: > > On 4/12/23 11:34 PM, Amit Kapila wrote: > > > On Tue, Apr 11, 2023 at 11:50 PM Andres Freund > > > > +1 to do one of the above. I think there is a good chance that > > > somebody might be doing more harm by using it so removing this > > > shouldn't be a problem. Personally, I have not heard of people using > > > it but OTOH it is difficult to predict so giving some time is also not > > > a bad idea. > > > > > > Do others have any opinion/suggestion on this matter? > > > > I need a bit more time to study this before formulating an opinion on > > whether we should remove it for v16. In any case, I'm not against > > documentation. > > [RMT hat] > > +1 for removing. I am not against this in principle, but I know that there are people using this parameter; see the discussion linked in https://postgr.es/m/e1jkzxe-0006dw...@gemulon.postgresql.org I can't say if they have a good use case for that parameter or not. Yours, Laurenz Albe
Re: Add standard collation UNICODE
On Thu, 2023-03-23 at 13:16 -0700, Jeff Davis wrote: > Another thought: for ICU, do we want the default collation to be > UNICODE (root collation)? What we have now gets the default from the > environment, which is consistent with the libc provider. > > But now that we have the UNICODE collation, it makes me wonder if we > should just default to that. The server's environment doesn't > necessarily say much about the locale of the data stored in it or the > locale of the applications accessing it. > > I don't have a strong opinion here, but I thought I'd raise the issue. > > By my count, >50% of locales are actually just the root locale. I'm not > sure if that should matter or not -- we don't want to weigh some > locales over others -- but I found it interesting. I second that. Most people don't pay attention to that when creating a cluster, so having a locale-agnostic collation is often better than inheriting whatever default happened to be set in your shell. For example, the Debian/Ubuntu binary packages create a cluster when you install the server package, and most people just go on using that. Yours, Laurenz Albe
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Fri, 2023-03-24 at 16:41 -0400, Tom Lane wrote: > Christoph Berg writes: > > Re: Tom Lane > > > I don't actually see why a postgres_fdw test case is needed at all? > > > The tests in explain.sql seem sufficient. > > > When I asked Laurenz about that he told me that local tables wouldn't > > exercise the code specific for EXEC_FLAG_EXPLAIN_GENERIC. > > But there isn't any ... or at least, I fail to see what isn't sufficiently > exercised by that new explain.sql test case that's identical to this one > except for being a non-foreign table. Perhaps at some point this patch > modified postgres_fdw code? But it doesn't now. > > I don't mind having a postgres_fdw test if there's something for it > to test, but it just looks duplicative to me. Other things being > equal, I'd prefer to test this feature in explain.sql, since (a) it's > a core feature and (b) the core tests are better parallelized than the > contrib tests, so the same test should be cheaper to run. > > > (Admittedly my knowledge of the planner wasn't deep enough to verify > > that. Laurenz is currently traveling, so I don't know if he could > > answer this himself now.) > > OK, thanks for the status update. What I'll do to get this off my > plate is to push the patch without the postgres_fdw test -- if > Laurenz wants to advocate for that when he returns, we can discuss it > more. Thanks for committing this. As Chrisoph mentioned, I found that I could not reproduce the problem that was addressed by the EXEC_FLAG_EXPLAIN_GENERIC hack using local partitioned tables. My optimizer knowledge is not deep enough to tell why, and it might well be a bug lurking in the FDW part of the optimizer code. It is not FDW specific, since I discovered it with oracle_fdw and could reproduce it with postgres_fdw. I was aware that it is awkward to add a test to a contrib module, but I thought that I should add a test that exercises the new code path. But I am fine without the postgres_fdw test. Yours, Laurenz Albe
Re: Make EXPLAIN generate a generic plan for a parameterized query
And here is v10, which includes tab completion for the new option. Yours, Laurenz Albe From dfe6d36d79c74fba7bf70b990fdada166d012ff4 Mon Sep 17 00:00:00 2001 From: Laurenz Albe Date: Thu, 23 Mar 2023 19:28:49 +0100 Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN This allows EXPLAIN to generate generic plans for parameterized statements (that have parameter placeholders like $1 in the statement text). Invent a new executor flag EXEC_FLAG_EXPLAIN_GENERIC that disables runtime partition pruning for such plans: that would fail if the non-existing parameters are involved, and we want to show all subplans anyway. Author: Laurenz Albe Reviewed-by: Julien Rouhaud, Christoph Berg, Michel Pelletier, Jim Jones Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at --- .../postgres_fdw/expected/postgres_fdw.out| 30 + contrib/postgres_fdw/sql/postgres_fdw.sql | 25 +++ doc/src/sgml/ref/explain.sgml | 14 +++ src/backend/commands/explain.c| 11 + src/backend/executor/execMain.c | 3 ++ src/backend/executor/execPartition.c | 9 ++-- src/backend/parser/analyze.c | 29 + src/bin/psql/tab-complete.c | 4 +- src/include/commands/explain.h| 1 + src/include/executor/executor.h | 18 +--- src/test/regress/expected/explain.out | 42 +++ src/test/regress/sql/explain.sql | 24 +++ 12 files changed, 199 insertions(+), 11 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 04a3ef450c..25b91ab2e1 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -11783,3 +11783,33 @@ ANALYZE analyze_table; -- cleanup DROP FOREIGN TABLE analyze_ftable; DROP TABLE analyze_table; +-- === +-- test EXPLAIN (GENERIC_PLAN) with foreign partitions +-- === +-- this is needed to exercise the EXEC_FLAG_EXPLAIN_GENERIC flag +CREATE TABLE gen_part ( +key1 integer NOT NULL, +key2 integer NOT NULL +) PARTITION BY LIST (key1); +CREATE TABLE gen_part_1 +PARTITION OF gen_part FOR VALUES IN (1) +PARTITION BY RANGE (key2); +CREATE FOREIGN TABLE gen_part_1_1 +PARTITION OF gen_part_1 FOR VALUES FROM (1) TO (2) +SERVER testserver1 OPTIONS (table_name 'whatever_1_1'); +CREATE FOREIGN TABLE gen_part_1_2 +PARTITION OF gen_part_1 FOR VALUES FROM (2) TO (3) +SERVER testserver1 OPTIONS (table_name 'whatever_1_2'); +CREATE FOREIGN TABLE gen_part_2 +PARTITION OF gen_part FOR VALUES IN (2) +SERVER testserver1 OPTIONS (table_name 'whatever_2'); +-- this should only scan "gen_part_1_1" and "gen_part_1_2", but not "gen_part_2" +EXPLAIN (GENERIC_PLAN, COSTS OFF) SELECT key1, key2 FROM gen_part WHERE key1 = 1 AND key2 = $1; + QUERY PLAN +--- + Append + -> Foreign Scan on gen_part_1_1 gen_part_1 + -> Foreign Scan on gen_part_1_2 gen_part_2 +(3 rows) + +DROP TABLE gen_part; diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 4f3088c03e..6adc3f2c78 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -3979,3 +3979,28 @@ ANALYZE analyze_table; -- cleanup DROP FOREIGN TABLE analyze_ftable; DROP TABLE analyze_table; + +-- === +-- test EXPLAIN (GENERIC_PLAN) with foreign partitions +-- === + +-- this is needed to exercise the EXEC_FLAG_EXPLAIN_GENERIC flag +CREATE TABLE gen_part ( +key1 integer NOT NULL, +key2 integer NOT NULL +) PARTITION BY LIST (key1); +CREATE TABLE gen_part_1 +PARTITION OF gen_part FOR VALUES IN (1) +PARTITION BY RANGE (key2); +CREATE FOREIGN TABLE gen_part_1_1 +PARTITION OF gen_part_1 FOR VALUES FROM (1) TO (2) +SERVER testserver1 OPTIONS (table_name 'whatever_1_1'); +CREATE FOREIGN TABLE gen_part_1_2 +PARTITION OF gen_part_1 FOR VALUES FROM (2) TO (3) +SERVER testserver1 OPTIONS (table_name 'whatever_1_2'); +CREATE FOREIGN TABLE gen_part_2 +PARTITION OF gen_part FOR VALUES IN (2) +SERVER testserver1 OPTIONS (table_name 'whatever_2'); +-- this should only scan "gen_part_1_1" and "gen_part_1_2", but not "gen_part_2" +EXPLAIN (GENERIC_PLAN, COSTS OFF) SELECT key1, key2 FROM gen_part WHERE key1 = 1 AND key2 = $1; +DROP TABLE gen_part; diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml index 0fce622423..4985545c78 100644 --- a/doc/src/sgml/ref/explain.sgml +++ b/doc/src/sgml/ref/explain.sgml @@ -40,6 +40,7 @@ EXPLA
Re: Make EXPLAIN generate a generic plan for a parameterized query
Thanks for the review!
On Tue, 2023-03-21 at 16:32 +0100, Christoph Berg wrote:
> I have some comments:
>
> > This allows EXPLAIN to generate generic plans for parameterized statements
> > (that have parameter placeholders like $1 in the statement text).
>
> > +
> > + GENERIC_PLAN
> > +
> > +
> > + Generate a generic plan for the statement (see > linkend="sql-prepare"/>
> > + for details about generic plans). The statement can contain
> > parameter
> > + placeholders like $1 (but then it has to be a
> > statement
> > + that supports parameters). This option cannot be used together with
> > + ANALYZE, since a statement with unknown parameters
> > + cannot be executed.
>
> Like in the commit message quoted above, I would put more emphasis on
> "parameterized query" here:
>
> Allow the statement to contain parameter placeholders like
> $1 and generate a generic plan for it.
> This option cannot be used together with ANALYZE.
I went with
Allow the statement to contain parameter placeholders like
$1 and generate a generic plan for it.
See for details about generic plans
and the statements that support parameters.
This option cannot be used together with ANALYZE.
> > + /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
> > + if (es->generic && es->analyze)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("EXPLAIN ANALYZE cannot be used
> > with GENERIC_PLAN")));
>
> To put that in line with the other error messages in that context, I'd
> inject an extra "option":
>
> errmsg("EXPLAIN option ANALYZE cannot be used with GENERIC_PLAN")));
Done.
> > --- a/src/test/regress/sql/explain.sql
> > +++ b/src/test/regress/sql/explain.sql
> > [...]
> > +create extension if not exists postgres_fdw;
>
> "create extension postgres_fdw" cannot be used from src/test/regress/
> since contrib/ might not have been built.
Ouch. Good catch.
> I suggest leaving this test in place here, but with local tables (to
> show that plan time pruning using the one provided parameter works),
> and add a comment here explaining that is being tested:
>
> -- create a partition hierarchy to show that plan time pruning removes
> -- the key1=2 table but generates a generic plan for key2=$1
I did that, with a different comment.
> The test involving postgres_fdw is still necessary to exercise the new
> EXEC_FLAG_EXPLAIN_GENERIC code path, but needs to be moved elsewhere,
> probably src/test/modules/.
Tests for postgres_fdw are in contrib/postgres_fdw/sql/postgres_fdw.sql,
so I added the test there.
Version 9 of the patch is attached.
Yours,
Laurenz Albe
From 85aa88280069ca2befe7f4308d7e6f724cdb143a Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Wed, 22 Mar 2023 14:08:49 +0100
Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN
This allows EXPLAIN to generate generic plans for parameterized statements
(that have parameter placeholders like $1 in the statement text).
Invent a new executor flag EXEC_FLAG_EXPLAIN_GENERIC that disables runtime
partition pruning for such plans: that would fail if the non-existing
parameters are involved, and we want to show all subplans anyway.
Author: Laurenz Albe
Reviewed-by: Julien Rouhaud, Christoph Berg, Michel Pelletier, Jim Jones
Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at
---
.../postgres_fdw/expected/postgres_fdw.out| 30 +
contrib/postgres_fdw/sql/postgres_fdw.sql | 25 +++
doc/src/sgml/ref/explain.sgml | 14 +++
src/backend/commands/explain.c| 11 +
src/backend/executor/execMain.c | 3 ++
src/backend/executor/execPartition.c | 9 ++--
src/backend/parser/analyze.c | 29 +
src/include/commands/explain.h| 1 +
src/include/executor/executor.h | 18 +---
src/test/regress/expected/explain.out | 42 +++
src/test/regress/sql/explain.sql | 24 +++
11 files changed, 197 insertions(+), 9 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 04a3ef450c..25b91ab2e1 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -11783,3 +11783,33 @@ ANALYZE analyze_table;
-- cleanup
DROP FOREIGN TABLE analyze_ftable;
DROP TABLE analyze_table;
+-- ===
+-- test EXPLAIN (GEN
Re: Allow tailoring of ICU locales with custom rules
On Fri, 2023-03-03 at 13:45 +0100, Peter Eisentraut wrote: > Ok, here is a version with an initdb option and also a createdb option > (to expose the CREATE DATABASE option). > > You can mess with people by setting up your databases like this: > > initdb -D data --locale-provider=icu --icu-rules=' < c < b < e < d' Looks good. I cannot get it to misbehave, "make check-world" is successful (the regression tests misbehave in interesting ways when running "make installcheck" on a cluster created with non-standard ICU rules, but that can be expected). I checked the documentation, tested "pg_dump" support, everything fine. I'll mark it as "ready for committer". Yours, Laurenz Albe
Re: Allow tailoring of ICU locales with custom rules
On Tue, 2023-03-07 at 22:06 -0800, Jeff Davis wrote: > On Fri, 2023-03-03 at 13:45 +0100, Peter Eisentraut wrote: > > You can mess with people by setting up your databases like this: > > > > initdb -D data --locale-provider=icu --icu-rules=' < c < b < e < d' > > > > ;-) > > Would we be the first major database to support custom collation rules? > This sounds useful for testing, experimentation, hacking, etc. > > What are some of the use cases? Is it helpful to comply with unusual or > outdated standards or formats? Maybe there are people using special > delimiters/terminators and they need them to be treated a certain way > during comparisons? I regularly see complaints about the sort order; recently this one: https://postgr.es/m/cafcrh--xt-j8awoavhb216kom6tqnap35ttveqqs5bhh7gm...@mail.gmail.com So being able to influence the sort order is useful. Yours, Laurenz Albe
Re: Allow tailoring of ICU locales with custom rules
On Wed, 2023-02-22 at 18:35 +0100, Peter Eisentraut wrote: > > - there doesn't seem to be a way to add rules to template1. > > If someone wants to have icu rules and initial contents to their new > > databases, I think they need to create a custom template database > > (from template0) for that purpose, in addition to template1. > > From a usability standpoint, this is a bit cumbersome, as it's > > normally the role of template1. > > To improve on that, shouldn't initdb be able to create template0 with > > rules too? > > Right, that would be an initdb option. Is that too many initdb options > then? It would be easy to add, if we think it's worth it. An alternative would be to document that you can drop "template1" and create it again using the ICU collation rules you need. But I'd prefer an "initdb" option. Yours, Laurenz Albe
Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)
On Wed, 2023-03-01 at 11:13 -0500, Kirk Wolak wrote: > Thanks, corrected, and confirmed Unix line endings. The patch builds fine and works as intended. I leave it to the committers to decide whether the patch is worth the effort or not, given that you can get a similar effect with %`date`. It adds some value by being simpler and uniform across all platforms. I'll mark the patch as "ready for committer". Yours, Laurenz Albe
Re: Move defaults toward ICU in 16?
On Fri, 2023-02-17 at 14:40 +0900, Michael Paquier wrote: > Separate question: what's the state of the Windows installers provided > by the community regarding libicu? Is that embedded in the MSI? The EDB installer installs a quite old version of the ICU library for compatibility reasons, as far as I know. Yours, Laurenz Albe
Re: Move defaults toward ICU in 16?
On Thu, 2023-02-16 at 15:05 +0530, Robert Haas wrote: > On Thu, Feb 16, 2023 at 1:01 AM Jeff Davis wrote: > > It feels very wrong to me to explain that sort order is defined by the > > operating system on which Postgres happens to run. Saying that it's > > defined by ICU, which is part of the Unicode consotium, is much better. > > It doesn't eliminate versioning issues, of course, but I think it's a > > better explanation for users. > > The fact that we can't use ICU on Windows, though, weakens this > argument a lot. In my experience, we have a lot of Windows users, and > they're not any happier with the operating system collations than > Linux users. Possibly less so. > > I feel like this is a very difficult kind of change to judge. If > everyone else feels this is a win, we should go with it, and hopefully > we'll end up better off. I do feel like there are things that could go > wrong, though, between the imperfect documentation, the fact that a > substantial chunk of our users won't be able to use it because they > run Windows, and everybody having to adjust to the behavior change. Unless I misunderstand, the lack of Windows support is not a matter of principle and can be added later on, right? I am in favor of changing the default. It might be good to add a section to the documentation in "Server setup and operation" recommending that if you go with the default choice of ICU, you should configure your package manager not to upgrade the ICU library. Yours, Laurenz Albe
Re: Allow tailoring of ICU locales with custom rules
On Mon, 2023-02-06 at 22:16 +0100, Peter Eisentraut wrote: > Right. Here is a new patch with this fixed. Thanks. I played some more with it, and still are still some missing odds and ends: - There is a new option ICU_RULES to CREATE DATABASE, but it is not reflected in \h CREATE DATABASE. sql_help_CREATE_DATABASE() needs to be amended. - There is no way to show the rules except by querying "pg_collation" or "pg_database". I think it would be good to show the rules with \dO+ and \l+. - If I create a collation "x" with RULES and then create a database with "ICU_LOCALE x", the rules are not copied over. I don't know if that is intended or not, but it surprises me. Should that be a WARNING? Or, since creating a database with a collation that does not exist in "template0" doesn't make much sense (or does it?), is there a way to forbid that? Yours, Laurenz Albe
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Mon, 2023-02-13 at 16:33 -0800, Andres Freund wrote:
> On 2023-02-05 18:24:03 +0100, Laurenz Albe wrote:
> > Anyway, attached is v7 that tries to do it that way.
>
> This consistently fails on CI:
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3962
>
> https://api.cirrus-ci.com/v1/artifact/task/5156723929907200/testrun/build/testrun/regress/regress/regression.diffs
Thanks for pointing out.
Here is an improved version.
Yours,
Laurenz Albe
From f55d14a50ab2773a450f5535aebbd4011c129ba1 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Tue, 14 Feb 2023 13:42:37 +0100
Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN
This allows EXPLAIN to generate generic plans for parameterized statements
(that have parameter placeholders like $1 in the statement text).
Invent a new executor flag EXEC_FLAG_EXPLAIN_GENERIC that disables runtime
partition pruning for such plans: that would fail if the non-existing
parameters are involved, and we don't want to remove subplans anyway.
Author: Laurenz Albe
Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at
---
doc/src/sgml/ref/explain.sgml | 15 +
src/backend/commands/explain.c| 11 +++
src/backend/executor/execMain.c | 3 ++
src/backend/executor/execPartition.c | 9 --
src/backend/parser/analyze.c | 29 +
src/include/commands/explain.h| 1 +
src/include/executor/executor.h | 18 +++
src/test/regress/expected/explain.out | 46 +++
src/test/regress/sql/explain.sql | 30 +
9 files changed, 153 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..a1fdd31bc7 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -40,6 +40,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
COSTS [ boolean ]
SETTINGS [ boolean ]
+GENERIC_PLAN [ boolean ]
BUFFERS [ boolean ]
WAL [ boolean ]
TIMING [ boolean ]
@@ -167,6 +168,20 @@ ROLLBACK;
+
+GENERIC_PLAN
+
+
+ Generate a generic plan for the statement (see
+ for details about generic plans). The statement can contain parameter
+ placeholders like $1 (but then it has to be a statement
+ that supports parameters). This option cannot be used together with
+ ANALYZE, since a statement with unknown parameters
+ cannot be executed.
+
+
+
+
BUFFERS
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index fbbf28cf06..1f3055c7af 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -190,6 +190,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
es->settings = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "generic_plan") == 0)
+ es->generic = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -227,6 +229,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
+ if (es->generic && es->analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
+
+ /* check that WAL is used with EXPLAIN ANALYZE */
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -574,6 +583,8 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
eflags = EXEC_FLAG_EXPLAIN_ONLY;
if (into)
eflags |= GetIntoRelEFlags(into);
+ if (es->generic)
+ eflags |= EXEC_FLAG_EXPLAIN_GENERIC;
/* call ExecutorStart to prepare the plan for execution */
ExecutorStart(queryDesc, eflags);
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 39bfb48dc2..6e91b56294 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -131,6 +131,9 @@ static void EvalPlanQualStart(EPQState *epqstate, Plan *planTree);
void
ExecutorStart(QueryDesc *queryDesc, int eflags)
{
+ /* EXEC_FLAG_EXPLAIN_GENERIC can only occur with EXEC_FLAG_EXPLAIN_ONLY */
+ Assert((eflags & EXEC_FLAG_EXPLAIN_ONLY) ||
+ !(eflags & EXEC_FLAG_EXPLAIN_GENERIC));
/*
* In some cases (e.g. an EXECUTE statement) a query execution will skip
* parse analysis, which means that the query_id won't be reported. Note
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 651ad24fc1..7fc2f0d1ab 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -2040,10 +2040,12
Re: Why cann't simplify stable function in planning phase?
On Wed, 2023-02-08 at 16:59 +0800, tender wang wrote: > In evaluate_function(), I find codes as shown below: > > /* > * Ordinarily we are only allowed to simplify immutable functions. But for > * purposes of estimation, we consider it okay to simplify functions that > * are merely stable; the risk that the result might change from planning > * time to execution time is worth taking in preference to not being able > * to estimate the value at all. > */ > if (funcform->provolatile == PROVOLATILE_IMMUTABLE) > /* okay */ ; > else if (context->estimate && funcform->provolatile == PROVOLATILE_STABLE) > /* okay */ ; > else > return NULL; > > The codes say that stable function can not be simplified here(e.g. planning > phase). > I want to know the reason why stable function can not be simplified in > planning phase. > Maybe show me a example that it will be incorrect for a query if simplify > stable function in > planning phases. Query planning and query execution can happen at different times and using different snapshots, so the result of a stable function can change in the meantime. Think of prepared statements using a generic plan. Yours, Laurenz Albe
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Fri, 2023-02-03 at 15:11 -0500, Tom Lane wrote:
> I can think of a couple of possible ways forward:
>
> * Fix things so that the generic parameters appear to have NULL
> values when inspected during executor startup. I'm not sure
> how useful that'd be though. In partition-pruning cases that'd
> lead to EXPLAIN (GENERIC_PLAN) showing the plan with all
> partitions pruned, other than the one for NULL values if there
> is one. Doesn't sound too helpful.
>
> * Invent another executor flag that's a "stronger" version of
> EXEC_FLAG_EXPLAIN_ONLY, and pass that when any generic parameters
> exist, and check it in CreatePartitionPruneState to decide whether
> to do startup pruning. This avoids changing behavior for existing
> cases, but I'm not sure how much it has to recommend it otherwise.
> Skipping the startup prune step seems like it'd produce misleading
> results in another way, ie showing too many partitions not too few.
>
> This whole business of partition pruning dependent on the
> generic parameters is making me uncomfortable. It seems like
> we *can't* show a plan that is either representative of real
> execution or comparable to what you get from more-traditional
> EXPLAIN usage. Maybe we need to step back and think more.
I slept over it, and the second idea now looks like the the right
approach to me. My idea of seeing a generic plan is that plan-time
partition pruning happens, but not execution-time pruning, so that
I get no "subplans removed".
Are there any weird side effects of skipping the startup prune step?
Anyway, attached is v7 that tries to do it that way. It feels fairly
good to me. I invented a new executor flag EXEC_FLAG_EXPLAIN_GENERIC.
To avoid having to change all the places that check EXEC_FLAG_EXPLAIN_ONLY
to also check for the new flag, I decided that the new flag can only be
used as "add-on" to EXEC_FLAG_EXPLAIN_ONLY.
Yours,
Laurenz Albe
From cd0b5a1a4f301bb7fad9088d5763989f5dde4636 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Sun, 5 Feb 2023 18:11:57 +0100
Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN
This allows EXPLAIN to generate generic plans for parameterized statements
(that have parameter placeholders like $1 in the statement text).
Invent a new executor flag EXEC_FLAG_EXPLAIN_GENERIC that disables runtime
partition pruning for such plans: that would fail if the non-existing
parameters are involved, and we don't want to remove subplans anyway.
Author: Laurenz Albe
Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at
---
doc/src/sgml/ref/explain.sgml | 15 +
src/backend/commands/explain.c| 11 +++
src/backend/executor/execMain.c | 3 ++
src/backend/executor/execPartition.c | 9 --
src/backend/parser/analyze.c | 29 +
src/include/commands/explain.h| 1 +
src/include/executor/executor.h | 18 +++
src/test/regress/expected/explain.out | 46 +++
src/test/regress/sql/explain.sql | 29 +
9 files changed, 152 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..a1fdd31bc7 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -40,6 +40,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
COSTS [ boolean ]
SETTINGS [ boolean ]
+GENERIC_PLAN [ boolean ]
BUFFERS [ boolean ]
WAL [ boolean ]
TIMING [ boolean ]
@@ -167,6 +168,20 @@ ROLLBACK;
+
+GENERIC_PLAN
+
+
+ Generate a generic plan for the statement (see
+ for details about generic plans). The statement can contain parameter
+ placeholders like $1 (but then it has to be a statement
+ that supports parameters). This option cannot be used together with
+ ANALYZE, since a statement with unknown parameters
+ cannot be executed.
+
+
+
+
BUFFERS
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 35c23bd27d..37ea4e5035 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -190,6 +190,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
es->settings = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "generic_plan") == 0)
+ es->generic = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -227,6 +229,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
+ if (es->generic && es->analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID
Re: Allow tailoring of ICU locales with custom rules
On Sat, 2023-02-04 at 14:41 +0100, Daniel Verite wrote: > Laurenz Albe wrote: > > > Cool so far. Now I created a database with that locale: > > > > CREATE DATABASE teutsch LOCALE_PROVIDER icu ICU_LOCALE german_phone > > LOCALE "de_AT.utf8" TEMPLATE template0; > > > > Now the rules are not in "pg_database": > > The parameter after ICU_LOCALE is passed directly to ICU as a locale > ID, as opposed to refering a collation name in the current database. > This CREATE DATABASE doesn't fail because ICU accepts pretty much > anything as a locale ID, ignoring what it can't parse instead of > erroring out. > > I think the way to express what you want should be: > > CREATE DATABASE teutsch > LOCALE_PROVIDER icu > ICU_LOCALE 'de_AT' > LOCALE 'de_AT.utf8' > ICU_RULES ' < g'; > > However it still leaves "daticurules" empty in the destination db, > because of an actual bug in the current patch. I see. Thanks for the explanation. > > I guess that it is not the fault of this patch that the collation > > isn't there, but I think it is surprising. What good is a database > > collation that does not exist in the database? > > Even if the above invocation of CREATE DATABASE worked as you > intuitively expected, by getting the characteristics from the > user-defined collation for the destination db, it still wouldn't work to > refer > to COLLATE "german_phone" in the destination database. > That's because there would be no "german_phone" entry in the > pg_collation of the destination db, as it's cloned from the template > db, which has no reason to have this collation. That makes sense. Then I think that the current behavior is buggy: You should not be allowed to specify a collation that does not exist in the template database. Otherwise you end up with something weird. This is not the fault of this patch though. Yours, Laurenz Albe
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Fri, 2023-02-03 at 09:44 -0500, Tom Lane wrote:
> Laurenz Albe writes:
> > I played around with it, and I ran into a problem with partitions that
> > are foreign tables:
> > ...
> > EXPLAIN (GENERIC_PLAN) SELECT * FROM looppart WHERE key = $1;
> > ERROR: no value found for parameter 1
>
> Hmm, offhand I'd say that something is doing something it has no
> business doing when EXEC_FLAG_EXPLAIN_ONLY is set (that is, premature
> evaluation of an expression). I wonder whether this failure is
> reachable without this patch.
Thanks for the pointer. Perhaps something like the attached?
The changes in "CreatePartitionPruneState" make my test case work,
but they cause a difference in the regression tests, which would be
intended if we have no partition pruning with plain EXPLAIN.
Yours,
Laurenz Albe
From ff16316aab8e5f5de84ae925e965a210d4368b75 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Fri, 3 Feb 2023 17:08:53 +0100
Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN
This allows EXPLAIN to generate generic plans for parameterized
statements (that have parameter placeholders like $1 in the
statement text).
Author: Laurenz Albe
Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at
---
doc/src/sgml/ref/explain.sgml | 15 ++
src/backend/commands/explain.c| 9 +
src/backend/executor/execPartition.c | 6 --
src/backend/parser/analyze.c | 29 +++
src/include/commands/explain.h| 1 +
src/test/regress/expected/explain.out | 14 +
src/test/regress/sql/explain.sql | 5 +
7 files changed, 77 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..58350624e7 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -40,6 +40,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
COSTS [ boolean ]
SETTINGS [ boolean ]
+GENERIC_PLAN [ boolean ]
BUFFERS [ boolean ]
WAL [ boolean ]
TIMING [ boolean ]
@@ -167,6 +168,20 @@ ROLLBACK;
+
+GENERIC_PLAN
+
+
+ Generate a generic plan for the statement (see
+ for details about generic plans). The statement can contain parameter
+ placeholders like $1, if it is a statement that can
+ use parameters. This option cannot be used together with
+ ANALYZE, since a statement with unknown parameters
+ cannot be executed.
+
+
+
+
BUFFERS
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 35c23bd27d..ab21a67862 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -190,6 +190,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
es->settings = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "generic_plan") == 0)
+ es->generic = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -227,6 +229,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
+ if (es->generic && es->analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
+
+ /* check that WAL is used with EXPLAIN ANALYZE */
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 651ad24fc1..38d14433a6 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -2043,7 +2043,8 @@ CreatePartitionPruneState(PlanState *planstate, PartitionPruneInfo *pruneinfo)
* Initialize pruning contexts as needed.
*/
pprune->initial_pruning_steps = pinfo->initial_pruning_steps;
- if (pinfo->initial_pruning_steps)
+ if (pinfo->initial_pruning_steps &&
+!(econtext->ecxt_estate->es_top_eflags & EXEC_FLAG_EXPLAIN_ONLY))
{
InitPartitionPruneContext(>initial_context,
pinfo->initial_pruning_steps,
@@ -2053,7 +2054,8 @@ CreatePartitionPruneState(PlanState *planstate, PartitionPruneInfo *pruneinfo)
prunestate->do_initial_prune = true;
}
pprune->exec_pruning_steps = pinfo->exec_pruning_steps;
- if (pinfo->exec_pruning_steps)
+ if (pinfo->exec_pruning_steps &&
+!(econtext->ecxt_estate->es_top_eflags & EXEC_FLAG_EXPLAIN_ONLY))
{
InitPartitionPruneContext(>exec_context,
pinfo->exec_pruning_steps,
diff --git a/src/backe
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Tue, 2023-01-31 at 13:49 -0500, Tom Lane wrote: > Laurenz Albe writes: > > [ 0001-Add-EXPLAIN-option-GENERIC_PLAN.v4.patch ] > > I took a closer look at this patch, and didn't like the implementation > much. You're not matching the behavior of PREPARE at all: for example, > this patch is content to let $1 be resolved with different types in > different places. We should be using the existing infrastructure that > parse_analyze_varparams uses. > > Also, I believe that in contexts such as plpgsql, it is possible that > there's an external source of $N definitions, which we should probably > continue to honor even with GENERIC_PLAN. > > So that leads me to think the code should be more like this. I'm not > sure if it's worth spending documentation and testing effort on the > case where we don't override an existing p_paramref_hook. Thanks, that looks way cleaner. I played around with it, and I ran into a problem with partitions that are foreign tables: CREATE TABLE loc1 (id integer NOT NULL, key integer NOT NULL CHECK (key = 1), value text); CREATE TABLE loc2 (id integer NOT NULL, key integer NOT NULL CHECK (key = 2), value text); CREATE TABLE looppart (id integer GENERATED ALWAYS AS IDENTITY, key integer NOT NULL, value text) PARTITION BY LIST (key); CREATE FOREIGN TABLE looppart1 PARTITION OF looppart FOR VALUES IN (1) SERVER loopback OPTIONS (table_name 'loc1'); CREATE FOREIGN TABLE looppart2 PARTITION OF looppart FOR VALUES IN (2) SERVER loopback OPTIONS (table_name 'loc2'); EXPLAIN (GENERIC_PLAN) SELECT * FROM looppart WHERE key = $1; ERROR: no value found for parameter 1 The solution could be to set up a dynamic parameter hook in the ExprContext in ecxt_param_list_info->paramFetch so that ExecEvalParamExtern doesn't complain about the missing parameter. Does that make sense? How do I best hook into the executor to set that up? Yours, Laurenz Albe
Re: pg_dump versus hash partitioning
On Wed, 2023-02-01 at 17:49 -0500, Tom Lane wrote: > Robert Haas writes: > > On Wed, Feb 1, 2023 at 5:08 PM Tom Lane wrote: > > > I can agree with that argument for range or list partitioning, where > > > the partitions have some semantic meaning to the user. I don't buy it > > > for hash partitioning. It was an implementation artifact to begin > > > with that a given row ended up in partition 3 not partition 11, so why > > > would users care which partition it ends up in after a dump/reload? > > > If they think there is a difference between the partitions, they need > > > education. > > > I see your point. I think it's somewhat valid. However, I also think > > it muddies the definition of what pg_dump is allowed to do in a way > > that I do not like. I think there's a difference between the CTID or > > XMAX of a tuple changing and it ending up in a totally different > > partition. It feels like it makes the definition of correctness > > subjective: we do think that people care about range and list > > partitions as individual entities, so we'll put the rows back where > > they were and complain if we can't, but we don't think they think > > about hash partitions that way, so we will err on the side of making > > the dump restoration succeed. That's a level of guessing what the user > > meant that I think is uncomfortable. > > I see your point too, and to me it's evidence for the position that > we should never have done hash partitioning in the first place. You suggested earlier to deprecate hash partitioning. That's a bit much, but I'd say that most use cases of hash partitioning that I can imagine would involve integers. We could warn against using hash partitioning for data types other than numbers and date/time in the documentation. I also understand the bad feeling of changing partitions during a dump/restore, but I cannot think of a better way out. > What do you think of "--load-via-partition-root=on/off/auto", where > auto means "not with hash partitions" or the like? That's perhaps the best way. So users who know that their hash partitions won't change and want the small speed benefit can have it. Yours, Laurenz Albe
Re: Allow tailoring of ICU locales with custom rules
On Mon, 2023-01-16 at 12:18 +0100, Peter Eisentraut wrote:
> Updated patch attached.
I like that patch. It applies and passes regression tests.
I played with it:
CREATE COLLATION german_phone (LOCALE = 'de-AT', PROVIDER = icu, RULES = '
< ö');
SELECT * FROM (VALUES ('od'), ('oe'), ('of'), ('p'), ('ö')) AS q(c)
ORDER BY c COLLATE german_phone;
c
od
oe
ö
of
p
(5 rows)
Cool so far. Now I created a database with that locale:
CREATE DATABASE teutsch LOCALE_PROVIDER icu ICU_LOCALE german_phone
LOCALE "de_AT.utf8" TEMPLATE template0;
Now the rules are not in "pg_database":
SELECT datcollate, daticulocale, daticurules FROM pg_database WHERE datname =
'teutsch';
datcollate │ daticulocale │ daticurules
╪══╪═
de_AT.utf8 │ german_phone │ ∅
(1 row)
I connect to the database and try:
SELECT * FROM (VALUES ('od'), ('oe'), ('of'), ('p'), ('ö')) AS q(c)
ORDER BY c COLLATE german_phone;
ERROR: collation "german_phone" for encoding "UTF8" does not exist
LINE 1: ... ('oe'), ('of'), ('p'), ('ö')) AS q(c) ORDER BY c COLLATE ge...
^
Indeed, the collation isn't there...
I guess that it is not the fault of this patch that the collation isn't there,
but I think it is surprising. What good is a database collation that does not
exist in the database?
What might be the fault of this patch, however, is that "daticurules" is not
set in "pg_database". Looking at the code, that column seems to be copied
from the template database, but cannot be overridden.
Perhaps this only needs more documentation, but I am confused.
Yours,
Laurenz Albe
Re: Something is wrong with wal_compression
On Fri, 2023-01-27 at 16:15 +1300, Thomas Munro wrote: > On Fri, Jan 27, 2023 at 3:04 PM Tom Lane wrote: > > Thomas Munro writes: > > > On Fri, Jan 27, 2023 at 1:30 PM Michael Paquier > > > wrote: > > > > My opinion would be to make this function more reliable, FWIW, even if > > > > that involves a performance impact when called in a close loop by > > > > forcing more WAL flushes to ensure its report durability and > > > > consistency. > > > > > Yeah, the other thread has a patch for that. But it would hurt some > > > workloads. > > > > I think we need to get the thing correct first and worry about > > performance later. What's wrong with simply making pg_xact_status > > write and flush a record of the XID's existence before returning it? > > Yeah, it will cost you if you use that function, but not if you don't. > > There is no > doubt that the current situation is unacceptable, though, so maybe we > really should just do it and make a faster one later. Anyone else > want to vote on this? I wasn't aware of the existence of pg_xact_status, so I suspect that it is not a widely known and used feature. After reading the documentation, I'd say that anybody who uses it will want it to give a reliable answer. So I'd agree that it is better to make it more expensive, but live up to its promise. Yours, Laurenz Albe
Re: document the need to analyze partitioned tables
On Wed, 2023-01-25 at 16:26 +1300, David Rowley wrote: > On Wed, 18 Jan 2023 at 22:15, Laurenz Albe wrote: > > Attached is a new version of my patch that tries to improve the wording. > > I had a look at this and agree that we should adjust the paragraph in > question if people are finding it confusing. > > I started to adjust that but since the text is fairly short it turned > out quite different from what you had. > > I ended up with: > > + With partitioned tables, since these do not directly store tuples, these > + do not require autovacuum to perform any VACUUM > + operations. Autovacuum simply performs a VACUUM on > the > + partitioned table's partitions the same as it does with normal tables. > + However, the same is true for ANALYZE operations, and > + this can be problematic as there are various places in the query planner > + that attempt to make use of table statistics for partitioned tables when > + partitioned tables are queried. For now, you can work around this > problem > + by running a manual ANALYZE command on the partitioned > + table when the partitioned table is first populated, and again whenever > + the distribution of data in its partitions changes significantly. > > which I've also attached in patch form. > > I know there's been a bit of debate on the wording and a few patches, > so I may not be helping. If nobody is against the above, then I don't > mind going ahead with it and backpatching to whichever version this > first applies to. I just felt I wasn't 100% happy with what was being > proposed. Thanks, your help is welcome. Did you see Justin's wording suggestion in https://postgr.es/m/20230118174919.GA9837%40telsasoft.com ? He didn't attach it as a patch, so you may have missed it. I was pretty happy with that. I think your first sentence it a bit clumsy and might be streamlined to Partitioned tables do not directly store tuples and consequently do not require autovacuum to perform any VACUUM operations. Also, I am a little bit unhappy about 1. Your paragraph states that partitioned table need no autovacuum, but doesn't state unmistakably that they will never be treated by autovacuum. 2. You make a distinction between table partitions and "normal tables", but really there is no distiction. Perhaps I am being needlessly picky here... Yours, Laurenz Albe
Re: Mutable CHECK constraints?
On Tue, 2023-01-24 at 01:38 -0500, Tom Lane wrote: > Laurenz Albe writes: > > We throw an error if the expression in a CREATE INDEX statement is not > > IMMUTABLE. > > But while the documentation notes that expressions in CHECK constraints are > > not > > to be immutable, we don't enforce that. Why don't we call something like > > CheckMutability inside cookConstraint? Sure, that wouldn't catch all abuse, > > but it would be better than nothing. > > > There is of course the worry of breaking upgrade for unsafe constraints, > > but is > > there any other reason not to enforce immutability? > > Yeah, that's exactly it, it's a historical exemption for compatibility > reasons. There are discussions about this in the archives, if memory > serves ... but I'm too tired to go digging. Thanks for the answer. A search turned up https://postgr.es/m/AANLkTikwFfvavEX9nDwcRD4_xJb_VAitMeP1IH4wpGIt%40mail.gmail.com Yours, Laurenz Albe
Mutable CHECK constraints?
We throw an error if the expression in a CREATE INDEX statement is not IMMUTABLE. But while the documentation notes that expressions in CHECK constraints are not to be immutable, we don't enforce that. Why don't we call something like CheckMutability inside cookConstraint? Sure, that wouldn't catch all abuse, but it would be better than nothing. There is of course the worry of breaking upgrade for unsafe constraints, but is there any other reason not to enforce immutability? Yours, Laurenz Albe
Re: ***Conflict with recovery error***
On Fri, 2023-01-20 at 09:59 +, Abhishek Prakash wrote: > We had set max_standby_streaming_delay = -1, but faced storage issues > nearly 3.5 TB of storage was consumed. Then either don't run queries that take that long or run fewer data modifications on the primary. Or invest in a few more TB disk storage. Yours, Laurenz Albe
Re: ***Conflict with recovery error***
On Fri, 2023-01-20 at 08:56 +, Abhishek Prakash wrote: > We are facing below issue with read replica we did work arounds by setting > hot_standby_feedback, max_standby_streaming_delay and > max_standby_archive_delay, > which indeed caused adverse effects on primary DB and storage. As our DB is > nearly 6 TB which runs as AWS Postgres RDS. > > Even the below error occurs on tables where vacuum is disabled and no DML > operations are permitted. Will there be any chances to see row versions > being changed even if vacuum is disabled. > Please advise. > > 2023-01-13 07:20:12 > UTC:10.64.103.75(61096):ubpreplica@ubprdb01:[17707]:ERROR: canceling > statement due to conflict with recovery > 2023-01-13 07:20:12 > UTC:10.64.103.75(61096):ubpreplica@ubprdb01:[17707]:DETAIL: User query might > have needed to see row versions that must be removed. It could be HOT chain pruning or an anti-wraparound autovacuum (which runs even if autovacuum is disabled). Disabling autovacuum is not a smart idea to begin with. Your best bet is to set "max_standby_streaming_delay = -1". More reading: https://www.cybertec-postgresql.com/en/streaming-replication-conflicts-in-postgresql/ Yours, Laurenz Albe
Re: document the need to analyze partitioned tables
On Thu, 2023-01-19 at 15:56 -0500, Bruce Momjian wrote: > On Thu, Jan 19, 2023 at 01:50:05PM +0100, Laurenz Albe wrote: > > On Wed, 2023-01-18 at 16:23 -0500, Bruce Momjian wrote: > > > Is it possible to document when partition table statistics helps? > > > > I think it would be difficult to come up with an exhaustive list. > > I was afraid of that. I asked only because most people assume > autovacuum handles _all_ statistics needs, but this case is not handled. > Do people even have any statistics maintenance process anymore, and if > not, how would they know they need to run a manual ANALYZE? Probably not. I think this would warrant an entry in the TODO list: "make autovacuum collect startistics for partitioned tables". Even if we cannot give better advice than "run ALANYZE manually if the execution plan looks fishy", the patch is still an improvement, isn't it? I have already seen several questions by people who read the current documentation and were worried that autovacuum wouldn't clean up their partitioned tables. Yours, Laurenz Albe
Re: minor bug
On Wed, 2023-01-18 at 15:03 -0500, Tom Lane wrote: > Laurenz Albe writes: > > On Tue, 2023-01-17 at 10:32 -0500, Tom Lane wrote: > > > I seem to recall that the original idea was to report the timestamp > > > of the commit/abort record we are stopping at. Maybe my memory is > > > faulty, but I think that'd be significantly more useful than the > > > current behavior, *especially* when the replay stopping point is > > > defined by something other than time. > > > (Also, the wording of the log message suggests that that's what > > > the reported time is supposed to be. I wonder if somebody messed > > > this up somewhere along the way.) > > > recoveryStopTime is set to recordXtime (the time of the xlog record) > > a few lines above that patch, so this is useful information if it is > > present. > > Ah, but that only happens if recoveryTarget == RECOVERY_TARGET_TIME. > Digging in the git history, I see that this did use to work as > I remember: we always extracted the record time before printing it. > That was accidentally broken in refactoring in c945af80c. I think > the correct fix is more like the attached. Yes, you are right. Your patch looks fine to me. Yours, Laurenz Albe
Re: document the need to analyze partitioned tables
On Wed, 2023-01-18 at 16:23 -0500, Bruce Momjian wrote: > Is it possible to document when partition table statistics helps? I think it would be difficult to come up with an exhaustive list. Yours, Laurenz Albe
Re: document the need to analyze partitioned tables
On Wed, 2023-01-18 at 11:49 -0600, Justin Pryzby wrote: > I tweaked this a bit to end up with: > > > - Partitioned tables are not processed by autovacuum. Statistics > > - should be collected by running a manual ANALYZE > > when it is > > + The leaf partitions of a partitioned table are normal tables and are > > processed > > + by autovacuum; however, autovacuum does not process the partitioned > > table itself. > > + This is no problem as far as VACUUM is concerned, > > since > > + there's no need to vacuum the empty, partitioned table. But, as > > mentioned in > > + , it also means that autovacuum > > won't > > + run ANALYZE on the partitioned table. > > + Although statistics are automatically gathered on its leaf partitions, > > some queries also need > > + statistics on the partitioned table to run optimally. You should > > collect statistics by > > + running a manual ANALYZE when the partitioned table > > is > > first populated, and again whenever the distribution of data in its > > partitions changes significantly. > > > > "partitions are normal tables" was techically wrong, as partitions can > also be partitioned. I am fine with your tweaks. I think this is good to go. Yours, Laurenz Albe
Re: document the need to analyze partitioned tables
On Tue, 2023-01-17 at 16:16 -0500, Bruce Momjian wrote: > On Tue, Jan 17, 2023 at 03:00:50PM -0600, Justin Pryzby wrote: > > Maybe (all?) the clarification the docs need is to say: > > "Partitioned tables are not *themselves* processed by autovacuum." > > Yes, I think the lack of autovacuum needs to be specifically mentioned > since most people assume autovacuum handles _all_ statistics updating. > > Can someone summarize how bad it is we have no statistics on partitioned > tables? It sounds bad to me. Andrey Lepikhov had an example earlier in this thread[1]. It doesn't take an exotic query. Attached is a new version of my patch that tries to improve the wording. Yours, Laurenz Albe [1]: https://postgr.es/m/3df5c68b-13aa-53d0-c0ec-ed98e6972e2e%40postgrespro.ru From 53da8083556364490d42077492e608152f9ae02e Mon Sep 17 00:00:00 2001 From: Laurenz Albe Date: Wed, 18 Jan 2023 10:09:21 +0100 Subject: [PATCH] Improve autovacuum doc on partitioned tables The documentation mentioned that autovacuum doesn't process partitioned tables, but it was unclear about the impact. The old wording could be interpreted to mean that there are problems with dead tuple cleanup on partitioned tables. Clarify that the only potential problem is autoanalyze, and that statistics for the partitions will be gathered. Author: Laurenz Albe Reviewed-by: Nathan Bossart, Justin Pryzby Discussion: https://postgr.es/m/1fd81ddc7710a154834030133c6fea41e55c8efb.camel%40cybertec.at --- doc/src/sgml/maintenance.sgml | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml index 759ea5ac9c..3954e797a4 100644 --- a/doc/src/sgml/maintenance.sgml +++ b/doc/src/sgml/maintenance.sgml @@ -860,8 +860,15 @@ analyze threshold = analyze base threshold + analyze scale factor * number of tu -Partitioned tables are not processed by autovacuum. Statistics -should be collected by running a manual ANALYZE when it is +The partitions of a partitioned table are normal tables and get processed +by autovacuum, but autovacuum doesn't process the partitioned table itself. +This is no problem as far as VACUUM is concerned, since +processing the partitions is sufficient. But, as mentioned in +, it also means that autovacuum won't +run ANALYZE on the partitioned table itself. +While statistics are gathered on the partitions, some queries may rely on +the statistics for the partitioned table. You should collect statistics by +running a manual ANALYZE when the partitioned table is first populated, and again whenever the distribution of data in its partitions changes significantly. -- 2.39.0
Re: minor bug
On Tue, 2023-01-17 at 10:32 -0500, Tom Lane wrote:
> Laurenz Albe writes:
> > On Mon, 2023-01-16 at 19:59 +0100, Torsten Förtsch wrote:
> > > So, the timestamp displayed in the log message is certainly wrong.
>
> > If recovery stops at a WAL record that has no timestamp, you get this
> > bogus recovery stop time. I think we should show the recovery stop time
> > only if time was the target, as in the attached patch.
>
> I don't think that is a tremendously useful definition: the user
> already knows what recoveryStopTime is, or can find it out from
> their settings easily enough.
>
> I seem to recall that the original idea was to report the timestamp
> of the commit/abort record we are stopping at. Maybe my memory is
> faulty, but I think that'd be significantly more useful than the
> current behavior, *especially* when the replay stopping point is
> defined by something other than time.
>
> (Also, the wording of the log message suggests that that's what
> the reported time is supposed to be. I wonder if somebody messed
> this up somewhere along the way.)
recoveryStopTime is set to recordXtime (the time of the xlog record)
a few lines above that patch, so this is useful information if it is
present.
I realized that my original patch might be a problem for translation;
here is an updated version that does not take any shortcuts.
Yours,
Laurenz Albe
From b01428486f7795f757edd14f66362ee575d2f168 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Wed, 18 Jan 2023 09:23:50 +0100
Subject: [PATCH] Don't show bogus recovery stop time
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
If target for archive recovery was different from time, the
WAL record that ended recovery might not have a timestamp.
In that case, the log message at the end of recovery would
show a recovery time of midnight on 2000-01-01.
Reported-by: Torsten Förtsch
Author: Laurenz Albe
Discussion: https://postgr.es/m/cakkg4_kuevpqbmyoflajx7opaqk6cvwkvx0hrcfjspfrptx...@mail.gmail.com
---
src/backend/access/transam/xlogrecovery.c | 35 +--
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index bc3c3eb3e7..73929a535e 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -2570,19 +2570,38 @@ recoveryStopsBefore(XLogReaderState *record)
recoveryStopLSN = InvalidXLogRecPtr;
recoveryStopName[0] = '\0';
+ /* this could be simplified, but that might break translatability */
if (isCommit)
{
- ereport(LOG,
- (errmsg("recovery stopping before commit of transaction %u, time %s",
- recoveryStopXid,
- timestamptz_to_str(recoveryStopTime;
+ if (recoveryStopTime != 0)
+ {
+ereport(LOG,
+ (errmsg("recovery stopping before commit of transaction %u, time %s",
+recoveryStopXid,
+timestamptz_to_str(recoveryStopTime;
+ }
+ else
+ {
+ereport(LOG,
+ (errmsg("recovery stopping before commit of transaction %u",
+recoveryStopXid)));
+ }
}
else
{
- ereport(LOG,
- (errmsg("recovery stopping before abort of transaction %u, time %s",
- recoveryStopXid,
- timestamptz_to_str(recoveryStopTime;
+ if (recoveryStopTime != 0)
+ {
+ereport(LOG,
+ (errmsg("recovery stopping before abort of transaction %u, time %s",
+recoveryStopXid,
+timestamptz_to_str(recoveryStopTime;
+ }
+ else
+ {
+ereport(LOG,
+ (errmsg("recovery stopping before abort of transaction %u",
+recoveryStopXid)));
+ }
}
}
--
2.39.0
Re: minor bug
On Mon, 2023-01-16 at 19:59 +0100, Torsten Förtsch wrote:
> not sure if this is known behavior.
>
> Server version is 14.6 (Debian 14.6-1.pgdg110+1).
>
> In a PITR setup I have these settings:
>
> recovery_target_xid = '852381'
> recovery_target_inclusive = 'false'
>
> In the log file I see this message:
>
> LOG: recovery stopping before commit of transaction 852381, time 2000-01-01
> 00:00:00+00
>
> But:
>
> postgres=# select * from pg_last_committed_xact();
> xid | timestamp | roident
> +---+-
> 852380 | 2023-01-16 18:00:35.054495+00 | 0
>
> So, the timestamp displayed in the log message is certainly wrong.
Redirected to -hackers.
If recovery stops at a WAL record that has no timestamp, you get this
bogus recovery stop time. I think we should show the recovery stop time
only if time was the target, as in the attached patch.
Yours,
Laurenz Albe
From 622e52bbd652fc8872448e46c3ca0bc78dd847fe Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Tue, 17 Jan 2023 10:38:40 +0100
Subject: [PATCH] Don't show bogus recovery stop time
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
If target for archive recovery was different from time, the
WAL record that ended recovery might not have a timestamp.
In that case, the log message at the end of recovery would
show a recovery time of midnight on 2000-01-01.
Reported-by: Torsten Förtsch
Author: Laurenz Albe
Discussion: https://postgr.es/m/cakkg4_kuevpqbmyoflajx7opaqk6cvwkvx0hrcfjspfrptx...@mail.gmail.com
---
src/backend/access/transam/xlogrecovery.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index bc3c3eb3e7..ce3718ca2d 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -2570,19 +2570,21 @@ recoveryStopsBefore(XLogReaderState *record)
recoveryStopLSN = InvalidXLogRecPtr;
recoveryStopName[0] = '\0';
- if (isCommit)
+ /* for targets other than time, we might have no stop time */
+ if (recoveryTarget == RECOVERY_TARGET_XID)
{
ereport(LOG,
- (errmsg("recovery stopping before commit of transaction %u, time %s",
+ (errmsg("recovery stopping before %s of transaction %u, time %s",
+ (isCommit ? "commit" : "abort"),
recoveryStopXid,
timestamptz_to_str(recoveryStopTime;
}
else
{
ereport(LOG,
- (errmsg("recovery stopping before abort of transaction %u, time %s",
- recoveryStopXid,
- timestamptz_to_str(recoveryStopTime;
+ (errmsg("recovery stopping before %s of transaction %u",
+ (isCommit ? "commit" : "abort"),
+ recoveryStopXid)));
}
}
--
2.39.0
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Tue, 2022-12-27 at 14:37 -0800, Michel Pelletier wrote:
> I built and tested this patch for review and it works well, although I got
> the following warning when building:
>
> analyze.c: In function 'transformStmt':
> analyze.c:2919:35: warning: 'generic_plan' may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> 2919 | pstate->p_generic_explain = generic_plan;
> | ~~^~
> analyze.c:2909:25: note: 'generic_plan' was declared here
> 2909 | bool generic_plan;
> | ^~~~
Thanks for checking. The variable should indeed be initialized, although
my compiler didn't complain.
Attached is a fixed version.
Yours,
Laurenz Albe
From baf60d9480d8022866d1ed77b00c7b8506f97f70 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Mon, 9 Jan 2023 17:37:40 +0100
Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN
This allows EXPLAIN to generate generic plans for parameterized
statements (that have parameter placeholders like $1 in the
statement text).
Author: Laurenz Albe
Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at
---
doc/src/sgml/ref/explain.sgml | 15 +++
src/backend/commands/explain.c| 9 +
src/backend/parser/analyze.c | 13 +
src/backend/parser/parse_coerce.c | 15 +++
src/backend/parser/parse_expr.c | 16
src/include/commands/explain.h| 1 +
src/include/parser/parse_node.h | 2 ++
src/test/regress/expected/explain.out | 14 ++
src/test/regress/sql/explain.sql | 5 +
9 files changed, 90 insertions(+)
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..221f905a59 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -40,6 +40,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
COSTS [ boolean ]
SETTINGS [ boolean ]
+GENERIC_PLAN [ boolean ]
BUFFERS [ boolean ]
WAL [ boolean ]
TIMING [ boolean ]
@@ -167,6 +168,20 @@ ROLLBACK;
+
+GENERIC_PLAN
+
+
+ Generate a generic plan for the statement (see
+ for details about generic plans). The statement can contain parameter
+ placeholders like $1 and must be a statement that can
+ use parameters. This option cannot be used together with
+ ANALYZE, since a statement with unknown parameters
+ cannot be executed.
+
+
+
+
BUFFERS
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e4621ef8d6..7ee3d24da2 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -190,6 +190,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
es->settings = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "generic_plan") == 0)
+ es->generic = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -227,6 +229,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
+ if (es->generic && es->analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
+
+ /* check that WAL is used with EXPLAIN ANALYZE */
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 5b90974e83..8b56eadf7e 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -27,6 +27,7 @@
#include "access/sysattr.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
@@ -2905,6 +2906,18 @@ static Query *
transformExplainStmt(ParseState *pstate, ExplainStmt *stmt)
{
Query *result;
+ ListCell *lc;
+ bool generic_plan = false;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem*opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "generic_plan") == 0)
+ generic_plan = defGetBoolean(opt);
+ /* don't "break", as we want the last value */
+ }
+ pstate->p_generic_explain = generic_plan;
/* transform contained query, allowing SELECT INTO */
stmt->query = (Node *) transformOptionalSelectInto(pstate, stmt->query);
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 34757da0fa..b9c8dc1a25 100644
--- a
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Tue, 2022-12-06 at 10:17 -0800, Andres Freund wrote:
> On 2022-10-29 10:35:26 +0200, Laurenz Albe wrote:
> > > > Here is a patch that
> > > > implements it with an EXPLAIN option named GENERIC_PLAN.
>
> This fails to build the docs:
>
> https://cirrus-ci.com/task/5609301511766016
>
> [17:47:01.064] ref/explain.sgml:179: parser error : Opening and ending tag
> mismatch: likeral line 179 and literal
> [17:47:01.064] ANALYZE, since a statement with
> unknown parameters
> [17:47:01.064] ^
*blush* Here is a fixed version.
Yours,
Laurenz Albe
From 8704d51f5810619be152ae68faa5743dcf26c7a9 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Fri, 28 Oct 2022 20:58:59 +0200
Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN
This allows EXPLAIN to generate generic plans for parameterized
statements (that have parameter placeholders like $1 in the
statement text).
Author: Laurenz Albe
Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at
---
doc/src/sgml/ref/explain.sgml | 15 +++
src/backend/commands/explain.c| 9 +
src/backend/parser/analyze.c | 13 +
src/backend/parser/parse_coerce.c | 15 +++
src/backend/parser/parse_expr.c | 16
src/include/commands/explain.h| 1 +
src/include/parser/parse_node.h | 2 ++
src/test/regress/expected/explain.out | 14 ++
src/test/regress/sql/explain.sql | 5 +
9 files changed, 90 insertions(+)
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..659d5c51b6 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -40,6 +40,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
COSTS [ boolean ]
SETTINGS [ boolean ]
+GENERIC_PLAN [ boolean ]
BUFFERS [ boolean ]
WAL [ boolean ]
TIMING [ boolean ]
@@ -167,6 +168,20 @@ ROLLBACK;
+
+GENERIC_PLAN
+
+
+ Generate a generic plan for the statement (see
+ for details about generic plans). The statement can contain parameter
+ placeholders like $1 and must be a statement that can
+ use parameters. This option cannot be used together with
+ ANALYZE, since a statement with unknown parameters
+ cannot be executed.
+
+
+
+
BUFFERS
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index f86983c660..7b7ca3f90a 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -190,6 +190,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
es->settings = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "generic_plan") == 0)
+ es->generic = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -227,6 +229,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
+ if (es->generic && es->analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
+
+ /* check that WAL is used with EXPLAIN ANALYZE */
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 6688c2a865..c849765151 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -27,6 +27,7 @@
#include "access/sysattr.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
@@ -2894,6 +2895,18 @@ static Query *
transformExplainStmt(ParseState *pstate, ExplainStmt *stmt)
{
Query *result;
+ ListCell *lc;
+ bool generic_plan;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem*opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "generic_plan") == 0)
+ generic_plan = defGetBoolean(opt);
+ /* don't "break", as we want the last value */
+ }
+ pstate->p_generic_explain = generic_plan;
/* transform contained query, allowing SELECT INTO */
stmt->query = (Node *) transformOptionalSelectInto(pstate, stmt->query);
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index c4e958e4aa..171d8c60a8 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -369,6 +369,21 @@ coerce_type(ParseState *pstate, Node *node,
return res
Re: Patch: Global Unique Index
On Wed, 2022-11-30 at 10:09 +0100, Vik Fearing wrote: > On 11/29/22 17:29, Laurenz Albe wrote: > > On Tue, 2022-11-29 at 13:58 +0100, Vik Fearing wrote: > > > I disagree. A user does not need to know that a table is partitionned, > > > and if the user wants a unique constraint on the table then making them > > > type an extra word to get it is just annoying. > > > > Hmm. But if I created a primary key without thinking too hard about it, > > only to discover later that dropping old partitions has become a problem, > > I would not be too happy either. > > I have not looked at this patch, but my understanding of its design is > the "global" part of the index just makes sure to check a unique index > on each partition. I don't see from that how dropping old partitions > would be a problem. Right, I should have looked closer. But, according to the parallel discussion, ATTACH PARTITION might be a problem. A global index is likely to be a footgun one way or the other, so I think it should at least have a safety on (CREATE PARTITIONED GLOBAL INDEX or something). Yours, Laurenz Albe
Re: Patch: Global Unique Index
On Tue, 2022-11-29 at 13:58 +0100, Vik Fearing wrote: > I disagree. A user does not need to know that a table is partitionned, > and if the user wants a unique constraint on the table then making them > type an extra word to get it is just annoying. Hmm. But if I created a primary key without thinking too hard about it, only to discover later that dropping old partitions has become a problem, I would not be too happy either. Yours, Laurenz Albe
Re: Decoupling antiwraparound autovacuum from special rules around auto cancellation
On Sat, 2022-11-26 at 11:00 -0800, Peter Geoghegan wrote: > > I think that is a good idea. > > Wouldn't it make sense to trigger that at *half* > > "autovacuum_freeze_max_age"? > > That would be equivalent to what I've done here, provided you also > double the autovacuum_freeze_max_age setting. That's exactly what I was trying to debate. Wouldn't it make sense to trigger VACUUM earlier so that it has a chance of being less heavy? On the other hand, if there are not sufficiently many modifications on the table to trigger autovacuum, perhaps it doesn't matter in many cases. > I did it this way > because I believe that it has fewer problems. The approach I took > makes the general perception that antiwraparound autovacuum are a > scary thing (really just needed for emergencies) become true, for the > first time. > > We should expect to see very few antiwraparound autovacuums with the > patch, but when we do see even one it'll be after a less aggressive > approach was given the opportunity to succeed, but (for whatever > reason) failed. Is that really so much less aggressive? Will that autovacuum run want to process all pages that are not all-frozen? If not, it probably won't do much good. If yes, it will be just as heavy as an anti-wraparound autovacuum (except that it won't block other sessions). > Just seeing any antiwraparound autovacuums will become > a useful signal of something being amiss in a way that it just isn't > at the moment. The docs can be updated to talk about antiwraparound > autovacuum as a thing that you should encounter approximately never. > This is possible even though the patch isn't invasive at all. True. On the other hand, it might happen that after this, people start worrying about normal autovacuum runs because they occasionally experience a table age autovacuum that is much heavier than the other ones. And they can no longer tell the reason, because it doesn't show up anywhere. Yours, Laurenz Albe
Re: Decoupling antiwraparound autovacuum from special rules around auto cancellation
On Fri, 2022-11-25 at 14:47 -0800, Peter Geoghegan wrote: > Attached WIP patch invents the idea of a regular autovacuum that is > tasked with advancing relfrozenxid -- which is really just another > trigger criteria, reported on in the server log in its autovacuum > reports. Of course we retain the idea of antiwraparound autovacuums. > The only difference is that they are triggered when table age has > advanced by twice the usual amount, which is presumably only possible > because a regular autovacuum couldn't start or couldn't complete in > time (most likely due to continually being auto-cancelled). > > As I said before, I think that the most important thing is to give > regular autovacuuming a chance to succeed. The exact approach taken > has a relatively large amount of slack, but that probably isn't > needed. So the new antiwraparound cutoffs were chosen because they're > easy to understand and remember, which is fairly arbitrary. The target is a table that receives no DML at all, right? I think that is a good idea. Wouldn't it make sense to trigger that at *half* "autovacuum_freeze_max_age"? Yours, Laurenz Albe
Re: New docs chapter on Transaction Management and related changes
On Tue, 2022-11-22 at 13:50 -0500, Bruce Momjian wrote: > Agreed, updated patch attached. I cannot find any more problems, and I shouldn't mention the extra empty line at the end of the patch. I'd change the commitfest status to "ready for committer" now if it were not already in that status. Yours, Laurenz Albe
Re: Reducing power consumption on idle servers
On Mon, 2022-11-21 at 12:11 -0500, Tom Lane wrote: > Robert Haas writes: > > The reason that I pushed back -- not as successfully as I would have > > liked -- on the changes to pg_stop_backup / pg_start_backup is that I > > know there are people using the old method successfully, and it's not > > just a 1:1 substitution. Here I don't, and it is. I'm totally open to > > the feedback that such people exist and to hearing why adopting one of > > the newer methods would be a problem for them, if that's the case. But > > if there's no evidence that such people exist or that changing is a > > problem for them, I don't think waiting 5 years on principle is good > > for the project. > > We make incompatible changes in every release; see the release notes. > Unless somebody can give a plausible use-case where this'd be a > difficult change to deal with, I concur that we don't need to > deprecate it ahead of time. Since I am the only one that seems to worry, I'll shut up. You are probably right that it the feature won't be missed by many users. Yours, Laurenz Albe
Re: New docs chapter on Transaction Management and related changes
On Fri, 2022-11-18 at 14:28 -0500, Bruce Momjian wrote: > New patch attached. Thanks. > --- a/doc/src/sgml/ref/release_savepoint.sgml > +++ b/doc/src/sgml/ref/release_savepoint.sgml > + RELEASE SAVEPOINT releases the named savepoint and > + all active savepoints that were created after the named savepoint, > + and frees their resources. All changes made since the creation of the > + savepoint, excluding rolled back savepoints changes, are merged into > + the transaction or savepoint that was active when the named savepoint > + was created. Changes made after RELEASE SAVEPOINT > + will also be part of this active transaction or savepoint. I am not sure if "rolled back savepoints changes" is clear enough. I understand that you are trying to avoid the term "subtransaction". How about: All changes made since the creation of the savepoint that didn't already get rolled back are merged ... > --- a/doc/src/sgml/ref/rollback.sgml > +++ b/doc/src/sgml/ref/rollback.sgml > > + If AND CHAIN is specified, a new (unaborted) *Sigh* I'll make one last plea for "not aborted". > --- /dev/null > +++ b/doc/src/sgml/xact.sgml > + > + Transactions can be created explicitly using BEGIN > + and COMMIT, which creates a transaction block. > + An SQL statement outside of a transaction block automatically uses > + a single-statement transaction. > + Sorry, I should have noted that the first time around. Transactions are not created using COMMIT. Perhaps we should also avoid the term "transaction block". Even without speaking of a "block", way too many people confuse PL/pgSQL's BEGIN ... END blocks with transactions. On the other hand, we use "transaction block" everywhere else in the documentation... How about: Multi-statement transactions can be created explicitly using BEGIN or START TRANSACTION and are ended using COMMIT or ROLLBACK. An SQL statement outside of a transaction block automatically uses a single-statement transaction. > + > + > + Transactions and Locking > + > + > + The transaction IDs of currently executing transactions are shown in > + pg_locks > + in columns virtualxid and > + transactionid. Read-only transactions > + will have virtualxids but NULL > + transactionids, while read-write transactions > + will have both as non-NULL. > + Perhaps the following will be prettier than "have both as non-NULL": ..., while both columns will be set in read-write transactions. Yours, Laurenz Albe
Re: Reducing power consumption on idle servers
On Mon, 2022-11-21 at 07:36 +, Simon Riggs wrote: > On Mon, 21 Nov 2022 at 05:07, Laurenz Albe wrote: > > > > On Mon, 2022-11-21 at 10:13 +1300, Thomas Munro wrote: > > > I'll wait 24 hours before committing, to > > > provide a last chance for anyone who wants to complain about dropping > > > promote_trigger_file. > > > > Remove "promote_trigger_file"? Now I have never seen anybody use that > > parameter, but I don't think that it is a good idea to deviate from our > > usual standard of deprecating a feature for about five years before > > actually removing it. > > We aren't removing the ability to promote, just enforcing a change to > a better mechanism, hence I don't see a reason for a long(er) > deprecation period than we have already had. We have had a deprecation period? I looked at the documentation, but found no mention of a deprecation. How hard can it be to leave the GUC and only poll for the existence of the file if it is set? I personally don't need the GUC, and I know nobody who does, but I think we should not be cavalier about introducing unnecessary compatibility breaks. Yours, Laurenz Albe
Re: Reducing power consumption on idle servers
On Mon, 2022-11-21 at 11:42 +0530, Bharath Rupireddy wrote: > On Mon, Nov 21, 2022 at 10:37 AM Laurenz Albe > wrote: > > > > On Mon, 2022-11-21 at 10:13 +1300, Thomas Munro wrote: > > > I'll wait 24 hours before committing, to > > > provide a last chance for anyone who wants to complain about dropping > > > promote_trigger_file. > > > > Remove "promote_trigger_file"? Now I have never seen anybody use that > > parameter, but I don't think that it is a good idea to deviate from our > > usual standard of deprecating a feature for about five years before > > actually removing it. > > I'm not sure what the guidelines are here, however years have gone by > since pg_ctl promote [1] in 2011 and pg_promote() [2] in 2018 were > added. With two such alternatives in place for many years, it was sort > of an undeclared deprecation of promote_trigger_file GUC. And the > changes required to move to newer ways from the GUC aren't that hard > for those who're still relying on the GUC. Therefore, I think it's now > time for us to do away with the GUC. I disagree. With the same argument, you could rip out "serial", since we have supported identity columns since v11. I understand the desire to avoid needless wakeups, but isn't it possible to only perform the regular poll if "promote_trigger_file" is set? Yours, Laurenz Albe
Re: Reducing power consumption on idle servers
On Mon, 2022-11-21 at 10:13 +1300, Thomas Munro wrote: > I'll wait 24 hours before committing, to > provide a last chance for anyone who wants to complain about dropping > promote_trigger_file. Remove "promote_trigger_file"? Now I have never seen anybody use that parameter, but I don't think that it is a good idea to deviate from our usual standard of deprecating a feature for about five years before actually removing it. Yours, Laurenz Albe
Re: New docs chapter on Transaction Management and related changes
On Thu, 2022-11-10 at 12:17 +0100, Alvaro Herrera wrote: > On 2022-Nov-10, Laurenz Albe wrote: > > On Wed, 2022-11-09 at 09:16 -0500, Robert Treat wrote: > > > > > - If AND CHAIN is specified, a new > > > > > transaction is > > > > > + If AND CHAIN is specified, a new unaborted > > > > > transaction is > > > > > immediately started with the same transaction characteristics > > > > > (see > > > > linkend="sql-set-transaction"/>) as the just finished one. > > > > > Otherwise, > > > > > no new transaction is started. > > > > A new transaction is never aborted in my understanding. Being aborted > > is not a characteristic of a transaction, but a state. > > I agree, but maybe it's good to make the point explicit, because it > doesn't seem obvious. Perhaps something like > > "If X is specified, a new transaction (never in aborted state) is > immediately started with the same transaction characteristics (see X) as > the just finished one. Otherwise ..." > > Getting the wording of that parenthical comment right is tricky, though. > What I propose above is not great, but I don't know how to make it > better. Other ideas that seem slightly worse but may inspire someone: > > ... a new transaction (which is never in aborted state) is ... > ... a new transaction (not in aborted state) is ... > ... a new transaction (never aborted, even if the previous is) is ... > ... a new (not-aborted) transaction is ... > ... a new (never aborted) transaction is ... > ... a new (never aborted, even if the previous is) transaction is ... > ... a new (never aborted, regardless of the status of the previous one) > transaction is ... > > > Maybe there's a way to reword the entire phrase that leads to a better > formulation of the idea. Any of your auggestions is better than "unaborted". How about: If AND CHAIN is specified, a new transaction is immediately started with the same transaction characteristics (see ) as the just finished one. This new transaction won't be in the aborted state, even if the old transaction was aborted. Yours, Laurenz Albe
Re: New docs chapter on Transaction Management and related changes
On Wed, 2022-11-09 at 09:16 -0500, Robert Treat wrote: > On Mon, Nov 7, 2022 at 5:04 PM Laurenz Albe wrote: > > Some comments: > > > > > > --- a/doc/src/sgml/ref/release_savepoint.sgml > > > +++ b/doc/src/sgml/ref/release_savepoint.sgml > > > @@ -34,23 +34,16 @@ RELEASE [ SAVEPOINT ] > > > savepoint_name > > > Description > > > > > > > > > - RELEASE SAVEPOINT destroys a savepoint previously > > > defined > > > - in the current transaction. > > > + RELEASE SAVEPOINT will subcommit the subtransaction > > > + established by the named savepoint, if one exists. This will release > > > + any resources held by the subtransaction. If there were any > > > + subtransactions of the named savepoint, these will also be > > > subcommitted. > > > > > > > > > > > > > "Subtransactions of the named savepoint" is somewhat confusing; how about > > "subtransactions of the subtransaction established by the named savepoint"? > > > > If that is too long and explicit, perhaps "subtransactions of that > > subtransaction". > > Personally, I think these are more confusing. Perhaps. I was worried because everywhere else, the wording makes a clear distinction between a savepoint and the subtransaction created by a savepoint. But perhaps some sloppiness is better to avoid such word cascades. > > > --- a/doc/src/sgml/ref/rollback.sgml > > > +++ b/doc/src/sgml/ref/rollback.sgml > > > @@ -56,11 +56,14 @@ ROLLBACK [ WORK | TRANSACTION ] [ AND [ NO ] CHAIN ] > > > AND CHAIN > > > > > > > > > - If AND CHAIN is specified, a new transaction is > > > + If AND CHAIN is specified, a new unaborted > > > transaction is > > > immediately started with the same transaction characteristics (see > > > > > linkend="sql-set-transaction"/>) as the just finished one. > > > Otherwise, > > > no new transaction is started. > > > > I don't think that is an improvement. "Unaborted" is an un-word. A new > > transaction > > is always "unaborted", isn't it? > > I thought about this as well when reviewing it, but I do think > something is needed for the case where you have a transaction which > has suffered an error and then you issue "rollback and chain"; if you > just say "a new transaction is immediately started with the same > transaction characteristics" it might imply to some the new > transaction has some kind of carry over of the previous broken > transaction... the use of the word unaborted makes it clear that the > new transaction is 100% functional. A new transaction is never aborted in my understanding. Being aborted is not a characteristic of a transaction, but a state. > > > > > > > + The internal transaction ID type xid is 32-bits wide > > > > There should be no hyphen in "32 bits wide", just as in "3 years old". > > Minor aside, we should clean up glossary.sgml as well. Right, it has this: The numerical, unique, sequentially-assigned identifier that each transaction receives when it first causes a database modification. Frequently abbreviated as xid. When stored on disk, xids are only 32-bits wide, so only approximately four billion write transaction IDs can be generated; to permit the system to run for longer than that, epochs are used, also 32 bits wide. Which reminds me that I should have suggested rather than where I complained about the use of . > > > > + Up to > > + 64 open subxids are cached in shared memory for each backend; after > > + that point, the overhead increases significantly since we must look > > + up subxid entries in pg_subtrans. > > > > Comma before "since". Perhaps you should mention that this means disk I/O. > > ISTR that you only use a comma before since in cases where the > preceding thought contains a negative. Not being a native speaker, I'll leave that to those who are; I went by feeling. > In any case, are you thinking something like this: > > " 64 open subxids are cached in shared memory for each backend; after > that point the overhead increases significantly due to additional disk I/O > from looking up subxid entries in pg_subtrans." Yes. Yours, Laurenz Albe
Re: New docs chapter on Transaction Management and related changes
On Mon, 2022-11-07 at 22:43 -0500, Bruce Momjian wrote: > > I thought some more about the patch, and I don't like the title > > "Transaction Management" for the new chapter. I'd expect some more > > from a chapter titled "Internals" / "Transaction Management". > > > > In reality, the new chapter is about transaction IDs. So perhaps the > > name should reflect that, so that it does not mislead the reader. > > I renamed it to "Transaction Processing" since we also cover locking and > subtransactions. How is that? It is better. Did you take my suggestions from [1] into account in your latest cumulative patch in [2]? Otherwise, it will be difficult to integrate both. Yours, Laurenz Albe [1]: https://postgr.es/m/3603e6e85544daa5300c7106c31bc52673711cd0.camel%40cybertec.at [2]: https://postgr.es/m/Y2nP04/3BHQOviVB%40momjian.us
Re: New docs chapter on Transaction Management and related changes
On Mon, 2022-11-07 at 23:04 +0100, Laurenz Albe wrote: > On Sat, 2022-11-05 at 10:08 +, Simon Riggs wrote: > > Agreed; new compilation patch attached, including mine and then > > Robert's suggested rewordings. > > Thanks. There is clearly a lot of usefule information in this. > > Some comments: [...] I thought some more about the patch, and I don't like the title "Transaction Management" for the new chapter. I'd expect some more from a chapter titled "Internals" / "Transaction Management". In reality, the new chapter is about transaction IDs. So perhaps the name should reflect that, so that it does not mislead the reader. Yours, Laurenz Albe
Re: New docs chapter on Transaction Management and related changes
l" chapter. > --- /dev/null > +++ b/doc/src/sgml/xact.sgml + Transaction Management + The word transaction is often abbreviated as "xact". Should use here. > + Transactions and Identifiers > + > +Once a transaction writes to the database, it is assigned a > +non-virtual TransactionId (or xid), > +e.g., 278394. Xids are assigned sequentially > +using a global counter used by all databases within the > +PostgreSQL cluster. This property is used by > +the transaction system to order transactions by their first database > +write, i.e., lower-numbered xids started writing before higher-numbered > +xids. Of course, transactions might start in a different order. > + "This property"? How about: "Because transaction IDs are assigned sequentially, the transaction system can use them to order transactions by their first database write" I would want some additional information here: why does the transaction system have to order transactions by their first database write? "Of course, transactions might start in a different order." Now that confuses me. Are you saying that BEGIN could be in a different order than the first database write? Perhaps like this: "Note that the order in which transactions perform their first database write might be different from the order in which the transactions started." > +The internal transaction ID type xid is 32-bits wide There should be no hyphen in "32 bits wide", just as in "3 years old". > +A 32-bit epoch is incremented during each > +wrap around. We usually call this "wraparound" without a space. > + There is also a 64-bit type xid8 which > +includes this epoch and therefore does not wrap around during the > +life of an installation and can be converted to xid by casting. Running "and"s. Better: "There is also ... and does not wrap ... life of an installation. xid8 can be converted to xid by casting." > + Xids are used as the > +basis for PostgreSQL's +linkend="mvcc">MVCC concurrency mechanism, +linkend="hot-standby">Hot Standby, and Read Replica servers. What is the difference between a hot standby and a read replica? I think one of these terms is sufficient. > +In addition to vxid and xid, > +when a transaction is prepared for two-phase commit it > +is also identified by a Global Transaction Identifier > +(GID). Better: "In addition to vxid and xid, prepared transactions also have a Global Transaction Identifier (GID) that is assigned when the transaction is prepared for two-phase commit." > + > + > + Transactions and Locking > + > + > +Currently-executing transactions are shown in +linkend="view-pg-locks">pg_locks > +in columns virtualxid and > +transactionid. Better: "The transaction IDs of currently executing transactions are shown in pg_locks in the columns virtualxid and transactionid." > +Lock waits on table-level locks are shown waiting for > +virtualxid, while lock waits on row-level > +locks are shown waiting for transactionid. That's not true. Transactions waiting for table-level locks are shown waiting for a "relation" lock in both "pg_stat_activity" and "pg_locks". > +Row-level read and write locks are recorded directly in locked > +rows and can be inspected using the > +extension. Row-level read locks might also require the assignment > +of multixact IDs (mxid). Mxids are recorded in > +the pg_multixact directory. "are recorded directly in *the* locked rows" I think the mention of multixacts should link to . Again, I would not specifically mention the directory, since it is already described in "storage.sgml", but I have no strong optinion there. > + > + > + Subtransactions > +The word subtransaction is often abbreviated as > +subxact. I'd use , not . > +If a subtransaction is assigned a non-virtual transaction ID, > +its transaction ID is referred to as a subxid. Again, I would use , since we don't "subxid" elsewhere. + Up to +64 open subxids are cached in shared memory for each backend; after +that point, the overhead increases significantly since we must look +up subxid entries in pg_subtrans. Comma before "since". Perhaps you should mention that this means disk I/O. Yours, Laurenz Albe
Re: Postgres auto vacuum - Disable
On Mon, 2022-11-07 at 12:12 +, Karthik Jagadish (kjagadis) wrote: > I have follow-up question where the vacuum process is waiting and not doing > it’s job. > When we grep on waiting process we see below output. Whenever we see this we > notice > that the vacuum is not happening and the system is running out of space. > > [root@zpah0031 ~]# ps -ef | grep 'waiting' > postgres 8833 62646 0 Jul28 ? 00:00:00 postgres: postgres cgms > [local] VACUUM waiting > postgres 18437 62646 0 Jul27 ? 00:00:00 postgres: postgres cgms > [local] VACUUM waiting > > > What could be the reason as to why the vacuum is not happening? Is it because > some lock is > present in the table/db or any other reason? Look in "pg_stat_activity". I didn't check, but I'm sure it's the intentional break configured with "autovacuum_vacuum_cost_delay". Reduce that parameter for more autovacuum speed. Yours, Laurenz Albe
Re: New docs chapter on Transaction Management and related changes
On Sat, 2022-10-15 at 21:08 -0400, Bruce Momjian wrote: > I therefore merged all three paragraphs into > one and tried to make the text saner; release_savepoint.sgml diff > attached, URL content updated. I wanted to have a look at this, but I am confused. The original patch was much bigger. Is this just an incremental patch? If yes, it would be nice to have a "grand total" patch, so that I can read it all in one go. Yours, Laurenz Albe
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
Thanks for the updated patch set! On Fri, 2022-10-28 at 17:59 -0500, Justin Pryzby wrote: > > > 0004, 0005, 0006, 0007: EXPLAIN (MACHINE) > > > > I think it is confusing that these are included in this patch set. > > EXPLAIN (MACHINE OFF) is similar to "explain_regress = on", only it goes > > even further: > > no query ID, no Heap Fetches, no Sort details, ... > > Why not add this functionality to the GUC? > > Good question, and one I've asked myself. > > explain_regress affects pre-existing uses of explain in the regression > tests, aiming to simplify current (or maybe only future) uses. And > is obviously used to allow enabling BUFFERS by default. > > explain(MACHINE off) is mostly about allowing "explain ANALYZE" to be > used in regression tests. Which, right now, is rare. Maybe I shouldn't > include those patches until the earlier patches progress (or, maybe we > should just defer their discussion). Essentially, "explain_regress" is to simplify the current regression tests, and MACHINE OFF is to simplify future regression tests. That does not seem like a meaningful distinction to me. Both are only useful for the regression tests, and I see no need for two different knobs. My opinion is now like this: +1 on enabling BUFFERS by default for EXPLAIN (ANALYZE) +0.2 on "explain_regress" -1 on EXPLAIN (MACHINE) in addition to "explain_regress" -0.1 on adding the MACHINE OFF functionality to "explain_regress" "explain_regress" reduces the EXPLAIN options you need for regression tests. This is somewhat useful, but not a big win. Also, it will make backpatching regression tests slightly harder for the next 5 years. Hence the +0.2 for "explain_regress". For me, an important question is: do we really want more EXPLAIN (ANALYZE) in the regression tests? It will probably slow the tests down, and I wonder if there is a lot of added value, if we have to remove all the machine-specific output anyway. Hence the -0.1. > > 0005 suppresses "rows removed by filter", but how is that machine > > dependent? > > It can vary with the number of parallel workers (see 13e8b2ee8), which > may not be "machine dependent" but the point of that patch is to allow > predictable output of EXPLAIN ANALYZE. Maybe it needs a better name (or > maybe it should be folded into the first patch). Now it makes sense to me. I just didn't think of that. > I'm not actually sure if this patch should try to address parallel > workers at all, or (if so) if what it's doing now is adequate. > > > > BTW, I think it may be that the GUC should be marked PGDLLIMPORT ? > > > > I think it is project policy to apply this mark wherever it is needed. Do > > you think > > that third-party extensions might need to use this in C code? > > Since v15 (8ec569479), the policy is: > > On Windows, export all the server's global variables using PGDLLIMPORT > > markers (Robert Haas) > > I'm convinced now that's what's intended here. You convinced me too. > > This is not enough. The patch would have to update all the examples > > that use EXPLAIN ANALYZE. > > Fair enough. I've left a comment to handle this later if the patch > gains any traction and 001 is progressed. I agree with that. I would really like to see 0003 go in, but it currently depends on 0001, which I am not so sure about. I understand that you did that so that "explain_regress" can turn off BUFFERS and there is no extra churn in the regression tests. Still, it would be a shame if resistance against "explain_regress" would be a show-stopper for 0003. If I could get my way, I'd want two separate patches: first, one to turn BUFFERS on, and second one for "explain_regress" with its current functionality on top of that. Yours, Laurenz Albe
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Tue, 2022-10-25 at 19:03 +0800, Julien Rouhaud wrote:
> On Tue, Oct 25, 2022 at 11:08:27AM +0200, Laurenz Albe wrote:
> > Here is a patch that
> > implements it with an EXPLAIN option named GENERIC_PLAN.
>
> I only have a quick look at the patch for now. Any reason why you don't rely
> on the existing explain_filter() function for emitting stable output (without
> having to remove the costs)? It would also take care of checking that it
> works
> in plpgsql.
No, there is no principled reason I did it like that. Version 2 does it like
you suggest.
Yours,
Laurenz Albe
From 8704d51f5810619be152ae68faa5743dcf26c7a9 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Fri, 28 Oct 2022 20:58:59 +0200
Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN
This allows EXPLAIN to generate generic plans for parameterized
statements (that have parameter placeholders like $1 in the
statement text).
Author: Laurenz Albe
Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at
---
doc/src/sgml/ref/explain.sgml | 15 +++
src/backend/commands/explain.c| 9 +
src/backend/parser/analyze.c | 13 +
src/backend/parser/parse_coerce.c | 15 +++
src/backend/parser/parse_expr.c | 16
src/include/commands/explain.h| 1 +
src/include/parser/parse_node.h | 2 ++
src/test/regress/expected/explain.out | 14 ++
src/test/regress/sql/explain.sql | 5 +
9 files changed, 90 insertions(+)
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..659d5c51b6 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -40,6 +40,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
COSTS [ boolean ]
SETTINGS [ boolean ]
+GENERIC_PLAN [ boolean ]
BUFFERS [ boolean ]
WAL [ boolean ]
TIMING [ boolean ]
@@ -167,6 +168,20 @@ ROLLBACK;
+
+GENERIC_PLAN
+
+
+ Generate a generic plan for the statement (see
+ for details about generic plans). The statement can contain parameter
+ placeholders like $1 and must be a statement that can
+ use parameters. This option cannot be used together with
+ ANALYZE, since a statement with unknown parameters
+ cannot be executed.
+
+
+
+
BUFFERS
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index f86983c660..7b7ca3f90a 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -190,6 +190,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
es->settings = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "generic_plan") == 0)
+ es->generic = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -227,6 +229,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
+ if (es->generic && es->analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
+
+ /* check that WAL is used with EXPLAIN ANALYZE */
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 6688c2a865..c849765151 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -27,6 +27,7 @@
#include "access/sysattr.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
@@ -2894,6 +2895,18 @@ static Query *
transformExplainStmt(ParseState *pstate, ExplainStmt *stmt)
{
Query *result;
+ ListCell *lc;
+ bool generic_plan;
+
+ foreach(lc, stmt->options)
+ {
+ DefElem*opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "generic_plan") == 0)
+ generic_plan = defGetBoolean(opt);
+ /* don't "break", as we want the last value */
+ }
+ pstate->p_generic_explain = generic_plan;
/* transform contained query, allowing SELECT INTO */
stmt->query = (Node *) transformOptionalSelectInto(pstate, stmt->query);
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index c4e958e4aa..171d8c60a8 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -369,6 +369,21 @@ coerce_type(ParseState *pstate, Node *node,
return result;
}
+ /*
+ * If we are to gen
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
On Thu, 2022-10-20 at 21:09 -0500, Justin Pryzby wrote:
> Rebased.
I had a look at the patch set.
It applies and builds cleanly and passes the regression tests.
0001: Add GUC: explain_regress
I like the idea of the "explain_regress" GUC. That should simplify
the regression tests.
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -625,7 +625,7 @@ initialize_environment(void)
* user's ability to set other variables through that.
*/
{
- const char *my_pgoptions = "-c intervalstyle=postgres_verbose";
+ const char *my_pgoptions = "-c intervalstyle=postgres_verbose -c
explain_regress=on";
const char *old_pgoptions = getenv("PGOPTIONS");
char *new_pgoptions;
@@ -2288,6 +2288,7 @@ regression_main(int argc, char *argv[],
fputs("log_lock_waits = on\n", pg_conf);
fputs("log_temp_files = 128kB\n", pg_conf);
fputs("max_prepared_transactions = 2\n", pg_conf);
+ // fputs("explain_regress = yes\n", pg_conf);
for (sl = temp_configs; sl != NULL; sl = sl->next)
{
This code comment is a leftover and should go.
0002: exercise explain_regress
This is the promised simplification. Looks good.
0003: Make explain default to BUFFERS TRUE
Yes, please!
This patch is independent from the previous patches.
I'd like this to be applied, even if the GUC is rejected.
(I understand that that would cause more changes in the regression
test output if we changed that without introducing "explain_regress".)
The patch changes the default value of "auto_explain.log_buffers"
to "on", which makes sense. However, the documentation in
doc/src/sgml/auto-explain.sgml should be updated to reflect that.
--- a/doc/src/sgml/perform.sgml
+++ b/doc/src/sgml/perform.sgml
@@ -731,8 +731,8 @@ EXPLAIN ANALYZE SELECT * FROM polygon_tbl WHERE f1 @
polygon '(0.5,2.0)';
-EXPLAIN has a BUFFERS option that
can be used with
-ANALYZE to get even more run time statistics:
+EXPLAIN ANALYZE has a BUFFERS
option which
+provides even more run time statistics:
EXPLAIN (ANALYZE, BUFFERS) SELECT * FROM tenk1 WHERE unique1 100 AND
unique2 9000;
This is not enough. The patch would have to update all the examples that use
EXPLAIN ANALYZE.
I see two options:
1. Change the output of all the examples and move this explanation to the
first example
with EXPLAIN ANALYZE:
The numbers in the Buffers: line help to identify
which parts
of the query are the most I/O-intensive.
2. Change all examples that currently do *not* use BUFFERS to EXPLAIN
(BUFFERS OFF).
Then you could change the example that shows BUFFERS to something like
If you do not suppress the BUFFERS option that can be
used with
EXPLAIN (ANALYZE), you get even more run time
statistics:
0004, 0005, 0006, 0007: EXPLAIN (MACHINE)
I think it is confusing that these are included in this patch set.
EXPLAIN (MACHINE OFF) is similar to "explain_regress = on", only it goes even
further:
no query ID, no Heap Fetches, no Sort details, ...
Why not add this functionality to the GUC?
0005 suppresses "rows removed by filter", but how is that machine dependent?
> BTW, I think it may be that the GUC should be marked PGDLLIMPORT ?
I think it is project policy to apply this mark wherever it is needed. Do you
think
that third-party extensions might need to use this in C code?
Yours,
Laurenz Albe
Re: Make EXPLAIN generate a generic plan for a parameterized query
On Wed, 2022-10-12 at 00:03 +0800, Julien Rouhaud wrote:
> On Tue, Oct 11, 2022 at 09:49:14AM -0400, Tom Lane wrote:
> > I think it might be better to drive it off an explicit EXPLAIN option,
> > perhaps
> >
> > EXPLAIN (GENERIC_PLAN) SELECT * FROM tab WHERE col = $1;
> >
> > If you're trying to investigate custom-plan behavior, then you
> > need to supply concrete parameter values somewhere, so I think
> > this approach is fine for that case. (Shoehorning parameter
> > values into EXPLAIN options seems like it'd be a bit much.)
> > However, investigating generic-plan behavior this way is tedious,
> > since you have to invent irrelevant parameter values, plus mess
> > with plan_cache_mode or else run the explain half a dozen times.
> > So I can get behind having a more convenient way for that.
>
> One common use case is tools identifying a slow query using
> pg_stat_statements,
> identifying some missing indexes and then wanting to check whether the index
> should be useful using some hypothetical index.
>
> FTR I'm working on such a project and for now we have to go to great lengths
> trying to "unjumble" such queries, so having a way to easily get the answer
> for
> a generic plan would be great.
Thanks for the suggestions and the encouragement. Here is a patch that
implements it with an EXPLAIN option named GENERIC_PLAN.
Yours,
Laurenz Albe
From 85991f35f0de6e4e0a0b5843373e2ba3d5976c85 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Tue, 25 Oct 2022 11:01:53 +0200
Subject: [PATCH] Add EXPLAIN option GENERIC_PLAN
This allows EXPLAIN to generate generic plans for parameterized
statements (that have parameter placeholders like $1 in the
statement text).
Author: Laurenz Albe
Discussion: https://postgr.es/m/0a29b954b10b57f0d135fe12aa0909bd41883eb0.camel%40cybertec.at
---
doc/src/sgml/ref/explain.sgml | 15 ++
src/backend/commands/explain.c| 9 +
src/backend/parser/analyze.c | 13 +
src/backend/parser/parse_coerce.c | 15 ++
src/backend/parser/parse_expr.c | 16 +++
src/include/commands/explain.h| 1 +
src/include/parser/parse_node.h | 2 ++
src/test/regress/expected/explain.out | 28 +++
src/test/regress/sql/explain.sql | 16 +++
9 files changed, 115 insertions(+)
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..659d5c51b6 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -40,6 +40,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
COSTS [ boolean ]
SETTINGS [ boolean ]
+GENERIC_PLAN [ boolean ]
BUFFERS [ boolean ]
WAL [ boolean ]
TIMING [ boolean ]
@@ -167,6 +168,20 @@ ROLLBACK;
+
+GENERIC_PLAN
+
+
+ Generate a generic plan for the statement (see
+ for details about generic plans). The statement can contain parameter
+ placeholders like $1 and must be a statement that can
+ use parameters. This option cannot be used together with
+ ANALYZE, since a statement with unknown parameters
+ cannot be executed.
+
+
+
+
BUFFERS
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index f86983c660..7b7ca3f90a 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -190,6 +190,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
es->wal = defGetBoolean(opt);
else if (strcmp(opt->defname, "settings") == 0)
es->settings = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "generic_plan") == 0)
+ es->generic = defGetBoolean(opt);
else if (strcmp(opt->defname, "timing") == 0)
{
timing_set = true;
@@ -227,6 +229,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* check that GENERIC_PLAN is not used with EXPLAIN ANALYZE */
+ if (es->generic && es->analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("EXPLAIN ANALYZE cannot be used with GENERIC_PLAN")));
+
+ /* check that WAL is used with EXPLAIN ANALYZE */
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 6688c2a865..c849765151 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -27,6 +27,7 @@
#include "access/sysattr.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
+#include "commands/defrem.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
@@ -2894,6 +
Re: [PATCH] Allow usage of archive .backup files as backup_label
On Tue, 2022-10-18 at 10:55 +0900, Michael Paquier wrote: > On Mon, Aug 22, 2022 at 05:16:58PM +0200, Michael Banck wrote: > > The .backup files written to the archive (if archiving is on) are very > > similar to the backup_label that's written/returned by > > pg_stop_backup()/pg_backup_stop(), they just have a few extra lines > > about the end of backup process that are missing from backup_label. > > Historically, there is "STOP WAL LOCATION" after "START WAL LOCATION", > and "STOP TIME"/"STOP TIMELINE" at the end. > > > The parser in xlogrecovery.c however barfs on them because it does not > > expect the additional STOP WAL LOCATION on line 2. > > Hm, no. I don't think that I'd want to expand the use of the backup > history file in the context of recovery, so as we are free to add any > extra information into it if necessary without impacting the > compatibility of the recovery code. This file is primarily here for > debugging, so I'd rather let it be used only for this purpose. > Opinions of others are welcome, of course. I tend to agree with you. It is easy to break PostgreSQL by manipulating or removing "backup_label", and copying a file from the WAL archive and renaming it to "backup_label" sounds like a footgun of the first order. There is nothing that prevents you from copying the wrong file. Such practices should not be encouraged. Anybody who knows enough about PostgreSQL to be sure that what they are doing is correct should be smart enough to know how to edit the copied file. Yours, Laurenz Albe
Make EXPLAIN generate a generic plan for a parameterized query
Today you get
test=> EXPLAIN SELECT * FROM tab WHERE col = $1;
ERROR: there is no parameter $1
which makes sense. Nonetheless, it would be great to get a generic plan
for such a query. Sometimes you don't have the parameters (if you grab
the statement from "pg_stat_statements", or if it is from an error message
in the log, and you didn't enable "log_parameter_max_length_on_error").
Sometimes it is just very painful to substitute the 25 parameters from
the detail message.
With the attached patch you can get the following:
test=> SET plan_cache_mode = force_generic_plan;
SET
test=> EXPLAIN (COSTS OFF) SELECT * FROM pg_proc WHERE oid = $1;
QUERY PLAN
═══
Index Scan using pg_proc_oid_index on pg_proc
Index Cond: (oid = $1)
(2 rows)
That's not the same as a full-fledged EXPLAIN (ANALYZE, BUFFERS),
but it can definitely be helpful.
I tied that behavior to the setting of "plan_cache_mode" where you
are guaranteed to get a generic plan; I couldn't think of a better way.
Yours,
Laurenz Albe
From 2bc91581acd478d4648176b58745cadb835d5fbc Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Tue, 11 Oct 2022 13:05:31 +0200
Subject: [PATCH] Add EXPLAIN support for parameterized statements
If "plan_cache_mode = force_generic_plan", allow EXPLAIN to
generate generic plans for parameterized statements (that
have parameter placeholders like $1 in the statement text).
This repurposes hooks used by PL/pgSQL, so we better not try
to do that inside PL/pgSQL.
---
doc/src/sgml/ref/explain.sgml | 10 +
src/backend/parser/analyze.c | 53 +++
src/test/regress/expected/explain.out | 28 ++
src/test/regress/sql/explain.sql | 13 +++
4 files changed, 104 insertions(+)
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index d4895b9d7d..928d67b9b4 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -321,6 +321,16 @@ ROLLBACK;
execution, and on machines that have relatively slow operating
system calls for obtaining the time of day.
+
+
+ If is set to
+ force_generic_plan, you can use EXPLAIN
+ to generate generic plans for statements that contain placeholders like
+ $1 without knowing the actual parameter type or value.
+ Note that expressions like $1 + $2 are ambiguous if you
+ don't specify the parameter data types, so you may have to add explicit type
+ casts in such cases.
+
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 6688c2a865..c481d45376 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -52,6 +52,7 @@
#include "utils/guc.h"
#include "utils/queryjumble.h"
#include "utils/rel.h"
+#include "utils/plancache.h"
#include "utils/syscache.h"
@@ -86,6 +87,10 @@ static Query *transformCallStmt(ParseState *pstate,
CallStmt *stmt);
static void transformLockingClause(ParseState *pstate, Query *qry,
LockingClause *lc, bool pushedDown);
+static Node * fakeUnknownParam(ParseState *pstate, ParamRef *pref);
+static Node * coerceUnknownParam(ParseState *pstate, Param *param,
+ Oid targetTypeId, int32 targetTypeMod,
+ int location);
#ifdef RAW_EXPRESSION_COVERAGE_TEST
static bool test_raw_expression_coverage(Node *node, void *context);
#endif
@@ -2895,6 +2900,22 @@ transformExplainStmt(ParseState *pstate, ExplainStmt *stmt)
{
Query *result;
+ /*
+ * If we EXPLAIN a statement and are certain to generate a generic plan,
+ * we can tolerate undefined parameters. For that purpose, supply
+ * parameters of type "unknown" and coerce them to the appropriate type
+ * as needed.
+ * If we are called from PL/pgSQL, the hooks are already set for the
+ * purpose of resolving variables, and we don't want to disturb that.
+ */
+ if (plan_cache_mode == PLAN_CACHE_MODE_FORCE_GENERIC_PLAN &&
+ pstate->p_paramref_hook == NULL &&
+ pstate->p_coerce_param_hook == NULL)
+ {
+ pstate->p_paramref_hook = fakeUnknownParam;
+ pstate->p_coerce_param_hook = coerceUnknownParam;
+ }
+
/* transform contained query, allowing SELECT INTO */
stmt->query = (Node *) transformOptionalSelectInto(pstate, stmt->query);
@@ -3466,6 +3487,38 @@ applyLockingClause(Query *qry, Index rtindex,
qry->rowMarks = lappend(qry->rowMarks, rc);
}
+/*
+ * Return an "unknown" parameter for use with EXPLAIN of a parameterized
+ * statement.
+ */
+Node *
+fakeUnknownParam(ParseState *pstate, ParamRef *pref)
+{
+ Param *param;
+
+ param = makeNode(Param);
+ param->paramkind = PARAM_EXTERN;
+ param->paramid = pref->number;
+ param->paramtype = UNKNOWNOID;
+ param->paramtypmod = -1;
+ param->paramcollid = InvalidOid;
+ param->location =
Re: document the need to analyze partitioned tables
On Mon, 2022-03-28 at 15:05 +0200, Tomas Vondra wrote: > I've pushed the last version, and backpatched it to 10 (not sure I'd > call it a bugfix, but I certainly agree with Justin it's worth > mentioning in the docs, even on older branches). I'd like to suggest an improvement to this. The current wording could be read to mean that dead tuples won't get cleaned up in partitioned tables. By the way, where are the statistics of a partitioned tables used? The actual tables scanned are always the partitions, and in the execution plans that I have seen, the optimizer always used the statistics of the partitions. Yours, Laurenz Albe From 5209f228f09e52780535edacfee5f7efd2c25081 Mon Sep 17 00:00:00 2001 From: Laurenz Albe Date: Wed, 5 Oct 2022 10:31:47 +0200 Subject: [PATCH] Improve autovacuum doc on partitioned tables The documentation mentioned that autovacuum doesn't process partitioned tables, but it was unclear about the impact. The old wording could be interpreted to mean that there are problems with dead tuple cleanup on partitioned tables. Clarify that the only potential problem is autoanalyze, and that statistics for the partitions will be gathered. --- doc/src/sgml/maintenance.sgml | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml index 759ea5ac9c..53e3fadbaf 100644 --- a/doc/src/sgml/maintenance.sgml +++ b/doc/src/sgml/maintenance.sgml @@ -860,10 +860,15 @@ analyze threshold = analyze base threshold + analyze scale factor * number of tu -Partitioned tables are not processed by autovacuum. Statistics -should be collected by running a manual ANALYZE when it is -first populated, and again whenever the distribution of data in its -partitions changes significantly. +Partitioned tables are not processed by autovacuum. This is no problem +as far as VACUUM is concerned, since autovacuum will process +the partitions. But, as mentioned in , +it also means that autovacuum won't run ANALYZE on the +partitioned table itself. While statistics are gathered for the partitions, +some queries may rely on the statistics for the partitioned table. You should +collect statistics by running a manual ANALYZE when the +partitioned table is first populated, and again whenever the distribution +of data in its partitions changes significantly. -- 2.37.3
Re: future of serial and identity columns
On Tue, 2022-10-04 at 09:41 +0200, Peter Eisentraut wrote: > In PostgreSQL 10, we added identity columns, as an alternative to serial > columns (since 6.something). They mostly work the same. Identity > columns are SQL-conforming, have some more features (e.g., overriding > clause), and are a bit more robust in schema management. Some of that > was described in [0]. AFAICT, there have been no complaints since that > identity columns lack features or are somehow a regression over serial > columns. > > But clearly, the syntax "serial" is more handy, and most casual examples > use that syntax. So it seems like we are stuck with maintaining these > two variants in parallel forever. I was thinking we could nudge this a > little by remapping "serial" internally to create an identity column > instead. At least then over time, the use of the older serial > mechanisms would go away. I think that would be great. That might generate some confusion among users who follow old tutorials and are surprised that the eventual table definition differs, but I'd say that is a good thing. Yours, Laurenz Albe
Re: postgres_fdw: dead lock in a same transaction when postgres_fdw server is lookback
On Sat, 2022-10-01 at 04:02 +, Xiaoran Wang wrote:
> I created a postgers_fdw server lookback as the test does. Then run the
> following SQLs
>
> [create a foreign server via loopback and manipulate the same data locally
> and via foreign table]
>
> Then the transaction got stuck. Should the "lookback" server be disabled in
> the postgres_fdw?
It shouldn't; there are good use cases for that ("autonomous transactions").
AT most, some cautioning documentation could be added, but I am not convinced
that even that is necessary.
I'd say that this is a pretty obvious case of pilot error.
Yours,
Laurenz Albe
Re: Pruning never visible changes
On Fri, 2022-09-16 at 10:26 -0400, Tom Lane wrote: > Simon Riggs writes: > > A user asked me whether we prune never visible changes, such as > > BEGIN; > > INSERT... > > UPDATE.. (same row) > > COMMIT; > > Didn't we just have this discussion in another thread? For reference: that was https://postgr.es/m/f6a491b32cb44bb5daaafec835364f7149348fa1.ca...@cybertec.at Yours, Laurenz Albe
Re: Tuples inserted and deleted by the same transaction
On Tue, 2022-09-13 at 16:13 +0200, Matthias van de Meent wrote: > On Tue, 13 Sept 2022 at 15:45, Tom Lane wrote: > > Laurenz Albe writes: > > > But once they are deleted or updated, even the transaction that created > > > them cannot > > > see them any more, right? > > > > I would not trust that claim very far. The transaction might have active > > snapshots with a command ID between the times of insertion and deletion. > > (Consider a query that is firing triggers as it goes, and the triggers > > are performing new actions that cause the command counter to advance. > > The outer query should not see the results of those actions.) > > I hadn't realized that triggers indeed consume command ids but might > not be visible to the outer query (that might still be running). That > invalidates the "or (e.g.) the existence of another tuple with the > same XID but with a newer CID" claim I made earlier, so thanks for > clarifying. Yes, that makes sense. Thanks. Yours, Laurenz Albe
Re: Tuples inserted and deleted by the same transaction
On Tue, 2022-09-13 at 11:47 +0300, Nikita Malakhov wrote:
> On Tue, Sep 13, 2022 at 11:06 AM Laurenz Albe
> wrote:
> > Shouldn't such tuples be considered dead right away, even if the inserting
> > transaction is still active? That would allow cleaning them up even before
> > the transaction is done.
> >
> > There is this code in HeapTupleSatisfiesVacuumHorizon:
> >
> > else if
> > (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple)))
> > {
> > [...]
> > /* inserted and then deleted by same xact */
> > if
> > (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetUpdateXid(tuple)))
> > return HEAPTUPLE_DELETE_IN_PROGRESS;
> >
> > Why HEAPTUPLE_DELETE_IN_PROGRESS and not HEAPTUPLE_DEAD?
>
> Please correct me if I'm wrong, despite tuples being inserted and deleted by
> the same
> transaction - they are visible inside the transaction and usable by it, so
> considering them
> dead and cleaning up during execution is a bad idea until the transaction is
> ended.
But once they are deleted or updated, even the transaction that created them
cannot
see them any more, right?
Yours,
Laurenz Albe
Tuples inserted and deleted by the same transaction
Shouldn't such tuples be considered dead right away, even if the inserting
transaction is still active? That would allow cleaning them up even before
the transaction is done.
There is this code in HeapTupleSatisfiesVacuumHorizon:
else if
(TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmin(tuple)))
{
[...]
/* inserted and then deleted by same xact */
if
(TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetUpdateXid(tuple)))
return HEAPTUPLE_DELETE_IN_PROGRESS;
Why HEAPTUPLE_DELETE_IN_PROGRESS and not HEAPTUPLE_DEAD?
Yours,
Laurenz Albe
Re: cataloguing NOT NULL constraints
On Thu, 2022-08-18 at 11:04 +0200, Alvaro Herrera wrote: > On 2022-Aug-18, Laurenz Albe wrote: > > On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > > > Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' > > > bit is lost when the last one such constraint goes away. > > > > Wouldn't it be the correct solution to set "attnotnumm" to FALSE only > > when the last NOT NULL constraint is dropped? > > ... when the last NOT NULL or PRIMARY KEY constraint is dropped. We > have to keep attnotnull set when a PK exists even if there's no specific > NOT NULL constraint. Of course, I forgot that. I hope that is not too hard to implement. > > > 2. If a table has a primary key, and a table is created that inherits > > > from it, then the child has its column(s) marked attnotnull but there > > > is no pg_constraint row for that. This is not okay. But what should > > > happen? > > > > > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > > > 2. a PRIMARY KEY () constraint is created > > > > I think it would be best to create a primary key constraint on the > > partition. > > Sorry, I wasn't specific enough. This applies to legacy inheritance > only; partitioning has its own solution (as you say: the PK constraint > exists), but legacy inheritance works differently. Creating a PK in > children tables is not feasible (because unicity cannot be maintained), > but creating a CHECK (NOT NULL) constraint is possible. > > I think a PRIMARY KEY should not be allowed to exist in an inheritance > parent, precisely because of this problem, but it seems too late to add > that restriction now. This behavior is absurd, but longstanding: My mistake; you clearly said "inherits". Since such an inheritance child currently does not have a primary key, you can insert duplicates. So automatically adding a NUT NULL constraint on the inheritance child seems the only solution that does not break backwards compatibility. pg_upgrade would have to be able to cope with that. Forcing a primary key constraint on the inheritance child could present an upgrade problem. Even if that is probably a rare and strange case, I don't think we should risk that. Moreover, if we force a primary key on the inheritance child, using ALTER TABLE ... INHERIT might have to create a unique index on the table, which can be cumbersome if the table is large. So I think a NOT NULL constraint is the least evil. Yours, Laurenz Albe
Re: cataloguing NOT NULL constraints
On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote: > I've been working on having NOT NULL constraints have pg_constraint > rows. > > Everything is working now. Some things are a bit weird, and I would > like opinions on them: > > 1. In my implementation, you can have more than one NOT NULL > pg_constraint row for a column. What should happen if the user does > ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL; > ? Currently it throws an error about the ambiguity (ie. which > constraint to drop). I'd say that is a good solution, particularly if there is a hint to drop the constraint instead, similar to when you try to drop an index that implements a constraint. > Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull' > bit is lost when the last one such constraint goes away. Wouldn't it be the correct solution to set "attnotnumm" to FALSE only when the last NOT NULL constraint is dropped? > 2. If a table has a primary key, and a table is created that inherits > from it, then the child has its column(s) marked attnotnull but there > is no pg_constraint row for that. This is not okay. But what should > happen? > > 1. a CHECK(col IS NOT NULL) constraint is created for each column > 2. a PRIMARY KEY () constraint is created I think it would be best to create a primary key constraint on the partition. Yours, Laurenz Albe
Re: Wrong provolatile value for to_timestamp (1 argument)
On Tue, 2022-07-05 at 19:37 +0900, Tatsuo Ishii wrote:
> > Are you sure? I'd say that "to_timestamp(double precision)" always
> > produces the same timestamp for the same argument. What changes with
> > the setting of "timezone" is how that timestamp is converted to a
> > string, but that's a different affair.
>
> Of course the internal representation of timestamp with time zone data
> type is not affected by the time zone setting. But why other form of
> to_timestamp is labeled as stable? If your theory is correct, then
> other form of to_timestamp shouldn't be labeled immutable as well?
The result of the two-argument form of "to_timestamp" can depend on
the setting of "lc_time":
test=> SET lc_time = 'en_US.utf8';
SET
test=> SELECT to_timestamp('2022-July-05', '-TMMonth-DD');
to_timestamp
2022-07-05 00:00:00+02
(1 row)
test=> SET lc_time = 'de_DE.utf8';
SET
test=> SELECT to_timestamp('2022-July-05', '-TMMonth-DD');
ERROR: invalid value "July-05" for "Month"
DETAIL: The given value did not match any of the allowed values for this field.
Yours,
Laurenz Albe
Re: Wrong provolatile value for to_timestamp (1 argument)
On Tue, 2022-07-05 at 17:29 +0900, Tatsuo Ishii wrote: > I found that provolatile attribute of to_timestamp in pg_proc is > wrong: > > test=# select provolatile, proargtypes from pg_proc where proname = > 'to_timestamp' and proargtypes[0] = 701; > provolatile | proargtypes > -+- > i | 701 > (1 row) > > 'i' (immutable) is clearly wrong s Are you sure? I'd say that "to_timestamp(double precision)" always produces the same timestamp for the same argument. What changes with the setting of "timezone" is how that timestamp is converted to a string, but that's a different affair. Yours, Laurenz Albe
Re: Hardening PostgreSQL via (optional) ban on local file system access
On Wed, 2022-06-29 at 00:05 -0700, Andres Freund wrote: > On 2022-06-29 08:51:10 +0200, Laurenz Albe wrote: > > On Tue, 2022-06-28 at 16:27 -0700, Andres Freund wrote: > > > > Experience shows that 99% of the time one can run PostgreSQL just fine > > > > without a superuser > > > > > > IME that's not at all true. It might not be needed interactively, but > > > that's > > > not all the same as not being needed at all. > > > > I also disagree with that. Not having a superuser is one of the pain > > points with using a hosted database: no untrusted procedural languages, > > no untrusted extensions (unless someone hacked up PostgreSQL or provided > > a workaround akin to a SECURITY DEFINER function), etc. > > I'm not sure what exactly you're disagreeing with? I'm not saying that > superuser isn't needed interactively in general, just that there are > reasonably common scenarios in which that's the case. I was unclear, sorry. I agreed with you that you can't do without superuser and disagreed with the claim that 99% of the time nobody needs superuser access. Yours, Laurenz Albe
Re: Can we do something to help stop users mistakenly using force_parallel_mode?
On Wed, 2022-06-29 at 15:23 +1200, David Rowley wrote: > Over on [1] I noticed that the user had set force_parallel_mode to > "on" in the hope that would trick the planner into making their query > run more quickly. Of course, that's not what they want since that GUC > is only there to inject some parallel nodes into the plan in order to > verify the tuple communication works. > > I get the idea that Robert might have copped some flak about this at > some point, given that he wrote the blog post at [2]. > > The user would have realised this if they'd read the documentation > about the GUC. However, I imagine they only went as far as finding a > GUC with a name which appears to be exactly what they need. I mean, > what else could force_parallel_mode possibly do? > > Should we maybe rename it to something less tempting? Maybe > debug_parallel_query? > > I wonder if \dconfig *parallel* is going to make force_parallel_mode > even easier to find once PG15 is out. > > [1] > https://www.postgresql.org/message-id/DB4PR02MB8774E06D595D3088BE04ED92E7B99%40DB4PR02MB8774.eurprd02.prod.outlook.com > [2] > https://www.enterprisedb.com/postgres-tutorials/using-forceparallelmode-correctly-postgresql I share the sentiment, but at the same time am worried about an unnecessary compatibility break. The parameter is not in "postgresql.conf" and documented as a "developer option", which should already be warning enough. Perhaps some stronger wording in the documetation would be beneficial. I have little sympathy with people who set unusual parameters without even glancing at the documentation. Yours, Laurenz Albe
Re: Hardening PostgreSQL via (optional) ban on local file system access
On Tue, 2022-06-28 at 16:27 -0700, Andres Freund wrote: > > Experience shows that 99% of the time one can run PostgreSQL just fine > > without a superuser > > IME that's not at all true. It might not be needed interactively, but that's > not all the same as not being needed at all. I also disagree with that. Not having a superuser is one of the pain points with using a hosted database: no untrusted procedural languages, no untrusted extensions (unless someone hacked up PostgreSQL or provided a workaround akin to a SECURITY DEFINER function), etc. Yours, Laurenz Albe
Re: Error from the foreign RDBMS on a foreign table I have no privilege on
On Fri, 2022-06-10 at 17:17 +0900, Etsuro Fujita wrote: > > I am not sure if it worth adding to the documentation. I would never have > >thought > > of the problem if Phil hadn't brought it up. On the other hand, I was > > surprised > > to learn that permissions aren't checked until the executor kicks in. > > It makes sense, but some documentation might help others in that situation. > > +1 for adding such a document. > > > I'll gladly leave the decision to your judgement as a committer. > > IIRC, there are no reports about this from the postgres_fdw users, so > my inclination would be to leave the documentation alone, for now. I understand that you are for documenting the timing of permission checks, but not in the postgres_fdw documentation. However, this is the only occasion where the user might notice unexpected behavior on account of the timing of permission checks. Other than that, I consider this below the threshold for user-facing documentation. I'm ok with just doing nothing here, I just wanted it discussed in public. Yours, Laurenz Albe
