From f40898bb3cde1c08a73166d4af4571414df18cec Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Wed, 10 May 2023 10:49:46 -0700 Subject: [PATCH 01/45] Doc: Fix link to fillfactor reloption. Fix a link from the "Heap-Only Tuples" documentation section. Previously, its "fillfactor" link pointed to the "CREATE TABLE" command's documentation. Now the link directly points to the fillfactor storage parameter documentation (which is about half way into the "CREATE TABLE" sect1). Oversight in commit 115464bb. Backpatch: 12-, the first version with a usable reloption link. --- doc/src/sgml/storage.sgml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml index 96a275a1110..41891c96b7d 100644 --- a/doc/src/sgml/storage.sgml +++ b/doc/src/sgml/storage.sgml @@ -1140,7 +1140,7 @@ data. Empty in ordinary tables. if columns used by indexes are not updated. You can increase the likelihood of sufficient page space for HOT updates by decreasing a table's fillfactor. + linkend="reloption-fillfactor">fillfactor. If you don't, HOT updates will still happen because new rows will naturally migrate to new pages and existing pages with sufficient free space for new row versions. The system view Date: Tue, 16 May 2023 10:53:42 -0400 Subject: [PATCH 02/45] Ensure Soundex difference() function handles empty input sanely. fuzzystrmatch's difference() function assumes that _soundex() always initializes its output buffer fully. This was not so for the case of a string containing no alphabetic characters, resulting in unstable output and Valgrind complaints. Fix by using memset() to fill the whole buffer in the early-exit case. Also make some cosmetic improvements (I didn't care for the random switches between "instr[0]" and "*instr" notation). Report and diagnosis by Alexander Lakhin (bug #17935). Back-patch to all supported branches. Discussion: https://postgr.es/m/17935-b99316aa79c18513@postgresql.org --- contrib/fuzzystrmatch/expected/fuzzystrmatch.out | 6 ++++++ contrib/fuzzystrmatch/fuzzystrmatch.c | 15 ++++++++------- contrib/fuzzystrmatch/sql/fuzzystrmatch.sql | 1 + 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/contrib/fuzzystrmatch/expected/fuzzystrmatch.out b/contrib/fuzzystrmatch/expected/fuzzystrmatch.out index 493c95cdfa5..2827e81eb47 100644 --- a/contrib/fuzzystrmatch/expected/fuzzystrmatch.out +++ b/contrib/fuzzystrmatch/expected/fuzzystrmatch.out @@ -23,6 +23,12 @@ SELECT soundex('Anne'), soundex('Margaret'), difference('Anne', 'Margaret'); A500 | M626 | 0 (1 row) +SELECT soundex(''), difference('', ''); + soundex | difference +---------+------------ + | 4 +(1 row) + SELECT levenshtein('GUMBO', 'GAMBOL'); levenshtein ------------- diff --git a/contrib/fuzzystrmatch/fuzzystrmatch.c b/contrib/fuzzystrmatch/fuzzystrmatch.c index d237772a3b2..e89055ec971 100644 --- a/contrib/fuzzystrmatch/fuzzystrmatch.c +++ b/contrib/fuzzystrmatch/fuzzystrmatch.c @@ -727,16 +727,14 @@ _soundex(const char *instr, char *outstr) AssertArg(instr); AssertArg(outstr); - outstr[SOUNDEX_LEN] = '\0'; - /* Skip leading non-alphabetic characters */ - while (!isalpha((unsigned char) instr[0]) && instr[0]) + while (*instr && !isalpha((unsigned char) *instr)) ++instr; - /* No string left */ - if (!instr[0]) + /* If no string left, return all-zeroes buffer */ + if (!*instr) { - outstr[0] = (char) 0; + memset(outstr, '\0', SOUNDEX_LEN + 1); return; } @@ -749,7 +747,7 @@ _soundex(const char *instr, char *outstr) if (isalpha((unsigned char) *instr) && soundex_code(*instr) != soundex_code(*(instr - 1))) { - *outstr = soundex_code(instr[0]); + *outstr = soundex_code(*instr); if (*outstr != '0') { ++outstr; @@ -766,6 +764,9 @@ _soundex(const char *instr, char *outstr) ++outstr; ++count; } + + /* And null-terminate */ + *outstr = '\0'; } PG_FUNCTION_INFO_V1(difference); diff --git a/contrib/fuzzystrmatch/sql/fuzzystrmatch.sql b/contrib/fuzzystrmatch/sql/fuzzystrmatch.sql index f05dc28ffb1..1d0e2197fbe 100644 --- a/contrib/fuzzystrmatch/sql/fuzzystrmatch.sql +++ b/contrib/fuzzystrmatch/sql/fuzzystrmatch.sql @@ -6,6 +6,7 @@ SELECT soundex('hello world!'); SELECT soundex('Anne'), soundex('Ann'), difference('Anne', 'Ann'); SELECT soundex('Anne'), soundex('Andrew'), difference('Anne', 'Andrew'); SELECT soundex('Anne'), soundex('Margaret'), difference('Anne', 'Margaret'); +SELECT soundex(''), difference('', ''); SELECT levenshtein('GUMBO', 'GAMBOL'); From eafdb7d13516b70c96d02e06674a8ee04d0259e1 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 18 May 2023 13:00:31 +0200 Subject: [PATCH 03/45] Fix handling of NULLs when merging BRIN summaries When merging BRIN summaries, union_tuples() did not correctly update the target hasnulls/allnulls flags. When merging all-NULL summary into a summary without any NULL values, the result had both flags set to false (instead of having hasnulls=true). This happened because the code only considered the hasnulls flags, ignoring the possibility the source summary has allnulls=true. Discovered while investigating issues with handling empty BRIN ranges and handling of NULL values, but it's a separate problem (has nothing to do with empty ranges). Fixed by considering both flags on the source summary, and updating the hasnulls flag on the target summary. Backpatch to 11. The bug exists since 9.5 (where BRIN indexes were introduced), but those releases are EOL already. Discussion: https://postgr.es/m/9d993d0d-e431-2196-9ccc-0554d0e60154%40enterprisedb.com --- src/backend/access/brin/brin.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index a6549d9dcbf..fc565a55cf3 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -1932,8 +1932,11 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) if (opcinfo->oi_regular_nulls) { + /* Does the "b" summary represent any NULL values? */ + bool b_has_nulls = (col_b->bv_hasnulls || col_b->bv_allnulls); + /* Adjust "hasnulls". */ - if (!col_a->bv_hasnulls && col_b->bv_hasnulls) + if (!col_a->bv_allnulls && b_has_nulls) col_a->bv_hasnulls = true; /* If there are no values in B, there's nothing left to do. */ @@ -1945,12 +1948,17 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) * values from B into A, and we're done. We cannot run the * operators in this case, because values in A might contain * garbage. Note we already established that B contains values. + * + * Also adjust "hasnulls" in order not to forget the summary + * represents NULL values. This is not redundant with the earlier + * update, because that only happens when allnulls=false. */ if (col_a->bv_allnulls) { int i; col_a->bv_allnulls = false; + col_a->bv_hasnulls = true; for (i = 0; i < opcinfo->oi_nstored; i++) col_a->bv_values[i] = From e4e5b26ca94b52a552fd8aab3bb768f437bba609 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 19 May 2023 12:38:18 +0900 Subject: [PATCH 04/45] pageinspect: Fix gist_page_items() with included columns Non-leaf pages of GiST indexes contain key attributes, leaf pages contain both key and non-key attributes, and gist_page_items() ignored the handling of non-key attributes. This caused a few problems when using gist_page_items() on a GiST index with INCLUDE: - On a non-leaf page, the function would crash. - On a leaf page, the function would work, but miss to display all the values for included attributes. This commit fixes gist_page_items() to handle such cases in a more appropriate way, and now displays the values of key and non-key attributes for each item separately in a style consistent with what ruleutils.c would generate for the attribute list, depending on the page type dealt with. In a way similar to how a record is displayed, values would be double-quoted for key or non-key attributes if required. ruleutils.c did not provide a routine able to control if non-key attributes should be displayed, so an extended() routine for index definitions is added to work around the leaf and non-leaf page differences. While on it, this commit fixes a third problem related to the amount of data reported for key attributes. The code originally relied on BuildIndexValueDescription() (used for error reports on constraints) that would not print all the data stored in the index but the index opclass's input type, so this limited the amount of information available. This switch makes gist_page_items() much cheaper as there is no need to run ACL checks for each item printed, which is not an issue anyway as superuser rights are required to execute the functions of pageinspect. Opclasses whose data cannot be displayed can rely on gist_page_items_bytea(). The documentation of this function was slightly incorrect for the output results generated on HEAD and v15, so adjust it on these branches. Author: Alexander Lakhin, Michael Paquier Discussion: https://postgr.es/m/17884-cb8c326522977acb@postgresql.org Backpatch-through: 14 --- contrib/pageinspect/expected/gist.out | 52 ++++++++++--- contrib/pageinspect/gistfuncs.c | 104 ++++++++++++++++++++++++-- contrib/pageinspect/sql/gist.sql | 14 ++++ doc/src/sgml/pageinspect.sgml | 18 ++--- src/backend/utils/adt/ruleutils.c | 16 ++++ src/include/utils/ruleutils.h | 5 ++ 6 files changed, 181 insertions(+), 28 deletions(-) diff --git a/contrib/pageinspect/expected/gist.out b/contrib/pageinspect/expected/gist.out index cae739219bd..056b1d89859 100644 --- a/contrib/pageinspect/expected/gist.out +++ b/contrib/pageinspect/expected/gist.out @@ -31,20 +31,25 @@ SELECT * FROM gist_page_opaque_info(get_raw_page('test_gist_idx', 2)); COMMIT; SELECT * FROM gist_page_items(get_raw_page('test_gist_idx', 0), 'test_gist_idx'); - itemoffset | ctid | itemlen | dead | keys -------------+-----------+---------+------+------------------- - 1 | (1,65535) | 40 | f | (p)=((669,669)) - 2 | (2,65535) | 40 | f | (p)=((1000,1000)) -(2 rows) + itemoffset | ctid | itemlen | dead | keys +------------+-----------+---------+------+------------------------------- + 1 | (1,65535) | 40 | f | (p)=("(166,166),(1,1)") + 2 | (2,65535) | 40 | f | (p)=("(332,332),(167,167)") + 3 | (3,65535) | 40 | f | (p)=("(498,498),(333,333)") + 4 | (4,65535) | 40 | f | (p)=("(664,664),(499,499)") + 5 | (5,65535) | 40 | f | (p)=("(830,830),(665,665)") + 6 | (6,65535) | 40 | f | (p)=("(996,996),(831,831)") + 7 | (7,65535) | 40 | f | (p)=("(1000,1000),(997,997)") +(7 rows) SELECT * FROM gist_page_items(get_raw_page('test_gist_idx', 1), 'test_gist_idx') LIMIT 5; - itemoffset | ctid | itemlen | dead | keys -------------+-------+---------+------+------------- - 1 | (0,1) | 40 | f | (p)=((1,1)) - 2 | (0,2) | 40 | f | (p)=((2,2)) - 3 | (0,3) | 40 | f | (p)=((3,3)) - 4 | (0,4) | 40 | f | (p)=((4,4)) - 5 | (0,5) | 40 | f | (p)=((5,5)) + itemoffset | ctid | itemlen | dead | keys +------------+-------+---------+------+--------------------- + 1 | (0,1) | 40 | f | (p)=("(1,1),(1,1)") + 2 | (0,2) | 40 | f | (p)=("(2,2),(2,2)") + 3 | (0,3) | 40 | f | (p)=("(3,3),(3,3)") + 4 | (0,4) | 40 | f | (p)=("(4,4),(4,4)") + 5 | (0,5) | 40 | f | (p)=("(5,5),(5,5)") (5 rows) -- gist_page_items_bytea prints the raw key data as a bytea. The output of that is @@ -99,4 +104,27 @@ SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex')); (1 row) +-- Test gist_page_items with included columns. +-- Non-leaf pages contain only the key attributes, and leaf pages contain +-- the included attributes. +ALTER TABLE test_gist ADD COLUMN i int DEFAULT NULL; +CREATE INDEX test_gist_idx_inc ON test_gist + USING gist (p) INCLUDE (t, i); +-- Mask the value of the key attribute to avoid alignment issues. +SELECT regexp_replace(keys, '\(p\)=\("(.*?)"\)', '(p)=("")') AS keys_nonleaf_1 + FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc') + WHERE itemoffset = 1; + keys_nonleaf_1 +---------------- + (p)=("") +(1 row) + +SELECT keys AS keys_leaf_1 + FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc') + WHERE itemoffset = 1; + keys_leaf_1 +------------------------------------------------------ + (p) INCLUDE (t, i)=("(1,1),(1,1)") INCLUDE (1, null) +(1 row) + DROP TABLE test_gist; diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c index 0ae8f7459c1..a9ded2037a0 100644 --- a/contrib/pageinspect/gistfuncs.c +++ b/contrib/pageinspect/gistfuncs.c @@ -21,8 +21,10 @@ #include "storage/itemptr.h" #include "utils/array.h" #include "utils/builtins.h" -#include "utils/rel.h" #include "utils/pg_lsn.h" +#include "utils/lsyscache.h" +#include "utils/rel.h" +#include "utils/ruleutils.h" #include "utils/varlena.h" PG_FUNCTION_INFO_V1(gist_page_opaque_info); @@ -233,8 +235,11 @@ gist_page_items(PG_FUNCTION_ARGS) Tuplestorestate *tupstore; MemoryContext oldcontext; Page page; + uint16 flagbits; + bits16 printflags = 0; OffsetNumber offset; OffsetNumber maxoff = InvalidOffsetNumber; + char *index_columns; if (!superuser()) ereport(ERROR, @@ -282,6 +287,27 @@ gist_page_items(PG_FUNCTION_ARGS) PG_RETURN_NULL(); } + flagbits = GistPageGetOpaque(page)->flags; + + /* + * Included attributes are added when dealing with leaf pages, discarded + * for non-leaf pages as these include only data for key attributes. + */ + printflags |= RULE_INDEXDEF_PRETTY; + if (flagbits & F_LEAF) + { + tupdesc = RelationGetDescr(indexRel); + } + else + { + tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel)); + tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel); + printflags |= RULE_INDEXDEF_KEYS_ONLY; + } + + index_columns = pg_get_indexdef_columns_extended(indexRelid, + printflags); + /* Avoid bogus PageGetMaxOffsetNumber() call with deleted pages */ if (GistPageIsDeleted(page)) elog(NOTICE, "page is deleted"); @@ -298,7 +324,8 @@ gist_page_items(PG_FUNCTION_ARGS) IndexTuple itup; Datum itup_values[INDEX_MAX_KEYS]; bool itup_isnull[INDEX_MAX_KEYS]; - char *key_desc; + StringInfoData buf; + int i; id = PageGetItemId(page, offset); @@ -307,7 +334,7 @@ gist_page_items(PG_FUNCTION_ARGS) itup = (IndexTuple) PageGetItem(page, id); - index_deform_tuple(itup, RelationGetDescr(indexRel), + index_deform_tuple(itup, tupdesc, itup_values, itup_isnull); memset(nulls, 0, sizeof(nulls)); @@ -317,16 +344,79 @@ gist_page_items(PG_FUNCTION_ARGS) values[2] = Int32GetDatum((int) IndexTupleSize(itup)); values[3] = BoolGetDatum(ItemIdIsDead(id)); - key_desc = BuildIndexValueDescription(indexRel, itup_values, itup_isnull); - if (key_desc) - values[4] = CStringGetTextDatum(key_desc); + if (index_columns) + { + initStringInfo(&buf); + appendStringInfo(&buf, "(%s)=(", index_columns); + + /* Most of this is copied from record_out(). */ + for (i = 0; i < tupdesc->natts; i++) + { + char *value; + char *tmp; + bool nq = false; + + if (itup_isnull[i]) + value = "null"; + else + { + Oid foutoid; + bool typisvarlena; + Oid typoid; + + typoid = tupdesc->attrs[i].atttypid; + getTypeOutputInfo(typoid, &foutoid, &typisvarlena); + value = OidOutputFunctionCall(foutoid, itup_values[i]); + } + + if (i == IndexRelationGetNumberOfKeyAttributes(indexRel)) + appendStringInfoString(&buf, ") INCLUDE ("); + else if (i > 0) + appendStringInfoString(&buf, ", "); + + /* Check whether we need double quotes for this value */ + nq = (value[0] == '\0'); /* force quotes for empty string */ + for (tmp = value; *tmp; tmp++) + { + char ch = *tmp; + + if (ch == '"' || ch == '\\' || + ch == '(' || ch == ')' || ch == ',' || + isspace((unsigned char) ch)) + { + nq = true; + break; + } + } + + /* And emit the string */ + if (nq) + appendStringInfoCharMacro(&buf, '"'); + for (tmp = value; *tmp; tmp++) + { + char ch = *tmp; + + if (ch == '"' || ch == '\\') + appendStringInfoCharMacro(&buf, ch); + appendStringInfoCharMacro(&buf, ch); + } + if (nq) + appendStringInfoCharMacro(&buf, '"'); + } + + appendStringInfoChar(&buf, ')'); + + values[4] = CStringGetTextDatum(buf.data); + nulls[4] = false; + } else { values[4] = (Datum) 0; nulls[4] = true; } - tuplestore_putvalues(tupstore, tupdesc, values, nulls); + tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, + values, nulls); } relation_close(indexRel, AccessShareLock); diff --git a/contrib/pageinspect/sql/gist.sql b/contrib/pageinspect/sql/gist.sql index 963d5d40a3c..7efd9196792 100644 --- a/contrib/pageinspect/sql/gist.sql +++ b/contrib/pageinspect/sql/gist.sql @@ -52,4 +52,18 @@ SELECT gist_page_items_bytea(decode(repeat('00', :block_size), 'hex')); SELECT gist_page_items(decode(repeat('00', :block_size), 'hex'), 'test_gist_idx'::regclass); SELECT gist_page_opaque_info(decode(repeat('00', :block_size), 'hex')); +-- Test gist_page_items with included columns. +-- Non-leaf pages contain only the key attributes, and leaf pages contain +-- the included attributes. +ALTER TABLE test_gist ADD COLUMN i int DEFAULT NULL; +CREATE INDEX test_gist_idx_inc ON test_gist + USING gist (p) INCLUDE (t, i); +-- Mask the value of the key attribute to avoid alignment issues. +SELECT regexp_replace(keys, '\(p\)=\("(.*?)"\)', '(p)=("")') AS keys_nonleaf_1 + FROM gist_page_items(get_raw_page('test_gist_idx_inc', 0), 'test_gist_idx_inc') + WHERE itemoffset = 1; +SELECT keys AS keys_leaf_1 + FROM gist_page_items(get_raw_page('test_gist_idx_inc', 1), 'test_gist_idx_inc') + WHERE itemoffset = 1; + DROP TABLE test_gist; diff --git a/doc/src/sgml/pageinspect.sgml b/doc/src/sgml/pageinspect.sgml index 24b5e463ed9..58dc93d7f13 100644 --- a/doc/src/sgml/pageinspect.sgml +++ b/doc/src/sgml/pageinspect.sgml @@ -714,15 +714,15 @@ test=# SELECT * FROM gist_page_opaque_info(get_raw_page('test_gist_idx', 2)); the data stored in a page of a GiST index. For example: test=# SELECT * FROM gist_page_items(get_raw_page('test_gist_idx', 0), 'test_gist_idx'); - itemoffset | ctid | itemlen | dead | keys -------------+-----------+---------+------+------------------- - 1 | (1,65535) | 40 | f | (p)=((166,166)) - 2 | (2,65535) | 40 | f | (p)=((332,332)) - 3 | (3,65535) | 40 | f | (p)=((498,498)) - 4 | (4,65535) | 40 | f | (p)=((664,664)) - 5 | (5,65535) | 40 | f | (p)=((830,830)) - 6 | (6,65535) | 40 | f | (p)=((996,996)) - 7 | (7,65535) | 40 | f | (p)=((1000,1000)) + itemoffset | ctid | itemlen | dead | keys +------------+-----------+---------+------+------------------------------- + 1 | (1,65535) | 40 | f | (p)=("(166,166),(1,1)") + 2 | (2,65535) | 40 | f | (p)=("(332,332),(167,167)") + 3 | (3,65535) | 40 | f | (p)=("(498,498),(333,333)") + 4 | (4,65535) | 40 | f | (p)=("(664,664),(499,499)") + 5 | (5,65535) | 40 | f | (p)=("(830,830),(665,665)") + 6 | (6,65535) | 40 | f | (p)=("(996,996),(831,831)") + 7 | (7,65535) | 40 | f | (p)=("(1000,1000),(997,997)") (7 rows) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index dbbb2a70a07..5a25c76c775 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -1204,6 +1204,22 @@ pg_get_indexdef_columns(Oid indexrelid, bool pretty) prettyFlags, false); } +/* Internal version, extensible with flags to control its behavior */ +char * +pg_get_indexdef_columns_extended(Oid indexrelid, bits16 flags) +{ + bool pretty = ((flags & RULE_INDEXDEF_PRETTY) != 0); + bool keys_only = ((flags & RULE_INDEXDEF_KEYS_ONLY) != 0); + int prettyFlags; + + prettyFlags = pretty ? (PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_SCHEMA) : PRETTYFLAG_INDENT; + + return pg_get_indexdef_worker(indexrelid, 0, NULL, + true, keys_only, + false, false, + prettyFlags, false); +} + /* * Internal workhorse to decompile an index definition. * diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h index d333e5e8a56..030c996808c 100644 --- a/src/include/utils/ruleutils.h +++ b/src/include/utils/ruleutils.h @@ -20,9 +20,14 @@ struct Plan; /* avoid including plannodes.h here */ struct PlannedStmt; +/* Flags for pg_get_indexdef_columns_extended() */ +#define RULE_INDEXDEF_PRETTY 0x01 +#define RULE_INDEXDEF_KEYS_ONLY 0x02 /* ignore included attributes */ extern char *pg_get_indexdef_string(Oid indexrelid); extern char *pg_get_indexdef_columns(Oid indexrelid, bool pretty); +extern char *pg_get_indexdef_columns_extended(Oid indexrelid, + bits16 flags); extern char *pg_get_partkeydef_columns(Oid relid, bool pretty); extern char *pg_get_partconstrdef_string(Oid partitionId, char *aliasname); From 8bb1ac286bf9d26305cfaeb2b258550a3c5e822d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 19 May 2023 10:57:46 -0400 Subject: [PATCH 05/45] Avoid naming conflict between transactions.sql and namespace.sql. Commits 681d9e462 et al added a test case in namespace.sql that implicitly relied on there not being a table "public.abc". However, the concurrently-run transactions.sql test creates precisely such a table, so with the right timing you'd get a failure. Creating a table named as generically as "abc" in a common schema seems like bad practice, so fix this by changing the name of transactions.sql's table. (Compare 2cf8c7aa4.) Marina Polyakova Discussion: https://postgr.es/m/80d0201636665d82185942e7112257b4@postgrespro.ru --- src/test/regress/expected/transactions.out | 76 +++++++++++----------- src/test/regress/sql/transactions.sql | 64 +++++++++--------- 2 files changed, 70 insertions(+), 70 deletions(-) diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out index 5ed976901ca..5dcd21665d7 100644 --- a/src/test/regress/expected/transactions.out +++ b/src/test/regress/expected/transactions.out @@ -578,10 +578,10 @@ drop function inverse(int); -- performed in the aborted subtransaction begin; savepoint x; -create table abc (a int); -insert into abc values (5); -insert into abc values (10); -declare foo cursor for select * from abc; +create table trans_abc (a int); +insert into trans_abc values (5); +insert into trans_abc values (10); +declare foo cursor for select * from trans_abc; fetch from foo; a --- @@ -594,11 +594,11 @@ fetch from foo; ERROR: cursor "foo" does not exist commit; begin; -create table abc (a int); -insert into abc values (5); -insert into abc values (10); -insert into abc values (15); -declare foo cursor for select * from abc; +create table trans_abc (a int); +insert into trans_abc values (5); +insert into trans_abc values (10); +insert into trans_abc values (15); +declare foo cursor for select * from trans_abc; -- CBDB: the order of value is not guaranteed -- start_ignore fetch from foo; @@ -680,7 +680,7 @@ COMMIT; DROP FUNCTION create_temp_tab(); DROP FUNCTION invert(x float8); -- Tests for AND CHAIN -CREATE TABLE abc (a int); +CREATE TABLE trans_abc (a int); -- set nondefault value so we have something to override below SET default_transaction_read_only = on; START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE; @@ -702,8 +702,8 @@ SHOW transaction_deferrable; on (1 row) -INSERT INTO abc VALUES (1); -INSERT INTO abc VALUES (2); +INSERT INTO trans_abc VALUES (1); +INSERT INTO trans_abc VALUES (2); COMMIT AND CHAIN; -- TBLOCK_END SHOW transaction_isolation; transaction_isolation @@ -723,11 +723,11 @@ SHOW transaction_deferrable; on (1 row) -INSERT INTO abc VALUES ('error'); +INSERT INTO trans_abc VALUES ('error'); ERROR: invalid input syntax for type integer: "error" -LINE 1: INSERT INTO abc VALUES ('error'); - ^ -INSERT INTO abc VALUES (3); -- check it's really aborted +LINE 1: INSERT INTO trans_abc VALUES ('error'); + ^ +INSERT INTO trans_abc VALUES (3); -- check it's really aborted ERROR: current transaction is aborted, commands ignored until end of transaction block COMMIT AND CHAIN; -- TBLOCK_ABORT_END SHOW transaction_isolation; @@ -748,7 +748,7 @@ SHOW transaction_deferrable; on (1 row) -INSERT INTO abc VALUES (4); +INSERT INTO trans_abc VALUES (4); COMMIT; START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE; SHOW transaction_isolation; @@ -770,10 +770,10 @@ SHOW transaction_deferrable; (1 row) SAVEPOINT x; -INSERT INTO abc VALUES ('error'); +INSERT INTO trans_abc VALUES ('error'); ERROR: invalid input syntax for type integer: "error" -LINE 1: INSERT INTO abc VALUES ('error'); - ^ +LINE 1: INSERT INTO trans_abc VALUES ('error'); + ^ COMMIT AND CHAIN; -- TBLOCK_ABORT_PENDING SHOW transaction_isolation; transaction_isolation @@ -793,7 +793,7 @@ SHOW transaction_deferrable; on (1 row) -INSERT INTO abc VALUES (5); +INSERT INTO trans_abc VALUES (5); COMMIT; START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE; SHOW transaction_isolation; @@ -855,7 +855,7 @@ SHOW transaction_deferrable; off (1 row) -INSERT INTO abc VALUES (6); +INSERT INTO trans_abc VALUES (6); ROLLBACK AND CHAIN; -- TBLOCK_ABORT_PENDING SHOW transaction_isolation; transaction_isolation @@ -875,10 +875,10 @@ SHOW transaction_deferrable; off (1 row) -INSERT INTO abc VALUES ('error'); +INSERT INTO trans_abc VALUES ('error'); ERROR: invalid input syntax for type integer: "error" -LINE 1: INSERT INTO abc VALUES ('error'); - ^ +LINE 1: INSERT INTO trans_abc VALUES ('error'); + ^ ROLLBACK AND CHAIN; -- TBLOCK_ABORT_END SHOW transaction_isolation; transaction_isolation @@ -904,7 +904,7 @@ COMMIT AND CHAIN; -- error ERROR: COMMIT AND CHAIN can only be used in transaction blocks ROLLBACK AND CHAIN; -- error ERROR: ROLLBACK AND CHAIN can only be used in transaction blocks -SELECT * FROM abc ORDER BY 1; +SELECT * FROM trans_abc ORDER BY 1; a --- 1 @@ -914,7 +914,7 @@ SELECT * FROM abc ORDER BY 1; (4 rows) RESET default_transaction_read_only; -DROP TABLE abc; +DROP TABLE trans_abc; -- Test assorted behaviors around the implicit transaction block created -- when multiple SQL commands are sent in a single Query message. These -- tests rely on the fact that psql will not break SQL commands apart at a @@ -1016,21 +1016,21 @@ SHOW transaction_read_only; off (1 row) -CREATE TABLE abc (a int); +CREATE TABLE trans_abc (a int); -- COMMIT/ROLLBACK + COMMIT/ROLLBACK AND CHAIN -INSERT INTO abc VALUES (7)\; COMMIT\; INSERT INTO abc VALUES (8)\; COMMIT AND CHAIN; -- 7 commit, 8 error +INSERT INTO trans_abc VALUES (7)\; COMMIT\; INSERT INTO trans_abc VALUES (8)\; COMMIT AND CHAIN; -- 7 commit, 8 error WARNING: there is no transaction in progress ERROR: COMMIT AND CHAIN can only be used in transaction blocks -INSERT INTO abc VALUES (9)\; ROLLBACK\; INSERT INTO abc VALUES (10)\; ROLLBACK AND CHAIN; -- 9 rollback, 10 error +INSERT INTO trans_abc VALUES (9)\; ROLLBACK\; INSERT INTO trans_abc VALUES (10)\; ROLLBACK AND CHAIN; -- 9 rollback, 10 error WARNING: there is no transaction in progress ERROR: ROLLBACK AND CHAIN can only be used in transaction blocks -- COMMIT/ROLLBACK AND CHAIN + COMMIT/ROLLBACK -INSERT INTO abc VALUES (11)\; COMMIT AND CHAIN\; INSERT INTO abc VALUES (12)\; COMMIT; -- 11 error, 12 not reached +INSERT INTO trans_abc VALUES (11)\; COMMIT AND CHAIN\; INSERT INTO trans_abc VALUES (12)\; COMMIT; -- 11 error, 12 not reached ERROR: COMMIT AND CHAIN can only be used in transaction blocks -INSERT INTO abc VALUES (13)\; ROLLBACK AND CHAIN\; INSERT INTO abc VALUES (14)\; ROLLBACK; -- 13 error, 14 not reached +INSERT INTO trans_abc VALUES (13)\; ROLLBACK AND CHAIN\; INSERT INTO trans_abc VALUES (14)\; ROLLBACK; -- 13 error, 14 not reached ERROR: ROLLBACK AND CHAIN can only be used in transaction blocks -- START TRANSACTION + COMMIT/ROLLBACK AND CHAIN -START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO abc VALUES (15)\; COMMIT AND CHAIN; -- 15 ok +START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO trans_abc VALUES (15)\; COMMIT AND CHAIN; -- 15 ok SHOW transaction_isolation; -- transaction is active at this point transaction_isolation ----------------------- @@ -1038,7 +1038,7 @@ SHOW transaction_isolation; -- transaction is active at this point (1 row) COMMIT; -START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO abc VALUES (16)\; ROLLBACK AND CHAIN; -- 16 ok +START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO trans_abc VALUES (16)\; ROLLBACK AND CHAIN; -- 16 ok SHOW transaction_isolation; -- transaction is active at this point transaction_isolation ----------------------- @@ -1048,7 +1048,7 @@ SHOW transaction_isolation; -- transaction is active at this point ROLLBACK; SET default_transaction_isolation = 'read committed'; -- START TRANSACTION + COMMIT/ROLLBACK + COMMIT/ROLLBACK AND CHAIN -START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO abc VALUES (17)\; COMMIT\; INSERT INTO abc VALUES (18)\; COMMIT AND CHAIN; -- 17 commit, 18 error +START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO trans_abc VALUES (17)\; COMMIT\; INSERT INTO trans_abc VALUES (18)\; COMMIT AND CHAIN; -- 17 commit, 18 error ERROR: COMMIT AND CHAIN can only be used in transaction blocks SHOW transaction_isolation; -- out of transaction block transaction_isolation @@ -1056,7 +1056,7 @@ SHOW transaction_isolation; -- out of transaction block read committed (1 row) -START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO abc VALUES (19)\; ROLLBACK\; INSERT INTO abc VALUES (20)\; ROLLBACK AND CHAIN; -- 19 rollback, 20 error +START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO trans_abc VALUES (19)\; ROLLBACK\; INSERT INTO trans_abc VALUES (20)\; ROLLBACK AND CHAIN; -- 19 rollback, 20 error ERROR: ROLLBACK AND CHAIN can only be used in transaction blocks SHOW transaction_isolation; -- out of transaction block transaction_isolation @@ -1065,7 +1065,7 @@ SHOW transaction_isolation; -- out of transaction block (1 row) RESET default_transaction_isolation; -SELECT * FROM abc ORDER BY 1; +SELECT * FROM trans_abc ORDER BY 1; a ---- 7 @@ -1073,7 +1073,7 @@ SELECT * FROM abc ORDER BY 1; 17 (3 rows) -DROP TABLE abc; +DROP TABLE trans_abc; -- Test for successful cleanup of an aborted transaction at session exit. -- THIS MUST BE THE LAST TEST IN THIS FILE. begin; diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql index 4df2f73ff8f..5fd3c85ed95 100644 --- a/src/test/regress/sql/transactions.sql +++ b/src/test/regress/sql/transactions.sql @@ -373,10 +373,10 @@ drop function inverse(int); begin; savepoint x; -create table abc (a int); -insert into abc values (5); -insert into abc values (10); -declare foo cursor for select * from abc; +create table trans_abc (a int); +insert into trans_abc values (5); +insert into trans_abc values (10); +declare foo cursor for select * from trans_abc; fetch from foo; rollback to x; @@ -386,11 +386,11 @@ commit; begin; -create table abc (a int); -insert into abc values (5); -insert into abc values (10); -insert into abc values (15); -declare foo cursor for select * from abc; +create table trans_abc (a int); +insert into trans_abc values (5); +insert into trans_abc values (10); +insert into trans_abc values (15); +declare foo cursor for select * from trans_abc; -- CBDB: the order of value is not guaranteed -- start_ignore @@ -449,7 +449,7 @@ DROP FUNCTION invert(x float8); -- Tests for AND CHAIN -CREATE TABLE abc (a int); +CREATE TABLE trans_abc (a int); -- set nondefault value so we have something to override below SET default_transaction_read_only = on; @@ -458,19 +458,19 @@ START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE; SHOW transaction_isolation; SHOW transaction_read_only; SHOW transaction_deferrable; -INSERT INTO abc VALUES (1); -INSERT INTO abc VALUES (2); +INSERT INTO trans_abc VALUES (1); +INSERT INTO trans_abc VALUES (2); COMMIT AND CHAIN; -- TBLOCK_END SHOW transaction_isolation; SHOW transaction_read_only; SHOW transaction_deferrable; -INSERT INTO abc VALUES ('error'); -INSERT INTO abc VALUES (3); -- check it's really aborted +INSERT INTO trans_abc VALUES ('error'); +INSERT INTO trans_abc VALUES (3); -- check it's really aborted COMMIT AND CHAIN; -- TBLOCK_ABORT_END SHOW transaction_isolation; SHOW transaction_read_only; SHOW transaction_deferrable; -INSERT INTO abc VALUES (4); +INSERT INTO trans_abc VALUES (4); COMMIT; START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE; @@ -478,12 +478,12 @@ SHOW transaction_isolation; SHOW transaction_read_only; SHOW transaction_deferrable; SAVEPOINT x; -INSERT INTO abc VALUES ('error'); +INSERT INTO trans_abc VALUES ('error'); COMMIT AND CHAIN; -- TBLOCK_ABORT_PENDING SHOW transaction_isolation; SHOW transaction_read_only; SHOW transaction_deferrable; -INSERT INTO abc VALUES (5); +INSERT INTO trans_abc VALUES (5); COMMIT; START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE; @@ -502,12 +502,12 @@ START TRANSACTION ISOLATION LEVEL SERIALIZABLE, READ WRITE, NOT DEFERRABLE; SHOW transaction_isolation; SHOW transaction_read_only; SHOW transaction_deferrable; -INSERT INTO abc VALUES (6); +INSERT INTO trans_abc VALUES (6); ROLLBACK AND CHAIN; -- TBLOCK_ABORT_PENDING SHOW transaction_isolation; SHOW transaction_read_only; SHOW transaction_deferrable; -INSERT INTO abc VALUES ('error'); +INSERT INTO trans_abc VALUES ('error'); ROLLBACK AND CHAIN; -- TBLOCK_ABORT_END SHOW transaction_isolation; SHOW transaction_read_only; @@ -518,11 +518,11 @@ ROLLBACK; COMMIT AND CHAIN; -- error ROLLBACK AND CHAIN; -- error -SELECT * FROM abc ORDER BY 1; +SELECT * FROM trans_abc ORDER BY 1; RESET default_transaction_read_only; -DROP TABLE abc; +DROP TABLE trans_abc; -- Test assorted behaviors around the implicit transaction block created @@ -587,39 +587,39 @@ SHOW transaction_read_only; SET TRANSACTION READ ONLY\; ROLLBACK AND CHAIN; -- error SHOW transaction_read_only; -CREATE TABLE abc (a int); +CREATE TABLE trans_abc (a int); -- COMMIT/ROLLBACK + COMMIT/ROLLBACK AND CHAIN -INSERT INTO abc VALUES (7)\; COMMIT\; INSERT INTO abc VALUES (8)\; COMMIT AND CHAIN; -- 7 commit, 8 error -INSERT INTO abc VALUES (9)\; ROLLBACK\; INSERT INTO abc VALUES (10)\; ROLLBACK AND CHAIN; -- 9 rollback, 10 error +INSERT INTO trans_abc VALUES (7)\; COMMIT\; INSERT INTO trans_abc VALUES (8)\; COMMIT AND CHAIN; -- 7 commit, 8 error +INSERT INTO trans_abc VALUES (9)\; ROLLBACK\; INSERT INTO trans_abc VALUES (10)\; ROLLBACK AND CHAIN; -- 9 rollback, 10 error -- COMMIT/ROLLBACK AND CHAIN + COMMIT/ROLLBACK -INSERT INTO abc VALUES (11)\; COMMIT AND CHAIN\; INSERT INTO abc VALUES (12)\; COMMIT; -- 11 error, 12 not reached -INSERT INTO abc VALUES (13)\; ROLLBACK AND CHAIN\; INSERT INTO abc VALUES (14)\; ROLLBACK; -- 13 error, 14 not reached +INSERT INTO trans_abc VALUES (11)\; COMMIT AND CHAIN\; INSERT INTO trans_abc VALUES (12)\; COMMIT; -- 11 error, 12 not reached +INSERT INTO trans_abc VALUES (13)\; ROLLBACK AND CHAIN\; INSERT INTO trans_abc VALUES (14)\; ROLLBACK; -- 13 error, 14 not reached -- START TRANSACTION + COMMIT/ROLLBACK AND CHAIN -START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO abc VALUES (15)\; COMMIT AND CHAIN; -- 15 ok +START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO trans_abc VALUES (15)\; COMMIT AND CHAIN; -- 15 ok SHOW transaction_isolation; -- transaction is active at this point COMMIT; -START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO abc VALUES (16)\; ROLLBACK AND CHAIN; -- 16 ok +START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO trans_abc VALUES (16)\; ROLLBACK AND CHAIN; -- 16 ok SHOW transaction_isolation; -- transaction is active at this point ROLLBACK; SET default_transaction_isolation = 'read committed'; -- START TRANSACTION + COMMIT/ROLLBACK + COMMIT/ROLLBACK AND CHAIN -START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO abc VALUES (17)\; COMMIT\; INSERT INTO abc VALUES (18)\; COMMIT AND CHAIN; -- 17 commit, 18 error +START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO trans_abc VALUES (17)\; COMMIT\; INSERT INTO trans_abc VALUES (18)\; COMMIT AND CHAIN; -- 17 commit, 18 error SHOW transaction_isolation; -- out of transaction block -START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO abc VALUES (19)\; ROLLBACK\; INSERT INTO abc VALUES (20)\; ROLLBACK AND CHAIN; -- 19 rollback, 20 error +START TRANSACTION ISOLATION LEVEL REPEATABLE READ\; INSERT INTO trans_abc VALUES (19)\; ROLLBACK\; INSERT INTO trans_abc VALUES (20)\; ROLLBACK AND CHAIN; -- 19 rollback, 20 error SHOW transaction_isolation; -- out of transaction block RESET default_transaction_isolation; -SELECT * FROM abc ORDER BY 1; +SELECT * FROM trans_abc ORDER BY 1; -DROP TABLE abc; +DROP TABLE trans_abc; -- Test for successful cleanup of an aborted transaction at session exit. From 284c2d64a93e954a13ca3d8ddb2d3989312e9ba0 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Thu, 25 May 2023 15:32:53 -0700 Subject: [PATCH 06/45] nbtree VACUUM: cope with right sibling link corruption. Avoid "right sibling's left-link doesn't match" errors when vacuuming a corrupt nbtree index. Just LOG the issue and press on. That way VACUUM will have a decent chance of finishing off all required processing for the index (and for the table as a whole). This error was seen in the field from time to time (it's more than a theoretical risk), so giving VACUUM the ability to press on like this has real value. Nothing short of a REINDEX is expected to fix the underlying index corruption, so giving up (by throwing an error) risks making a bad situation far worse. Anything that blocks forward progress by VACUUM like this might go unnoticed for a long time. This could eventually lead to a wraparound/xidStopLimit outage. Note that _bt_unlink_halfdead_page() has always been able to bail on page deletion when the target page's left sibling page was in an inconsistent state. It now does the same thing (returns false to back out of the second phase of deletion) when it notices sibling link corruption in the target page's right sibling page. This is similar to the work from commit 5b861baa (later backpatched as commit 43e409ce), which taught nbtree to press on with vacuuming an index when page deletion fails to "re-find" a downlink in the target page's parent page. The "re-find" check seems to make VACUUM bail on page deletion more often in practice, but there is no reason to take any chances here. Author: Peter Geoghegan Reviewed-By: Heikki Linnakangas Discussion: https://postgr.es/m/CAH2-Wzko2q2kP1+UvgJyP9g0mF4hopK0NtQZcxwvMv9_ytGhkQ@mail.gmail.com Backpatch: 11- (all supported versions). --- src/backend/access/nbtree/nbtpage.c | 62 ++++++++++++++++++++--------- src/backend/access/nbtree/nbtree.c | 3 +- 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c index 901573fa433..1e27ee4c569 100644 --- a/src/backend/access/nbtree/nbtpage.c +++ b/src/backend/access/nbtree/nbtpage.c @@ -2420,24 +2420,22 @@ _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf, BlockNumber scanblkno, if (!leftsibvalid) { - if (target != leafblkno) - { - /* we have only a pin on target, but pin+lock on leafbuf */ - ReleaseBuffer(buf); - _bt_relbuf(rel, leafbuf); - } - else - { - /* we have only a pin on leafbuf */ - ReleaseBuffer(leafbuf); - } - + /* + * This is known to fail in the field; sibling link corruption + * is relatively common. Press on with vacuuming rather than + * just throwing an ERROR. + */ ereport(LOG, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg_internal("valid left sibling for deletion target could not be located: " - "left sibling %u of target %u with leafblkno %u and scanblkno %u in index \"%s\"", + "left sibling %u of target %u with leafblkno %u and scanblkno %u on level %u of index \"%s\"", leftsib, target, leafblkno, scanblkno, - RelationGetRelationName(rel)))); + targetlevel, RelationGetRelationName(rel)))); + + /* Must release all pins and locks on failure exit */ + ReleaseBuffer(buf); + if (target != leafblkno) + _bt_relbuf(rel, leafbuf); return false; } @@ -2512,13 +2510,40 @@ _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf, BlockNumber scanblkno, rbuf = _bt_getbuf(rel, rightsib, BT_WRITE); page = BufferGetPage(rbuf); opaque = (BTPageOpaque) PageGetSpecialPointer(page); + + /* + * Validate target's right sibling page. Its left link must point back to + * the target page. + */ if (opaque->btpo_prev != target) - ereport(ERROR, + { + /* + * This is known to fail in the field; sibling link corruption is + * relatively common. Press on with vacuuming rather than just + * throwing an ERROR (same approach used for left-sibling's-right-link + * validation check a moment ago). + */ + ereport(LOG, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg_internal("right sibling's left-link doesn't match: " - "block %u links to %u instead of expected %u in index \"%s\"", - rightsib, opaque->btpo_prev, target, - RelationGetRelationName(rel)))); + "right sibling %u of target %u with leafblkno %u " + "and scanblkno %u spuriously links to non-target %u " + "on level %u of index \"%s\"", + rightsib, target, leafblkno, + scanblkno, opaque->btpo_prev, + targetlevel, RelationGetRelationName(rel)))); + + /* Must release all pins and locks on failure exit */ + if (BufferIsValid(lbuf)) + _bt_relbuf(rel, lbuf); + _bt_relbuf(rel, rbuf); + _bt_relbuf(rel, buf); + if (target != leafblkno) + _bt_relbuf(rel, leafbuf); + + return false; + } + rightsib_is_rightmost = P_RIGHTMOST(opaque); *rightsib_empty = (P_FIRSTDATAKEY(opaque) > PageGetMaxOffsetNumber(page)); @@ -2742,6 +2767,7 @@ _bt_unlink_halfdead_page(Relation rel, Buffer leafbuf, BlockNumber scanblkno, */ _bt_pendingfsm_add(vstate, target, safexid); + /* Success - hold on to lock on leafbuf (might also have been target) */ return true; } diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c index 7c3563024db..8edf4694401 100644 --- a/src/backend/access/nbtree/nbtree.c +++ b/src/backend/access/nbtree/nbtree.c @@ -1196,8 +1196,7 @@ btvacuumpage(BTVacState *vstate, BlockNumber scanblkno) * can't be half-dead because only an interrupted VACUUM process can * leave pages in that state, so we'd definitely have dealt with it * back when the page was the scanblkno page (half-dead pages are - * always marked fully deleted by _bt_pagedel()). This assumes that - * there can be only one vacuum process running at a time. + * always marked fully deleted by _bt_pagedel(), barring corruption). */ if (!opaque || !P_ISLEAF(opaque) || P_ISHALFDEAD(opaque)) { From 6d7f2bb43459ecda7bd7166af2271dc2475576ec Mon Sep 17 00:00:00 2001 From: Bruce Momjian Date: Thu, 1 Jun 2023 10:22:16 -0400 Subject: [PATCH 07/45] doc: add missing "the" in LATERAL sentence. Backpatch-through: 11 --- doc/src/sgml/queries.sgml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/src/sgml/queries.sgml b/doc/src/sgml/queries.sgml index 95559ef1ac4..cf9c0890af3 100644 --- a/doc/src/sgml/queries.sgml +++ b/doc/src/sgml/queries.sgml @@ -858,7 +858,7 @@ ORDER BY p; - A LATERAL item can appear at top level in the + A LATERAL item can appear at the top level in the FROM list, or within a JOIN tree. In the latter case it can also refer to any items that are on the left-hand side of a JOIN that it is on the right-hand side of. From d86b6fd21689f01918e0c2a5f26fb21d0edb0b44 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 4 Jun 2023 13:05:54 -0400 Subject: [PATCH 08/45] Fix pg_dump's failure to honor dependencies of SQL functions. A new-style SQL function can contain a parse-time dependency on a unique index, much as views and matviews can (such cases arise from GROUP BY and ON CONFLICT clauses, for example). To dump and restore such a function successfully, pg_dump must postpone the function until after the unique index is created, which will happen in the post-data part of the dump. Therefore we have to remove the normal constraint that functions are dumped in pre-data. Add code similar to the existing logic that handles this for matviews. I added test cases for both as well, since code coverage tests showed that we weren't testing the matview logic. Per report from Sami Imseih. Back-patch to v14 where new-style SQL functions came in. Discussion: https://postgr.es/m/2C1933AB-C2F8-499B-9D18-4AC1882256A0@amazon.com --- src/bin/pg_dump/pg_dump.c | 5 +++- src/bin/pg_dump/pg_dump.h | 5 ++++ src/bin/pg_dump/pg_dump_sort.c | 44 ++++++++++++++++++++++++++++++++ src/bin/pg_dump/t/002_pg_dump.pl | 40 +++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 1 deletion(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 23757c62573..fbd67b5599e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -6697,6 +6697,7 @@ getAggregates(Archive *fout, int *numAggs) agginfo[i].aggfn.argtypes, agginfo[i].aggfn.nargs); } + agginfo[i].aggfn.postponed_def = false; /* might get set during sort */ /* Decide whether we want to dump it */ selectDumpableAggregate(&(agginfo[i]), fout); @@ -7030,6 +7031,7 @@ getFuncs(Archive *fout, int *numFuncs) parseOidArray(PQgetvalue(res, i, i_proargtypes), finfo[i].argtypes, finfo[i].nargs); } + finfo[i].postponed_def = false; /* might get set during sort */ /* Decide whether we want to dump it */ selectDumpableFunction(&finfo[i], fout); @@ -13733,7 +13735,8 @@ dumpFunc(Archive *fout, const FuncInfo *finfo) .namespace = finfo->dobj.namespace->dobj.name, .owner = finfo->rolname, .description = keyword, - .section = SECTION_PRE_DATA, + .section = finfo->postponed_def ? + SECTION_POST_DATA : SECTION_PRE_DATA, .createStmt = q->data, .dropStmt = delqry->data)); diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index d53d2ab623e..d3d3b9a90d2 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -242,6 +242,11 @@ typedef struct _funcInfo int nargs; Oid *argtypes; Oid prorettype; + char *proacl; + char *rproacl; + char *initproacl; + char *initrproacl; + bool postponed_def; /* function must be postponed into post-data */ } FuncInfo; /* AggInfo is a superset of FuncInfo */ diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index c35a6da1f23..de182562a93 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -867,6 +867,28 @@ repairMatViewBoundaryMultiLoop(DumpableObject *boundaryobj, } } +/* + * If a function is involved in a multi-object loop, we can't currently fix + * that by splitting it into two DumpableObjects. As a stopgap, we try to fix + * it by dropping the constraint that the function be dumped in the pre-data + * section. This is sufficient to handle cases where a function depends on + * some unique index, as can happen if it has a GROUP BY for example. + */ +static void +repairFunctionBoundaryMultiLoop(DumpableObject *boundaryobj, + DumpableObject *nextobj) +{ + /* remove boundary's dependency on object after it in loop */ + removeObjectDependency(boundaryobj, nextobj->dumpId); + /* if that object is a function, mark it as postponed into post-data */ + if (nextobj->objType == DO_FUNC) + { + FuncInfo *nextinfo = (FuncInfo *) nextobj; + + nextinfo->postponed_def = true; + } +} + /* * Because we make tables depend on their CHECK constraints, while there * will be an automatic dependency in the other direction, we need to break @@ -1061,6 +1083,28 @@ repairDependencyLoop(DumpableObject **loop, } } + /* Indirect loop involving function and data boundary */ + if (nLoop > 2) + { + for (i = 0; i < nLoop; i++) + { + if (loop[i]->objType == DO_FUNC) + { + for (j = 0; j < nLoop; j++) + { + if (loop[j]->objType == DO_PRE_DATA_BOUNDARY) + { + DumpableObject *nextobj; + + nextobj = (j < nLoop - 1) ? loop[j + 1] : loop[0]; + repairFunctionBoundaryMultiLoop(loop[j], nextobj); + return; + } + } + } + } + } + /* Table and CHECK constraint */ if (nLoop == 2 && loop[0]->objType == DO_TABLE && diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index 5ec7e3f2b20..c472d5d5cee 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -2167,6 +2167,27 @@ }, }, + 'Check ordering of a function that depends on a primary key' => { + create_order => 41, + create_sql => ' + CREATE TABLE dump_test.ordering_table (id int primary key, data int); + CREATE FUNCTION dump_test.ordering_func () + RETURNS SETOF dump_test.ordering_table + LANGUAGE sql BEGIN ATOMIC + SELECT * FROM dump_test.ordering_table GROUP BY id; END;', + regexp => qr/^ + \QALTER TABLE ONLY dump_test.ordering_table\E + \n\s+\QADD CONSTRAINT ordering_table_pkey PRIMARY KEY (id);\E + .*^ + \QCREATE FUNCTION dump_test.ordering_func\E/xms, + like => + { %full_runs, %dump_test_schema_runs, section_post_data => 1, }, + unlike => { + exclude_dump_test_schema => 1, + only_dump_measurement => 1, + }, + }, + 'CREATE PROCEDURE dump_test.ptest1' => { create_order => 41, create_sql => 'CREATE PROCEDURE dump_test.ptest1(a int) @@ -2472,6 +2493,25 @@ }, }, + 'Check ordering of a matview that depends on a primary key' => { + create_order => 42, + create_sql => ' + CREATE MATERIALIZED VIEW dump_test.ordering_view AS + SELECT * FROM dump_test.ordering_table GROUP BY id;', + regexp => qr/^ + \QALTER TABLE ONLY dump_test.ordering_table\E + \n\s+\QADD CONSTRAINT ordering_table_pkey PRIMARY KEY (id);\E + .*^ + \QCREATE MATERIALIZED VIEW dump_test.ordering_view AS\E + \n\s+\QSELECT ordering_table.id,\E/xms, + like => + { %full_runs, %dump_test_schema_runs, section_post_data => 1, }, + unlike => { + exclude_dump_test_schema => 1, + only_dump_measurement => 1, + }, + }, + 'CREATE POLICY p1 ON test_table' => { create_order => 22, create_sql => 'CREATE POLICY p1 ON dump_test.test_table From 963e26871b3ecf0ea36d3edbd009f8fb2e8a7e86 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 4 Jun 2023 13:27:34 -0400 Subject: [PATCH 09/45] Doc: explain about dependency tracking for new-style SQL functions. 5.14 Dependency Tracking was not updated when we added new-style SQL functions. Improve that. Noted by Sami Imseih. Back-patch to v14 where new-style SQL functions came in. Discussion: https://postgr.es/m/2C1933AB-C2F8-499B-9D18-4AC1882256A0@amazon.com --- doc/src/sgml/ddl.sgml | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 53ca87cc020..cae307470c8 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5072,8 +5072,9 @@ DROP TABLE products CASCADE; - For user-defined functions, PostgreSQL tracks - dependencies associated with a function's externally-visible properties, + For a user-defined function or procedure whose body is defined as a string + literal, PostgreSQL tracks + dependencies associated with the function's externally-visible properties, such as its argument and result types, but not dependencies that could only be known by examining the function body. As an example, consider this situation: @@ -5101,6 +5102,23 @@ CREATE FUNCTION get_color_note (rainbow) RETURNS text AS table is missing, though executing it would cause an error; creating a new table of the same name would allow the function to work again. + + + On the other hand, for a SQL-language function or procedure whose body + is written in SQL-standard style, the body is parsed at function + definition time and all dependencies recognized by the parser are + stored. Thus, if we write the function above as + + +CREATE FUNCTION get_color_note (rainbow) RETURNS text +BEGIN ATOMIC + SELECT note FROM my_colors WHERE color = $1; +END; + + + then the function's dependency on the my_colors + table will be known and enforced by DROP. + From e0d769582f72ae19666ddecb7b98ad5378475947 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Wed, 7 Jun 2023 16:48:50 +0200 Subject: [PATCH 10/45] Use per-tuple context in ExecGetAllUpdatedCols Commit fc22b6623b (generated columns) replaced ExecGetUpdatedCols() with ExecGetAllUpdatedCols() in a couple places handling UPDATE (triggers and lock mode). However, ExecGetUpdatedCols() did exec_rt_fetch() while ExecGetAllUpdatedCols() also allocates memory through bms_union() without paying attention to the memory context and happened to use the long-lived ExecutorState, leaking the memory until the end of the query. The amount of leaked memory is proportional to the number of (updated) attributes, types of UPDATE triggers, and the number of processed rows (which for UPDATE ... FROM ... may be much higher than updated rows). Fixed by switching to the per-tuple context in GetAllUpdatedColumns(). This is fine for all in-core callers, but external callers may need to copy the result. But we're not aware of any such callers. Note the issue was introduced by fc22b6623b, but the macros were later renamed by f50e888990. Backpatch to 12, where the issue was introduced. Reported-by: Tomas Vondra Reviewed-by: Andres Freund, Tom Lane, Jakub Wartak Backpatch-through: 12 Discussion: https://postgr.es/m/222a3442-7f7d-246c-ed9b-a76209d19239@enterprisedb.com --- src/backend/executor/execUtils.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index d8fdc752326..78ba25e2b6b 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -1400,12 +1400,27 @@ ExecGetExtraUpdatedCols(ResultRelInfo *relinfo, EState *estate) return NULL; } -/* Return columns being updated, including generated columns */ +/* + * Return columns being updated, including generated columns + * + * The bitmap is allocated in per-tuple memory context. It's up to the caller to + * copy it into a different context with the appropriate lifespan, if needed. + */ Bitmapset * ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate) { - return bms_union(ExecGetUpdatedCols(relinfo, estate), - ExecGetExtraUpdatedCols(relinfo, estate)); + + Bitmapset *ret; + MemoryContext oldcxt; + + oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate)); + + ret = bms_union(ExecGetUpdatedCols(relinfo, estate), + ExecGetExtraUpdatedCols(relinfo, estate)); + + MemoryContextSwitchTo(oldcxt); + + return ret; } /* From b3bc41d5476dde0a8e303891f4f470287008c5aa Mon Sep 17 00:00:00 2001 From: Fujii Masao Date: Thu, 8 Jun 2023 20:11:52 +0900 Subject: [PATCH 11/45] doc: Fix example command for ALTER FOREIGN TABLE ... OPTIONS. In the documentation, previously the example command for ALTER FOREIGN TABLE ... OPTIONS incorrectly included both the option name and value with the DROP operation. The correct syntax for the DROP operation requires only the name of the option to be specified. This commit fixes the example by removing the option value from the DROP operation. Back-patch to all supported versions. Author: Mehmet Emin KARAKAS Reviewed-by: Fujii Masao Discussion: https://postgr.es/m/CANQrdXAHzbcEYhjGoe5A42OmfvdQhHFJzyKj9gJvHuDKyOF5Ng@mail.gmail.com --- doc/src/sgml/ref/alter_foreign_table.sgml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/alter_foreign_table.sgml b/doc/src/sgml/ref/alter_foreign_table.sgml index 7ca03f3ac9f..5d373ae9a34 100644 --- a/doc/src/sgml/ref/alter_foreign_table.sgml +++ b/doc/src/sgml/ref/alter_foreign_table.sgml @@ -521,7 +521,7 @@ ALTER FOREIGN TABLE distributors ALTER COLUMN street SET NOT NULL; To change options of a foreign table: -ALTER FOREIGN TABLE myschema.distributors OPTIONS (ADD opt1 'value', SET opt2 'value2', DROP opt3 'value3'); +ALTER FOREIGN TABLE myschema.distributors OPTIONS (ADD opt1 'value', SET opt2 'value2', DROP opt3); From 50ff8fbeb045b118f846218b714a480dfb8e711d Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 9 Jun 2023 09:37:34 +0900 Subject: [PATCH 12/45] Refactor log check logic for connect_ok/fails in PostgreSQL::Test::Cluster This commit refactors a bit the code in charge of checking for log patterns when connections fail or succeed, by moving the log pattern checks into their own routine, for clarity. This has come up as something to improve while discussing the refactoring of find_in_log(). Backpatch down to 14 where these routines are used, to ease the introduction of new tests that could rely on them. Author: Vignesh C, Michael Paquier Discussion: https://postgr.es/m/CALDaNm0YSiLpjCmajwLfidQrFOrLNKPQir7s__PeVvh9U3uoTQ@mail.gmail.com Backpatch-through: 14 --- src/test/perl/PostgresNode.pm | 119 +++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 54 deletions(-) diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index 50ba9d041bc..b81f6e7b68d 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -2442,15 +2442,9 @@ If this regular expression is set, matches it with the output generated. =item log_like => [ qr/required message/ ] -If given, it must be an array reference containing a list of regular -expressions that must match against the server log, using -C. - =item log_unlike => [ qr/prohibited message/ ] -If given, it must be an array reference containing a list of regular -expressions that must NOT match against the server log. They will be -passed to C. +See C. =back @@ -2471,16 +2465,6 @@ sub connect_ok $sql = "SELECT \$\$connected with $connstr\$\$"; } - my (@log_like, @log_unlike); - if (defined($params{log_like})) - { - @log_like = @{ $params{log_like} }; - } - if (defined($params{log_unlike})) - { - @log_unlike = @{ $params{log_unlike} }; - } - my $log_location = -s $self->logfile; # Never prompt for a password, any callers of this routine should @@ -2498,19 +2482,7 @@ sub connect_ok { like($stdout, $params{expected_stdout}, "$test_name: matches"); } - if (@log_like or @log_unlike) - { - my $log_contents = TestLib::slurp_file($self->logfile, $log_location); - - while (my $regex = shift @log_like) - { - like($log_contents, $regex, "$test_name: log matches"); - } - while (my $regex = shift @log_unlike) - { - unlike($log_contents, $regex, "$test_name: log does not match"); - } - } + $self->log_check($test_name, $log_location, %params); } =pod @@ -2530,7 +2502,7 @@ If this regular expression is set, matches it with the output generated. =item log_unlike => [ qr/prohibited message/ ] -See C, above. +See C. =back @@ -2541,16 +2513,6 @@ sub connect_fails local $Test::Builder::Level = $Test::Builder::Level + 1; my ($self, $connstr, $test_name, %params) = @_; - my (@log_like, @log_unlike); - if (defined($params{log_like})) - { - @log_like = @{ $params{log_like} }; - } - if (defined($params{log_unlike})) - { - @log_unlike = @{ $params{log_unlike} }; - } - my $log_location = -s $self->logfile; # Never prompt for a password, any callers of this routine should @@ -2568,19 +2530,7 @@ sub connect_fails like($stderr, $params{expected_stderr}, "$test_name: matches"); } - if (@log_like or @log_unlike) - { - my $log_contents = TestLib::slurp_file($self->logfile, $log_location); - - while (my $regex = shift @log_like) - { - like($log_contents, $regex, "$test_name: log matches"); - } - while (my $regex = shift @log_unlike) - { - unlike($log_contents, $regex, "$test_name: log does not match"); - } - } + $self->log_check($test_name, $log_location, %params); } =pod @@ -2753,6 +2703,67 @@ sub issues_sql_like =pod +=item $node->log_check($offset, $test_name, %parameters) + +Check contents of server logs. + +=over + +=item $test_name + +Name of test for error messages. + +=item $offset + +Offset of the log file. + +=item log_like => [ qr/required message/ ] + +If given, it must be an array reference containing a list of regular +expressions that must match against the server log, using +C. + +=item log_unlike => [ qr/prohibited message/ ] + +If given, it must be an array reference containing a list of regular +expressions that must NOT match against the server log. They will be +passed to C. + +=back + +=cut + +sub log_check +{ + my ($self, $test_name, $offset, %params) = @_; + + my (@log_like, @log_unlike); + if (defined($params{log_like})) + { + @log_like = @{ $params{log_like} }; + } + if (defined($params{log_unlike})) + { + @log_unlike = @{ $params{log_unlike} }; + } + + if (@log_like or @log_unlike) + { + my $log_contents = TestLib::slurp_file($self->logfile, $offset); + + while (my $regex = shift @log_like) + { + like($log_contents, $regex, "$test_name: log matches"); + } + while (my $regex = shift @log_unlike) + { + unlike($log_contents, $regex, "$test_name: log does not match"); + } + } +} + +=pod + =item $node->run_log(...) Runs a shell command like TestLib::run_log, but with connection parameters set From c257e28f38383403d145e7da78881a13882d9336 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Sun, 11 Jun 2023 10:33:46 +0900 Subject: [PATCH 13/45] Fix missing initializations of MyProc.delayChkptEnd This commit fixes an oversight introduced in 10520f4, that has added delayChkptEnd to PGPROC to avoid ABI breakages on stable branches, where two spots have missed to initialize this variable (delayChkpt was switched back from int to bool, and it was initialized as 0 so there was no consequences for it): - InitProcess(), where the per-process data structures of a backend are initialized. - InitAuxiliaryProcess(), same but for auxiliary processes. An interruption during relation truncation while this flag is set could cause an assertion failure when a follow-up process does a relation truncation while reusing the same PGPROC entry. A second effect could be incorrect checkpoint end delays. While on it, add an assertion in ProcArrayClearTransaction() for delayChkptEnd to be in line with 5788e25. This is needed only for v14. This issue affects v11~v14, but not v15~, as we use a single field called delayChkptFlags to delay checkpoints there. Author: suyu.cmj (mengjuan.cmj@alibaba-inc.com) Reviewed-by: Kyotaro Horiguchi, Michael Paquier Discussion: https://postgr.es/m/9c3d2a49-db5f-43cb-840b-d58f9a684295.mengjuan.cmj@alibaba-inc.com Backpatch-through: 11 --- src/backend/storage/ipc/procarray.c | 1 + src/backend/storage/lmgr/proc.c | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 208874f793e..8eea86a16ad 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -1029,6 +1029,7 @@ ProcArrayClearTransaction(PGPROC *proc) Assert(!(proc->statusFlags & PROC_VACUUM_STATE_MASK)); Assert(!proc->delayChkpt); + Assert(!proc->delayChkptEnd); /* * Need to increment completion count even though transaction hasn't diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index cb9d3e6d580..b505b111f29 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -475,7 +475,8 @@ InitProcess(void) MyProc->roleId = InvalidOid; MyProc->tempNamespaceId = InvalidOid; MyProc->isBackgroundWorker = IsBackgroundWorker; - MyProc->delayChkpt = 0; + MyProc->delayChkpt = false; + MyProc->delayChkptEnd = false; MyProc->statusFlags = 0; /* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */ if (IsAutoVacuumWorkerProcess()) @@ -742,7 +743,8 @@ InitAuxiliaryProcess(void) MyProc->mppIsWriter = false; MyProc->tempNamespaceId = InvalidOid; MyProc->isBackgroundWorker = IsBackgroundWorker; - MyProc->delayChkpt = 0; + MyProc->delayChkpt = false; + MyProc->delayChkptEnd = false; MyProc->statusFlags = 0; MyProc->lwWaiting = false; MyProc->lwWaitMode = 0; From c821196c3a990b0beb28f3f139d1f63698dfa2af Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 12 Jun 2023 09:14:14 +0900 Subject: [PATCH 14/45] hstore: Tighten key/value parsing check for whitespaces isspace() can be locale-sensitive depending on the platform, causing hstore to consider as whitespaces characters it should not see as such. For example, U+0105, being decoded as 0xC4 0x85 in UTF-8, would be discarded from the input given. This problem is similar to 9ae2661, though it was missed that hstore can also manipulate non-ASCII inputs, so replace the existing isspace() calls with scanner_isspace(). This problem exists for a long time, so backpatch all the way down. Author: Evan Jones Discussion: https://postgr.es/m/CA+HWA9awUW0+RV_gO9r1ABZwGoZxPztcJxPy8vMFSTbTfi4jig@mail.gmail.com Backpatch-through: 11 --- contrib/hstore/Makefile | 2 +- contrib/hstore/expected/hstore_utf8.out | 36 +++++++++++++++++++++++ contrib/hstore/expected/hstore_utf8_1.out | 8 +++++ contrib/hstore/hstore_io.c | 9 +++--- contrib/hstore/sql/hstore_utf8.sql | 19 ++++++++++++ 5 files changed, 69 insertions(+), 5 deletions(-) create mode 100644 contrib/hstore/expected/hstore_utf8.out create mode 100644 contrib/hstore/expected/hstore_utf8_1.out create mode 100644 contrib/hstore/sql/hstore_utf8.sql diff --git a/contrib/hstore/Makefile b/contrib/hstore/Makefile index c4e339b57c1..48ee98f0d5c 100644 --- a/contrib/hstore/Makefile +++ b/contrib/hstore/Makefile @@ -22,7 +22,7 @@ PGFILEDESC = "hstore - key/value pair data type" HEADERS = hstore.h -REGRESS = hstore +REGRESS = hstore hstore_utf8 ifdef USE_PGXS PG_CONFIG = pg_config diff --git a/contrib/hstore/expected/hstore_utf8.out b/contrib/hstore/expected/hstore_utf8.out new file mode 100644 index 00000000000..44058244132 --- /dev/null +++ b/contrib/hstore/expected/hstore_utf8.out @@ -0,0 +1,36 @@ +/* + * This test must be run in a database with UTF-8 encoding, + * because other encodings don't support all the characters used. + */ +SELECT getdatabaseencoding() <> 'UTF8' + AS skip_test \gset +\if :skip_test +\quit +\endif +SET client_encoding = utf8; +-- UTF-8 locale bug on macOS: isspace(0x85) returns true. \u0105 encodes +-- as 0xc4 0x85 in UTF-8; the 0x85 was interpreted here as a whitespace. +SELECT E'key\u0105=>value\u0105'::hstore; + hstore +------------------ + "keyą"=>"valueą" +(1 row) + +SELECT 'keyą=>valueą'::hstore; + hstore +------------------ + "keyą"=>"valueą" +(1 row) + +SELECT 'ą=>ą'::hstore; + hstore +---------- + "ą"=>"ą" +(1 row) + +SELECT 'keyąfoo=>valueą'::hstore; + hstore +--------------------- + "keyąfoo"=>"valueą" +(1 row) + diff --git a/contrib/hstore/expected/hstore_utf8_1.out b/contrib/hstore/expected/hstore_utf8_1.out new file mode 100644 index 00000000000..37aead89c0c --- /dev/null +++ b/contrib/hstore/expected/hstore_utf8_1.out @@ -0,0 +1,8 @@ +/* + * This test must be run in a database with UTF-8 encoding, + * because other encodings don't support all the characters used. + */ +SELECT getdatabaseencoding() <> 'UTF8' + AS skip_test \gset +\if :skip_test +\quit diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c index b3304ff8445..03057f085d1 100644 --- a/contrib/hstore/hstore_io.c +++ b/contrib/hstore/hstore_io.c @@ -12,6 +12,7 @@ #include "hstore.h" #include "lib/stringinfo.h" #include "libpq/pqformat.h" +#include "parser/scansup.h" #include "utils/builtins.h" #include "utils/json.h" #include "utils/jsonb.h" @@ -88,7 +89,7 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped) { st = GV_WAITESCIN; } - else if (!isspace((unsigned char) *(state->ptr))) + else if (!scanner_isspace((unsigned char) *(state->ptr))) { *(state->cur) = *(state->ptr); state->cur++; @@ -111,7 +112,7 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped) state->ptr--; return true; } - else if (isspace((unsigned char) *(state->ptr))) + else if (scanner_isspace((unsigned char) *(state->ptr))) { return true; } @@ -219,7 +220,7 @@ parse_hstore(HSParser *state) { elog(ERROR, "Unexpected end of string"); } - else if (!isspace((unsigned char) *(state->ptr))) + else if (!scanner_isspace((unsigned char) *(state->ptr))) { elog(ERROR, "Syntax error near \"%.*s\" at position %d", pg_mblen(state->ptr), state->ptr, @@ -271,7 +272,7 @@ parse_hstore(HSParser *state) { return; } - else if (!isspace((unsigned char) *(state->ptr))) + else if (!scanner_isspace((unsigned char) *(state->ptr))) { elog(ERROR, "Syntax error near \"%.*s\" at position %d", pg_mblen(state->ptr), state->ptr, diff --git a/contrib/hstore/sql/hstore_utf8.sql b/contrib/hstore/sql/hstore_utf8.sql new file mode 100644 index 00000000000..face878324c --- /dev/null +++ b/contrib/hstore/sql/hstore_utf8.sql @@ -0,0 +1,19 @@ +/* + * This test must be run in a database with UTF-8 encoding, + * because other encodings don't support all the characters used. + */ + +SELECT getdatabaseencoding() <> 'UTF8' + AS skip_test \gset +\if :skip_test +\quit +\endif + +SET client_encoding = utf8; + +-- UTF-8 locale bug on macOS: isspace(0x85) returns true. \u0105 encodes +-- as 0xc4 0x85 in UTF-8; the 0x85 was interpreted here as a whitespace. +SELECT E'key\u0105=>value\u0105'::hstore; +SELECT 'keyą=>valueą'::hstore; +SELECT 'ą=>ą'::hstore; +SELECT 'keyąfoo=>valueą'::hstore; From f5cd2cbbadba1b7d56ffe8e754bb17eba9381480 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 12 Jun 2023 10:54:28 -0400 Subject: [PATCH 15/45] Accept fractional seconds in jsonpath's datetime() method. Commit 927d9abb6 purported to make datetime() accept any string that could be output for a datetime value by to_jsonb(). But it overlooked the possibility of fractional seconds being present, so that cases as simple as to_jsonb(now()) would defeat it. Fix by adding formats that include ".US" to the list in executeDateTimeMethod(). (Note that while this is nominally microseconds, it'll do the right thing for fractions with fewer than six digits.) In passing, re-order the list to restore the datatype ordering specified in its comment. The violation accidentally did not break anything; but the next edit might be less lucky, so add more comments. Per report from Tim Field. Back-patch to v13 where datetime() was added, like the previous patch. Discussion: https://postgr.es/m/014A028B-5CE6-4FDF-AC24-426CA6FC9CEE@mohiohio.com --- src/backend/utils/adt/jsonpath_exec.c | 17 +++++++++++++---- src/test/regress/expected/jsonb_jsonpath.out | 15 +++++++++++++++ src/test/regress/sql/jsonb_jsonpath.sql | 3 +++ 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c index cd2ac04d89e..ae1a9f86052 100644 --- a/src/backend/utils/adt/jsonpath_exec.c +++ b/src/backend/utils/adt/jsonpath_exec.c @@ -1838,20 +1838,29 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp, * According to SQL/JSON standard enumerate ISO formats for: date, * timetz, time, timestamptz, timestamp. * - * We also support ISO 8601 for timestamps, because to_json[b]() - * functions use this format. + * We also support ISO 8601 format (with "T") for timestamps, because + * to_json[b]() functions use this format. */ static const char *fmt_str[] = { - "yyyy-mm-dd", + "yyyy-mm-dd", /* date */ + "HH24:MI:SS.USTZH:TZM", /* timetz */ + "HH24:MI:SS.USTZH", "HH24:MI:SSTZH:TZM", "HH24:MI:SSTZH", + "HH24:MI:SS.US", /* time without tz */ "HH24:MI:SS", + "yyyy-mm-dd HH24:MI:SS.USTZH:TZM", /* timestamptz */ + "yyyy-mm-dd HH24:MI:SS.USTZH", "yyyy-mm-dd HH24:MI:SSTZH:TZM", "yyyy-mm-dd HH24:MI:SSTZH", - "yyyy-mm-dd HH24:MI:SS", + "yyyy-mm-dd\"T\"HH24:MI:SS.USTZH:TZM", + "yyyy-mm-dd\"T\"HH24:MI:SS.USTZH", "yyyy-mm-dd\"T\"HH24:MI:SSTZH:TZM", "yyyy-mm-dd\"T\"HH24:MI:SSTZH", + "yyyy-mm-dd HH24:MI:SS.US", /* timestamp without tz */ + "yyyy-mm-dd HH24:MI:SS", + "yyyy-mm-dd\"T\"HH24:MI:SS.US", "yyyy-mm-dd\"T\"HH24:MI:SS" }; diff --git a/src/test/regress/expected/jsonb_jsonpath.out b/src/test/regress/expected/jsonb_jsonpath.out index 328a6b39199..6659bc9091a 100644 --- a/src/test/regress/expected/jsonb_jsonpath.out +++ b/src/test/regress/expected/jsonb_jsonpath.out @@ -1920,6 +1920,21 @@ select jsonb_path_query('"2017-03-10T12:34:56+3:10"', '$.datetime()'); select jsonb_path_query('"2017-03-10t12:34:56+3:10"', '$.datetime()'); ERROR: datetime format is not recognized: "2017-03-10t12:34:56+3:10" HINT: Use a datetime template argument to specify the input data format. +select jsonb_path_query('"2017-03-10 12:34:56.789+3:10"', '$.datetime()'); + jsonb_path_query +--------------------------------- + "2017-03-10T12:34:56.789+03:10" +(1 row) + +select jsonb_path_query('"2017-03-10T12:34:56.789+3:10"', '$.datetime()'); + jsonb_path_query +--------------------------------- + "2017-03-10T12:34:56.789+03:10" +(1 row) + +select jsonb_path_query('"2017-03-10t12:34:56.789+3:10"', '$.datetime()'); +ERROR: datetime format is not recognized: "2017-03-10t12:34:56.789+3:10" +HINT: Use a datetime template argument to specify the input data format. select jsonb_path_query('"12:34:56"', '$.datetime().type()'); jsonb_path_query -------------------------- diff --git a/src/test/regress/sql/jsonb_jsonpath.sql b/src/test/regress/sql/jsonb_jsonpath.sql index bd025077d52..e0ce509264a 100644 --- a/src/test/regress/sql/jsonb_jsonpath.sql +++ b/src/test/regress/sql/jsonb_jsonpath.sql @@ -414,6 +414,9 @@ select jsonb_path_query('"2017-03-10 12:34:56+3:10"', '$.datetime().type()'); select jsonb_path_query('"2017-03-10 12:34:56+3:10"', '$.datetime()'); select jsonb_path_query('"2017-03-10T12:34:56+3:10"', '$.datetime()'); select jsonb_path_query('"2017-03-10t12:34:56+3:10"', '$.datetime()'); +select jsonb_path_query('"2017-03-10 12:34:56.789+3:10"', '$.datetime()'); +select jsonb_path_query('"2017-03-10T12:34:56.789+3:10"', '$.datetime()'); +select jsonb_path_query('"2017-03-10t12:34:56.789+3:10"', '$.datetime()'); select jsonb_path_query('"12:34:56"', '$.datetime().type()'); select jsonb_path_query('"12:34:56"', '$.datetime()'); select jsonb_path_query('"12:34:56+3"', '$.datetime().type()'); From d5725aeb1eadb3c7d7bab5da6a6fdf64a3195a1e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 13 Jun 2023 15:58:37 -0400 Subject: [PATCH 16/45] Correctly update hasSubLinks while mutating a rule action. rewriteRuleAction neglected to check for SubLink nodes in the securityQuals of range table entries. This could lead to failing to convert such a SubLink to a SubPlan, resulting in assertion crashes or weird errors later in planning. In passing, fix some poor coding in rewriteTargetView: we should not pass the source parsetree's hasSubLinks field to ReplaceVarsFromTargetList's outer_hasSubLinks. ReplaceVarsFromTargetList knows enough to ignore that when a Query node is passed, but it's still confusing and bad precedent: if we did try to update that flag we'd be updating a stale copy of the parsetree. Per bug #17972 from Alexander Lakhin. This has been broken since we added RangeTblEntry.securityQuals (although the presented test case only fails back to 215b43cdc), so back-patch all the way. Discussion: https://postgr.es/m/17972-f422c094237847d0@postgresql.org --- src/backend/rewrite/rewriteHandler.c | 4 ++- src/test/regress/expected/updatable_views.out | 32 +++++++++++++++++++ src/test/regress/sql/updatable_views.sql | 17 ++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 9da36260c77..d83bc0e8b5e 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -538,6 +538,8 @@ rewriteRuleAction(Query *parsetree, /* other RTE types don't contain bare expressions */ break; } + sub_action->hasSubLinks |= + checkExprHasSubLink((Node *) rte->securityQuals); if (sub_action->hasSubLinks) break; /* no need to keep scanning rtable */ } @@ -3497,7 +3499,7 @@ rewriteTargetView(Query *parsetree, Relation view) view_targetlist, REPLACEVARS_REPORT_ERROR, 0, - &parsetree->hasSubLinks); + NULL); /* * Update all other RTI references in the query that point to the view diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out index e6642dc4e4a..efc2174a00c 100644 --- a/src/test/regress/expected/updatable_views.out +++ b/src/test/regress/expected/updatable_views.out @@ -2583,6 +2583,38 @@ DROP VIEW v1; DROP TABLE t2; DROP TABLE t1; -- +-- Test sub-select in nested security barrier views, per bug #17972 +-- +CREATE TABLE t1 (a int); +CREATE VIEW v1 WITH (security_barrier = true) AS + SELECT * FROM t1; +CREATE RULE v1_upd_rule AS ON UPDATE TO v1 DO INSTEAD + UPDATE t1 SET a = NEW.a WHERE a = OLD.a; +CREATE VIEW v2 WITH (security_barrier = true) AS + SELECT * FROM v1 WHERE EXISTS (SELECT 1); +EXPLAIN (COSTS OFF) UPDATE v2 SET a = 1; + QUERY PLAN +--------------------------------------------------- + Update on t1 + InitPlan 1 (returns $0) + -> Result + -> Merge Join + Merge Cond: (t1.a = v1.a) + -> Sort + Sort Key: t1.a + -> Seq Scan on t1 + -> Sort + Sort Key: v1.a + -> Subquery Scan on v1 + -> Result + One-Time Filter: $0 + -> Seq Scan on t1 t1_1 +(14 rows) + +DROP VIEW v2; +DROP VIEW v1; +DROP TABLE t1; +-- -- Test CREATE OR REPLACE VIEW turning a non-updatable view into an -- auto-updatable view and adding check options in a single step -- diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql index 4e3f3c88e5f..8708404e480 100644 --- a/src/test/regress/sql/updatable_views.sql +++ b/src/test/regress/sql/updatable_views.sql @@ -1301,6 +1301,23 @@ DROP VIEW v1; DROP TABLE t2; DROP TABLE t1; +-- +-- Test sub-select in nested security barrier views, per bug #17972 +-- +CREATE TABLE t1 (a int); +CREATE VIEW v1 WITH (security_barrier = true) AS + SELECT * FROM t1; +CREATE RULE v1_upd_rule AS ON UPDATE TO v1 DO INSTEAD + UPDATE t1 SET a = NEW.a WHERE a = OLD.a; +CREATE VIEW v2 WITH (security_barrier = true) AS + SELECT * FROM v1 WHERE EXISTS (SELECT 1); + +EXPLAIN (COSTS OFF) UPDATE v2 SET a = 1; + +DROP VIEW v2; +DROP VIEW v1; +DROP TABLE t1; + -- -- Test CREATE OR REPLACE VIEW turning a non-updatable view into an -- auto-updatable view and adding check options in a single step From bf80a0973e24f194b8427de354dcca0f4a2bde65 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Thu, 15 Jun 2023 13:45:40 +0900 Subject: [PATCH 17/45] intarray: Prevent out-of-bound memory reads with gist__int_ops As gist__int_ops stands in intarray, it is possible to store GiST entries for leaf pages that can cause corruptions when decompressed. Leaf nodes are stored as decompressed all the time by the compression method, and the decompression method should map with that, retrieving the contents of the page without doing any decompression. However, the code authorized the insertion of leaf page data with a higher number of array items than what can be supported, generating a NOTICE message to inform about this matter (199 for a 8k page, for reference). When calling the decompression method, a decompression would be attempted on this leaf node item but the contents should be retrieved as they are. The NOTICE message generated when dealing with the compression of a leaf page and too many elements in the input array for gist__int_ops has been introduced by 08ee64e, removing the marker stored in the array to track if this is actually a leaf node. However, it also missed the fact that the decompression path should do nothing for a leaf page. Hence, as the code stand, a too-large array would be stored as uncompressed but the decompression path would attempt a decompression rather that retrieving the contents as they are. This leads to various problems. First, even if 08ee64e tried to address that, it is possible to do out-of-bound chunk writes with a large input array, with the backend informing about that with WARNINGs. On decompression, retrieving the stored leaf data would lead to incorrect memory reads, leading to crashes or even worse. Perhaps somebody would be interested in expanding the number of array items that can be handled in a leaf page for this operator in the future, which would require revisiting the choice done in 08ee64e, but based on the lack of reports about this problem since 2005 it does not look so. For now, this commit prevents the insertion of data for leaf pages when using more array items that the code can handle on decompression, switching the NOTICE message to an ERROR. If one wishes to use more array items, gist__intbig_ops is an optional choice. While on it, use ERRCODE_PROGRAM_LIMIT_EXCEEDED as error code when a limit is reached, because that's what the module is facing in such cases. Author: Ankit Kumar Pandey, Alexander Lakhin Reviewed-by: Richard Guo, Michael Paquier Discussion: https://postgr.es/m/796b65c3-57b7-bddf-b0d5-a8afafb8b627@gmail.com Discussion: https://postgr.es/m/17888-f72930e6b5ce8c14@postgresql.org Backpatch-through: 11 --- contrib/intarray/_int_gist.c | 12 ++++++++---- contrib/intarray/expected/_int.out | 2 ++ contrib/intarray/sql/_int.sql | 2 ++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/contrib/intarray/_int_gist.c b/contrib/intarray/_int_gist.c index f1817a6cce3..ea79c4bb51f 100644 --- a/contrib/intarray/_int_gist.c +++ b/contrib/intarray/_int_gist.c @@ -180,8 +180,10 @@ g_int_compress(PG_FUNCTION_ARGS) PREPAREARR(r); if (ARRNELEMS(r) >= 2 * num_ranges) - elog(NOTICE, "input array is too big (%d maximum allowed, %d current), use gist__intbig_ops opclass instead", - 2 * num_ranges - 1, ARRNELEMS(r)); + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("input array is too big (%d maximum allowed, %d current), use gist__intbig_ops opclass instead", + 2 * num_ranges - 1, ARRNELEMS(r)))); retval = palloc(sizeof(GISTENTRY)); gistentryinit(*retval, PointerGetDatum(r), @@ -269,7 +271,8 @@ g_int_compress(PG_FUNCTION_ARGS) lenr = internal_size(dr, len); if (lenr < 0 || lenr > MAXNUMELTS) ereport(ERROR, - (errmsg("data is too sparse, recreate index using gist__intbig_ops opclass instead"))); + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("data is too sparse, recreate index using gist__intbig_ops opclass instead"))); r = resize_intArrayType(r, len); retval = palloc(sizeof(GISTENTRY)); @@ -331,7 +334,8 @@ g_int_decompress(PG_FUNCTION_ARGS) lenr = internal_size(din, lenin); if (lenr < 0 || lenr > MAXNUMELTS) ereport(ERROR, - (errmsg("compressed array is too big, recreate index using gist__intbig_ops opclass instead"))); + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("compressed array is too big, recreate index using gist__intbig_ops opclass instead"))); r = new_intArrayType(lenr); dr = ARRPTR(r); diff --git a/contrib/intarray/expected/_int.out b/contrib/intarray/expected/_int.out index ae4bc4ea746..b78175b87a0 100644 --- a/contrib/intarray/expected/_int.out +++ b/contrib/intarray/expected/_int.out @@ -552,6 +552,8 @@ SELECT count(*) from test__int WHERE a @@ '!20 & !21'; 6343 (1 row) +INSERT INTO test__int SELECT array(SELECT x FROM generate_series(1, 1001) x); -- should fail +ERROR: input array is too big (199 maximum allowed, 1001 current), use gist__intbig_ops opclass instead DROP INDEX text_idx; CREATE INDEX text_idx on test__int using gist (a gist__int_ops(numranges = 0)); ERROR: value 0 out of bounds for option "numranges" diff --git a/contrib/intarray/sql/_int.sql b/contrib/intarray/sql/_int.sql index 1adac50f2fb..4994188c2e4 100644 --- a/contrib/intarray/sql/_int.sql +++ b/contrib/intarray/sql/_int.sql @@ -112,6 +112,8 @@ SELECT count(*) from test__int WHERE a @@ '(20&23)|(50&68)'; SELECT count(*) from test__int WHERE a @@ '20 | !21'; SELECT count(*) from test__int WHERE a @@ '!20 & !21'; +INSERT INTO test__int SELECT array(SELECT x FROM generate_series(1, 1001) x); -- should fail + DROP INDEX text_idx; CREATE INDEX text_idx on test__int using gist (a gist__int_ops(numranges = 0)); CREATE INDEX text_idx on test__int using gist (a gist__int_ops(numranges = 1021)); From 76fbad100f8350d9f6cab4547de157a604528274 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Fri, 16 Jun 2023 10:04:22 +0900 Subject: [PATCH 18/45] Fix typo in comment. Back-patch down to 11. Author: Sho Kato () Discussion: https://postgr.es/m/TYCPR01MB68499042A33BC32241193AAF9F5BA%40TYCPR01MB6849.jpnprd01.prod.outlook.com --- src/backend/rewrite/rewriteHandler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index d83bc0e8b5e..25312745495 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -3519,7 +3519,7 @@ rewriteTargetView(Query *parsetree, Relation view) * columns to be affected. * * Note that this destroys the resno ordering of the targetlist, but that - * will be fixed when we recurse through rewriteQuery, which will invoke + * will be fixed when we recurse through RewriteQuery, which will invoke * rewriteTargetListIU again on the updated targetlist. */ if (parsetree->commandType != CMD_DELETE) From 4faba31bc5bfcf5226f563dcd55301f9833a331e Mon Sep 17 00:00:00 2001 From: David Rowley Date: Mon, 19 Jun 2023 13:01:58 +1200 Subject: [PATCH 19/45] Don't use partial unique indexes for unique proofs in the planner Here we adjust relation_has_unique_index_for() so that it no longer makes use of partial unique indexes as uniqueness proofs. It is incorrect to use these as the predicates used by check_index_predicates() to set predOK makes use of not only baserestrictinfo quals as proofs, but also qual from join conditions. For relation_has_unique_index_for()'s case, we need to know the relation is unique for a given set of columns before any joins are evaluated, so if predOK was only set to true due to some join qual, then it's unsafe to use such indexes in relation_has_unique_index_for(). The final plan may not even make use of that index, which could result in reading tuples that are not as unique as the planner previously expected them to be. Bug: #17975 Reported-by: Tor Erik Linnerud Backpatch-through: 11, all supported versions Discussion: https://postgr.es/m/17975-98a90c156f25c952%40postgresql.org --- src/backend/optimizer/path/indxpath.c | 9 ++++++--- src/backend/optimizer/plan/analyzejoins.c | 9 ++++----- src/test/regress/expected/join.out | 17 +++++++++++++++++ src/test/regress/sql/join.sql | 9 +++++++++ 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index 1037ada162d..6573dea982b 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -3623,10 +3623,13 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel, /* * If the index is not unique, or not immediately enforced, or if it's - * a partial index that doesn't match the query, it's useless here. + * a partial index, it's useless here. We're unable to make use of + * predOK partial unique indexes due to the fact that + * check_index_predicates() also makes use of join predicates to + * determine if the partial index is usable. Here we need proofs that + * hold true before any joins are evaluated. */ - if (!ind->unique || !ind->immediate || - (ind->indpred != NIL && !ind->predOK)) + if (!ind->unique || !ind->immediate || ind->indpred != NIL) continue; /* diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index 9151ab4ba2e..01cb1213c06 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -592,9 +592,9 @@ rel_supports_distinctness(PlannerInfo *root, RelOptInfo *rel) /* * For a plain relation, we only know how to prove uniqueness by * reference to unique indexes. Make sure there's at least one - * suitable unique index. It must be immediately enforced, and if - * it's a partial index, it must match the query. (Keep these - * conditions in sync with relation_has_unique_index_for!) + * suitable unique index. It must be immediately enforced, and not a + * partial index. (Keep these conditions in sync with + * relation_has_unique_index_for!) */ ListCell *lc; @@ -602,8 +602,7 @@ rel_supports_distinctness(PlannerInfo *root, RelOptInfo *rel) { IndexOptInfo *ind = (IndexOptInfo *) lfirst(lc); - if (ind->unique && ind->immediate && - (ind->indpred == NIL || ind->predOK)) + if (ind->unique && ind->immediate && ind->indpred == NIL) return true; } } diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 73d95559569..1704b9e310b 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -7039,6 +7039,23 @@ left join j2 on j1.id1 = j2.id1 where j1.id2 = 1; Settings: enable_nestloop=on, optimizer=off (17 rows) +create unique index j1_id2_idx on j1(id2) where id2 is not null; +-- ensure we don't use a partial unique index as unique proofs +explain (verbose, costs off) +select * from j1 +inner join j2 on j1.id2 = j2.id2; + QUERY PLAN +------------------------------------------ + Nested Loop + Output: j1.id1, j1.id2, j2.id1, j2.id2 + Join Filter: (j1.id2 = j2.id2) + -> Seq Scan on public.j2 + Output: j2.id1, j2.id2 + -> Seq Scan on public.j1 + Output: j1.id1, j1.id2 +(7 rows) + +drop index j1_id2_idx; -- validate logic in merge joins which skips mark and restore. -- it should only do this if all quals which were used to detect the unique -- are present as join quals, and not plain quals. diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 1b994597d3c..7a2956eac52 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2328,6 +2328,15 @@ explain (verbose, costs off) select * from j1 left join j2 on j1.id1 = j2.id1 where j1.id2 = 1; +create unique index j1_id2_idx on j1(id2) where id2 is not null; + +-- ensure we don't use a partial unique index as unique proofs +explain (verbose, costs off) +select * from j1 +inner join j2 on j1.id2 = j2.id2; + +drop index j1_id2_idx; + -- validate logic in merge joins which skips mark and restore. -- it should only do this if all quals which were used to detect the unique -- are present as join quals, and not plain quals. From d45544be8bd81da37c6f3e3a71be17770fab7a60 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 20 Jun 2023 10:25:45 +0900 Subject: [PATCH 20/45] Enable archiving in recovery TAP test 009_twophase.pl This is a follow-up of f663b00, that has been committed to v13 and v14, tweaking the TAP test for two-phase transactions so as it provides coverage for the bug that has been fixed. This change is done in its own commit for clarity, as v15 and HEAD did not show the problematic behavior, still missed coverage for it. While on it, this adds a comment about the dependency of the last partial segment rename and RecoverPreparedTransactions() at the end of recovery, as that can be easy to miss. Author: Michael Paquier Reviewed-by: Kyotaro Horiguchi Discussion: https://postgr.es/m/743b9b45a2d4013bd90b6a5cba8d6faeb717ee34.camel@cybertec.at Backpatch-through: 13 --- src/backend/access/transam/xlog.c | 6 +++++- src/test/recovery/t/009_twophase.pl | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 17ebce7303e..cc465f296d8 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -8471,7 +8471,11 @@ StartupXLOG(void) TrimCLOG(); TrimMultiXact(); - /* Reload shared-memory state for prepared transactions */ + /* + * Reload shared-memory state for prepared transactions. This needs to + * happen before renaming the last partial segment of the old timeline as + * it may be possible that we have to recovery some transactions from it. + */ RecoverPreparedTransactions(); /* Shut down xlogreader */ diff --git a/src/test/recovery/t/009_twophase.pl b/src/test/recovery/t/009_twophase.pl index 18a4cfcece3..9aaa9b11049 100644 --- a/src/test/recovery/t/009_twophase.pl +++ b/src/test/recovery/t/009_twophase.pl @@ -39,7 +39,9 @@ sub configure_and_reload # Setup london node my $node_london = get_new_node("london"); -$node_london->init(allows_streaming => 1); +# Archiving is used to provide coverage with the creation of .partial segments +# done at the end of recovery and the recovery of two-phase transactions. +$node_london->init(allows_streaming => 1, has_archiving => 1); $node_london->append_conf( 'postgresql.conf', qq( max_prepared_transactions = 10 From 035494695da8d76d7d9cca148077f9b26889eaf7 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 20 Jun 2023 17:47:36 -0400 Subject: [PATCH 21/45] Fix hash join when inner hashkey expressions contain Params. If the inner-side expressions contain PARAM_EXEC Params, we must re-hash whenever the values of those Params change. The executor mechanism for that exists already, but we failed to invoke it because finalize_plan() neglected to search the Hash.hashkeys field for Params. This allowed a previous scan's hash table to be re-used when it should not be, leading to rows missing from the join's output. (I believe incorrectly-included join rows are impossible however, since checking the real hashclauses would reject false matches.) This bug is very ancient, dating probably to d24d75ff1 of 7.4. Sadly, this simple fix depends on the plan representational changes made by 2abd7ae9b, so it will only work back to v12. I thought about trying to make some kind of hack for v11, but I'm leery of putting code significantly different from what is used in the newer branches into a nearly-EOL branch. Seeing that the bug escaped detection for a full twenty years, problematic cases must be rare; so I don't feel too awful about leaving v11 as-is. Per bug #17985 from Zuming Jiang. Back-patch to v12. Discussion: https://postgr.es/m/17985-748b66607acd432e@postgresql.org --- src/backend/optimizer/plan/subselect.c | 6 ++++- src/test/regress/expected/join_hash.out | 36 +++++++++++++++++++++++++ src/test/regress/sql/join_hash.sql | 19 +++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index c4da4c1b2da..b63abab8a6d 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -3172,6 +3172,11 @@ finalize_plan(PlannerInfo *root, Plan *plan, &context); break; + case T_Hash: + finalize_primnode((Node *) ((Hash *) plan)->hashkeys, + &context); + break; + case T_Limit: finalize_primnode(((Limit *) plan)->limitOffset, &context); @@ -3292,7 +3297,6 @@ finalize_plan(PlannerInfo *root, Plan *plan, break; case T_ProjectSet: - case T_Hash: case T_RuntimeFilter: case T_Material: case T_Sort: diff --git a/src/test/regress/expected/join_hash.out b/src/test/regress/expected/join_hash.out index e5f74c18d28..fba2aaca178 100644 --- a/src/test/regress/expected/join_hash.out +++ b/src/test/regress/expected/join_hash.out @@ -1257,3 +1257,39 @@ WHERE (1 row) ROLLBACK; +-- Verify that we behave sanely when the inner hash keys contain parameters +-- (that is, outer or lateral references). This situation has to defeat +-- re-use of the inner hash table across rescans. +begin; +set local enable_hashjoin = on; +explain (costs off) +select i8.q2, ss.* from +int8_tbl i8, +lateral (select t1.fivethous, i4.f1 from tenk1 t1 join int4_tbl i4 + on t1.fivethous = i4.f1+i8.q2 order by 1,2) ss; + QUERY PLAN +----------------------------------------------------------- + Nested Loop + -> Seq Scan on int8_tbl i8 + -> Sort + Sort Key: t1.fivethous, i4.f1 + -> Hash Join + Hash Cond: (t1.fivethous = (i4.f1 + i8.q2)) + -> Seq Scan on tenk1 t1 + -> Hash + -> Seq Scan on int4_tbl i4 +(9 rows) + +select i8.q2, ss.* from +int8_tbl i8, +lateral (select t1.fivethous, i4.f1 from tenk1 t1 join int4_tbl i4 + on t1.fivethous = i4.f1+i8.q2 order by 1,2) ss; + q2 | fivethous | f1 +-----+-----------+---- + 456 | 456 | 0 + 456 | 456 | 0 + 123 | 123 | 0 + 123 | 123 | 0 +(4 rows) + +rollback; diff --git a/src/test/regress/sql/join_hash.sql b/src/test/regress/sql/join_hash.sql index 2978e155ecd..3f9f616b640 100644 --- a/src/test/regress/sql/join_hash.sql +++ b/src/test/regress/sql/join_hash.sql @@ -642,3 +642,22 @@ WHERE AND hjtest_1.a <> hjtest_2.b; ROLLBACK; + +-- Verify that we behave sanely when the inner hash keys contain parameters +-- (that is, outer or lateral references). This situation has to defeat +-- re-use of the inner hash table across rescans. +begin; +set local enable_hashjoin = on; + +explain (costs off) +select i8.q2, ss.* from +int8_tbl i8, +lateral (select t1.fivethous, i4.f1 from tenk1 t1 join int4_tbl i4 + on t1.fivethous = i4.f1+i8.q2 order by 1,2) ss; + +select i8.q2, ss.* from +int8_tbl i8, +lateral (select t1.fivethous, i4.f1 from tenk1 t1 join int4_tbl i4 + on t1.fivethous = i4.f1+i8.q2 order by 1,2) ss; + +rollback; From 4f334af4e764c323724a141a07820b1616f17b18 Mon Sep 17 00:00:00 2001 From: Amit Kapila Date: Wed, 21 Jun 2023 10:09:28 +0530 Subject: [PATCH 22/45] Fix the errhint message and docs for drop subscription failure. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing errhint message and docs were missing the fact that we can't disassociate from the slot unless the subscription is disabled. Author: Robert Sjöblom, Peter Smith Reviewed-by: Peter Eisentraut, Amit Kapila Backpatch-through: 11 Discussion: https://postgr.es/m/807bdf85-61ea-88e2-5712-6d9fcd4eabff@fortnox.se --- doc/src/sgml/ref/drop_subscription.sgml | 8 +++++--- src/backend/commands/subscriptioncmds.c | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml index aee96155463..d79f137c1e8 100644 --- a/doc/src/sgml/ref/drop_subscription.sgml +++ b/doc/src/sgml/ref/drop_subscription.sgml @@ -85,9 +85,11 @@ DROP SUBSCRIPTION [ IF EXISTS ] nameDROP SUBSCRIPTION command will fail. To proceed in - this situation, disassociate the subscription from the replication slot by - executing ALTER SUBSCRIPTION ... SET (slot_name = NONE). + the DROP SUBSCRIPTION command will fail. To proceed + in this situation, first disable the subscription by executing + ALTER SUBSCRIPTION ... DISABLE, and then disassociate + it from the replication slot by executing + ALTER SUBSCRIPTION ... SET (slot_name = NONE). After that, DROP SUBSCRIPTION will no longer attempt any actions on a remote host. Note that if the remote replication slot still exists, it (and any related table synchronization slots) should then be diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index e748ab06966..ad765535eaf 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -1654,7 +1654,8 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err) errmsg("could not connect to publisher when attempting to " "drop replication slot \"%s\": %s", slotname, err), /* translator: %s is an SQL ALTER command */ - errhint("Use %s to disassociate the subscription from the slot.", + errhint("Use %s to disable the subscription, and then use %s to disassociate it from the slot.", + "ALTER SUBSCRIPTION ... DISABLE", "ALTER SUBSCRIPTION ... SET (slot_name = NONE)"))); } From 52395f21e417158ba97c747fa777323c452a5bd9 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 21 Jun 2023 16:16:24 +0900 Subject: [PATCH 23/45] Disable use of archiving in 009_twophase.pl This partially reverts 68cb5af, as using archiving to enforce the rename of the last partial segment of the old timeline at promotion to use .partial as suffix is impacting the tests when it does switchovers. As showed by the logs gathered by the CI in the tests that failed, a new standby may fail to find the WAL segment it needs to follow a promoted instance with its timeline jump, as it got renamed to .partial. This problem would manifest as a run timeout with 009_twophase.pl, as the new standby repeatedly requests a segment from the promoted primary that it would not find. Reported-by: Nathan Bossart Discussion: https://postgr.es/m/20230621043345.GA787473@nathanxps13 Backpatch-through: 13 --- src/test/recovery/t/009_twophase.pl | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/test/recovery/t/009_twophase.pl b/src/test/recovery/t/009_twophase.pl index 9aaa9b11049..18a4cfcece3 100644 --- a/src/test/recovery/t/009_twophase.pl +++ b/src/test/recovery/t/009_twophase.pl @@ -39,9 +39,7 @@ sub configure_and_reload # Setup london node my $node_london = get_new_node("london"); -# Archiving is used to provide coverage with the creation of .partial segments -# done at the end of recovery and the recovery of two-phase transactions. -$node_london->init(allows_streaming => 1, has_archiving => 1); +$node_london->init(allows_streaming => 1); $node_london->append_conf( 'postgresql.conf', qq( max_prepared_transactions = 10 From 6d6cdf5f668f8fe3977db65683e18340dd738917 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 21 Jun 2023 11:07:11 -0400 Subject: [PATCH 24/45] Avoid Assert failure when processing empty statement in aborted xact. exec_parse_message() wants to create a cached plan in all cases, including for empty input. The empty-input path does not have a test for being in an aborted transaction, making it possible that plancache.c will fail due to trying to do database lookups even though there's no real work to do. One solution would be to throw an aborted-transaction error in this path too, but it's not entirely clear whether the lack of such an error was intentional or whether some clients might be relying on non-error behavior. Instead, let's hack plancache.c so that it treats empty statements with the same logic it already had for transaction control commands, ensuring that it can soldier through even in an already-aborted transaction. Per bug #17983 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/17983-da4569fcb878672e@postgresql.org --- src/backend/utils/cache/plancache.c | 4 +++- src/test/regress/expected/psql.out | 11 +++++++++++ src/test/regress/sql/psql.sql | 8 ++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c index 1f1c7635517..e157171339f 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -79,9 +79,11 @@ /* * We must skip "overhead" operations that involve database access when the * cached plan's subject statement is a transaction control command. + * For the convenience of postgres.c, treat empty statements as control + * commands too. */ #define IsTransactionStmtPlan(plansource) \ - ((plansource)->raw_parse_tree && \ + ((plansource)->raw_parse_tree == NULL || \ IsA((plansource)->raw_parse_tree->stmt, TransactionStmt)) /* diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out index 800e2761083..7e548c76f12 100644 --- a/src/test/regress/expected/psql.out +++ b/src/test/regress/expected/psql.out @@ -237,6 +237,17 @@ SELECT 3 AS x, 'Hello', 4 AS y, true AS "dirty\name" \gdesc \g 3 | Hello | 4 | t (1 row) +-- test for server bug #17983 with empty statement in aborted transaction +set search_path = default; +begin; +bogus; +ERROR: syntax error at or near "bogus" +LINE 1: bogus; + ^ +; +\gdesc +The command has no result, or the result has no columns. +rollback; -- \gexec create temporary table gexec_test(a int, b text, c date, d float); select format('create index on gexec_test(%I)', attname) diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql index 36a68595d5e..d1193eba1c1 100644 --- a/src/test/regress/sql/psql.sql +++ b/src/test/regress/sql/psql.sql @@ -119,6 +119,14 @@ SELECT 1 AS x, 'Hello', 2 AS y, true AS "dirty\name" -- all on one line SELECT 3 AS x, 'Hello', 4 AS y, true AS "dirty\name" \gdesc \g +-- test for server bug #17983 with empty statement in aborted transaction +set search_path = default; +begin; +bogus; +; +\gdesc +rollback; + -- \gexec create temporary table gexec_test(a int, b text, c date, d float); From be977c838fb46cedc95fa0da426e0655b86ada91 Mon Sep 17 00:00:00 2001 From: Bruce Momjian Date: Wed, 21 Jun 2023 19:20:07 -0400 Subject: [PATCH 25/45] doc: update PG history as over "three decades" Reported-by: Pierre Discussion: https://postgr.es/m/168724660637.399156.7642965215720120947@wrigleys.postgresql.org Backpatch-through: 11 --- doc/src/sgml/history.sgml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/src/sgml/history.sgml b/doc/src/sgml/history.sgml index aa171823a8d..a2aa19da024 100644 --- a/doc/src/sgml/history.sgml +++ b/doc/src/sgml/history.sgml @@ -12,7 +12,7 @@ The object-relational database management system now known as PostgreSQL is derived from the POSTGRES package written at the - University of California at Berkeley. With over two decades of + University of California at Berkeley. With over three decades of development behind it, PostgreSQL is now the most advanced open-source database available anywhere. From 1eac9179eb7037802575a81c3c09ac6b4decdfae Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Wed, 21 Jun 2023 17:41:54 -0700 Subject: [PATCH 26/45] nbtree VACUUM: cope with topparent inconsistencies. Avoid "right sibling %u of block %u is not next child" errors when vacuuming a corrupt nbtree index. Just LOG the issue and press on. That way VACUUM will have a decent chance of finishing off all required processing for the index (and for the table as a whole). This is similar to recent work from commit 5abff197, as well as work from commit 5b861baa (later backpatched as commit 43e409ce), which taught nbtree VACUUM to keep going when its "re-find" check fails. The hardening added by this commit takes place directly after the "re-find" check, right before the critical section for the first stage of page deletion. Author: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-Wz=dayg0vjs4+er84TS9ami=csdzjpuiCGbEw=idhwqhzQ@mail.gmail.com Backpatch: 11- (all supported versions). --- src/backend/access/nbtree/nbtpage.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c index 1e27ee4c569..ac8665a4bc1 100644 --- a/src/backend/access/nbtree/nbtpage.c +++ b/src/backend/access/nbtree/nbtpage.c @@ -2153,12 +2153,6 @@ _bt_mark_page_halfdead(Relation rel, Buffer leafbuf, BTStack stack) &topparent, &topparentrightsib)) return false; - /* - * Check that the parent-page index items we're about to delete/overwrite - * in subtree parent page contain what we expect. This can fail if the - * index has become corrupt for some reason. We want to throw any error - * before entering the critical section --- otherwise it'd be a PANIC. - */ page = BufferGetPage(subtreeparent); opaque = (BTPageOpaque) PageGetSpecialPointer(page); @@ -2176,8 +2170,17 @@ _bt_mark_page_halfdead(Relation rel, Buffer leafbuf, BTStack stack) nextoffset = OffsetNumberNext(poffset); itemid = PageGetItemId(page, nextoffset); itup = (IndexTuple) PageGetItem(page, itemid); + + /* + * Check that the parent-page index items we're about to delete/overwrite + * in subtree parent page contain what we expect. This can fail if the + * index has become corrupt for some reason. When that happens we back + * out of deletion of the leafbuf subtree. (This is just like the case + * where _bt_lock_subtree_parent() cannot "re-find" leafbuf's downlink.) + */ if (BTreeTupleGetDownLink(itup) != topparentrightsib) - ereport(ERROR, + { + ereport(LOG, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg_internal("right sibling %u of block %u is not next child %u of block %u in index \"%s\"", topparentrightsib, topparent, @@ -2185,6 +2188,11 @@ _bt_mark_page_halfdead(Relation rel, Buffer leafbuf, BTStack stack) BufferGetBlockNumber(subtreeparent), RelationGetRelationName(rel)))); + _bt_relbuf(rel, subtreeparent); + Assert(false); + return false; + } + /* * Any insert which would have gone on the leaf block will now go to its * right sibling. In other words, the key space moves right. @@ -2839,6 +2847,7 @@ _bt_lock_subtree_parent(Relation rel, BlockNumber child, BTStack stack, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg_internal("failed to re-find parent key in index \"%s\" for deletion target page %u", RelationGetRelationName(rel), child))); + Assert(false); return false; } From 46661f6f67db605b9157cc21706945c6c92e9c96 Mon Sep 17 00:00:00 2001 From: David Rowley Date: Thu, 22 Jun 2023 12:47:53 +1200 Subject: [PATCH 27/45] Doc: mention that extended stats aren't used for joins Statistics defined by the CREATE STATISTICS command are only used to assist with the selectivity estimations of base relations, never for joins. Here we mention this fact in the notes section of the CREATE STATISTICS command. Discussion: https://postgr.es/m/CAApHDvrMuVgDOrmg_EtFDZ=AOovq6EsJNnHH1ddyZ8EqL4yzMw@mail.gmail.com Backpatch-through: 11 --- doc/src/sgml/ref/create_statistics.sgml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/src/sgml/ref/create_statistics.sgml b/doc/src/sgml/ref/create_statistics.sgml index e3e5c297ddb..a145c41f04b 100644 --- a/doc/src/sgml/ref/create_statistics.sgml +++ b/doc/src/sgml/ref/create_statistics.sgml @@ -167,6 +167,12 @@ CREATE STATISTICS [ IF NOT EXISTS ] statistics_na maintenance. Expression statistics are built automatically for each expression in the statistics object definition. + + + Extended statistics are not currently used by the planner for selectivity + estimations made for table joins. This limitation will likely be removed + in a future version of PostgreSQL. + From 8b65e43dfa5e8d59b2036518431f9477bcc91013 Mon Sep 17 00:00:00 2001 From: Amit Kapila Date: Thu, 22 Jun 2023 12:16:51 +0530 Subject: [PATCH 28/45] Doc: Clarify the behavior of triggers/rules in a logical subscriber. By default, triggers and rules do not fire on a logical replication subscriber based on the "session_replication_role" GUC being set to "replica". However, the docs in the logical replication section assumed that the reader understood how this GUC worked. This modifies the docs to be more explicit and links back to the GUC itself. Author: Jonathan Katz, Peter Smith Reviewed-by: Vignesh C, Euler Taveira Backpatch-through: 11 Discussion: https://postgr.es/m/5bb2c9a2-499f-e1a2-6e33-5ce96b35cc4a@postgresql.org --- doc/src/sgml/logical-replication.sgml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml index 88646bc859d..5ff37a20186 100644 --- a/doc/src/sgml/logical-replication.sgml +++ b/doc/src/sgml/logical-replication.sgml @@ -465,9 +465,13 @@ The apply process on the subscriber database always runs with - session_replication_role set - to replica, which produces the usual effects on triggers - and constraints. + session_replication_role + set to replica. This means that, by default, + triggers and rules will not fire on a subscriber. Users can optionally choose to + enable triggers and rules on a table using the + ALTER TABLE command + and the ENABLE TRIGGER and ENABLE RULE + clauses. From e38c85a03f63ce4b0c99cb551a98f78daffd9b73 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 23 Jun 2023 17:50:28 +0900 Subject: [PATCH 29/45] Fix incorrect error message in libpq_pipeline One of the tests for the pipeline mode with portal description expects a non-NULL PQgetResult, but used an incorrect error message on failure, telling that PQgetResult being NULL was the expected result. Author: Jelte Fennema Discussion: https://postgr.es/m/CAGECzQTkShHecFF+EZrm94Lbsu2ej569T=bz+PjMbw9Aiioxuw@mail.gmail.com Backpatch-through: 14 --- src/test/modules/libpq_pipeline/libpq_pipeline.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c index 95e653c8c3b..b6f5fd28b23 100644 --- a/src/test/modules/libpq_pipeline/libpq_pipeline.c +++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c @@ -949,7 +949,7 @@ test_prepared(PGconn *conn) pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn)); res = PQgetResult(conn); if (res == NULL) - pg_fatal("expected NULL result"); + pg_fatal("PQgetResult returned null"); if (PQresultStatus(res) != PGRES_COMMAND_OK) pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res))); From 2347be5db12ef851e05a2e2724b31442e1aec6bc Mon Sep 17 00:00:00 2001 From: Bruce Momjian Date: Fri, 23 Jun 2023 22:50:55 -0400 Subject: [PATCH 30/45] doc: rename "decades" to be more generic Reported-by: Michael Paquier Discussion: https://postgr.es/m/ZJTzwD2rTbHWWQ9g@paquier.xyz Backpatch-through: 11 --- doc/src/sgml/history.sgml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/src/sgml/history.sgml b/doc/src/sgml/history.sgml index a2aa19da024..120b4b3c047 100644 --- a/doc/src/sgml/history.sgml +++ b/doc/src/sgml/history.sgml @@ -12,7 +12,7 @@ The object-relational database management system now known as PostgreSQL is derived from the POSTGRES package written at the - University of California at Berkeley. With over three decades of + University of California at Berkeley. With decades of development behind it, PostgreSQL is now the most advanced open-source database available anywhere. From 986f5772719feec8e32f21627735932b3f5b1f6a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 24 Jun 2023 17:18:08 -0400 Subject: [PATCH 31/45] Check for interrupts and stack overflow in TParserGet(). TParserGet() recurses for some token types, meaning it's possible to drive it to stack overflow. Since this is a minority behavior, I chose to add the check_stack_depth() call to the two places that recurse rather than doing it during every single call. While at it, add CHECK_FOR_INTERRUPTS(), because this can run unpleasantly long for long inputs. Per bug #17995 from Zuming Jiang. This is old, so back-patch to all supported branches. Discussion: https://postgr.es/m/17995-9f20ff3e6389db4c@postgresql.org --- src/backend/tsearch/wparser_def.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/backend/tsearch/wparser_def.c b/src/backend/tsearch/wparser_def.c index 98029e78fb4..290c2223205 100644 --- a/src/backend/tsearch/wparser_def.c +++ b/src/backend/tsearch/wparser_def.c @@ -18,6 +18,7 @@ #include "catalog/pg_collation.h" #include "commands/defrem.h" +#include "miscadmin.h" #include "tsearch/ts_locale.h" #include "tsearch/ts_public.h" #include "tsearch/ts_type.h" @@ -632,6 +633,12 @@ p_ishost(TParser *prs) tmpprs->wanthost = true; + /* + * Check stack depth before recursing. (Since TParserGet() doesn't + * normally recurse, we put the cost of checking here not there.) + */ + check_stack_depth(); + if (TParserGet(tmpprs) && tmpprs->type == HOST) { prs->state->posbyte += tmpprs->lenbytetoken; @@ -655,6 +662,12 @@ p_isURLPath(TParser *prs) tmpprs->state = newTParserPosition(tmpprs->state); tmpprs->state->state = TPS_InURLPathFirst; + /* + * Check stack depth before recursing. (Since TParserGet() doesn't + * normally recurse, we put the cost of checking here not there.) + */ + check_stack_depth(); + if (TParserGet(tmpprs) && tmpprs->type == URLPATH) { prs->state->posbyte += tmpprs->lenbytetoken; @@ -1698,6 +1711,8 @@ TParserGet(TParser *prs) { const TParserStateActionItem *item = NULL; + CHECK_FOR_INTERRUPTS(); + Assert(prs->state); if (prs->state->posbyte >= prs->lenstr) From e4863e5f275eb0b6f389a71a76eedbd99066e7a1 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 27 Jun 2023 10:11:31 +0300 Subject: [PATCH 32/45] Fix comment on clearing padding. Author: Japin Li Discussion: https://www.postgresql.org/message-id/MEYP282MB16696317B5DA7D0D92306149B627A@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM --- contrib/pg_stat_statements/pg_stat_statements.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 94c39f44f0c..e9b300a8237 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1258,7 +1258,7 @@ pgss_store(const char *query, uint64 queryId, /* Set up key for hashtable search */ - /* memset() is required when pgssHashKey is without padding only */ + /* clear padding */ memset(&key, 0, sizeof(pgssHashKey)); key.userid = GetUserId(); From 29194781d253615ee9be12c22f0ebf73c8ae7f54 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 28 Jun 2023 15:57:48 +0900 Subject: [PATCH 33/45] Ignore invalid indexes when enforcing index rules in ALTER TABLE ATTACH PARTITION A portion of ALTER TABLE .. ATTACH PARTITION is to ensure that the partition being attached to the partitioned table has a correct set of indexes, so as there is a consistent index mapping between the partitioned table and its new-to-be partition. However, as introduced in 8b08f7d, the current logic could choose an invalid index as a match, which is something that can exist when dealing with more than two levels of partitioning, like attaching a partitioned table (that has partitions, with an index created by CREATE INDEX ON ONLY) to another partitioned table. A partitioned index with indisvalid set to false is equivalent to an incomplete partition tree, meaning that an invalid partitioned index does not have indexes defined in all its partitions. Hence, choosing an invalid partitioned index can create inconsistent partition index trees, where the parent attaching to is valid, but its partition may be invalid. In the report from Alexander Lakhin, this showed up as an assertion failure when validating an index. Without assertions enabled, the partition index tree would be actually broken, as indisvalid should be switched to true for a partitioned index once all its partitions are themselves valid. With two levels of partitioning, the top partitioned table used a valid index and was able to link to an invalid index stored on its partition, itself a partitioned table. I have studied a few options here (like the possibility to switch indisvalid to false for the parent), but came down to the conclusion that we'd better rely on a simple rule: invalid indexes had better never be chosen, so as the partition attached uses and creates indexes that the parent expects. Some regression tests are added to provide some coverage. Note that the existing coverage is not impacted. This is a problem since partitioned indexes exist, so backpatch all the way down to v11. Reported-by: Alexander Lakhin Discussion: https://postgr.es/14987634-43c0-0cb3-e075-94d423607e08@gmail.com Backpatch-through: 11 --- src/backend/commands/tablecmds.c | 8 +++++-- src/test/regress/expected/indexing.out | 30 ++++++++++++++++++++++++++ src/test/regress/sql/indexing.sql | 22 +++++++++++++++++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 76e939cf54d..3d3b3d9b78b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -21967,8 +21967,8 @@ AttachPartitionEnsureIndexes(Relation rel, Relation attachrel) /* * Scan the list of existing indexes in the partition-to-be, and mark - * the first matching, unattached one we find, if any, as partition of - * the parent index. If we find one, we're done. + * the first matching, valid, unattached one we find, if any, as + * partition of the parent index. If we find one, we're done. */ for (i = 0; i < list_length(attachRelIdxs); i++) { @@ -21979,6 +21979,10 @@ AttachPartitionEnsureIndexes(Relation rel, Relation attachrel) if (attachrelIdxRels[i]->rd_rel->relispartition) continue; + /* If this index is invalid, can't use it */ + if (!attachrelIdxRels[i]->rd_index->indisvalid) + continue; + if (CompareIndexInfo(attachInfos[i], info, attachrelIdxRels[i]->rd_indcollation, idxRel->rd_indcollation, diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index 6573c0040cc..3d0cd1b305f 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -1453,3 +1453,33 @@ Indexes: "parted_index_col_drop11_b_idx" btree (b) drop table parted_index_col_drop; +-- Check that invalid indexes are not selected when attaching a partition. +create table parted_inval_tab (a int) partition by range (a); +create index parted_inval_idx on parted_inval_tab (a); +create table parted_inval_tab_1 (a int) partition by range (a); +create table parted_inval_tab_1_1 partition of parted_inval_tab_1 + for values from (0) to (10); +create table parted_inval_tab_1_2 partition of parted_inval_tab_1 + for values from (10) to (20); +-- this creates an invalid index. +create index parted_inval_ixd_1 on only parted_inval_tab_1 (a); +-- this creates new indexes for all the partitions of parted_inval_tab_1, +-- discarding the invalid index created previously as what is chosen. +alter table parted_inval_tab attach partition parted_inval_tab_1 + for values from (1) to (100); +select indexrelid::regclass, indisvalid, + indrelid::regclass, inhparent::regclass + from pg_index idx left join + pg_inherits inh on (idx.indexrelid = inh.inhrelid) + where indexrelid::regclass::text like 'parted_inval%' + order by indexrelid::regclass::text collate "C"; + indexrelid | indisvalid | indrelid | inhparent +----------------------------+------------+----------------------+-------------------------- + parted_inval_idx | t | parted_inval_tab | + parted_inval_ixd_1 | f | parted_inval_tab_1 | + parted_inval_tab_1_1_a_idx | t | parted_inval_tab_1_1 | parted_inval_tab_1_a_idx + parted_inval_tab_1_2_a_idx | t | parted_inval_tab_1_2 | parted_inval_tab_1_a_idx + parted_inval_tab_1_a_idx | t | parted_inval_tab_1 | parted_inval_idx +(5 rows) + +drop table parted_inval_tab; diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index 0053083c224..c9ea9b1f8b4 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -789,3 +789,25 @@ alter table parted_index_col_drop drop column c; \d parted_index_col_drop2 \d parted_index_col_drop11 drop table parted_index_col_drop; + +-- Check that invalid indexes are not selected when attaching a partition. +create table parted_inval_tab (a int) partition by range (a); +create index parted_inval_idx on parted_inval_tab (a); +create table parted_inval_tab_1 (a int) partition by range (a); +create table parted_inval_tab_1_1 partition of parted_inval_tab_1 + for values from (0) to (10); +create table parted_inval_tab_1_2 partition of parted_inval_tab_1 + for values from (10) to (20); +-- this creates an invalid index. +create index parted_inval_ixd_1 on only parted_inval_tab_1 (a); +-- this creates new indexes for all the partitions of parted_inval_tab_1, +-- discarding the invalid index created previously as what is chosen. +alter table parted_inval_tab attach partition parted_inval_tab_1 + for values from (1) to (100); +select indexrelid::regclass, indisvalid, + indrelid::regclass, inhparent::regclass + from pg_index idx left join + pg_inherits inh on (idx.indexrelid = inh.inhrelid) + where indexrelid::regclass::text like 'parted_inval%' + order by indexrelid::regclass::text collate "C"; +drop table parted_inval_tab; From 603707e2d7ef949baec58c1507ff61faa445d8a7 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Thu, 29 Jun 2023 08:05:10 +0900 Subject: [PATCH 34/45] pg_stat_statements: Fix incorrect comment with entry resets Oversight in 6b4d23f. Author: Japin Li, Richard Guo Discussion: https://postgr.es/m/MEYP282MB1669FC91C764E277821936D3B624A@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM Backpatch-through: 14 --- contrib/pg_stat_statements/pg_stat_statements.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index e9b300a8237..62f99efc8fb 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -2535,10 +2535,8 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid) if (entry) /* found */ num_remove++; - /* Also remove entries for top level statements */ + /* Also remove the top-level entry if it exists. */ key.toplevel = true; - - /* Remove the key if exists */ entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL); if (entry) /* found */ num_remove++; From bd8faa4c70a436c4ab4daf778b0aa62fef976338 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Thu, 29 Jun 2023 09:17:34 +0900 Subject: [PATCH 35/45] pg_stat_statements: Fix second comment related to entry resets This should have been part of dc73db6, but it got lost in the mix. Oversight in 6b4d23f. Author: Japin Li Discussion: https://postgr.es/m/MEYP282MB1669FC91C764E277821936D3B624A@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM Backpatch-through: 14 --- contrib/pg_stat_statements/pg_stat_statements.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 62f99efc8fb..348b2e0ab0b 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -2529,7 +2529,9 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid) key.dbid = dbid; key.queryid = queryid; - /* Remove the key if it exists, starting with the top-level entry */ + /* + * Remove the key if it exists, starting with the non-top-level entry. + */ key.toplevel = false; entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL); if (entry) /* found */ From 5ddd5e12b13b243de7140e88baf0e8d1d1498c3f Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 29 Jun 2023 10:30:55 +0200 Subject: [PATCH 36/45] Remove inappropriate raw_expression_tree_walker() code It was walking into the ColumnDef->compression field, which is not a node but a string. This code is currently not reachable (because the compression field is only set in situations that don't go through raw_expression_tree_walker()), but if it had been, this could have behaved erratically. --- src/backend/nodes/nodeFuncs.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c index 6c45d349524..1eb9f6daaa6 100644 --- a/src/backend/nodes/nodeFuncs.c +++ b/src/backend/nodes/nodeFuncs.c @@ -4236,8 +4236,6 @@ raw_expression_tree_walker(Node *node, if (walker(coldef->typeName, context)) return true; - if (walker(coldef->compression, context)) - return true; if (walker(coldef->raw_default, context)) return true; if (walker(coldef->collClause, context)) From 57079526c6be91833e6ffb256de88d91d90f5adf Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 29 Jun 2023 10:19:10 -0400 Subject: [PATCH 37/45] Fix order of operations in ExecEvalFieldStoreDeForm(). If the given composite datum is toasted out-of-line, DatumGetHeapTupleHeader will perform database accesses to detoast it. That can invalidate the result of get_cached_rowtype, as documented (perhaps not plainly enough) in that function's API spec; which leads to strange errors or crashes when we try to use the TupleDesc to read the tuple. In short then, trying to update a field of a composite column could fail intermittently if the overall column value is wide enough to require toasting. We can fix the bug at no cost by just changing the order of operations, since we don't need the TupleDesc until after detoasting. (Other callers of get_cached_rowtype appear to get this right already, so there's only one bug.) Note that the added regression test case reveals this bug reliably only with debug_discard_caches/CLOBBER_CACHE_ALWAYS. Per bug #17994 from Alexander Lakhin. Sadly, this patch does not fix the missing-values issue revealed in the bug discussion; we'll need some more work to cover that. Discussion: https://postgr.es/m/17994-5c7100b51b4790e9@postgresql.org --- src/backend/executor/execExprInterp.c | 29 +++++++++++++++----------- src/test/regress/expected/rowtypes.out | 9 ++++++++ src/test/regress/sql/rowtypes.sql | 5 +++++ 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 56874a3f78a..5da5134ccd2 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -2051,7 +2051,8 @@ CheckOpSlotCompatibility(ExprEvalStep *op, TupleTableSlot *slot) * changed: if not NULL, *changed is set to true on any update * * The returned TupleDesc is not guaranteed pinned; caller must pin it - * to use it across any operation that might incur cache invalidation. + * to use it across any operation that might incur cache invalidation, + * including for example detoasting of input tuples. * (The TupleDesc is always refcounted, so just use IncrTupleDescRefCount.) * * NOTE: because composite types can change contents, we must be prepared @@ -3239,17 +3240,6 @@ ExecEvalFieldSelect(ExprState *state, ExprEvalStep *op, ExprContext *econtext) void ExecEvalFieldStoreDeForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext) { - TupleDesc tupDesc; - - /* Lookup tupdesc if first time through or if type changes */ - tupDesc = get_cached_rowtype(op->d.fieldstore.fstore->resulttype, -1, - op->d.fieldstore.rowcache, NULL); - - /* Check that current tupdesc doesn't have more fields than we allocated */ - if (unlikely(tupDesc->natts > op->d.fieldstore.ncolumns)) - elog(ERROR, "too many columns in composite type %u", - op->d.fieldstore.fstore->resulttype); - if (*op->resnull) { /* Convert null input tuple into an all-nulls row */ @@ -3265,6 +3255,7 @@ ExecEvalFieldStoreDeForm(ExprState *state, ExprEvalStep *op, ExprContext *econte Datum tupDatum = *op->resvalue; HeapTupleHeader tuphdr; HeapTupleData tmptup; + TupleDesc tupDesc; tuphdr = DatumGetHeapTupleHeader(tupDatum); tmptup.t_len = HeapTupleHeaderGetDatumLength(tuphdr); @@ -3272,6 +3263,20 @@ ExecEvalFieldStoreDeForm(ExprState *state, ExprEvalStep *op, ExprContext *econte tmptup.t_tableOid = InvalidOid; tmptup.t_data = tuphdr; + /* + * Lookup tupdesc if first time through or if type changes. Because + * we don't pin the tupdesc, we must not do this lookup until after + * doing DatumGetHeapTupleHeader: that could do database access while + * detoasting the datum. + */ + tupDesc = get_cached_rowtype(op->d.fieldstore.fstore->resulttype, -1, + op->d.fieldstore.rowcache, NULL); + + /* Check that current tupdesc doesn't have more fields than allocated */ + if (unlikely(tupDesc->natts > op->d.fieldstore.ncolumns)) + elog(ERROR, "too many columns in composite type %u", + op->d.fieldstore.fstore->resulttype); + heap_deform_tuple(&tmptup, tupDesc, op->d.fieldstore.values, op->d.fieldstore.nulls); diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out index c49469102ad..03dc2ab3a79 100644 --- a/src/test/regress/expected/rowtypes.out +++ b/src/test/regress/expected/rowtypes.out @@ -145,6 +145,15 @@ select (fn).first, substr((fn).last, 1, 20), length((fn).last) from people; Jim | abcdefghijklabcdefgh | 1200000 (2 rows) +-- try an update on a toasted composite value, too +update people set fn.first = 'Jack'; +select (fn).first, substr((fn).last, 1, 20), length((fn).last) from people; + first | substr | length +-------+----------------------+--------- + Jack | Blow | 4 + Jack | abcdefghijklabcdefgh | 1200000 +(2 rows) + -- Test row comparison semantics. Prior to PG 8.2 we did this in a totally -- non-spec-compliant way. select ROW(1,2) < ROW(1,3) as true; diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql index f4986fab488..54f338af1f8 100644 --- a/src/test/regress/sql/rowtypes.sql +++ b/src/test/regress/sql/rowtypes.sql @@ -90,6 +90,11 @@ insert into people select ('Jim', f1, null)::fullname, current_date from pp; select (fn).first, substr((fn).last, 1, 20), length((fn).last) from people; +-- try an update on a toasted composite value, too +update people set fn.first = 'Jack'; + +select (fn).first, substr((fn).last, 1, 20), length((fn).last) from people; + -- Test row comparison semantics. Prior to PG 8.2 we did this in a totally -- non-spec-compliant way. From 073c5f0e8248c91c4bedb1901839738f7acfbf4f Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 30 Jun 2023 13:54:56 +0900 Subject: [PATCH 38/45] Fix marking of indisvalid for partitioned indexes at creation The logic that introduced partitioned indexes missed a few things when invalidating a partitioned index when these are created, still the code is written to handle recursions: 1) If created from scratch because a mapping index could not be found, the new index created could be itself invalid, if for example it was a partitioned index with one of its leaves invalid. 2) A CCI was missing when indisvalid is set for a parent index, leading to inconsistent trees when recursing across more than one level for a partitioned index creation if an invalidation of the parent was required. This could lead to the creation of a partition index tree where some of the partitioned indexes are marked as invalid, but some of the parents are marked valid, which is not something that should happen (as validatePartitionedIndex() defines, indisvalid is switched to true for a partitioned index iff all its partitions are themselves valid). This patch makes sure that indisvalid is set to false on a partitioned index if at least one of its partition is invalid. The flag is set to true if *all* its partitions are valid. The regression test added in this commit abuses of a failed concurrent index creation, marked as invalid, that maps with an index created on its partitioned table afterwards. Reported-by: Alexander Lakhin Reviewed-by: Alexander Lakhin Discussion: https://postgr.es/m/14987634-43c0-0cb3-e075-94d423607e08@gmail.com Backpatch-through: 11 --- src/backend/commands/indexcmds.c | 29 ++++++++++++++++----- src/test/regress/expected/indexing.out | 35 ++++++++++++++++++++++++++ src/test/regress/sql/indexing.sql | 26 +++++++++++++++++++ 3 files changed, 84 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 6618d75cdbd..0a6df42bce9 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1818,6 +1818,7 @@ DefineIndex(Oid relationId, IndexStmt *childStmt = copyObject(stmt); bool found_whole_row; ListCell *lc; + ObjectAddress childAddr; /* * We can't use the same index name for the child index, @@ -1871,14 +1872,24 @@ DefineIndex(Oid relationId, Assert(GetUserId() == child_save_userid); SetUserIdAndSecContext(root_save_userid, root_save_sec_context); - DefineIndex(childRelid, childStmt, - InvalidOid, /* no predefined OID */ - indexRelationId, /* this is our child */ - createdConstraintId, - is_alter_table, check_rights, check_not_in_use, - skip_build, quiet); + childAddr = + DefineIndex(childRelid, childStmt, + InvalidOid, /* no predefined OID */ + indexRelationId, /* this is our child */ + createdConstraintId, + is_alter_table, check_rights, + check_not_in_use, + skip_build, quiet); SetUserIdAndSecContext(child_save_userid, child_save_sec_context); + + /* + * Check if the index just created is valid or not, as it + * could be possible that it has been switched as invalid + * when recursing across multiple partition levels. + */ + if (!get_index_isvalid(childAddr.objectId)) + invalidate_parent = true; } pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, @@ -1910,6 +1921,12 @@ DefineIndex(Oid relationId, ReleaseSysCache(tup); table_close(pg_index, RowExclusiveLock); heap_freetuple(newtup); + + /* + * CCI here to make this update visible, in case this recurses + * across multiple partition levels. + */ + CommandCounterIncrement(); } } diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index 3d0cd1b305f..ae16990f0f0 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -1483,3 +1483,38 @@ select indexrelid::regclass, indisvalid, (5 rows) drop table parted_inval_tab; +-- Check setup of indisvalid across a complex partition tree on index +-- creation. If one index in a partition index is invalid, so should its +-- partitioned index. +create table parted_isvalid_tab (a int, b int) partition by range (a); +create table parted_isvalid_tab_1 partition of parted_isvalid_tab + for values from (1) to (10) partition by range (a); +create table parted_isvalid_tab_2 partition of parted_isvalid_tab + for values from (10) to (20) partition by range (a); +create table parted_isvalid_tab_11 partition of parted_isvalid_tab_1 + for values from (1) to (5); +create table parted_isvalid_tab_12 partition of parted_isvalid_tab_1 + for values from (5) to (10); +-- create an invalid index on one of the partitions. +insert into parted_isvalid_tab_11 values (1, 0); +create index concurrently parted_isvalid_idx_11 on parted_isvalid_tab_11 ((a/b)); +ERROR: division by zero +-- The previous invalid index is selected, invalidating all the indexes up to +-- the top-most parent. +create index parted_isvalid_idx on parted_isvalid_tab ((a/b)); +select indexrelid::regclass, indisvalid, + indrelid::regclass, inhparent::regclass + from pg_index idx left join + pg_inherits inh on (idx.indexrelid = inh.inhrelid) + where indexrelid::regclass::text like 'parted_isvalid%' + order by indexrelid::regclass::text collate "C"; + indexrelid | indisvalid | indrelid | inhparent +--------------------------------+------------+-----------------------+------------------------------- + parted_isvalid_idx | f | parted_isvalid_tab | + parted_isvalid_idx_11 | f | parted_isvalid_tab_11 | parted_isvalid_tab_1_expr_idx + parted_isvalid_tab_12_expr_idx | t | parted_isvalid_tab_12 | parted_isvalid_tab_1_expr_idx + parted_isvalid_tab_1_expr_idx | f | parted_isvalid_tab_1 | parted_isvalid_idx + parted_isvalid_tab_2_expr_idx | t | parted_isvalid_tab_2 | parted_isvalid_idx +(5 rows) + +drop table parted_isvalid_tab; diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index c9ea9b1f8b4..3d388638d26 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -811,3 +811,29 @@ select indexrelid::regclass, indisvalid, where indexrelid::regclass::text like 'parted_inval%' order by indexrelid::regclass::text collate "C"; drop table parted_inval_tab; + +-- Check setup of indisvalid across a complex partition tree on index +-- creation. If one index in a partition index is invalid, so should its +-- partitioned index. +create table parted_isvalid_tab (a int, b int) partition by range (a); +create table parted_isvalid_tab_1 partition of parted_isvalid_tab + for values from (1) to (10) partition by range (a); +create table parted_isvalid_tab_2 partition of parted_isvalid_tab + for values from (10) to (20) partition by range (a); +create table parted_isvalid_tab_11 partition of parted_isvalid_tab_1 + for values from (1) to (5); +create table parted_isvalid_tab_12 partition of parted_isvalid_tab_1 + for values from (5) to (10); +-- create an invalid index on one of the partitions. +insert into parted_isvalid_tab_11 values (1, 0); +create index concurrently parted_isvalid_idx_11 on parted_isvalid_tab_11 ((a/b)); +-- The previous invalid index is selected, invalidating all the indexes up to +-- the top-most parent. +create index parted_isvalid_idx on parted_isvalid_tab ((a/b)); +select indexrelid::regclass, indisvalid, + indrelid::regclass, inhparent::regclass + from pg_index idx left join + pg_inherits inh on (idx.indexrelid = inh.inhrelid) + where indexrelid::regclass::text like 'parted_isvalid%' + order by indexrelid::regclass::text collate "C"; +drop table parted_isvalid_tab; From 4ee20e96cc04e28abcf5c07db2d2553407eb38ce Mon Sep 17 00:00:00 2001 From: Bruce Momjian Date: Fri, 30 Jun 2023 08:37:15 -0400 Subject: [PATCH 39/45] doc: PG _14_ relnotes, remove duplicate commit comment Backpatch-through: 14 only --- doc/src/sgml/release-14.sgml | 2 -- 1 file changed, 2 deletions(-) diff --git a/doc/src/sgml/release-14.sgml b/doc/src/sgml/release-14.sgml index f4d6d11ca4d..6ddf4591286 100644 --- a/doc/src/sgml/release-14.sgml +++ b/doc/src/sgml/release-14.sgml @@ -7357,8 +7357,6 @@ Author: Tom Lane From 56b9e0e779182945874569eeb77d102f7015b36a Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sun, 2 Jul 2023 18:54:09 +0200 Subject: [PATCH 40/45] Fix memory leak in Incremental Sort rescans The Incremental Sort had a couple issues, resulting in leaking memory during rescans, possibly triggering OOM. The code had a couple of related flaws: 1. During rescans, the sort states were reset but then also set to NULL (despite the comment saying otherwise). ExecIncrementalSort then sees NULL and initializes a new sort state, leaking the memory used by the old one. 2. Initializing the sort state also automatically rebuilt the info about presorted keys, leaking the already initialized info. presorted_keys was also unnecessarily reset to NULL. Patch by James Coleman, based on patches by Laurenz Albe and Tom Lane. Backpatch to 13, where Incremental Sort was introduced. Author: James Coleman, Laurenz Albe, Tom Lane Reported-by: Laurenz Albe, Zu-Ming Jiang Backpatch-through: 13 Discussion: https://postgr.es/m/b2bd02dff61af15e3526293e2771f874cf2a3be7.camel%40cybertec.at Discussion: https://postgr.es/m/db03c582-086d-e7cd-d4a1-3bc722f81765%40inf.ethz.ch --- src/backend/executor/nodeIncrementalSort.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c index b3d653c3568..7a1db175446 100644 --- a/src/backend/executor/nodeIncrementalSort.c +++ b/src/backend/executor/nodeIncrementalSort.c @@ -1138,7 +1138,6 @@ ExecReScanIncrementalSort(IncrementalSortState *node) node->outerNodeDone = false; node->n_fullsort_remaining = 0; node->bound_Done = 0; - node->presorted_keys = NULL; node->execution_status = INCSORT_LOADFULLSORT; @@ -1151,15 +1150,9 @@ ExecReScanIncrementalSort(IncrementalSortState *node) * cause a leak. */ if (node->fullsort_state != NULL) - { tuplesort_reset(node->fullsort_state); - node->fullsort_state = NULL; - } if (node->prefixsort_state != NULL) - { tuplesort_reset(node->prefixsort_state); - node->prefixsort_state = NULL; - } /* * If chgParam of subnode is not null, then the plan will be re-scanned by From e1a2e5fe0bbb88192767e1305733b0da80751460 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sun, 2 Jul 2023 20:29:01 +0200 Subject: [PATCH 41/45] Fix oversight in handling of modifiedCols since f24523672d Commit f24523672d fixed a memory leak by moving the modifiedCols bitmap into the per-row memory context. In the case of AFTER UPDATE triggers, the bitmap is however referenced from an event kept until the end of the query, resulting in a use-after-free bug. Fixed by copying the bitmap into the AfterTriggerEvents memory context, which is the one where we keep the trigger events. There's only one place that needs to do the copy, but the memory context may not exist yet. Doing that in a separate function seems more readable. Report by Alexander Pyhalov, fix by me. Backpatch to 13, where the bitmap was added to the event by commit 71d60e2aa0. Reported-by: Alexander Pyhalov Backpatch-through: 13 Discussion: https://postgr.es/m/acddb17c89b0d6cb940eaeda18c08bbe@postgrespro.ru --- src/backend/commands/trigger.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index b52c35fe6b7..45794a55753 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -3743,6 +3743,37 @@ afterTriggerCheckState(AfterTriggerShared evtshared) return ((evtshared->ats_event & AFTER_TRIGGER_INITDEFERRED) != 0); } +/* ---------- + * afterTriggerCopyBitmap() + * + * Copy bitmap into AfterTriggerEvents memory context, which is where the after + * trigger events are kept. + * ---------- + */ +static Bitmapset * +afterTriggerCopyBitmap(Bitmapset *src) +{ + Bitmapset *dst; + MemoryContext oldcxt; + + if (src == NULL) + return NULL; + + /* Create event context if we didn't already */ + if (afterTriggers.event_cxt == NULL) + afterTriggers.event_cxt = + AllocSetContextCreate(TopTransactionContext, + "AfterTriggerEvents", + ALLOCSET_DEFAULT_SIZES); + + oldcxt = MemoryContextSwitchTo(afterTriggers.event_cxt); + + dst = bms_copy(src); + + MemoryContextSwitchTo(oldcxt); + + return dst; +} /* ---------- * afterTriggerAddEvent() @@ -6020,7 +6051,7 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo, new_shared.ats_table = transition_capture->tcs_private; else new_shared.ats_table = NULL; - new_shared.ats_modifiedcols = modifiedCols; + new_shared.ats_modifiedcols = afterTriggerCopyBitmap(modifiedCols); afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_depth].events, &new_event, &new_shared); From d642fd27d375c5092ba5eee32699dc740f939c14 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 3 Jul 2023 10:06:16 +0900 Subject: [PATCH 42/45] Make PG_TEST_NOCLEAN work for temporary directories in TAP tests When set, this environment variable was only effective for data directories but not for all the other temporary files created by PostgreSQL::Test::Utils. Keeping the temporary files after a successful run can be useful for debugging purposes. The documentation is updated to reflect the new behavior, with contents available in doc/ since v16 and in src/test/perl/README since v15. Author: Jacob Champion Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/CAAWbhmgHtDH1SGZ+Fw05CsXtE0mzTmjbuUxLB9mY9iPKgM6cUw@mail.gmail.com Discussion: https://postgr.es/m/YyPd9unV14SX2bLF@paquier.xyz Backpatch-through: 11 --- src/test/perl/TestLib.pm | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm index 2a354ea4ef9..7860425b543 100644 --- a/src/test/perl/TestLib.pm +++ b/src/test/perl/TestLib.pm @@ -268,7 +268,7 @@ sub all_tests_passing Securely create a temporary directory inside C<$tmp_check>, like C, and return its name. The directory will be removed automatically at the -end of the tests. +end of the tests, unless the environment variable PG_TEST_NOCLEAN is provided. If C is given, the new directory is templated as C<${prefix}_XXXX>. Otherwise the template is C. @@ -282,7 +282,7 @@ sub tempdir return File::Temp::tempdir( $prefix . '_XXXX', DIR => $tmp_check, - CLEANUP => 1); + CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'}); } =pod @@ -297,7 +297,8 @@ name, to avoid path length issues. sub tempdir_short { - return File::Temp::tempdir(CLEANUP => 1); + return File::Temp::tempdir( + CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'}); } =pod From 20b2f1403444b1abf8b71b5c8201427f1a770314 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Mon, 3 Jul 2023 10:06:26 -0400 Subject: [PATCH 43/45] Improve pg_basebackup long file name test Windows robustness Creation of a file with a very long name can create problems on Windows due to its file path limits. Work around that by creating the file via a symlink with a shorter name. Error displayed by buildfarm animal fairywren.o Backpatch to all live branches --- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index abc4f388906..e55a0bddf5b 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -208,8 +208,12 @@ '-T with invalid format fails'); # Tar format doesn't support filenames longer than 100 bytes. +# Create the test file via a short name directory so it doesn't blow the +# Windows path limit. +my $lftmp = PostgreSQL::Test::Utils::tempdir_short; +dir_symlink "$pgdata", "$lftmp/pgdata"; my $superlongname = "superlongname_" . ("x" x 100); -my $superlongpath = "$pgdata/$superlongname"; +my $superlongpath = "$lftmp/pgdata/$superlongname"; open my $file, '>', "$superlongpath" or die "unable to create file $superlongpath"; @@ -217,7 +221,7 @@ $node->command_fails( [ 'pg_basebackup', '-D', "$tempdir/tarbackup_l1", '--target-gp-dbid', '123', '-Ft' ], 'pg_basebackup tar with long name fails'); -unlink "$pgdata/$superlongname"; +unlink "$superlongpath"; # The following tests are for symlinks. From f3cf04ccd609556349eabb5b5126c871dc8f2fed Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Mon, 3 Jul 2023 10:46:49 -0400 Subject: [PATCH 44/45] Use older package name in pg_basebackup test Commit 83ed4de20f inadvertently used the new package names. In version 14 or older, use TestLib intead of using PostgreSQL::Test::Utils --- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index e55a0bddf5b..eeadbd1257b 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -210,7 +210,7 @@ # Tar format doesn't support filenames longer than 100 bytes. # Create the test file via a short name directory so it doesn't blow the # Windows path limit. -my $lftmp = PostgreSQL::Test::Utils::tempdir_short; +my $lftmp = TestLib::tempdir_short; dir_symlink "$pgdata", "$lftmp/pgdata"; my $superlongname = "superlongname_" . ("x" x 100); my $superlongpath = "$lftmp/pgdata/$superlongname"; From 038e7341c0b851d210c2850f00cdd3a94913b6c6 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 3 Jul 2023 18:16:58 +0200 Subject: [PATCH 45/45] Remove expensive test of postgres_fdw batch inserts The test inserted 70k rows into a foreign table, in order to verify correct behavior with more than 65535 parameters, and was added in response to a bug report. However, this is rather expensive, especially when running the tests under valgrind, CLOBBER_CACHE_ALWAYS etc. It doesn't seem worth it to keep running the test, so remove it from all branches (14+). Backpatch-through: 14 Discussion: https://postgr.es/m/2131017.1623451468@sss.pgh.pa.us --- contrib/postgres_fdw/expected/postgres_fdw.out | 11 ----------- contrib/postgres_fdw/sql/postgres_fdw.sql | 7 ------- 2 files changed, 18 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 9b70906a4a9..6de00516fed 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -10096,17 +10096,6 @@ SELECT COUNT(*) FROM ftable; 34 (1 row) -TRUNCATE batch_table; -DROP FOREIGN TABLE ftable; --- try if large batches exceed max number of bind parameters -CREATE FOREIGN TABLE ftable ( x int ) SERVER loopback OPTIONS ( table_name 'batch_table', batch_size '100000' ); -INSERT INTO ftable SELECT * FROM generate_series(1, 70000) i; -SELECT COUNT(*) FROM ftable; - count -------- - 70000 -(1 row) - TRUNCATE batch_table; DROP FOREIGN TABLE ftable; -- Disable batch insert diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index ac290d3ba30..0b33965a6ac 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -3119,13 +3119,6 @@ SELECT COUNT(*) FROM ftable; TRUNCATE batch_table; DROP FOREIGN TABLE ftable; --- try if large batches exceed max number of bind parameters -CREATE FOREIGN TABLE ftable ( x int ) SERVER loopback OPTIONS ( table_name 'batch_table', batch_size '100000' ); -INSERT INTO ftable SELECT * FROM generate_series(1, 70000) i; -SELECT COUNT(*) FROM ftable; -TRUNCATE batch_table; -DROP FOREIGN TABLE ftable; - -- Disable batch insert CREATE FOREIGN TABLE ftable ( x int ) SERVER loopback OPTIONS ( table_name 'batch_table', batch_size '1' ); EXPLAIN (VERBOSE, COSTS OFF) INSERT INTO ftable VALUES (1), (2);