Re: Optimize numeric.c mul_var() using the Karatsuba algorithm
On Fri, Jun 14, 2024, at 03:07, Aaron Altman wrote: > Thanks for the detailed background and comments, Joel! > > The new status of this patch is: Ready for Committer Thanks for reviewing. Attached, rebased version of the patch that implements the Karatsuba algorithm in numeric.c's mul_var(). /Joel 0001-numeric-mul_var-karatsuba.patch Description: Binary data
RE: Partial aggregates pushdown
Hi Alexander, hackers. > From: Alexander Pyhalov > Sent: Tuesday, May 28, 2024 2:45 PM > The fix was to set child_agg->agg_partial to orig_agg->agg_partial in > convert_combining_aggrefs(), it's already in the patch, as well as the > example - > without this fix I've just realized that you've added the necessary tests. I forgot to respond, my apologies. > From: Alexander Pyhalov > Sent: Tuesday, May 28, 2024 2:58 PM > BTW, there's I have an issue with test results in the last version of the > patch. > Attaching regression diffs. > I have partial sum over c_interval instead of sum(c_interval). I think the difference stems from the commit[1], which add serialization function to sum(interval). I will fix it. Best regards, Yuki Fujii -- Yuki Fujii Information Technology R&D Center, Mitsubishi Electric Corporation [1] https://github.com/postgres/postgres/commit/519fc1bd9e9d7b408903e44f55f83f6db30742b7
Re: PATCH: Add query for operators unusable with RLS to documentation
On Sat, 18 May 2024 16:54:52 -0700 Josh Snyder wrote: > When deploying RLS, I was surprised to find that certain queries which used > only builtin indexes and operators had dramatically different query plans > when > a policy is applied. In my case, the query `tsvector @@ tsquery` over a GIN > index was no longer able to use that index. I was able to find one other > instance [1] of someone being surprised by this behavior on the mailing lists. > > The docs already discuss the LEAKPROOF semantics in the abstract, but I think > they place not enough focus on the idea that builtin operators can be (and > frequently are) not leakproof. Based on the query given in the attached patch, > I found that 387 operators are not leakproof versus 588 that are. > > The attached patch updates the documentation to provide an easy query over > system catalogs as a way of determining which operators will no longer perform > well under RLS or a security-barrier view. I think it would be worth mentioning an index involving non-LEAKPROOF operator could not work with RLS or a security-barrier view in the documentation. (e.g. like https://www.postgresql.org/message-id/2273225.DEBA8KRT0r%40peanuts2) It may help to avoid other users from facing the surprise you got. However, I am not sure if it is appropriate to write the query consulting pg_amop in this part of the documentation.It is enough to add a reference to the other part describing operation familiar, for example, "11.10. Operator Classes and Operator Families"? Additionally, is it useful to add LEAKPROOF information to the result of psql \dAo(+) meta-comand, or a function that can check given index or operator is leakproof or not? Regards, Yugo Nagata > Thanks, > Josh > > [1] > https://www.postgresql.org/message-id/CAGrP7a2t%2BJbeuxpQY%2BRSvNe4fr3%2B%3D%3DUmyimwV0GCD%2BwcrSSb%3Dw%40mail.gmail.com -- Yugo NAGATA
Re: Meson far from ready on Windows
On 2024-06-21 Fr 11:15 AM, Robert Haas wrote: As a practical matter, I don't think MSVC is coming back. The buildfarm was already changed over to use meson, and it would be pretty disruptive to try to re-add buildfarm coverage for a resurrected MSVC on the eve of beta2. I think we should focus on improving whatever isn't quite right in meson -- plenty of other people have also complained about various things there, me included -- rather than trying to undo over a year's worth of work by lots of people to get things on Windows switched over to MSVC. As a practical matter, whether the buildfarm client uses meson or not is a matter of one line in the client's config file. Support for the old system is still there, of course, as it's required on older branches. So the impact would be pretty minimal if we did decide to re-enable the old build system. There are only two MSVC animals building master right now: drongo (run by me) and hammerkop (run by our friends at SRA OSS). I am not necessarily advocating it, just setting the record straight about how easy it would be to switch the buildfarm. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: recoveryCheck/008_fsm_truncation is failing on dodo in v14- (due to slow fsync?)
22.06.2024 12:00, Alexander Lakhin wrote: On the other hand, backporting a7f417107 could fix the issue too, but I'm afraid we'll still see other tests (027_stream_regress) failing like [4]. When similar failures were observed on Andres Freund's animals, Andres came to conclusion that they were caused by fsync too ([5]). It seems to me that another dodo failure [1] has the same cause: t/001_emergency_vacuum.pl .. ok # Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 29 just after 2. t/002_limits.pl Dubious, test returned 29 (wstat 7424, 0x1d00) All 2 subtests passed t/003_wraparounds.pl ... ok Test Summary Report --- t/002_limits.pl (Wstat: 7424 Tests: 2 Failed: 0) Non-zero exit status: 29 Parse errors: No plan found in TAP output Files=3, Tests=10, 4235 wallclock secs ( 0.10 usr 0.13 sys + 18.05 cusr 12.76 csys = 31.04 CPU) Result: FAIL Unfortunately, the buildfarm log doesn't contain regress_log_002_limits, but I managed to reproduce the failure on that my device, when it's storage as slow as: $ dd if=/dev/zero of=./test count=1024 oflag=dsync bs=128k 1024+0 records in 1024+0 records out 134217728 bytes (134 MB, 128 MiB) copied, 33.9446 s, 4.0 MB/s The test fails as below: [15:36:04.253](729.754s) ok 1 - warn-limit reached Begin standard error psql::1: WARNING: database "postgres" must be vacuumed within 37483631 transactions HINT: To avoid XID assignment failures, execute a database-wide VACUUM in that database. You might also need to commit or roll back old prepared transactions, or drop stale replication slots. End standard error [15:36:45.220](40.968s) ok 2 - stop-limit [15:36:45.222](0.002s) # issuing query via background psql: COMMIT IPC::Run: timeout on timer #1 at /usr/share/perl5/IPC/Run.pm line 2944. It looks like this bump (coined at [2]) is not enough for machines that are that slow: # Bump the query timeout to avoid false negatives on slow test systems. my $psql_timeout_secs = 4 * $PostgreSQL::Test::Utils::timeout_default; [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dodo&dt=2024-06-20%2007%3A18%3A46 [2] https://www.postgresql.org/message-id/CAD21AoBKBVkXyEwkApSUqN98CuOWw%3DYQdbkeE6gGJ0zH7z-TBw%40mail.gmail.com Best regards, Alexander
Re: Unable parse a comment in gram.y
"David G. Johnston" writes: > On Saturday, June 22, 2024, Tatsuo Ishii wrote: >>> Perhaps s/As func_expr/Like func_expr/ would be less confusing? >> +1. It's easier to understand at least for me. > +1 OK. I looked through the rest of src/backend/parser/ and couldn't find any other similar wording. There's plenty of "As with ..." and "As in ...", but at least to me those don't seem confusing. I'll plan to push the attached after the release freeze lifts. regards, tom lane diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 4d582950b7..4a4b47ca50 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -15667,7 +15667,7 @@ func_expr: func_application within_group_clause filter_clause over_clause ; /* - * As func_expr but does not accept WINDOW functions directly + * Like func_expr but does not accept WINDOW functions directly * (but they can still be contained in arguments for functions etc). * Use this when window expressions are not allowed, where needed to * disambiguate the grammar (e.g. in CREATE INDEX).
Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
On Sat, 22 Jun 2024 at 17:00, Alexander Lakhin wrote: > @@ -2775,6 +2775,7 @@ > SET LOCAL statement_timeout = '10ms'; > select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- > this takes very long > ERROR: canceling statement due to statement timeout > +WARNING: could not get result of cancel request due to timeout > COMMIT; As you describe it, this problem occurs when the cancel request is processed by the foreign server, before the query is actually received. And postgres then (rightly) ignores the cancel request. I'm not sure if the existing test is easily changeable to fix this. The only thing that I can imagine works in practice is increasing the statement_timeout, e.g. to 100ms. > I also came across another failure of the test: > @@ -2774,7 +2774,7 @@ > BEGIN; > SET LOCAL statement_timeout = '10ms'; > select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- > this takes very long > -ERROR: canceling statement due to statement timeout > +ERROR: canceling statement due to user request > COMMIT; > > which is reproduced with a sleep added here: > @@ -1065,6 +1065,7 @@ exec_simple_query(const char *query_string) >*/ > parsetree_list = pg_parse_query(query_string); > +pg_usleep(11000); After investigating, I realized this actually exposes a bug in our statement timeout logic. It has nothing to do with posgres_fdw and reproduces with any regular postgres query too. Attached is a patch that fixes this issue. This one should probably be backported. v1-0001-Do-not-reset-statement_timeout-indicator-outside-.patch Description: Binary data
Re: Add pg_get_acl() function get the ACL for a database object
On Sun, Jun 23, 2024 at 08:48:46AM +0200, Joel Jacobson wrote:
> On Sat, Jun 22, 2024, at 11:44, Joel Jacobson wrote:
>> * v5-0001-Add-pg_get_acl.patch
>> * v2-0002-Add-pg_get_acl-overloads.patch
>
> Rename files to ensure cfbot applies them in order; both need to
> have same version prefix.
+
+Returns the Access Control List (ACL) for a database object,
+specified by catalog OID and object OID.
Rather unrelated to this patch, still this patch makes the situation
more complicated in the docs, but wouldn't it be better to add ACL as
a term in acronyms.sql, and reuse it here? It would be a doc-only
patch that applies on top of the rest (could be on a new thread of its
own), with some markups added where needed.
+SELECT
+(pg_identify_object(s.classid,s.objid,s.objsubid)).*,
+pg_catalog.pg_get_acl(s.classid,s.objid)
+FROM pg_catalog.pg_shdepend AS s
+JOIN pg_catalog.pg_database AS d ON d.datname = current_database() AND d.oid =
s.dbid
+JOIN pg_catalog.pg_authid AS a ON a.oid = s.refobjid AND s.refclassid =
'pg_authid'::regclass
+WHERE s.deptype = 'a';
Could be a bit prettier. That's a good addition.
+ catalogId = (classId == LargeObjectRelationId) ?
LargeObjectMetadataRelationId : classId;
Indeed, and we need to live with this tweak as per the reason in
inv_api.c related to clients, so that's fine. Still a comment is
adapted for this particular case?
+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);
+ pg_get_acl
+
+
+(1 row)
How about adding a bit more coverage? I'd suggest the following
additions:
- class ID as 0 in input.
- object ID as 0 in input.
- Both class and object ID as 0 in input.
+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);
+
pg_get_acl
+--
+
{regress_priv_user1=arwdDxtm/regress_priv_user1,regress_priv_user2=r/regress_priv_user1,regress_priv_user3=w/regress_priv_user1,regress_priv_user4=a/regress_priv_user1,regress_priv_user5=D/regress_priv_user1}
+(1 row)
This is hard to parse. I would add an unnest() and order the entries
so as modifications are easier to catch, with a more predictible
result.
FWIW, I'm still a bit meh with the addition of the functions
overloading the arguments with reg inputs. I'm OK with that when we
know that the input would be of a given object type, like
pg_partition_ancestors or pg_partition_tree, but for a case as generic
as this one this is less appealing to me.
--
Michael
signature.asc
Description: PGP signature
Avoid incomplete copy string (src/backend/access/transam/xlog.c)
Hi. In src/include/access/xlogbackup.h, the field *name* has one byte extra to store null-termination. But, in the function *do_pg_backup_start*, I think that is a mistake in the line (8736): memcpy(state->name, backupidstr, strlen(backupidstr)); memcpy with strlen does not copy the whole string. strlen returns the exact length of the string, without the null-termination. So, I think this can result in errors, like in the function *build_backup_content* (src/backend/access/transam/xlogbackup.c) Where *appendStringInfo* expects a string with null-termination. appendStringInfo(result, "LABEL: %s\n", state->name); To fix, copy strlen size plus one byte, to include the null-termination. Trivial patch attached. best regards, Ranier Vilela avoid-incomplete-copy-string-do_pg_backup_start.patch Description: Binary data
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
On Sun, 23 Jun 2024 at 20:51 Ranier Vilela wrote: > Hi. > > In src/include/access/xlogbackup.h, the field *name* > has one byte extra to store null-termination. > > But, in the function *do_pg_backup_start*, > I think that is a mistake in the line (8736): > > memcpy(state->name, backupidstr, strlen(backupidstr)); > > memcpy with strlen does not copy the whole string. > strlen returns the exact length of the string, without > the null-termination. > > So, I think this can result in errors, > like in the function *build_backup_content* > (src/backend/access/transam/xlogbackup.c) > Where *appendStringInfo* expects a string with null-termination. > > appendStringInfo(result, "LABEL: %s\n", state->name); > > To fix, copy strlen size plus one byte, to include the null-termination. > > Doesn’t “sizeof” solve the problem? It take in account the null-termination character. Fabrízio de Royes Mello
Re: Optimize numeric.c mul_var() using the Karatsuba algorithm
On Sun, Jun 23, 2024 at 09:00:29AM +0200, Joel Jacobson wrote: > Attached, rebased version of the patch that implements the Karatsuba > algorithm in numeric.c's mul_var(). It's one of these areas where Dean Rasheed would be a good match for a review, so adding him in CC. He has been doing a lot of stuff in this area over the years. +#define KARATSUBA_BASE_LIMIT 384 +#define KARATSUBA_VAR1_MIN1 128 +#define KARATSUBA_VAR1_MIN2 2000 +#define KARATSUBA_VAR2_MIN1 2500 +#define KARATSUBA_VAR2_MIN2 9000 +#define KARATSUBA_SLOPE 0.764 +#define KARATSUBA_INTERCEPT 90.737 These numbers feel magic, and there are no explanations behind these choices so it is hard to know whether these are good or not, or if there are potentially "better" choices. I'd suggest to explain why these variables are here as well as the basics of the method in this area of the code, with the function doing the operation pointing at that so as all the explanations are in a single place. Okay, these are thresholds based on the number of digits to decide if the normal or Karatsuba's method should be used, but grouping all the explanations in a single place is simpler. I may have missed something, but did you do some benchmark when the thresholds are at their limit where we would fallback to the calculation method of HEAD? I guess that the answer to my question of "Is HEAD performing better across these thresholds?" is clearly "no" based on what I read at [1] and the threshold numbers chosen, still asking. [1]: https://en.wikipedia.org/wiki/Karatsuba_algorithm -- Michael signature.asc Description: PGP signature
Re: Unable parse a comment in gram.y
>>> +1. It's easier to understand at least for me. > >> +1 > > OK. I looked through the rest of src/backend/parser/ and couldn't > find any other similar wording. There's plenty of "As with ..." > and "As in ...", but at least to me those don't seem confusing. > I'll plan to push the attached after the release freeze lifts. Excellent! -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
On Sun, Jun 23, 2024 at 09:08:47PM -0300, Fabrízio de Royes Mello wrote: > Doesn’t “sizeof” solve the problem? It take in account the null-termination > character. The size of BackupState->name is fixed with MAXPGPATH + 1, so it would be a better practice to use strlcpy() with sizeof(name) anyway? -- Michael signature.asc Description: PGP signature
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
Em dom., 23 de jun. de 2024 às 21:08, Fabrízio de Royes Mello < fabriziome...@gmail.com> escreveu: > > On Sun, 23 Jun 2024 at 20:51 Ranier Vilela wrote: > >> Hi. >> >> In src/include/access/xlogbackup.h, the field *name* >> has one byte extra to store null-termination. >> >> But, in the function *do_pg_backup_start*, >> I think that is a mistake in the line (8736): >> >> memcpy(state->name, backupidstr, strlen(backupidstr)); >> >> memcpy with strlen does not copy the whole string. >> strlen returns the exact length of the string, without >> the null-termination. >> >> So, I think this can result in errors, >> like in the function *build_backup_content* >> (src/backend/access/transam/xlogbackup.c) >> Where *appendStringInfo* expects a string with null-termination. >> >> appendStringInfo(result, "LABEL: %s\n", state->name); >> >> To fix, copy strlen size plus one byte, to include the null-termination. >> > >> > Doesn’t “sizeof” solve the problem? It take in account the > null-termination character. sizeof is is preferable when dealing with constants such as: memcpy(name, "string test1", sizeof( "string test1"); Using sizeof in this case will always copy MAXPGPATH + 1. Modern compilers will optimize strlen, copying fewer bytes. best regards, Ranier Vilela
Re: replace strtok()
On Sat, Jun 22, 2024 at 11:48:21AM -0400, Tom Lane wrote: > Peter Eisentraut writes: > > On 18.06.24 13:43, Ranier Vilela wrote: > >> I found another implementation of strsep, it seems lighter to me. > >> I will attach it for consideration, however, I have not done any testing. > > > Yeah, surely there are many possible implementations. I'm thinking, > > since we already took other str*() functions from OpenBSD, it makes > > sense to do this here as well, so we have only one source to deal with. > > Why not use strpbrk? That's equally thread-safe, it's been there > since C89, and it doesn't have the problem that you can't find out > which of the delimiter characters was found. Yeah, strpbrk() has been used in the tree as far as 2003 without any port/ implementation. -- Michael signature.asc Description: PGP signature
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
Em dom., 23 de jun. de 2024 às 21:24, Michael Paquier escreveu: > On Sun, Jun 23, 2024 at 09:08:47PM -0300, Fabrízio de Royes Mello wrote: > > Doesn’t “sizeof” solve the problem? It take in account the > null-termination > > character. > > The size of BackupState->name is fixed with MAXPGPATH + 1, so it would > be a better practice to use strlcpy() with sizeof(name) anyway? > It's not critical code, so I think it's ok to use strlen, even because the result of strlen will already be available using modern compilers. So, I think it's ok to use memcpy with strlen + 1. best regards, Ranier Vilela
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote: > It's not critical code, so I think it's ok to use strlen, even because the > result of strlen will already be available using modern compilers. > > So, I think it's ok to use memcpy with strlen + 1. It seems to me that there is a pretty good argument to just use strlcpy() for the same reason as the one you cite: this is not a performance-critical code, and that's just safer. -- Michael signature.asc Description: PGP signature
Proposal: Division operator for (interval / interval => double precision)
Is there a desire to have a division operator / that takes dividend
and divisor of types interval, and results in a quotient of type
double precision.
This would be helpful in calculating how many times the divisor
interval can fit into the dividend interval.
To complement this division operator, it would be desirable to also
have a remainder operator %.
For example,
('365 days'::interval / '5 days'::interval) => 73
('365 days'::interval % '5 days'::interval) => 0
('365 days'::interval / '3 days'::interval) => 121
('365 days'::interval % '3 days'::interval) => 2
Best regards,
Gurjeet
http://Gurje.et
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
Em dom., 23 de jun. de 2024 às 21:54, Michael Paquier escreveu: > On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote: > > It's not critical code, so I think it's ok to use strlen, even because > the > > result of strlen will already be available using modern compilers. > > > > So, I think it's ok to use memcpy with strlen + 1. > > It seems to me that there is a pretty good argument to just use > strlcpy() for the same reason as the one you cite: this is not a > performance-critical code, and that's just safer. > Yeah, I'm fine with strlcpy. I'm not against it. New version, attached. best regards, Ranier Vilela v1-avoid-incomplete-copy-string-do_pg_backup_start.patch Description: Binary data
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
Em dom., 23 de jun. de 2024 às 22:05, Ranier Vilela escreveu: > Em dom., 23 de jun. de 2024 às 21:54, Michael Paquier > escreveu: > >> On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote: >> > It's not critical code, so I think it's ok to use strlen, even because >> the >> > result of strlen will already be available using modern compilers. >> > >> > So, I think it's ok to use memcpy with strlen + 1. >> >> It seems to me that there is a pretty good argument to just use >> strlcpy() for the same reason as the one you cite: this is not a >> performance-critical code, and that's just safer. >> > Yeah, I'm fine with strlcpy. I'm not against it. > Perhaps, like the v2? Either v1 or v2, to me, looks good. best regards, Ranier Vilela > v2-avoid-incomplete-copy-string-do_pg_backup_start.patch Description: Binary data
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
Em dom., 23 de jun. de 2024 às 22:14, Ranier Vilela escreveu: > Em dom., 23 de jun. de 2024 às 22:05, Ranier Vilela > escreveu: > >> Em dom., 23 de jun. de 2024 às 21:54, Michael Paquier < >> mich...@paquier.xyz> escreveu: >> >>> On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote: >>> > It's not critical code, so I think it's ok to use strlen, even because >>> the >>> > result of strlen will already be available using modern compilers. >>> > >>> > So, I think it's ok to use memcpy with strlen + 1. >>> >>> It seems to me that there is a pretty good argument to just use >>> strlcpy() for the same reason as the one you cite: this is not a >>> performance-critical code, and that's just safer. >>> >> Yeah, I'm fine with strlcpy. I'm not against it. >> > Perhaps, like the v2? > > Either v1 or v2, to me, looks good. > Thinking about, does not make sense the field size MAXPGPATH + 1. all other similar fields are just MAXPGPATH. If we copy MAXPGPATH + 1, it will also be wrong. So it is necessary to adjust logbackup.h as well. So, I think that v3 is ok to fix. best regards, Ranier Vilela > > best regards, > Ranier Vilela > >> v3-avoid-incomplete-copy-string-do_pg_backup_start.patch Description: Binary data
RE: Improve EXPLAIN output for multicolumn B-Tree Index
> I am unable to decide whether reporting the bound quals is just enough to > decide the efficiency of index without knowing the difference in the number > of index tuples selectivity and heap tuple selectivity. The difference seems > to be a better indicator of index efficiency whereas the bound quals will > help debug the in-efficiency, if any. > Also, do we want to report bound quals even if they are the same as index > conditions or just when they are different? Thank you for your comment. After receiving your comment, I thought it would be better to also report information that would make the difference in selectivity understandable. One idea I had is to output the number of index tuples inefficiently extracted, like “Rows Removed by Filter”. Users can check the selectivity and efficiency by looking at the number. Also, I thought it would be better to change the way bound quals are reported to align with the "Filter". I think it would be better to modify it so that it does not output when the bound quals are the same as the index conditions. In my local PoC patch, I have modified the output as follows, what do you think? =# EXPLAIN (VERBOSE, ANALYZE) SELECT * FROM test WHERE id1 = 1 AND id2 = 101; QUERY PLAN - Index Scan using test_idx on ikedamsh.test (cost=0.42..8.45 rows=1 width=18) (actual time=0.082..0.086 rows=1 loops=1) Output: id1, id2, id3, value Index Cond: ((test.id1 = 1) AND (test.id2 = 101)) -- If it’s efficient, the output won’t change. Planning Time: 5.088 ms Execution Time: 0.162 ms (5 rows) =# EXPLAIN (VERBOSE, ANALYZE) SELECT * FROM test WHERE id1 = 1 AND id3 = 101; QUERY PLAN --- Index Scan using test_idx on ikedamsh.test (cost=0.42..12630.10 rows=1 width=18) (actual time=0.175..279.819 rows=1 loops=1) Output: id1, id2, id3, value Index Cond: (test.id1 = 1) -- Change the output. Show only the bound quals. Index Filter: (test.id3 = 101) -- New. Output quals which are not used as the bound quals Rows Removed by Index Filter: 49-- New. Output when ANALYZE option is specified Planning Time: 0.354 ms Execution Time: 279.908 ms (7 rows) Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Re: Pgoutput not capturing the generated columns
Hi, here are some patch v9-0001 comments. I saw Kuroda-san has already posted comments for this patch so there may be some duplication here. == GENERAL 1. The later patches 0002 etc are checking to support only STORED gencols. But, doesn't that restriction belong in this patch 0001 so VIRTUAL columns are not decoded etc in the first place... (??) ~~~ 2. The "Generated Columns" docs mentioned in my previous review comment [2] should be modified by this 0001 patch. ~~~ 3. I think the "Message Format" page mentioned in my previous review comment [3] should be modified by this 0001 patch. == Commit message 4. The patch name is still broken as previously mentioned [1, #1] == doc/src/sgml/protocol.sgml 5. Should this docs be referring to STORED generated columns, instead of just generated columns? == doc/src/sgml/ref/create_subscription.sgml 6. Should this be docs referring to STORED generated columns, instead of just generated columns? == src/bin/pg_dump/pg_dump.c getSubscriptions: NITPICK - tabs NITPICK - patch removed a blank line it should not be touching NITPICK = patch altered indents it should not be touching NITPICK - a missing blank line that was previously present 7. + else + appendPQExpBufferStr(query, + " false AS subincludegencols,\n"); There is an unwanted comma here. ~ dumpSubscription NITPICK - patch altered indents it should not be touching == src/bin/pg_dump/pg_dump.h NITPICK - unnecessary blank line == src/bin/psql/describe.c describeSubscriptions NITPICK - bad indentation 8. In my previous review [1, #4b] I suggested this new column should be in a different order (e.g. adjacent to the other ones ahead of 'Conninfo'), but this is not yet addressed. == src/test/subscription/t/011_generated.pl NITPICK - missing space in comment NITPICK - misleading "because" wording in the comment == 99. See also my attached nitpicks diff, for cosmetic issues. Please apply whatever you agree with. == [1] My v8-0001 review - https://www.postgresql.org/message-id/CAHut%2BPujrRQ63ju8P41tBkdjkQb4X9uEdLK_Wkauxum1MVUdfA%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAHut%2BPvsRWq9t2tEErt5ZWZCVpNFVZjfZ_owqfdjOhh4yXb_3Q%40mail.gmail.com [3] https://www.postgresql.org/message-id/CAHut%2BPsHsT3V1wQ5uoH9ynbmWn4ZQqOe34X%2Bg37LSi7sgE_i2g%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia. diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 1fb19f5..9f2cac9 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -4739,7 +4739,7 @@ getSubscriptions(Archive *fout) int i_suboriginremotelsn; int i_subenabled; int i_subfailover; - int i_subincludegencols; + int i_subincludegencols; int i, ntups; @@ -4770,6 +4770,7 @@ getSubscriptions(Archive *fout) " s.subowner,\n" " s.subconninfo, s.subslotname, s.subsynccommit,\n" " s.subpublications,\n"); + if (fout->remoteVersion >= 14) appendPQExpBufferStr(query, " s.subbinary,\n"); else @@ -4804,7 +4805,7 @@ getSubscriptions(Archive *fout) if (dopt->binary_upgrade && fout->remoteVersion >= 17) appendPQExpBufferStr(query, " o.remote_lsn AS suboriginremotelsn,\n" - " s.subenabled,\n"); +" s.subenabled,\n"); else appendPQExpBufferStr(query, " NULL AS suboriginremotelsn,\n" " false AS subenabled,\n"); @@ -4815,12 +4816,14 @@ getSubscriptions(Archive *fout) else appendPQExpBuffer(query, " false AS subfailover,\n"); + if (fout->remoteVersion >= 17) appendPQExpBufferStr(query, " s.subincludegencols\n"); else appendPQExpBufferStr(query, " false AS subincludegencols,\n"); + appendPQExpBufferStr(query, "FROM pg_subscription s\n"); diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index a2c35fe..8c07933 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -672,7 +672,6 @@ typedef struct _SubscriptionInfo char *suboriginremotelsn; char *subfailover; char *subincludegencols; - } SubscriptionInfo; /* diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index 491fcb9..00f3131 100644 --- a/src/bin/psql/describe.c
RE: Improve EXPLAIN output for multicolumn B-Tree Index
> > * Is this feature useful? Is there a possibility it will be accepted? > > I think adding such information to EXPLAIN outputs is useful because it will > help users > confirm the effect of a multicolumn index on a certain query and decide to > whether > leave, drop, or recreate the index, and so on. Thank you for your comments and for empathizing with the utility of the approach. > > * Are there any other ideas for determining if multicolumn indexes are > > > > being used efficiently? Although I considered calculating the > > efficiency using > > > > pg_statio_all_indexes.idx_blks_read and > > pg_stat_all_indexes.idx_tup_read, > > > > I believe improving the EXPLAIN output is better because it can be > > output > > > > per query and it's more user-friendly. > > It seems for me improving EXPLAIN is a natural way to show information on > query > optimization like index scans. OK, I'll proceed with the way. > > * Is "Index Bound Cond" the proper term?I also considered changing > > > > "Index Cond" to only show quals for the boundary condition and adding > > > > a new term "Index Filter". > > "Index Bound Cond" seems not intuitive for me because I could not find > description > explaining what this means from the documentation. I like "Index Filter" that > implies the > index has to be scanned. OK, I think you are right. Even at this point, there are things like ‘Filter’ and ‘Rows Removed by Filter’, so it seems natural to align with them. I described a new output example in the previous email, how about that? > > * Would it be better to add new interfaces to Index AM? Is there any > > case > > > > to output the EXPLAIN for each index context? At least, I think it's > > worth > > > > considering whether it's good for amcostestimate() to modify the > > > > IndexPath directly as the PoC patch does. > > I am not sure it is the best way to modify IndexPath in amcostestimate(), but > I don't > have better ideas for now. OK, I’ll consider what the best way to change is. In addition, if we add "Rows Removed by Index Filter", we might need to consider a method to receive the number of filtered tuples at execution time from Index AM. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
On Mon, Jun 24, 2024 at 7:51 AM Ranier Vilela wrote: > In src/include/access/xlogbackup.h, the field *name* > has one byte extra to store null-termination. > > But, in the function *do_pg_backup_start*, > I think that is a mistake in the line (8736): > > memcpy(state->name, backupidstr, strlen(backupidstr)); > > memcpy with strlen does not copy the whole string. > strlen returns the exact length of the string, without > the null-termination. I noticed that the two callers of do_pg_backup_start both allocate BackupState with palloc0. Can we rely on this to ensure that the BackupState.name is initialized with null-termination? Thanks Richard
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
On Sun, 23 Jun 2024 22:34:03 -0300
Ranier Vilela wrote:
> Em dom., 23 de jun. de 2024 às 22:14, Ranier Vilela
> escreveu:
>
> > Em dom., 23 de jun. de 2024 às 22:05, Ranier Vilela
> > escreveu:
> >
> >> Em dom., 23 de jun. de 2024 às 21:54, Michael Paquier <
> >> mich...@paquier.xyz> escreveu:
> >>
> >>> On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote:
> >>> > It's not critical code, so I think it's ok to use strlen, even because
> >>> the
> >>> > result of strlen will already be available using modern compilers.
> >>> >
> >>> > So, I think it's ok to use memcpy with strlen + 1.
> >>>
> >>> It seems to me that there is a pretty good argument to just use
> >>> strlcpy() for the same reason as the one you cite: this is not a
> >>> performance-critical code, and that's just safer.
> >>>
> >> Yeah, I'm fine with strlcpy. I'm not against it.
> >>
> > Perhaps, like the v2?
> >
> > Either v1 or v2, to me, looks good.
> >
> Thinking about, does not make sense the field size MAXPGPATH + 1.
> all other similar fields are just MAXPGPATH.
>
> If we copy MAXPGPATH + 1, it will also be wrong.
> So it is necessary to adjust logbackup.h as well.
I am not sure whether we need to change the size of the field,
but if change it, I wonder it is better to modify the following
message from MAXPGPATH to MAXPGPATH -1.
errmsg("backup label too long (max %d bytes)",
MAXPGPATH)));
Regards,
Yugo Nagata
>
> So, I think that v3 is ok to fix.
>
> best regards,
> Ranier Vilela
>
> >
> > best regards,
> > Ranier Vilela
> >
> >>
--
Yugo NAGATA
Re: Proposal: Division operator for (interval / interval => double precision)
Hi
Its always a good idea to extend the functionality of PG.
Thanks
Kashif Zeeshan
On Mon, Jun 24, 2024 at 5:57 AM Gurjeet Singh wrote:
> Is there a desire to have a division operator / that takes dividend
> and divisor of types interval, and results in a quotient of type
> double precision.
>
> This would be helpful in calculating how many times the divisor
> interval can fit into the dividend interval.
>
> To complement this division operator, it would be desirable to also
> have a remainder operator %.
>
> For example,
>
> ('365 days'::interval / '5 days'::interval) => 73
> ('365 days'::interval % '5 days'::interval) => 0
>
> ('365 days'::interval / '3 days'::interval) => 121
> ('365 days'::interval % '3 days'::interval) => 2
>
> Best regards,
> Gurjeet
> http://Gurje.et
>
>
>
Buildfarm animal caiman showing a plperl test issue with newer Perl versions
Hello hackers, As recent caiman failures ([1], [2], ...) show, plperl.sql is incompatible with Perl 5.40. (The last successful test runs took place when cayman had Perl 5.38.2 installed: [3].) FWIW, I've found an already-existing fix for the issue [4] and a note describing the change for Perl 5.39.10 [5]. [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2024-06-24%2001%3A34%3A23 [2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2024-06-24%2000%3A15%3A16 [3] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2024-05-02%2021%3A57%3A17 [4] https://git.alpinelinux.org/aports/tree/community/postgresql14/fix-test-plperl-5.8-pragma.patch?id=28aeb872811f59a7f646aa29ed7c9dc30e698e65 [5] https://metacpan.org/release/PEVANS/perl-5.39.10/changes#Selected-Bug-Fixes Best regards, Alexander
Re: Proposal: Division operator for (interval / interval => double precision)
On Sun, Jun 23, 2024 at 5:57 PM Gurjeet Singh wrote:
> Is there a desire to have a division operator / that takes dividend
> and divisor of types interval, and results in a quotient of type
> double precision.
[...]
> ('365 days'::interval / '3 days'::interval) => 121
> ('365 days'::interval % '3 days'::interval) => 2
>
>
Is it double or biginteger that your operation is producing?
How about making the % operator output an interval? What is the answer to:
'1 day 12 hours 59 min 10 sec' / '3 hours 22 min 6 sec'?
Though I'd rather add functions to produce numbers from intervals then let
the existing math operations be used on those. These seem independently
useful though like this feature I've not really seen demand for them from
others.
in_years(interval) -> numeric
in_days(interval) -> numeric
in_hours(interval) -> numeric
in_microseconds(interval) -> numeric
etc...
That said, implementing the inverse of the existing
interval/double->interval operator has a nice symmetry. Though the 4
examples are trivial, single unit, single scale, divisions, so exactly how
that translates into support for a possibly messy example like above I'm
uncertain.
There is no precedence, but why not add a new composite type, (whole
bigint, remainder bigint) that, for you example #2, would be
(121,2*24*60*60*1000*1000), the second field being 2 days in microseconds?
Possibly under a different operator so those who just want integer division
can have it more cheaply and easily.
David J.
Re: Support "Right Semi Join" plan shapes
Hi, Richard
> On Apr 25, 2024, at 11:28, Richard Guo wrote:
>
> Here is another rebase with a commit message to help review. I also
> tweaked some comments.
Thank you for updating the patch, here are some comments on the v5 patch.
+ /*
+* For now we do not support RIGHT_SEMI join in mergejoin or nestloop
+* join.
+*/
+ if (jointype == JOIN_RIGHT_SEMI)
+ return;
+
How about adding some reasons here?
+ * this is a right-semi join, or this is a right/right-anti/full join and
+ * there are nonmergejoinable join clauses. The executor's mergejoin
Maybe we can put the right-semi join together with the right/right-anti/full
join. Is there any other significance by putting it separately?
Maybe the following comments also should be updated. Right?
diff --git a/src/backend/optimizer/prep/prepjointree.c
b/src/backend/optimizer/prep/prepjointree.c
index 5482ab85a7..791cbc551e 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -455,8 +455,8 @@ pull_up_sublinks_jointree_recurse(PlannerInfo *root, Node
*jtnode,
* point of the available_rels machinations is to ensure that we only
* pull up quals for which that's okay.
*
- * We don't expect to see any pre-existing JOIN_SEMI, JOIN_ANTI, or
- * JOIN_RIGHT_ANTI jointypes here.
+ * We don't expect to see any pre-existing JOIN_SEMI, JOIN_ANTI,
+ * JOIN_RIGHT_SEMI, or JOIN_RIGHT_ANTI jointypes here.
*/
switch (j->jointype)
{
@@ -2951,6 +2951,7 @@ reduce_outer_joins_pass2(Node *jtnode,
* so there's no way that upper quals could refer to their
* righthand sides, and no point in checking. We don't expect
* to see JOIN_RIGHT_ANTI yet.
+ * Does JOIN_RIGHT_SEMI is expected here?
*/
break;
default:
Re: Introduce XID age and inactive timeout based replication slot invalidation
Hi, On Mon, Jun 17, 2024 at 5:55 PM Bharath Rupireddy wrote: > > Here are my thoughts on when to do the XID age invalidation. In all > the patches sent so far, the XID age invalidation happens in two > places - one during the slot acquisition, and another during the > checkpoint. As the suggestion is to do it during the vacuum (manual > and auto), so that even if the checkpoint isn't happening in the > database for whatever reasons, a vacuum command or autovacuum can > invalidate the slots whose XID is aged. > > An idea is to check for XID age based invalidation for all the slots > in ComputeXidHorizons() before it reads replication_slot_xmin and > replication_slot_catalog_xmin, and obviously before the proc array > lock is acquired. A potential problem with this approach is that the > invalidation check can become too aggressive as XID horizons are > computed from many places. > > Another idea is to check for XID age based invalidation for all the > slots in higher levels than ComputeXidHorizons(), for example in > vacuum() which is an entry point for both vacuum command and > autovacuum. This approach seems similar to vacuum_failsafe_age GUC > which checks each relation for the failsafe age before vacuum gets > triggered on it. I am attaching the patches implementing the idea of invalidating replication slots during vacuum when current slot xmin limits (procArray->replication_slot_xmin and procArray->replication_slot_catalog_xmin) are aged as per the new XID age GUC. When either of these limits are aged, there must be at least one replication slot that is aged, because the xmin limits, after all, are the minimum of xmin or catalog_xmin of all replication slots. In this approach, the new XID age GUC will help vacuum when needed, because the current slot xmin limits are recalculated after invalidating replication slots that are holding xmins for longer than the age. The code is placed in vacuum() which is common for both vacuum command and autovacuum, and gets executed only once every vacuum cycle to not be too aggressive in invalidating. However, there might be some concerns with this approach like the following: 1) Adding more code to vacuum might not be acceptable 2) What if invalidation of replication slots emits an error, will it block vacuum forever? Currently, InvalidateObsoleteReplicationSlots() is also called as part of the checkpoint, and emitting ERRORs from within is avoided already. Therefore, there is no concern here for now. 3) What if there are more replication slots to be invalidated, will it delay the vacuum? If yes, by how much? <> 4) Will the invalidation based on just current replication slot xmin limits suffice irrespective of vacuum cutoffs? IOW, if the replication slots are invalidated but vacuum isn't going to do any work because vacuum cutoffs are not yet met? Is the invalidation work wasteful here? 5) Is it okay to take just one more time the proc array lock to get current replication slot xmin limits via ProcArrayGetReplicationSlotXmin() once every vacuum cycle? <> 6) Vacuum command can't be run on the standby in recovery. So, to help invalidate replication slots on the standby, I have for now let the checkpointer also do the XID age based invalidation. I know invalidating both in checkpointer and vacuum may not be a great idea, but I'm open to thoughts. Following are some of the alternative approaches which IMHO don't help vacuum when needed: a) Let the checkpointer do the XID age based invalidation, and call it out in the documentation that if the checkpoint doesn't happen, the new GUC doesn't help even if the vacuum is run. This has been the approach until v40 patch. b) Checkpointer and/or other backends add an autovacuum work item via AutoVacuumRequestWork(), and autovacuum when it gets to it will invalidate the replication slots. But, what to do for the vacuum command here? Please find the attached v41 patches implementing the idea of vacuum doing the invalidation. Thoughts? Thanks to Sawada-san for a detailed off-list discussion. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com From dc1eeed06377c11b139724702370ba47cd5d5be3 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Sun, 23 Jun 2024 14:07:25 + Subject: [PATCH v41 1/2] Add inactive_timeout based replication slot invalidation Till now, postgres has the ability to invalidate inactive replication slots based on the amount of WAL (set via max_slot_wal_keep_size GUC) that will be needed for the slots in case they become active. However, choosing a default value for max_slot_wal_keep_size is tricky. Because the amount of WAL a customer generates, and their allocated storage will vary greatly in production, making it difficult to pin down a one-size-fits-all value. It is often easy for developers to set a timeout of say 1 or 2 or 3 days, after which the inactive slots get invalidated. To achieve the above, postgres introduces a GUC a
Re: Inconsistency between try_mergejoin_path and create_mergejoin_plan
On Wed, Jun 19, 2024 at 10:24 PM Alexander Pyhalov wrote: > Richard Guo писал(а) 2024-06-19 16:30: > > I think we can simply verify the validity of commutators for clauses in > > the form "inner op outer" when selecting mergejoin/hash clauses. If a > > clause lacks a commutator, we should consider it unusable for the given > > pair of outer and inner rels. Please see the attached patch. > This seems to be working for my test cases. Thank you for confirming. Here is an updated patch with some tweaks to the comments and commit message. I've parked this patch in the July commitfest. Thanks Richard v3-0001-Check-the-validity-of-commutators-for-merge-hash-clauses.patch Description: Binary data
Re: speed up a logical replica setup
On Sun, Jun 23, 2024 at 11:52 AM Noah Misch wrote:
>
> > +static char *
> > +create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo
> > *dbinfo)
> > +{
> > + PQExpBuffer str = createPQExpBuffer();
> > + PGresult *res = NULL;
> > + const char *slot_name = dbinfo->replslotname;
> > + char *slot_name_esc;
> > + char *lsn = NULL;
> > +
> > + Assert(conn != NULL);
> > +
> > + pg_log_info("creating the replication slot \"%s\" on database \"%s\"",
> > + slot_name, dbinfo->dbname);
> > +
> > + slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
> > +
> > + appendPQExpBuffer(str,
> > + "SELECT lsn FROM
> > pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput', false, false,
> > false)",
>
> This is passing twophase=false, but the patch does not mention prepared
> transactions. Is the intent to not support workloads containing prepared
> transactions? If so, the documentation should say that, and the tool likely
> should warn on startup if max_prepared_transactions != 0.
>
The other point to note in this regard is that if we don't support
two_phase in the beginning during subscription/slot setup, users won't
be able to change it as we don't yet support changing it via alter
subscription (though the patch to alter two_pc is proposed for PG18).
--
With Regards,
Amit Kapila.
RE: speed up a logical replica setup
Dear Noah,
> pg_createsubscriber fails on a dbname containing a space. Use
> appendConnStrVal() here and for other params in get_sub_conninfo(). See the
> CVE-2016-5424 commits for more background. For one way to test this
> scenario,
> see generate_db() in the pg_upgrade test suite.
Thanks for pointing out. I made a fix patch. Test code was also modified
accordingly.
> > +static char *
> > +create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo
> > *dbinfo)
> > +{
> > + PQExpBuffer str = createPQExpBuffer();
> > + PGresult *res = NULL;
> > + const char *slot_name = dbinfo->replslotname;
> > + char *slot_name_esc;
> > + char *lsn = NULL;
> > +
> > + Assert(conn != NULL);
> > +
> > + pg_log_info("creating the replication slot \"%s\" on database \"%s\"",
> > + slot_name, dbinfo->dbname);
> > +
> > + slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
> > +
> > + appendPQExpBuffer(str,
> > + "SELECT lsn FROM
> pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput', false, false,
> false)",
>
> This is passing twophase=false, but the patch does not mention prepared
> transactions. Is the intent to not support workloads containing prepared
> transactions? If so, the documentation should say that, and the tool likely
> should warn on startup if max_prepared_transactions != 0.
IIUC, We decided because it is a default behavior of logical replication. See
[1].
+1 for improving a documentation, but not sure it is helpful for adding output.
I want to know opinions from others.
> > +static void
> > +create_publication(PGconn *conn, struct LogicalRepInfo *dbinfo)
> > +{
>
> > + appendPQExpBuffer(str, "CREATE PUBLICATION %s FOR ALL TABLES",
> > + ipubname_esc);
>
> This tool's documentation says it "guarantees that no transaction will be
> lost." I tried to determine whether achieving that will require something
> like the fix from
> https://postgr.es/m/flat/de52b282-1166-1180-45a2-8d8917ca74c6@enterprise
> db.com.
> (Not exactly the fix from that thread, since that thread has not discussed the
> FOR ALL TABLES version of its race condition.) I don't know. On the one
> hand, pg_createsubscriber benefits from creating a logical slot after creating
> the publication. That snapbuild.c process will wait for running XIDs. On the
> other hand, an INSERT/UPDATE/DELETE acquires its RowExclusiveLock and
> builds
> its relcache entry before assigning an XID, so perhaps the snapbuild.c process
> isn't enough to prevent that thread's race condition. What do you think?
IIUC, documentation just intended to say that a type of replication will be
switched from stream to logical one, at the certain point. Please give sometime
for analyzing.
[1]:
https://www.postgresql.org/message-id/270ad9b8-9c46-40c3-b6c5-3d25b91d3a7d%40app.fastmail.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
0001-pg_createsubscriber-Fix-cases-which-connection-param.patch
Description: 0001-pg_createsubscriber-Fix-cases-which-connection-param.patch
