pg_stat_statements: use spaces to indent in upgrade scripts
Hi hackers, I just noticed that all indents in upgrade scripts of pg_stat_statemets are made using spaces, except for "CREATE FUNCTION pg_stat_statements_reset" statement in pg_stat_statements--1.6--1.7.sql and pg_stat_statements--1.10--1.11.sql. I made a patch to fix it in pg_stat_statements--1.10--1.11.sql if it's not too late for PostgreSQL 17 yet. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/ From 2273e72e35fe2d4ff500a2127413121b1709be17 Mon Sep 17 00:00:00 2001 From: Karina Litskevich Date: Mon, 23 Sep 2024 16:06:25 +0300 Subject: [PATCH v1] pg_stat_statements: use spaces to indent in upgrade scripts --- .../pg_stat_statements/pg_stat_statements--1.10--1.11.sql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.10--1.11.sql b/contrib/pg_stat_statements/pg_stat_statements--1.10--1.11.sql index 0bb2c39771..86769184a3 100644 --- a/contrib/pg_stat_statements/pg_stat_statements--1.10--1.11.sql +++ b/contrib/pg_stat_statements/pg_stat_statements--1.10--1.11.sql @@ -70,9 +70,9 @@ CREATE VIEW pg_stat_statements AS GRANT SELECT ON pg_stat_statements TO PUBLIC; CREATE FUNCTION pg_stat_statements_reset(IN userid Oid DEFAULT 0, - IN dbid Oid DEFAULT 0, - IN queryid bigint DEFAULT 0, - IN minmax_only boolean DEFAULT false +IN dbid Oid DEFAULT 0, +IN queryid bigint DEFAULT 0, +IN minmax_only boolean DEFAULT false ) RETURNS timestamp with time zone AS 'MODULE_PATHNAME', 'pg_stat_statements_reset_1_11' -- 2.34.1
Re: Invalid "trailing junk" error message when non-English letters are used
On Thu, Sep 5, 2024 at 6:07 PM Karina Litskevich
wrote:
> In v3 of the patch I grouped all the *_junk rules together and included
> the suggested comment with a little added something.
Oops, I forgot to attach the patch, here it is.
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From b993d45d34c0bff676bf439e75dfd8f58244f8ed Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Wed, 28 Aug 2024 11:52:25 +0300
Subject: [PATCH v3] Improve error message for rejecting trailing junk after
numeric literals
Rejecting trailing junk after numeric literals was introduced in
commit 2549f066 to prevent scanning a number immediately followed
by an identifier without whitespace as number and identifier.
Unfortunately, all the tokens made to catch such numeric literals
followed by non-digits match a numeric literal and the next byte. The
lexemes found by these tokens are broken in case the next symbol after
a numeric literal is presented by several bytes as only the first byte
of the symbol gets to the lexeme. When this lexeme is then printed as
a part of an error message that message became broken too along with
the whole log file where it goes.
This commit fixes the problem by using tokens that match a numeric
literal immediately followed by an identifier, not only one byte. This
also improves error messages in cases with English letters. For 123abc
the error message now will say that the error appeared at or near
"123abc" instead of "123a".
Since anything that matches hexinteger, octinteger or bininteger
followed by identifier also matches decinteger followed by identifier,
use one common rule for that.
---
src/backend/parser/scan.l| 43
src/fe_utils/psqlscan.l | 40 --
src/interfaces/ecpg/preproc/pgc.l| 40 --
src/test/regress/expected/numerology.out | 14 +---
src/test/regress/sql/numerology.sql | 1 +
5 files changed, 76 insertions(+), 62 deletions(-)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index f74059e7b0..1a7c9b9d8f 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -412,16 +412,29 @@ numericfail {decinteger}\.\.
real ({decinteger}|{numeric})[Ee][-+]?{decinteger}
realfail ({decinteger}|{numeric})[Ee][-+]
-decinteger_junk {decinteger}{ident_start}
-hexinteger_junk {hexinteger}{ident_start}
-octinteger_junk {octinteger}{ident_start}
-bininteger_junk {bininteger}{ident_start}
-numeric_junk {numeric}{ident_start}
-real_junk {real}{ident_start}
-
/* Positional parameters don't accept underscores. */
param \${decdigit}+
-param_junk \${decdigit}+{ident_start}
+
+/*
+ * An identifier immediately following a numeric literal is disallowed because
+ * in some cases it's ambiguous what is meant: for example, 0x1234 could be
+ * either a hexinteger or a decinteger "0" and an identifier "x1234". We can
+ * detect such problems by seeing if integer_junk matches a longer substring
+ * than any of the XXXinteger patterns (decinteger, hexinteger, octinteger,
+ * and bininteger). One "junk" pattern is sufficient because
+ * {decinteger}{identifier} will match all the same strings we'd match with
+ * {hexinteger}{identifier} etc.
+ *
+ * Note that the rule for integer_junk must appear after the ones for
+ * XXXinteger to make this work correctly.
+ *
+ * Also disallow strings matched by numeric_junk, real_junk and param_junk for
+ * consistency.
+ */
+integer_junk {decinteger}{identifier}
+numeric_junk {numeric}{identifier}
+real_junk {real}{identifier}
+param_junk \${decdigit}+{identifier}
other .
@@ -1055,19 +1068,7 @@ other .
SET_YYLLOC();
yyerror("trailing junk after numeric literal");
}
-{decinteger_junk} {
- SET_YYLLOC();
- yyerror("trailing junk after numeric literal");
-}
-{hexinteger_junk} {
- SET_YYLLOC();
- yyerror("trailing junk after numeric literal");
-}
-{octinteger_junk} {
- SET_YYLLOC();
- yyerror("trailing junk after numeric literal");
-}
-{bininteger_junk} {
+{integer_junk} {
SET_YYLLOC();
yyerror("trailing junk after numeric literal");
}
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index ddc4658b92..f1c24d505b 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -348,16 +348,29 @@ numericfail {decinteger}\.\.
real ({decinteger}|{numeric})[Ee][-+]?{decinteger}
realfail ({decinteger}|{numeric})[Ee][-+]
-decinteger_junk {decinteger}{ident_start}
-hexinteger_junk {hexinteger}{ident_start}
-octinteger_junk {octinteger}{ident_start}
-bininteger_junk {bininteger}{ident_start}
-numeric_junk {numeric}{ident_start}
-real_junk {real}{ident_start}
-
/* Positional parameters don't accept underscores. */
param \${decdigit}+
-param_junk \${decdig
Re: Invalid "trailing junk" error message when non-English letters are used
On Thu, Sep 5, 2024 at 1:52 AM Tom Lane wrote:
> Karina Litskevich writes:
> > I see the two solutions here: either move the rule for decinteger_junk
> > below the rules for hexinteger_junk, octinteger_junk and
bininteger_junk,
> > or just use a single rule decinteger_junk for all these cases, since the
> > error message is the same anyway. I implemented the latter in the second
> > version of the patch, also renamed this common rule to integer_junk.
>
> That seems reasonable, but IMO this code was unacceptably
> undercommented before and what you've done has made it worse.
> We really need a comment block associated with the flex macros,
> perhaps along the lines of
>
> /*
> * An identifier immediately following a numeric literal is disallowed
> * because in some cases it's ambiguous what is meant: for example,
> * 0x1234 could be either a hexinteger or a decinteger "0" and an
> * identifier "x1234". We can detect such problems by seeing if
> * integer_junk matches a longer substring than any of the XXXinteger
> * patterns. (One "junk" pattern is sufficient because this will match
> * all the same strings we'd match with {hexinteger}{identifier} etc.)
> * Note that the rule for integer_junk must appear after the ones for
> * XXXinteger to make this work correctly.
> */
Thank you, this piece of code definitely needs a comment.
> (Hmm, actually, is that last sentence true? My flex is a bit rusty.)
Yes, the rule for integer_junk must appear after the ones for XXXinteger.
Here is a quote from
https://ftp.gnu.org/old-gnu/Manuals/flex-2.5.4/html_mono/flex.html
"If it finds more than one match, it takes the one matching the most text
(...). If it finds two or more matches of the same length, the rule listed
first in the flex input file is chosen."
For example, 0x123 is matched by both integer_junk and hexinteger, and we
want the rule for hexinteger to be chosen, so we should place it before
the rule for integer_junk.
> param_junk really needs a similar comment, or maybe we could put
> all the XXX_junk macros together and use one comment for all.
In v3 of the patch I grouped all the *_junk rules together and included
the suggested comment with a little added something.
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
Re: Invalid "trailing junk" error message when non-English letters are used
On Wed, Aug 28, 2024 at 12:06 AM Pavel Borisov
wrote:
> I see the following compile time warnings:
> scan.l:1062: warning, rule cannot be matched
> scan.l:1066: warning, rule cannot be matched
> scan.l:1070: warning, rule cannot be matched
> pgc.l:1030: warning, rule cannot be matched
> pgc.l:1033: warning, rule cannot be matched
> pgc.l:1036: warning, rule cannot be matched
> psqlscan.l:905: warning, rule cannot be matched
> psqlscan.l:908: warning, rule cannot be matched
> psqlscan.l:911: warning, rule cannot be matched
>
Thanks for the feedback!
I somehow missed these warnings, my bad. The problem is with queries like
"select 0x12junk;". In master "0x" matches decinteger_junk token and
"0x12j" matches hexinteger_junk token and flex chooses the longest match,
no conflict. But with the patch "0x12junk" matches both decinteger_junk
(decinteger "0" + identifier "x12junk") and hexinteger_junk (hexinteger
"0x12" + identifier "junk"). Since any match to hexinteger_junk also
matches decinteger_junk, and the rule for hexinteger_junk is below the
rule for decinteger_junk, it's never reached.
I see the two solutions here: either move the rule for decinteger_junk
below the rules for hexinteger_junk, octinteger_junk and bininteger_junk,
or just use a single rule decinteger_junk for all these cases, since the
error message is the same anyway. I implemented the latter in the second
version of the patch, also renamed this common rule to integer_junk.
Additionally, I noticed that this patch is going to change error messages
in some cases, though I don't think it's a big deal. Example:
Without patch:
postgres=# select 0xyz;
ERROR: invalid hexadecimal integer at or near "0x"
With patch:
postgres=# select 0xyz;
ERROR: trailing junk after numeric literal at or near "0xyz"
> FWIW output of the whole string in the error message doesnt' look nice to
> me, but other places of code do this anyway e.g:
> select ('1'||repeat('p',100))::integer;
> This may be worth fixing.
>
That's interesting. I didn't know we could do that to create a long error
message. At first I thought that it's not a problem for error messages
from the scanner, since its "at or near" string cannot be longer than the
query typed in psql or written in a script file so it won't be enormously
big. But that's just not true, because we can send a generated query.
Something like that:
With patch:
postgres=# select 'select '||'1'||repeat('p',100) \gexec
ERROR: trailing junk after numeric literal at or near "1ppp"
And another query that leads to this without patch:
postgres=# select 'select 1'||repeat('@',100)||'1' \gexec
ERROR: operator too long at or near "@@@"
It would be nice to prevent such long strings in error messages. Maybe a
GUC variable to set the maximum length for such strings could work. But
how do we determine all places where it is needed?
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From e0d49c4e81054e5c66be1f78cb57ad2ae87fa02d Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Wed, 28 Aug 2024 11:52:25 +0300
Subject: [PATCH v2] Improve error message for rejecting trailing junk after
numeric literals
Rejecting trailing junk after numeric literals was introduced in
commit 2549f066 to prevent scanning a number immediately followed
by an identifier without whitespace as number and identifier.
Unfortunately, all the tokens made to catch such numeric literals
followed by non-digits match a numeric literal and the next byte. The
lexemes found by these tokens are broken in case the next symbol after
a numeric literal is presented by several bytes as only the first byte
of the symbol gets to the lexeme. When this lexeme is then printed as
a part of an error message that message became broken too along with
the whole log file where it goes.
This commit fixes the problem by using tokens that match a numeric
literal immediately followed by an identifier, not only one byte. This
also improves error messages in cases with English letters. For 123abc
the error message now will say that the error appeared at or near
"123abc" instead of "123a".
Since anything that matches hexinteger, octinteger or bininteger
followed by identifier also matches decinteger followed by identifier,
use one common token for that.
---
src/backend/parser/scan.l| 25 +---
src/fe_utils/psqlscan.l | 22 +
src/interfaces/ecpg/preproc/pgc.l| 22 +
src/test/regress/expected/numerology.out | 14 -
src/test/regress/sql/numerology.sql | 1 +
5 files changed, 25 insertions(+), 59 de
Invalid "trailing junk" error message when non-English letters are used
Hi hackers,
When error "trailing junk after numeric literal" occurs at a number
followed by a symbol that is presented by more than one byte, that symbol
in the error message is not displayed correctly. Instead of that symbol
there is only its first byte. That makes the error message an invalid
UTF-8 (or whatever encoding is set). The whole log file where this error
message goes also becomes invalid. That could lead to problems with
reading logs. You can see an invalid message by trying "SELECT 123ä;".
Rejecting trailing junk after numeric literals was introduced in commit
2549f066 to prevent scanning a number immediately followed by an
identifier without whitespace as number and identifier. All the tokens
that made to catch such cases match a numeric literal and the next byte,
and that is where the problem comes from. I thought that it could be fixed
just by using tokens that match a numeric literal immediately followed by
an identifier, not only one byte. This also improves error messages in
cases with English letters. After these changes, for "SELECT 123abc;" the
error message will say that the error appeared at or near "123abc" instead
of "123a".
I've attached the patch. Are there any pitfalls I can't see? It just keeps
bothering me why wasn't it done from the beginning. Matching the whole
identifier after a numeric literal just seems more obvious to me than
matching its first byte.
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From 281303312471b4ef831f57d124208ce1699aed54 Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Tue, 27 Aug 2024 17:01:49 +0300
Subject: [PATCH v1] Improve error message for rejecting trailing junk after
numeric literals
Rejecting trailing junk after numeric literals was introduced in
commit 2549f066 to prevent scanning a number immediately followed
by an identifier without whitespace as number and identifier.
Unfortunately, all the tokens made to catch such numeric literals
followed by non-digits match a numeric literal and the next byte. The
lexemes found by these tokens are broken in case the next symbol after
a numeric literal is presented by several bytes as only the first byte
of the symbol gets to the lexeme. When this lexeme is then printed as
a part of an error message that message became broken too along with
the whole log file where it goes.
This commit fixes the problem by using tokens that match a numeric
literal immediately followed by an identifier, not only one byte. This
also improves error messages in cases with English letters. For 123abc
the error message now will say that the error appeared at or near
"123abc" instead of "123a".
---
src/backend/parser/scan.l| 14 +++---
src/fe_utils/psqlscan.l | 14 +++---
src/interfaces/ecpg/preproc/pgc.l| 14 +++---
src/test/regress/expected/numerology.out | 14 +-
src/test/regress/sql/numerology.sql | 1 +
5 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index f74059e7b0..448f9b5afe 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -412,16 +412,16 @@ numericfail {decinteger}\.\.
real ({decinteger}|{numeric})[Ee][-+]?{decinteger}
realfail ({decinteger}|{numeric})[Ee][-+]
-decinteger_junk {decinteger}{ident_start}
-hexinteger_junk {hexinteger}{ident_start}
-octinteger_junk {octinteger}{ident_start}
-bininteger_junk {bininteger}{ident_start}
-numeric_junk {numeric}{ident_start}
-real_junk {real}{ident_start}
+decinteger_junk {decinteger}{identifier}
+hexinteger_junk {hexinteger}{identifier}
+octinteger_junk {octinteger}{identifier}
+bininteger_junk {bininteger}{identifier}
+numeric_junk {numeric}{identifier}
+real_junk {real}{identifier}
/* Positional parameters don't accept underscores. */
param \${decdigit}+
-param_junk \${decdigit}+{ident_start}
+param_junk \${decdigit}+{identifier}
other .
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index ddc4658b92..282f117b81 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -348,16 +348,16 @@ numericfail {decinteger}\.\.
real ({decinteger}|{numeric})[Ee][-+]?{decinteger}
realfail ({decinteger}|{numeric})[Ee][-+]
-decinteger_junk {decinteger}{ident_start}
-hexinteger_junk {hexinteger}{ident_start}
-octinteger_junk {octinteger}{ident_start}
-bininteger_junk {bininteger}{ident_start}
-numeric_junk {numeric}{ident_start}
-real_junk {real}{ident_start}
+decinteger_junk {decinteger}{identifier}
+hexinteger_junk {hexinteger}{identifier}
+octinteger_junk {octinteger}{identifier}
+bininteger_junk {bininteger}{identifier}
+numeric_junk {numeric}{identifier}
+real_junk {real}{identifier}
/* Positional parameters don't accept underscores. */
param \${decdigit}+
-param_junk \${decdigit}+{ident_start}
+
Re: Use pgBufferUsage for block reporting in analyze
I wrote: > > I suggest assigning values > bufferusage.shared_blks_read + bufferusage.local_blks_read > and > bufferusage.shared_blks_dirtied + bufferusage.local_blks_dirtied > to new variables and using them. This would keep the changed lines within > the 80 symbols limit, and make the code more readable overall. > The same applies to bufferusage.shared_blks_hit + bufferusage.local_blks_hit
Re: Use pgBufferUsage for block reporting in analyze
Hi Anthonin, I suggest assigning values bufferusage.shared_blks_read + bufferusage.local_blks_read and bufferusage.shared_blks_dirtied + bufferusage.local_blks_dirtied to new variables and using them. This would keep the changed lines within the 80 symbols limit, and make the code more readable overall. I also believe that changing "misses" to "reads" should belong to 0001 patch since we only change it because we replace AnalyzePageMiss with bufferusage.shared_blks_read + bufferusage.local_blks_read in 0001. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/ >
Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
Hi, hackers! On Thu, Apr 25, 2024 at 4:00 PM Pavel Borisov wrote: > 0005: Rename checkunique parameter to more user friendly as proposed by > Peter Eisentraut and Alexander Korotkov > I'm not sure renaming checkunique is a good idea. Other arguments of bt_index_check and bt_index_parent_check functions (heapallindexed and rootdescend) don't have underscore character in them. Corresponding pg_amcheck options (--heapallindexed and --rootdescend) are also written in one piece. check_unique and --check-unique stand out. Making arguments and options in different styles doesn't seem user friendly to me. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
Correct the documentation of extension building infrastructure
Hi hackers, I noticed that the documentation of extension building infrastructure [1] has a paragraph about isolation tests since v12, but they actually can be run since v14 only. Actual isolation tests support for extensions built with PGXS was added in [2] and it was agreed there that it should not be backpatched. How about fixing the documentation for v12 and v13 then? [1]: https://www.postgresql.org/docs/12/extend-pgxs.html [2]: https://www.postgresql.org/message-id/flat/CAMsr%2BYFsCMH3B4uOPFE%2B2qWM6k%3Do%3Dhf9LGiPNCfwqKdUPz_BsQ%40mail.gmail.com Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
Re: BufferUsage counters' values have changed
Nazir, Andres, thank you both for help! On Wed, Sep 13, 2023 at 10:10 PM Andres Freund wrote: > On 2023-09-13 16:04:00 +0300, Nazir Bilal Yavuz wrote: > > local_blks_read became zero because: > > 1_ One more cache hit. It was supposed to be local_blks_read but it is > > local_blks_hit now. This is an assumption, I didn't check this deeply. > > 2_ Before fcdda1e4b5, there was one local_blks_read coming from > > buf = ReadBufferExtended(rel, VISIBILITYMAP_FORKNUM, blkno, > > RBM_ZERO_ON_ERROR, NULL) in freespace.c -> ReadBuffer_common() -> > > pgBufferUsage.local_blks_read++. > > But buf = ReadBufferExtended(rel, VISIBILITYMAP_FORKNUM, blkno, > > RBM_ZERO_ON_ERROR, NULL) is moved into the else case, so it didn't > > called and local_blks_read isn't incremented. > > That imo is a legitimate difference / improvement. The read we previously > did > here was unnecessary. > > > > local_blks_written is greater because of the combination of fcdda1e4b5 > > and 00d1e02be2. > > In PG_15: > > RelationGetBufferForTuple() -> ReadBufferBI(P_NEW, RBM_ZERO_AND_LOCK) > > -> ReadBufferExtended() -> ReadBuffer_common() -> > > pgBufferUsage.local_blks_written++; (called 5 times) [0] > > In PG_16: > > 1_ 5 of the local_blks_written is coming from: > > RelationGetBufferForTuple() -> RelationAddBlocks() -> > > ExtendBufferedRelBy() -> ExtendBufferedRelCommon() -> > > ExtendBufferedRelLocal() -> pgBufferUsage.local_blks_written += > > extend_by; (extend_by is 1, this is called 5 times) [1] > > 2_ 3 of the local_blks_written is coming from: > > RelationGetBufferForTuple() -> RecordAndGetPageWithFreeSpace() -> > > fsm_set_and_search() -> fsm_readbuf() -> fsm_extend() -> > > ExtendBufferedRelTo() -> ExtendBufferedRelCommon() -> > > ExtendBufferedRelLocal() -> pgBufferUsage.local_blks_written += > > extend_by; (extend_by is 3, this is called 1 time) [2] > > > > I think [0] is the same path as [1] but [2] is new. 'fsm extends' > > wasn't counted in local_blks_written in PG_15. Calling > > ExtendBufferedRelTo() from fsm_extend() caused 'fsm extends' to be > > counted in local_blks_written. I am not sure which one is correct. > > I think it's correct to count the fsm writes here. The pg_stat_statement > columns aren't specific to the main relation for or such... If anything it > was > a bug to not count them before. > Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
Re: BufferUsage counters' values have changed
On Mon, Sep 11, 2023 at 9:08 AM Karina Litskevich < litskevichkar...@gmail.com> wrote: > I found a bug that causes one of the differences. Wrong counter is > incremented > in ExtendBufferedRelLocal(). The attached patch fixes it and should be > applied > to REL_16_STABLE and master. > I've forgotten to attach the patch. Here it is. From 999a3d533a9b74c8568cc8a3d715c287de45dd2c Mon Sep 17 00:00:00 2001 From: Karina Litskevich Date: Thu, 7 Sep 2023 17:44:40 +0300 Subject: [PATCH v1] Fix local_blks_written counter incrementation --- src/backend/storage/buffer/localbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index 1735ec7141..567b8d15ef 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -431,7 +431,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr, *extended_by = extend_by; - pgBufferUsage.temp_blks_written += extend_by; + pgBufferUsage.local_blks_written += extend_by; return first_block; } -- 2.34.1
BufferUsage counters' values have changed
Hi hackers, I noticed that BufferUsage counters' values are strangely different for the same queries on REL_15_STABLE and REL_16_STABLE. For example, I run CREATE EXTENSION pg_stat_statements; CREATE TEMP TABLE test(b int); INSERT INTO test(b) SELECT generate_series(1,1000); SELECT query, local_blks_hit, local_blks_read, local_blks_written, local_blks_dirtied, temp_blks_written FROM pg_stat_statements; and output on REL_15_STABLE contains query | INSERT INTO test(b) SELECT generate_series($1,$2) local_blks_hit | 1005 local_blks_read| 2 local_blks_written | 5 local_blks_dirtied | 5 temp_blks_written | 0 while output on REL_16_STABLE contains query | INSERT INTO test(b) SELECT generate_series($1,$2) local_blks_hit | 1006 local_blks_read| 0 local_blks_written | 0 local_blks_dirtied | 5 temp_blks_written | 8 I found a bug that causes one of the differences. Wrong counter is incremented in ExtendBufferedRelLocal(). The attached patch fixes it and should be applied to REL_16_STABLE and master. With the patch applied output contains query | INSERT INTO test(b) SELECT generate_series($1,$2) local_blks_hit | 1006 local_blks_read| 0 local_blks_written | 8 local_blks_dirtied | 5 temp_blks_written | 0 I still wonder why local_blks_written is greater than it was on REL_15_STABLE, and why local_blks_read became zero. These changes are caused by fcdda1e4b5. This code is new to me, and I'm still trying to understand whether it's a bug in computing the counters or just changes in how many blocks are read/written during the query execution. If anyone can help me, I would be grateful. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
Re: Avoid unused value (src/fe_utils/print.c)
On Tue, Jul 25, 2023 at 1:04 AM Ranier Vilela wrote: > Checked today, Coverity does not complain about need_recordsep. > Great! Thanks. So v2 patch makes Coverity happy, and as for me doesn't make the code less readable. Does anyone have any objections? Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
Re: Avoid unused value (src/fe_utils/print.c)
On Wed, Jul 12, 2023 at 1:46 AM Ranier Vilela wrote:
> As there is consensus to keep the no-op assignment,
> I will go ahead and reject the patch.
>
In your patch you suggest removing two assignments, and we only have
consensus to keep one of them. The other one is an obvious no-op.
I attached a patch that removes only one assignment. Could you please try
it and check whether Coverity is still complaining about need_recordsep
variable?
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From 92c86657181c13c48a4b4176bf4ad1d6221cf9b5 Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Fri, 21 Jul 2023 14:43:44 +0300
Subject: [PATCH v2] Avoid unused value in print.c
---
src/fe_utils/print.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 7af1ccb6b5..d10f33cd9f 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -484,10 +484,7 @@ print_unaligned_text(const printTableContent *cont, FILE *fout)
for (f = footers; f; f = f->next)
{
if (need_recordsep)
-{
print_separator(cont->opt->recordSep, fout);
- need_recordsep = false;
-}
fputs(f->data, fout);
need_recordsep = true;
}
--
2.34.1
Re: Avoid unused value (src/fe_utils/print.c)
> > > >> But >> it's not obvious, so I also have doubts about removing this line. If >> someday >> print options are changed, for example to support printing footers and not >> printing headers, or anything else changes in this function, the output >> might >> be unexpected with this line removed. > > > That part I didn't understand. > How are we going to make this function less readable by removing the > complicating part. > My point is, technically right now you won't see any difference in output if you remove the line. Because if we get to that line the need_recordsep is already true. However, understanding why it is true is complicated. That's why if you remove the line people who read the code will wonder why we don't need a separator after "fputs"ing a footer. So keeping that line will make the code more readable. Moreover, removing the line will possibly complicate the future maintenance. As I wrote in the part you just quoted, if the function changes in the way that need_recordsep is not true right before printing footers any more, then output will be unexpected. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
MarkGUCPrefixReserved() doesn't check all options
Hi hackers, Ekaterina Sokolova and I have found a bug in PG 15. Since 88103567cb MarkGUCPrefixReserved() is supposed not only reporting GUCs that haven't been defined by the extension, but also removing them. However, it removes them in a wrong way, so that a GUC that goes after the removed GUC is never checked. To reproduce the bug add the following to the postgresql.conf shared_preload_libraries = 'pg_stat_statements' pg_stat_statements.nonexisting_option_1 = on pg_stat_statements.nonexisting_option_2 = on pg_stat_statements.nonexisting_option_3 = on pg_stat_statements.nonexisting_option_4 = on and start the server. In the logfile you'll see only first and third options reported invalid and removed. In master MarkGUCPrefixReserved() iterates a hash table, not an array as in PG 15. I'm not sure whether it is safe to remove an entry from this hash table while iterating it, but at least I can't reproduce the bug on master. I attached a bugfix for PG 15. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/ From 5f0e59b225ebb408a0c971bc78e22050b296ae9a Mon Sep 17 00:00:00 2001 From: Karina Litskevich Date: Thu, 6 Jul 2023 11:00:36 +0300 Subject: [PATCH v1] Fix MarkGUCPrefixReserved() to check all options --- src/backend/utils/misc/guc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 915f557c68..c410ba532d 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -9723,6 +9723,7 @@ MarkGUCPrefixReserved(const char *className) num_guc_variables--; memmove(&guc_variables[i], &guc_variables[i + 1], (num_guc_variables - i) * sizeof(struct config_generic *)); + i--; } } -- 2.25.1
Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)
Hi,
Good catch. Indeed, eb.rel shouldn't be NULL there and the tests should be
unnecessary. However, it doesn't follow from the code of these functions.
>From what I can see eb.rel can be NULL in both of these functions. There is
the following Assert in the beginning of the ExtendBufferedRelTo() function:
Assert((eb.rel != NULL) != (eb.smgr != NULL));
And ExtendBufferedRelShared() is only called from ExtendBufferedRelCommon()
which can be called from ExtendBufferedRelTo() or ExtendBufferedRelBy() that
also has the same Assert(). And none of these functions assigns eb.rel, so
it can be NULL from the very beginning and it stays the same.
And there is the following call in xlogutils.c, which is exactly the case
when
eb.rel is NULL:
buffer = ExtendBufferedRelTo(EB_SMGR(smgr, RELPERSISTENCE_PERMANENT),
forknum,
NULL,
EB_PERFORMING_RECOVERY |
EB_SKIP_EXTENSION_LOCK,
blkno + 1,
mode);
So as for me calling LockRelationForExtension() and
UnlockRelationForExtension()
without testing eb.rel first looks more like a bug here. However, they are
never
actually called with eb.rel=NULL because of the EB_* flags, so there is no
bug
here. I believe we should add Assert checking that when eb.rel is NULL,
flags
are such that we won't use eb.rel. And yes, we can remove unnecessary checks
where the flags value guaranty us that eb.rel is not NULL.
And another minor observation. It seems to me that we don't need a "could
have
been closed while waiting for lock" in ExtendBufferedRelShared(), because I
believe the comment above already explains why updating eb.smgr:
* Note that another backend might have extended the relation by the time
* we get the lock.
I attached the new version of the patch as I see it.
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From 7425d5b1b9d30f0da75aabd94e862906f9635a60 Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Fri, 30 Jun 2023 21:26:18 +0300
Subject: [PATCH v4] Remove always true checks
Also add asserts that help understanding these checks are always true
---
src/backend/storage/buffer/bufmgr.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 3c59bbd04e..7b1e2e1284 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -851,6 +851,8 @@ ExtendBufferedRelBy(ExtendBufferedWhat eb,
{
Assert((eb.rel != NULL) != (eb.smgr != NULL));
Assert(eb.smgr == NULL || eb.relpersistence != 0);
+ Assert(eb.rel != NULL || (flags & EB_SKIP_EXTENSION_LOCK) &&
+ !(flags & EB_CREATE_FORK_IF_NEEDED));
Assert(extend_by > 0);
if (eb.smgr == NULL)
@@ -887,6 +889,8 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb,
Assert((eb.rel != NULL) != (eb.smgr != NULL));
Assert(eb.smgr == NULL || eb.relpersistence != 0);
+ Assert(eb.rel != NULL || (flags & EB_SKIP_EXTENSION_LOCK) &&
+ !(flags & EB_CREATE_FORK_IF_NEEDED));
Assert(extend_to != InvalidBlockNumber && extend_to > 0);
if (eb.smgr == NULL)
@@ -908,8 +912,7 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb,
LockRelationForExtension(eb.rel, ExclusiveLock);
/* could have been closed while waiting for lock */
- if (eb.rel)
- eb.smgr = RelationGetSmgr(eb.rel);
+ eb.smgr = RelationGetSmgr(eb.rel);
/* recheck, fork might have been created concurrently */
if (!smgrexists(eb.smgr, fork))
@@ -1875,8 +1878,7 @@ ExtendBufferedRelShared(ExtendBufferedWhat eb,
if (!(flags & EB_SKIP_EXTENSION_LOCK))
{
LockRelationForExtension(eb.rel, ExclusiveLock);
- if (eb.rel)
- eb.smgr = RelationGetSmgr(eb.rel);
+ eb.smgr = RelationGetSmgr(eb.rel);
}
/*
--
2.25.1
Re: Avoid unused value (src/fe_utils/print.c)
Hi, Alexander wrote: > It also aligns the code with print_unaligned_vertical(), but I can't see why > need_recordsep = true; > is a no-op here (scan-build dislikes only need_recordsep = false;). > I suspect that removing that line will change the behaviour in cases when > need_recordsep = false after the loop "print cells" and the loop > "for (footers)" is executed. As I understand cont->cells is supoused to have all cont->ncolumns * cont->nrows entries filled so the loop "print cells" always assigns need_recordsep = true, except when there are no cells at all or cancel_pressed == true. If cancel_pressed == true then footers are not printed. So to have need_recordsep == false before the loop "for (footers)" table should be empty, and need_recordsep should be false before the loop "print cells". It can only be false there when cont->opt->start_table == true and opt_tuples_only == true so that headers are not printed. But when opt_tuples_only == true footers are not printed either. So technically removing "need_recordsep = true;" won't change the outcome. But it's not obvious, so I also have doubts about removing this line. If someday print options are changed, for example to support printing footers and not printing headers, or anything else changes in this function, the output might be unexpected with this line removed. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
Re: Possible false valgrind error reports
Thank you, I moved comment changes to 0001 and rewrote the fix through Min(). > The first hunk in 0001 doesn't seem quite right yet: > > * old allocation. > */ > #ifdef USE_VALGRIND > -if (oldsize > chunk->requested_size) > +if (size > chunk->requested_size && oldsize > chunk->requested_size) > VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + > chunk->requested_size, > oldsize - chunk->requested_size); > #endif > > If size < oldsize, aren't we still doing the wrong thing? Seems like > maybe it has to be like If size > chunk->requested_size than chksize >= oldsize and so we can mark this memory without worries. Region from size to chksize will be marked NOACCESS later anyway: /* Ensure any padding bytes are marked NOACCESS. */ VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size); I agree that it's not obvious, so I changed the first hunk like this: - if (oldsize > chunk->requested_size) + if (Min(size, oldsize) > chunk->requested_size) VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size, - oldsize - chunk->requested_size); + Min(size, oldsize) - chunk->requested_size); Any ideas on how to make this place easier to understand and comment above it concise and clear are welcome. There is another thing about this version. New line + Min(size, oldsize) - chunk->requested_size); is longer than 80 symbols and I don't know what's the best way to avoid this without making it look weird. I also noticed that if RANDOMIZE_ALLOCATED_MEMORY is defined then randomize_mem() have already marked this memory UNDEFINED. So we only "may need to adjust trailing bytes" if RANDOMIZE_ALLOCATED_MEMORY isn't defined. I reflected it in v2 of 0001 too. From 009ef9bbabcd71e0d5f2b60799c0b71d21fb9767 Mon Sep 17 00:00:00 2001 From: Karina Litskevich Date: Fri, 17 Feb 2023 15:32:05 +0300 Subject: [PATCH v2 2/2] Change variable name in AllocSetRealloc() --- src/backend/utils/mmgr/aset.c | 32 +--- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c index ace4907ce9..9584d50b14 100644 --- a/src/backend/utils/mmgr/aset.c +++ b/src/backend/utils/mmgr/aset.c @@ -1112,7 +1112,7 @@ AllocSetRealloc(void *pointer, Size size) AllocBlock block; AllocSet set; MemoryChunk *chunk = PointerGetMemoryChunk(pointer); - Size oldsize; + Size oldchksize; int fidx; /* Allow access to private part of chunk header. */ @@ -1140,11 +1140,11 @@ AllocSetRealloc(void *pointer, Size size) set = block->aset; - oldsize = block->endptr - (char *) pointer; + oldchksize = block->endptr - (char *) pointer; #ifdef MEMORY_CONTEXT_CHECKING /* Test for someone scribbling on unused space in chunk */ - Assert(chunk->requested_size < oldsize); + Assert(chunk->requested_size < oldchksize); if (!sentinel_ok(pointer, chunk->requested_size)) elog(WARNING, "detected write past chunk end in %s %p", set->header.name, chunk); @@ -1197,15 +1197,15 @@ AllocSetRealloc(void *pointer, Size size) #else /* * If this is an increase, realloc() will have left any newly-allocated - * part (from oldsize to chksize) UNDEFINED, but we may need to adjust + * part (from oldchksize to chksize) UNDEFINED, but we may need to adjust * trailing bytes from the old allocation (from chunk->requested_size to - * oldsize) as they are marked NOACCESS. Make sure not to mark extra - * bytes in case chunk->requested_size < size < oldsize. + * oldchksize) as they are marked NOACCESS. Make sure not to mark extra + * bytes in case chunk->requested_size < size < oldchksize. */ #ifdef USE_VALGRIND - if (Min(size, oldsize) > chunk->requested_size) + if (Min(size, oldchksize) > chunk->requested_size) VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size, - Min(size, oldsize) - chunk->requested_size); + Min(size, oldchksize) - chunk->requested_size); #endif #endif @@ -1223,7 +1223,7 @@ AllocSetRealloc(void *pointer, Size size) * portion DEFINED. Make sure not to mark memory behind the new * allocation in case it's smaller than old one. */ - VALGRIND_MAKE_MEM_DEFINED(pointer, Min(size, oldsize)); + VALGRIND_MAKE_MEM_DEFINED(pointer, Min(size, oldchksize)); #endif /* Ensure any padding bytes are marked NOACCESS. */ @@ -1248,11 +1248,11 @@ AllocSetRealloc(void *pointer, Size size) fidx = MemoryChunkGetValue(chunk); Assert(FreeListIdxIsValid(fidx)); - oldsize = GetChunkSizeFromFreeListIdx(fidx); + oldchksize = GetChunkSizeFromFreeListIdx(fidx); #ifdef MEMORY_CONTEXT_CHECKING /* Test for someone scribbling on unused
Possible false valgrind error reports
Hi hackers,
In 82d0a46ea32 AllocSetRealloc() was changed to allow decreasing size of
external chunks and give memory back to the malloc pool. Two
VALGRIND_MAKE_MEM_UNDEFINED() calls were not changed to work properly in the
case of decreasing size: they can mark memory behind the new allocated
memory
UNDEFINED. If this memory was already allocated and initialized, it's
expected
to be DEFINED. So it can cause false valgrind error reports. I fixed it in
0001
patch.
Also, it took me a while to understand what's going on there, so in 0002
patch
I tried to improve comments and renamed a variable. Its name "oldsize"
confused
me. I first thought "oldsize" and "size" represent the same parameters of
the
old and new chunk. But actually "size" is new "chunk->requested_size" and
"oldsize" is old "chksize". So I believe it's better to rename "oldsize"
into
"oldchksize".
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From 9556b7918e6abccb2dc19f20dbf572d41cd35cb4 Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Tue, 14 Feb 2023 17:13:17 +0300
Subject: [PATCH v1 1/2] Fix VALGRIND_MAKE_MEM_DEFINED() calls
---
src/backend/utils/mmgr/aset.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 740729b5d0..1ff0bb83cb 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1199,7 +1199,7 @@ AllocSetRealloc(void *pointer, Size size)
* old allocation.
*/
#ifdef USE_VALGRIND
- if (oldsize > chunk->requested_size)
+ if (size > chunk->requested_size && oldsize > chunk->requested_size)
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
oldsize - chunk->requested_size);
#endif
@@ -1215,7 +1215,10 @@ AllocSetRealloc(void *pointer, Size size)
* allocation; it could have been as small as one byte. We have to be
* conservative and just mark the entire old portion DEFINED.
*/
- VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ if (size >= oldsize)
+ VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ else
+ VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif
/* Ensure any padding bytes are marked NOACCESS. */
--
2.25.1
From 7cdaee9caaf96564237e4cfa8279699a36942624 Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Tue, 14 Feb 2023 17:18:30 +0300
Subject: [PATCH v1 2/2] Better comments and variable name in AllocSetRealloc()
---
src/backend/utils/mmgr/aset.c | 46 ---
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 1ff0bb83cb..35007750a9 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1112,7 +1112,7 @@ AllocSetRealloc(void *pointer, Size size)
AllocBlock block;
AllocSet set;
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
- Size oldsize;
+ Size oldchksize;
int fidx;
/* Allow access to private part of chunk header. */
@@ -1140,11 +1140,11 @@ AllocSetRealloc(void *pointer, Size size)
set = block->aset;
- oldsize = block->endptr - (char *) pointer;
+ oldchksize = block->endptr - (char *) pointer;
#ifdef MEMORY_CONTEXT_CHECKING
/* Test for someone scribbling on unused space in chunk */
- Assert(chunk->requested_size < oldsize);
+ Assert(chunk->requested_size < oldchksize);
if (!sentinel_ok(pointer, chunk->requested_size))
elog(WARNING, "detected write past chunk end in %s %p",
set->header.name, chunk);
@@ -1194,14 +1194,17 @@ AllocSetRealloc(void *pointer, Size size)
#endif
/*
- * realloc() (or randomize_mem()) will have left any newly-allocated
- * part UNDEFINED, but we may need to adjust trailing bytes from the
- * old allocation.
+ * If this is an increase, realloc() (or randomize_mem()) will have left
+ * any newly-allocated part UNDEFINED, but we may need to adjust
+ * trailing bytes from the old allocation as they are marked NOACCESS.
*/
#ifdef USE_VALGRIND
- if (size > chunk->requested_size && oldsize > chunk->requested_size)
+ if (size > chunk->requested_size && oldchksize > chunk->requested_size)
+ {
+ Assert(chksize >= oldchksize);
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
- oldsize - chunk->requested_size);
+ oldchksize - chunk->requested_size);
+ }
#endif
chunk->requested_size = size;
@@ -1211,12 +1214,15 @@ AllocSetRealloc(void *pointer, Size size)
#else /* !MEMORY_CONTEXT_CHECKING */
/*
- * We don't know how much of the old chunk size was the actual
- * allocation; it could have been as small as one byte. We have to be
- * conservative and just mark the entire ol
Re: Error for WITH options on partitioned tables
Hi David, > I am not very clear about why `build_reloptions` is removed in patch > `v2-0002-better-error-message-for-setting-parameters-for-p.patch`, if > you can help explain would be great. "build_reloptions" parses "reloptions" and takes for it a list of allowed options defined by the 5th argument "relopt_elems" and the 6th argument "num_relopt_elems", which are NULL and 0 in the removed call. If "validate" is false, it ignores options, which are not in the list, while parsing. If "validate" is true, it "elog"s ERROR when it meets option, which is not in the allowed list. As in the deleted call "build_reloptions" is called with an empty list of allowed options, it does nothing (returns NULL) when "validate" is false, and "elog"s ERROR when "validate" is true and "reloptions" is not empty. That is what the deleted comment above the deleted call about. This call is there only for validation. So as I wanted to make a specific error message for the case of partitioned tables, I added the validation in "partitioned_table_reloptions" and saw no reason to call "build_reloptions" any more because it would just return NULL in other cases. > This generic error message is reported by > `errdetail_relkind_not_supported`, and there is a similar error message > for partitioned tables. Anyone knows if this can be an option for > reporting this `fillfactor` parameter not supported error. This error is reported by "ATSimplePermissions" and, as we can see in the beginning of this function, it makes no difference between regular relations and partitioned tables now. To make it report errors for partitioned tables we should add new "alter table target-type flag" and add it to a mask of each "AlterTableType" if partitioned table is an allowed target for it (see that huge switch-case in function "ATPrepCmd"). Then "partitioned_table_reloptions" may become useless and we also should check weather some other functions become useless. Maybe that is the right way, but it looks much harder than the existing solutions, so I believe, before anyone began going this way, it's better to know whether there are any pitfalls there. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/
Re: Error for WITH options on partitioned tables
Hi, Simon!
The new error message looks better. But I believe it is better to use
"parameters" instead of "options" as it is called "storage parameters"
in the documentation. I also believe it is better to report error in
partitioned_table_reloptions() (thanks to Japin Li for mentioning this
function) as it also fixes the error message in the following situation:
test=# CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST
(a);
CREATE TABLE
test=# ALTER TABLE parted_col_comment SET (fillfactor=100);
ERROR: unrecognized parameter "fillfactor"
I attached the new versions of patches.
I'm not sure about the errcode. May be it is better to report error with
ERRCODE_INVALID_OBJECT_DEFINITION for CREATE TABLE and with
ERRCODE_WRONG_OBJECT_TYPE for ALTER TABLE (as when you try "ALTER TABLE
partitioned INHERIT nonpartitioned;" an ERROR with ERRCODE_WRONG_OBJECT_TYPE
is reported). Then either the desired code should be passed to
partitioned_table_reloptions() or similar checks and ereport calls should be
placed in two different places. I'm not sure whether it is a good idea to
change the code in one of these ways just to change the error code.
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From 6407538f0bfd3eb56f5a4574d8893f9494f2a810 Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Fri, 14 Oct 2022 17:48:27 +0300
Subject: [PATCH v2 2/2] better error message for setting parameters for
partitioned table
---
src/backend/access/common/reloptions.c | 13 ++---
src/test/regress/expected/alter_table.out | 3 ++-
src/test/regress/expected/create_table.out | 3 ++-
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 6458a9c276..75d6ff5040 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1984,13 +1984,12 @@ build_local_reloptions(local_relopts *relopts, Datum options, bool validate)
bytea *
partitioned_table_reloptions(Datum reloptions, bool validate)
{
- /*
- * There are no options for partitioned tables yet, but this is able to do
- * some validation.
- */
- return (bytea *) build_reloptions(reloptions, validate,
- RELOPT_KIND_PARTITIONED,
- 0, NULL, 0);
+ if (validate && reloptions)
+ ereport(ERROR,
+errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("cannot specify storage parameters for a partitioned table"),
+errhint("specify storage parameters on leaf partitions instead"));
+ return NULL;
}
/*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index cec7bfa1f1..a719ef22c7 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3803,7 +3803,8 @@ ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
ERROR: cannot alter column "b" because it is part of the partition key of relation "partitioned"
-- specifying storage parameters for partitioned tables is not supported
ALTER TABLE partitioned SET (fillfactor=100);
-ERROR: unrecognized parameter "fillfactor"
+ERROR: cannot specify storage parameters for a partitioned table
+HINT: specify storage parameters on leaf partitions instead
-- partitioned table cannot participate in regular inheritance
CREATE TABLE nonpartitioned (
a int,
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 45c1a0a23e..f16be87729 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -989,7 +989,8 @@ Number of partitions: 0
DROP TABLE parted_col_comment;
-- specifying storage parameters for partitioned tables is not supported
CREATE TABLE parted_col_comment (a int, b text) PARTITION BY LIST (a) WITH (fillfactor=100);
-ERROR: unrecognized parameter "fillfactor"
+ERROR: cannot specify storage parameters for a partitioned table
+HINT: specify storage parameters on leaf partitions instead
-- list partitioning on array type column
CREATE TABLE arrlp (a int[]) PARTITION BY LIST (a);
CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN ('{1}', '{2}');
--
2.25.1
From 580566815bc5abd6193f86ddbd55fac1ac941873 Mon Sep 17 00:00:00 2001
From: Karina Litskevich
Date: Fri, 14 Oct 2022 16:22:57 +0300
Subject: [PATCH v2 1/2] test parameters for partitioned table
---
src/test/regress/expected/alter_table.out | 3 +++
src/test/regress/expected/create_table.out | 3 +++
src/test/regress/sql/alter_table.sql | 3 +++
src/test/regress/sql/create_table.sql | 3 +++
4 files changed, 12 insertions(+)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 346f594ad0..cec7bfa1f1 100644
--- a/src/test/regress/expected/alter_t
Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
Hi,
I also would like to suggest a cosmetic change.
In v15 a new field checkunique is added after heapallindexed and before
no_btree_expansion fields in struct definition, but in initialisation it is
added after no_btree_expansion:
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -102,6 +102,7 @@ typedef struct AmcheckOptions
bool parent_check;
bool rootdescend;
bool heapallindexed;
+ bool checkunique;
/* heap and btree hybrid option */
bool no_btree_expansion;
@@ -132,7 +133,8 @@ static AmcheckOptions opts = {
.parent_check = false,
.rootdescend = false,
.heapallindexed = false,
- .no_btree_expansion = false
+ .no_btree_expansion = false,
+ .checkunique = false
};
I suggest to add checkunique field between heapallindexed and
no_btree_expansion fields in initialisation as well as in definition:
@@ -132,6 +133,7 @@ static AmcheckOptions opts = {
.parent_check = false,
.rootdescend = false,
.heapallindexed = false,
+ .checkunique = false,
.no_btree_expansion = false
};
--
Best regards,
Litskevich Karina
Postgres Professional: http://postgrespro.com/
From 56fe2b608b46c6c97900bbb63b2169e2997bc8cc Mon Sep 17 00:00:00 2001
From: Pavel Borisov
Date: Wed, 11 May 2022 15:54:13 +0400
Subject: [PATCH v16] Add option for amcheck and pg_amcheck to check unique
constraint for btree indexes.
Add 'checkunique' argument to bt_index_check() and bt_index_parent_check().
When the flag is specified the procedures will check the unique constraint
violation for unique indexes. Only one heap entry for all equal keys in
the index should be visible (including posting list entries). Report an error
otherwise.
pg_amcheck called with --checkunique option will do the same check for all
the indexes it checks.
Author: Anastasia Lubennikova
Author: Pavel Borisov
Author: Maxim Orlov
Reviewed-by: Mark Dilger
Reviewed-by: Zhihong Yu
Reviewed-by: Peter Geoghegan
Reviewed-by: Aleksander Alekseev
Discussion: https://postgr.es/m/CALT9ZEHRn5xAM5boga0qnrCmPV52bScEK2QnQ1HmUZDD301JEg%40mail.gmail.com
---
contrib/amcheck/Makefile | 2 +-
contrib/amcheck/amcheck--1.3--1.4.sql | 29 ++
contrib/amcheck/amcheck.control | 2 +-
contrib/amcheck/expected/check_btree.out | 42 +++
contrib/amcheck/sql/check_btree.sql | 14 +
contrib/amcheck/t/004_verify_nbtree_unique.pl | 235 +
contrib/amcheck/verify_nbtree.c | 329 +-
doc/src/sgml/amcheck.sgml | 14 +-
doc/src/sgml/ref/pg_amcheck.sgml | 11 +
src/bin/pg_amcheck/pg_amcheck.c | 48 ++-
src/bin/pg_amcheck/t/003_check.pl | 45 +++
src/bin/pg_amcheck/t/005_opclass_damage.pl| 65
12 files changed, 813 insertions(+), 23 deletions(-)
create mode 100644 contrib/amcheck/amcheck--1.3--1.4.sql
create mode 100644 contrib/amcheck/t/004_verify_nbtree_unique.pl
diff --git a/contrib/amcheck/Makefile b/contrib/amcheck/Makefile
index b82f221e50..88271687a3 100644
--- a/contrib/amcheck/Makefile
+++ b/contrib/amcheck/Makefile
@@ -7,7 +7,7 @@ OBJS = \
verify_nbtree.o
EXTENSION = amcheck
-DATA = amcheck--1.2--1.3.sql amcheck--1.1--1.2.sql amcheck--1.0--1.1.sql amcheck--1.0.sql
+DATA = amcheck--1.3--1.4.sql amcheck--1.2--1.3.sql amcheck--1.1--1.2.sql amcheck--1.0--1.1.sql amcheck--1.0.sql
PGFILEDESC = "amcheck - function for verifying relation integrity"
REGRESS = check check_btree check_heap
diff --git a/contrib/amcheck/amcheck--1.3--1.4.sql b/contrib/amcheck/amcheck--1.3--1.4.sql
new file mode 100644
index 00..75574eaa64
--- /dev/null
+++ b/contrib/amcheck/amcheck--1.3--1.4.sql
@@ -0,0 +1,29 @@
+/* contrib/amcheck/amcheck--1.3--1.4.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "ALTER EXTENSION amcheck UPDATE TO '1.4'" to load this file. \quit
+
+-- In order to avoid issues with dependencies when updating amcheck to 1.4,
+-- create new, overloaded versions of the 1.2 bt_index_parent_check signature,
+-- and 1.1 bt_index_check signature.
+
+--
+-- bt_index_parent_check()
+--
+CREATE FUNCTION bt_index_parent_check(index regclass,
+heapallindexed boolean, rootdescend boolean, checkunique boolean)
+RETURNS VOID
+AS 'MODULE_PATHNAME', 'bt_index_parent_check'
+LANGUAGE C STRICT PARALLEL RESTRICTED;
+--
+-- bt_index_check()
+--
+CREATE FUNCTION bt_index_check(index regclass,
+heapallindexed boolean, checkunique boolean)
+RETURNS VOID
+AS 'MODULE_PATHNAME', 'bt_index_check'
+LANGUAGE C STRICT PARALLEL RESTRICTED;
+
+-- We don't want this to be available to public
+REVOKE ALL ON FUNCTION bt_index_parent_check(regclass, boolean, boolean, boolean) FROM PUBLIC;
+REVOKE ALL ON FUNCTION bt_index_check(regclass, boolean, boolean) FROM PUBLIC;
diff --git a/contrib/amcheck/amcheck.control b/contrib/amcheck/amcheck.control
index ab50931f75..e67ace01c9 100644
--- a/contrib/amcheck/amcheck.control
+++ b/contrib/amcheck/amc
