starts_with, ^@ and index usage
Greetings hackers, I'm seeing some odd behavior around string prefix searching - hopefully I've missed something here (thanks to Nino Floris for originally flagging this). In PostgreSQL 11, a starts_with function and a ^@ operators were added for string prefix checking, as an alternative to LIKE 'foo%' [1] [2]. I've ran a few scenarios and have seen the following behavior: Queries tested: 1. EXPLAIN SELECT * FROM data WHERE name LIKE 'foo10%'; 2. EXPLAIN SELECT * FROM data WHERE name ^@ 'foo10'; 3. EXPLAIN SELECT * FROM data WHERE starts_with(name, 'foo10'); ... running against a table with 500k rows and enable_seqscan turned off. Results: Index | Operator class | LIKE 'X%' | ^@| starts_with -- | | - | - | --- btree | text_ops | Parallel seq scan | Parallel seq scan | Seq scan btree | text_pattern_ops | Index scan| Parallel seq scan | Seq scan spgist | | Index scan| Index Scan| Seq scan First, starts_with doesn't seem to use SP-GIST indexes, contrary to the patch description (and also doesn't trigger a parallel seq scan) - is this intentional? The function is listed front-and-center on the string functions and operators page[3], and receives mention on the pattern matching page[4], without any mention of it being so problematic. Note that ^@ isn't documented on the string functions and operators, so it's not very discoverable; if added to the docs, I'd recommend adding a note on SP-GIST being required, since uninformed new users would probably expect a default btree index to work as well. Shay [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=710d90da1fd8c1d028215ecaf7402062079e99e9 [2] https://www.postgresql.org/message-id/flat/03300255-cff2-b508-50f4-f00cca0a57a1%40sigaev.ru#38d2020edf92f96d204cd2679d362c38 [3] https://www.postgresql.org/docs/current/functions-string.html [4] https://www.postgresql.org/docs/current/functions-matching.html
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?
On Sat, Oct 9, 2021 at 3:56 AM Bharath Rupireddy wrote: > > I would think that these would fall under "pg_read_all_stats", in > > particular, which is explicitly documented as: Read all pg_stat_* views > > and use various statistics related extensions, even those normally > > visible only to superusers. > > > > (the last bit being particularly relevant in this case) > > +1. I will prepare the patch with the pg_read_all_stats role. Here's the v1, please review it further. I've also made a CF entry - https://commitfest.postgresql.org/35/3352/ Regards, Bharath Rupireddy. v1-0001-change-privileges-of-pg_backend_memory_contexts-p.patch Description: Binary data
Re: RFC: compression dictionaries for JSONB
On Fri, 8 Oct 2021 at 21:21, Alvaro Herrera wrote: > > On 2021-Oct-08, Matthias van de Meent wrote: > > > That's a good point, but if we're extending this syntax to allow the > > ability of including other types, then I'd instead extend the syntax > > that of below, so that the type of the dictionary entries is required > > in the syntax: > > > > CREATE TYPE name AS DICTIONARY OF jsonb [ ( ...entries ) ] [ WITH ( > > ...options ) ]; > > I don't think this gives you any guarantees of the sort you seem to > expect. See CREATE AGGREGATE as a precedent where there are some > options in the parenthesized options list you cannot omit. Bikeshedding on syntax: I guess? I don't really like 'required options' patterns. If you're required to use/specify an option, then it's not optional, and should thus not be included in the group of 'options'. > > > The pg_type entry would have to provide some support procedure that > > > makes use of the dictionary in some way. This seems better than tying > > > the SQL object to a specific type. > > > > Agreed, but this might mean that much more effort would be required to > > get such a useful quality-of-life feature committed. > > I don't understand what you mean by that. I'm not saying that the patch > has to provide support for any additional datatypes. Its only > obligation would be to provide a new column in pg_type which is zero for > all rows except jsonb, and in that row it is the OID of a > jsonb_dictionary() function that's called from all the right places and > receives all the right arguments. This seems feasable to do, but I still have limited knowledge on the intricacies of the type system, and as such I don't see how this part would function: I was expecting more something in the line of how array types seem to work: Type _A is an array type, containing elements of Type A. It's containing type is defined in pg_type.typbasetype. No special functions are defined on base types to allow their respective array types, that part is handled by the array infrastructure. Same for Domain types. Now that I think about it, we should still provide the information on _how_ to find the type functions for the dictionaried type: Arrays and domains are generic, but dictionaries will require deep understanding of the underlying type. So, yes, you are correct, there should be one more function, which would supply the necessary pg_type functions that CREATE TYPE DICTIONARY can then register in the pg_type entry of the dictionary type. The alternative would initially be hardcoding this for the base types that have dictionary support, which definitely would be possible for a first iteration, but wouldn't be great. Kind regards, Matthias
Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
Hi, It looks like auxiliary processes will not have a valid MyBackendId as they don't call InitPostgres() and SharedInvalBackendInit() unlike backends. But the startup process (which is an auxiliary process) in hot standby mode seems to be different in the way that it does have a valid MyBackendId as SharedInvalBackendInit() gets called from InitRecoveryTransactionEnvironment(). The SharedInvalBackendInit() usually stores the MyBackendId in the caller's PGPROC structure i.e. MyProc->backendId. The auxiliary processes (including the startup process) usually register themselves in procsignal array with ProcSignalInit(MaxBackends + MyAuxProcType + 1) unlike the backends with ProcSignalInit(MyBackendId) after SharedInvalBackendInit() in InitPostgres(). The problem comes when a postgres process wants to send a multiplexed SIGUSR1 signal (probably using SendProcSignal()) to the startup process after receiving its ProcSignal->psh_slot[] with its backendId from the PGPROC (the postgres process can get the startup process PGPROC structure from AuxiliaryPidGetProc()). Remember the startup process has registered in the procsignal array with ProcSignalInit(MaxBackends + MyAuxProcType + 1), not with the ProcSignalInit(MyBackendId) like the backends did. So, the postgres process, wanting to send SIGUSR1 to the startup process, refers to the wrong ProcSignal->psh_slot[] and may not send the signal. Is this inconsistency of MyBackendId for a startup process a problem at all? Thoughts? These are the following ways I think we can fix it, if at all some other hacker agrees that it is actually an issue: 1) Fix the startup process code, probably by unregistering the procsignal array entry that was made with ProcSignalInit(MaxBackends + MyAuxProcType + 1) in AuxiliaryProcessMain() and register with ProcSignalInit(MyBackendId) immediately after SharedInvalBackendInit() calculates the MyBackendId in with SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). This seems risky to me as unregistering and registering ProcSignal array involves some barriers and during the registering and unregistering window, the startup process may miss the SIGUSR1. 2) Ensure that the process, that wants to send the startup process SIGUSR1 signal, doesn't use the backendId from the startup process PGPROC, in which case it has to loop over all the entries of ProcSignal->psh_slot[] array to find the entry with the startup process PID. It seems easier and less riskier but only caveat is that the sending process shouldn't look at the backendId from auxiliary process PGPROC, instead it should just traverse the entire proc signal array to find the right slot. 3) Add a comment around AuxiliaryPidGetProc() that says "auxiliary processes don't have valid backend ids, so don't use the backendId from the returned PGPROC". (2) and (3) seem reasonable to me. Thoughts? Regards, Bharath Rupireddy.
enhance pg_log_backend_memory_contexts() to log memory contexts of auxiliary processes
Hi, Currently pg_log_backend_memory_contexts() doesn't log the memory contexts of auxiliary processes such as bgwriter, checkpointer, wal writer, archiver, startup process and wal receiver. It will be useful to look at the memory contexts of these processes too, for debugging purposes and better understanding of the memory usage pattern of these processes. Inside the code, we could use the AuxiliaryPidGetProc() to get the PGPROC of these processes. Note that, neither AuxiliaryPidGetProc() nor BackendPidGetProc() can return PGPROC(as they don't have PGPROC entries at all) entries for the syslogger, stats collector processes. Open points: 1) I'm not sure if it's a good idea to log postmaster memory usage too. Thoughts? 2) Since with this change pg_log_backend_memory_contexts() will work for auxiliary processes too, do we need to change the function name from pg_log_backend_memory_contexts() to pg_log_backend_memory_contexts()/pg_log_memory_contexts()/some other name? Or is it a good idea to have a separate function for auxiliary processes alone, pg_log_auxilliary_process_memory_contexts()? Thoughts? I will attach the patch, if possible with test cases, once we agree on the above open points. Regards, Bharath Rupireddy.
Re: Fix pg_log_backend_memory_contexts() 's delay
On Fri, Oct 8, 2021 at 8:58 PM Fujii Masao wrote: > >>> Thanks for the patch. Do we also need to do the change in > >>> HandleMainLoopInterrupts, HandleCheckpointerInterrupts, > >>> HandlePgArchInterrupts, HandleWalWriterInterrupts as we don't call > >>> CHECK_FOR_INTERRUPTS() there? > > > >> Yeah, that's still some information that the user asked for. Looking > >> at the things that have a PGPROC entry, should we worry about the main > >> loop of the logical replication launcher? > > > > ・Now, the target of “pg_log_backend_memory_contexts()” is “autovacuum > > launcher” and “logical replication launcher”. I observed that the delay > > occurred only in “autovacuum launcher” not in “logical replication > > launcher”. > > ・”autovacuum launcher” used “HandleAutoVacLauncherInterrupts()” ( not > > including “ProcessLogMemoryContextInterrupt()” ) instead of > > “ProcessInterrupts() ( including “ProcessLogMemoryContextInterrupt()” ). > > “ProcessLogMemoryContextInterrupt()” will not be executed until the next > > “ProcessInterrupts()” is executed. So, I added > > “ProcessLogMemoryContextInterrupt()”. > > ・”logical replication launcher” uses only “ProcessInterrupts()”. So, We > > don’t have to fix it. > > Yes. +1 to keep this thread for fixing the pg_log_backend_memory_contexts() issue for the autovacuum launcher. And the patch "fix_log_output_delay" looks good to me. I think we can add a CF entry. > >> IMHO, we can support all the processes which return a PGPROC entry by > >> both BackendPidGetProc and AuxiliaryPidGetProc where the > >> AuxiliaryPidGetProc would cover the following processes. I'm not sure > >> one is interested in the memory context info of auxiliary processes. > > I like this idea because it seems helpful at least for debug purpose. > > > > ・The purpose of this patch is to solve the delay problem, so I would like > > another patch to deal with “ BackendPidGetProc” and “AuxiliaryPidGetProc”. > > +1 to improve those things separately. I started a separate thread [1], and I have a couple of open points there. Please feel free to provide your thoughts in [1]. [1] https://www.postgresql.org/message-id/flat/CALj2ACU1nBzpacOK2q%3Da65S_4%2BOaz_rLTsU1Ri0gf7YUmnmhfQ%40mail.gmail.com Regards, Bharath Rupireddy.
Re: starts_with, ^@ and index usage
Shay Rojansky writes: > In PostgreSQL 11, a starts_with function and a ^@ operators were added > for string prefix checking, as an alternative to LIKE 'foo%' [1] [2]. > First, starts_with doesn't seem to use SP-GIST indexes, contrary to > the patch description (and also doesn't trigger a parallel seq scan) - > is this intentional? The function is listed front-and-center on the > string functions and operators page[3], and receives mention on the > pattern matching page[4], without any mention of it being so > problematic. It seems like it didn't occur to anybody to tie starts_with() into the machinery for derived index operators. That wouldn't be hard, but it wasn't done. Before (I think) v12, function invocations never could be converted to indexquals anyway, so it's not surprising that a v11-era patch wouldn't have thought it needed to address that point. I do see that starts_with() is marked parallel safe, so it's not clear why it wouldn't be amenable to a parallel seqscan. The function (as opposed to the operator) isn't tied into selectivity estimation either, so maybe that has something to do with using a default selectivity estimate for it? But said estimate would almost always be too high, which doesn't seem like the direction that would discourage parallelism. > Note that ^@ isn't documented on the string functions and operators, That's another oversight. It seems clear that the original patch author was pretty narrowly focused on use of the operator with SP-GIST, and didn't think about how it should fit into the larger ecosystem. regards, tom lane
Re: Reword docs of feature "Remove temporary files after backend crash"
On Fri, Oct 8, 2021 at 4:27 PM Bharath Rupireddy wrote: > > Hi, > > The commit [1] for the feature "Remove temporary files after backend > crash" introduced following in the docs: > + > +When set to on, which is the default, > +PostgreSQL will automatically remove > +temporary files after a backend crash. If disabled, the files will be > +retained and may be used for debugging, for example. Repeated crashes > +may however result in accumulation of useless files. > + > > The term backend means the user sessions (see from the glossary, at > [2]). It looks like the code introduced by the commit [1] i.e. the > temp table removal gets hit not only after the backend crash, but also > after checkpointer, bg writer, wal writer, auto vac launcher, logical > repl launcher and so on. It is sort of misleading to the normal users. > With the commit [3] clarifying these processes in master branch [4], > do we also need to modify the doc added by commit [1] in PG master at > least? > > [1] commit cd91de0d17952b5763466cfa663e98318f26d357 > Author: Tomas Vondra > Date: Thu Mar 18 16:05:03 2021 +0100 > > Remove temporary files after backend crash > > [2] PG 14 - > https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-BACKEND > > [3] commit d3014fff4cd4dcaf4b2764d96ad038f3be7413b0 > Author: Alvaro Herrera > Date: Mon Sep 20 12:22:02 2021 -0300 > > Doc: add glossary term for "auxiliary process" > > [4] PG master - https://www.postgresql.org/docs/devel/glossary.html Here's the patch modifying the docs slightly. Please review it. Regards, Bharath Rupireddy. v1-0001-Reword-docs-of-feature-Remove-temporary-files-aft.patch Description: Binary data
Re: Reword docs of feature "Remove temporary files after backend crash"
On Sat, Oct 09, 2021 at 09:18:24PM +0530, Bharath Rupireddy wrote: > On Fri, Oct 8, 2021 at 4:27 PM Bharath Rupireddy > wrote: > > > > Hi, > > > > The commit [1] for the feature "Remove temporary files after backend > > crash" introduced following in the docs: > > + > > +When set to on, which is the default, > > +PostgreSQL will automatically remove > > +temporary files after a backend crash. If disabled, the files will > > be > > +retained and may be used for debugging, for example. Repeated > > crashes > > +may however result in accumulation of useless files. > > + > > > > The term backend means the user sessions (see from the glossary, at > > [2]). It looks like the code introduced by the commit [1] i.e. the > > temp table removal gets hit not only after the backend crash, but also > > after checkpointer, bg writer, wal writer, auto vac launcher, logical > > repl launcher and so on. It is sort of misleading to the normal users. > > With the commit [3] clarifying these processes in master branch [4], > > do we also need to modify the doc added by commit [1] in PG master at > > least? > > > > [1] commit cd91de0d17952b5763466cfa663e98318f26d357 > > Author: Tomas Vondra > > Date: Thu Mar 18 16:05:03 2021 +0100 > > > > Remove temporary files after backend crash > > > > [2] PG 14 - > > https://www.postgresql.org/docs/current/glossary.html#GLOSSARY-BACKEND > > > > [3] commit d3014fff4cd4dcaf4b2764d96ad038f3be7413b0 > > Author: Alvaro Herrera > > Date: Mon Sep 20 12:22:02 2021 -0300 > > > > Doc: add glossary term for "auxiliary process" > > > > [4] PG master - https://www.postgresql.org/docs/devel/glossary.html > > Here's the patch modifying the docs slightly. Please review it. I doubt there's much confusion here - crashes are treated the same. I think the fix would be to say "server crash" rather than backend crash. -- Justin
Re: Reword docs of feature "Remove temporary files after backend crash"
On Sat, Oct 9, 2021 at 9:42 PM Justin Pryzby wrote: > > I doubt there's much confusion here - crashes are treated the same. I think > the fix would be to say "server crash" rather than backend crash. IIUC, the "server crash" includes any backend, auxiliary process, postmaster crash i.e. database cluster/instance crash. The commit cd91de0d1 especially added the temp file cleanup support if any backend or auxiliary process (except syslogger and stats collector) crashes. The temp file cleanup in postmaster crash does exist prior to the commit cd91de0d1. When we add the description about the new GUC introduced by the commit cd91de0d1, let's be clear as to which crash triggers the new temp file cleanup path. Regards, Bharath Rupireddy.
Add planner support function for starts_with()
When starts_with() and the equivalent ^@ operator were added, they
were plugged into the planner in only a rather half-baked way.
Selectivity estimation got taught about the operator, but the
other infrastructure associated with LIKE/regex matching wasn't
updated. This causes these operators to be planned more stupidly
than a functionally-equivalent LIKE/regex pattern [1].
With the (admittedly later) introduction of planner support functions,
it's really quite easy to do better. The attached patch adds a planner
support function for starts_with(), with these benefits:
* A condition such as "textcol ^@ constant" can now use a regular
btree index, not only an SP-GiST index, so long as the index's
collation is C. (This works just like "textcol LIKE 'foo%'".)
* "starts_with(textcol, constant)" can be optimized the same as
"textcol ^@ constant".
I also rejiggered match_pattern_prefix() a bit, with the effect
that fixed-prefix LIKE and regex patterns are now more like
starts_with() in another way: if you apply one to an SPGiST-indexed
column, you'll get an index condition using ^@ rather than two
index conditions with >= and <. That should be more efficient
at runtime, though I didn't try to do any performance testing.
regards, tom lane
[1]
https://www.postgresql.org/message-id/CADT4RqB13KQHOJqqQ%2BWXmYtJrukS2UiFdtfTvT-XA3qYLyB6Cw%40mail.gmail.com
diff --git a/src/backend/utils/adt/like_support.c b/src/backend/utils/adt/like_support.c
index 241e6f0f59..988568825e 100644
--- a/src/backend/utils/adt/like_support.c
+++ b/src/backend/utils/adt/like_support.c
@@ -143,6 +143,14 @@ texticregexeq_support(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(like_regex_support(rawreq, Pattern_Type_Regex_IC));
}
+Datum
+text_starts_with_support(PG_FUNCTION_ARGS)
+{
+ Node *rawreq = (Node *) PG_GETARG_POINTER(0);
+
+ PG_RETURN_POINTER(like_regex_support(rawreq, Pattern_Type_Prefix));
+}
+
/* Common code for the above */
static Node *
like_regex_support(Node *rawreq, Pattern_Type ptype)
@@ -246,6 +254,7 @@ match_pattern_prefix(Node *leftop,
Oid eqopr;
Oid ltopr;
Oid geopr;
+ Oid preopr = InvalidOid;
bool collation_aware;
Expr *expr;
FmgrInfo ltproc;
@@ -302,12 +311,20 @@ match_pattern_prefix(Node *leftop,
switch (ldatatype)
{
case TEXTOID:
- if (opfamily == TEXT_PATTERN_BTREE_FAM_OID ||
-opfamily == TEXT_SPGIST_FAM_OID)
+ if (opfamily == TEXT_PATTERN_BTREE_FAM_OID)
+ {
+eqopr = TextEqualOperator;
+ltopr = TextPatternLessOperator;
+geopr = TextPatternGreaterEqualOperator;
+collation_aware = false;
+ }
+ else if (opfamily == TEXT_SPGIST_FAM_OID)
{
eqopr = TextEqualOperator;
ltopr = TextPatternLessOperator;
geopr = TextPatternGreaterEqualOperator;
+/* This opfamily has direct support for prefixing */
+preopr = TextPrefixOperator;
collation_aware = false;
}
else
@@ -360,20 +377,6 @@ match_pattern_prefix(Node *leftop,
return NIL;
}
- /*
- * If necessary, verify that the index's collation behavior is compatible.
- * For an exact-match case, we don't have to be picky. Otherwise, insist
- * that the index collation be "C". Note that here we are looking at the
- * index's collation, not the expression's collation -- this test is *not*
- * dependent on the LIKE/regex operator's collation.
- */
- if (collation_aware)
- {
- if (!(pstatus == Pattern_Prefix_Exact ||
- lc_collate_is_c(indexcollation)))
- return NIL;
- }
-
/*
* If necessary, coerce the prefix constant to the right type. The given
* prefix constant is either text or bytea type, therefore the only case
@@ -409,8 +412,31 @@ match_pattern_prefix(Node *leftop,
}
/*
- * Otherwise, we have a nonempty required prefix of the values.
- *
+ * Otherwise, we have a nonempty required prefix of the values. Some
+ * opclasses support prefix checks directly, otherwise we'll try to
+ * generate a range constraint.
+ */
+ if (OidIsValid(preopr) && op_in_opfamily(preopr, opfamily))
+ {
+ expr = make_opclause(preopr, BOOLOID, false,
+ (Expr *) leftop, (Expr *) prefix,
+ InvalidOid, indexcollation);
+ result = list_make1(expr);
+ return result;
+ }
+
+ /*
+ * Since we need a range constraint, it's only going to work reliably if
+ * the index is collation-insensitive or has "C" collation. Note that
+ * here we are looking at the index's collation, not the expression's
+ * collation -- this test is *not* dependent on the LIKE/regex operator's
+ * collation.
+ */
+ if (collation_aware &&
+ !lc_collate_is_c(indexcollation))
+ return NIL;
+
+ /*
* We can always say "x >= prefix".
*/
if (!op_in_opfamily(geopr, opfamily))
@@ -1165,7 +1191,6 @@ pattern_fixed_prefix(Const *patt, Pattern_Type ptype, Oid collation,
case Pattern_Type_Prefix:
/* Prefix type work is trivial. */
result = Pattern_Prefix_Partial;
- *rest_selec = 1.0; /* all */
*prefix = makeConst(patt->consttype,
Re: starts_with, ^@ and index usage
I wrote: > Shay Rojansky writes: >> First, starts_with doesn't seem to use SP-GIST indexes, contrary to >> the patch description (and also doesn't trigger a parallel seq scan) - >> is this intentional? > It seems like it didn't occur to anybody to tie starts_with() into > the machinery for derived index operators. That wouldn't be hard, > but it wasn't done. I've started another thread with a patch for that [1]. >> Note that ^@ isn't documented on the string functions and operators, > That's another oversight. Well, "oversight" might be too strong a word. AFAICS from a quick look in pg_operator, most operators on type text are comparisons, pattern match, or text search, none of which do I want to fold into section 9.4. The only exceptions are ||, which we do document there under SQL operators, and ^@. Commit 710d90da1 apparently decided to treat ^@ as a pattern match operator, which I guess it could be if you hold your head at the right angle, but I doubt most people would think to look for it in section 9.7. I guess the most practical answer is to rename table 9.10 from "Other String Functions" to "Other String Functions and Operators", which is more parallel to table 9.9 anyway. Just as in 9.9, it would look weird to have a one-entry table of operators. (Maybe someday in the far future it'd make sense to split 9.10 into two tables.) regards, tom lane [1] https://www.postgresql.org/message-id/232599.1633800229%40sss.pgh.pa.us
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
On Fri, Oct 08, 2021 at 12:03:41PM -0400, Tom Lane wrote: > Daniel Gustafsson writes: > > On 8 Oct 2021, at 06:24, Noah Misch wrote: > >> That's obvious from "cpanm install IPC::Run". Surely if any other non-core > >> module were allowed, the recipe would list it in a similar way. > > > The proposed changes talks about with core modules are allowed to use, I > > think > > that's a different thing. The distinction between core and non-core modules > > may not be known/clear to people who haven't used Perl in the past. > > Yeah, I don't really think that this recipe makes it plain that we have > a policy. It certainly fails to explain that you're allowed to use > additional modules if you're willing to skip the relevant tests. True, +1 for mentioning that tests can use less-available modules if they skip when those modules are absent. I'm only -0 for adding the other English (unlike the -1 for the original proposal of removing the shell commands). > >> If there's something to change, it's improving the actual recipe: > > > That we should do as well. > > You're not going to get far with "improving the recipe", because it's > just not possible. To check this, I installed perlbrew on a Fedora 34 Your test result is evidence that "cpanm install Test::More@0.87" is the wrong shell command, but it's quite a leap to "just not possible". Surely there exist other shell commands that install http://backpan.perl.org/modules/by-authors/id/M/MS/MSCHWERN/Test-Simple-0.87_03.tar.gz. (Perhaps none of us will care enough to identify them, but they exist.) By the way, I suspect 93fb39e introduced a regression in the recipe. (I haven't tested, though.) Before commit 93fb39e, "cpanm install IPC::Run" would update Test::More. As of 5.8.3, the core version of Test::More is new enough for IPC::Run but not new enough for PostgreSQL. I recommend adding "cpanm install Test::More" to restore the pre-93fb39e functionality.
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
Noah Misch writes: > On Fri, Oct 08, 2021 at 12:03:41PM -0400, Tom Lane wrote: >> You're not going to get far with "improving the recipe", because it's >> just not possible. To check this, I installed perlbrew on a Fedora 34 > Your test result is evidence that "cpanm install Test::More@0.87" is the wrong > shell command, but it's quite a leap to "just not possible". Surely there > exist other shell commands that install > http://backpan.perl.org/modules/by-authors/id/M/MS/MSCHWERN/Test-Simple-0.87_03.tar.gz. > (Perhaps none of us will care enough to identify them, but they exist.) Oh, I never heard of backpan before. Now I'm tempted to see how far I can downgrade prairiedog before it breaks ;-). However, I agree that most people won't care about that, and probably shouldn't need to. > By the way, I suspect 93fb39e introduced a regression in the recipe. (I > haven't tested, though.) Before commit 93fb39e, "cpanm install IPC::Run" > would update Test::More. As of 5.8.3, the core version of Test::More is new > enough for IPC::Run but not new enough for PostgreSQL. I recommend adding > "cpanm install Test::More" to restore the pre-93fb39e functionality. Good point. So how about like the attached? regards, tom lane diff --git a/src/test/perl/README b/src/test/perl/README index bfc7cdcfa3..ab986f14bc 100644 --- a/src/test/perl/README +++ b/src/test/perl/README @@ -70,20 +70,33 @@ perldoc for the test modules, e.g.: perldoc src/test/perl/PostgresNode.pm -Required Perl -- +Portability +--- + +Avoid using any bleeding-edge Perl features. We have buildfarm animals +running Perl versions as old as 5.8.3, so your tests will be expected +to pass on that. -Tests must run on perl 5.8.3 and newer. perlbrew is a good way to obtain such -a Perl; see http://perlbrew.pl . +Also, do not use any non-core Perl modules except IPC::Run. Or, if you +must do so for a particular test, arrange to skip the test when the needed +module isn't present. If unsure, you can consult Module::CoreList to find +out whether a given module is part of the Perl core, and which module +versions shipped with which Perl releases. -Just install and +One way to test for compatibility with old Perl versions is to use +perlbrew; see http://perlbrew.pl . After installing that, do perlbrew --force install 5.8.3 perlbrew use 5.8.3 perlbrew install-cpanm +cpanm install Test::More cpanm install IPC::Run -then re-run configure to ensure the correct Perl is used when running -tests. To verify that the right Perl was found: +Then re-run Postgres' configure to ensure the correct Perl is used when +running tests. To verify that the right Perl was found: grep ^PERL= config.log + +(Note: this recipe does not create a perfect reproduction of a +back-in-the-day Perl installation, because it will update Test::More +and IPC::Run to current versions. In most cases that won't matter.)
ERROR: unexpected duplicate for tablespace 16389, relfilenode 484036
Hi, we have a replica product that access to wal files with logical replication. After a reboot following a database fault we receive the following issue. 2021-10-08 16:07:31.829 CEST:127.0.0.1(49880):cdcadm@REPLICA:[12976]: ERROR: unexpected duplicate for tablespace 16389, relfilenode 484036 But the object is not duplicated: SELECT pg_filenode_relation (16389,484036); pg_filenode_relation | | mobile.vw_cella_site | Thanks for the support Massimo
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
On Sat, Oct 09, 2021 at 03:44:17PM -0400, Tom Lane wrote: > > By the way, I suspect 93fb39e introduced a regression in the recipe. (I > > haven't tested, though.) Before commit 93fb39e, "cpanm install IPC::Run" > > would update Test::More. As of 5.8.3, the core version of Test::More is new > > enough for IPC::Run but not new enough for PostgreSQL. I recommend adding > > "cpanm install Test::More" to restore the pre-93fb39e functionality. > > Good point. So how about like the attached? Fine with me.
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
Hah ... your backpan link led me to realize the actual problem with Test::More. It got folded into Test::Simple at some point, and evidently cpanm isn't smart enough to handle a request for a back version in such cases. But this works: $ cpanm install Test::Simple@0.87_01 ... $ perl -MTest::More -e 'print $Test::More::VERSION, "\n";' 0.8701 So we oughta recommend that instead. Now I'm wondering what version of IPC::Run to recommend. regards, tom lane
ldap/t/001_auth.pl fails with openldap 2.5
Hi,
The freebsd image I use for CI runs just failed because the package name for
openldap changed (it's now either openldap{24,25}-{client,server}, instead of
openldap-..}. I naively resolved that conflict by choosing the openldap25-*
packages. Which unfortunately turns out to break 001_auth.pl :(
https://api.cirrus-ci.com/v1/artifact/task/5061394509856768/tap/src/test/ldap/tmp_check/log/regress_log_001_auth
# Running: ldapsearch -h localhost -p 51649 -s base -b dc=example,dc=net -D
cn=Manager,dc=example,dc=net -y
/tmp/cirrus-ci-build/src/test/ldap/tmp_check/ldappassword -n 'objectclass=*'
ldapsearch: unrecognized option -h
usage: ldapsearch [options] [filter [attributes...]]
Seems we need to replace -h & -p with a -H ldap://server:port/ style URI? I
think that's fine to do unconditionally, the -H schema is pretty old I think
(I seem to recall using it in the mid 2000s, when I learned to not like ldap
by experience).
The only reason I'm hesitating a bit is that f0e60ee4bc0, the commit adding
the ldap test suite, used an ldap:// uri for the server, but then 27cd521e6e7
(adding the ldapsearch) didn't use that for the ldapsearch? Thomas?
So, does anybody see a reason not to go for the trivial
diff --git i/src/test/ldap/t/001_auth.pl w/src/test/ldap/t/001_auth.pl
index f670bc5e0d5..a025a641b02 100644
--- i/src/test/ldap/t/001_auth.pl
+++ w/src/test/ldap/t/001_auth.pl
@@ -130,8 +130,8 @@ while (1)
last
if (
system_log(
-"ldapsearch", "-h", $ldap_server, "-p",
-$ldap_port, "-s", "base", "-b",
+"ldapsearch", "-H", "$ldap_url", "-s",
+"base", "-b",
$ldap_basedn, "-D", $ldap_rootdn, "-y",
$ldap_pwfile, "-n", "'objectclass=*'") == 0);
die "cannot connect to slapd" if ++$retries >= 300;
Although I'm mildly tempted to rewrap the parameters, it's kinda odd how the
trailing parameter on one line, has its value on the next line.
Greetings,
Andres Freund
Re: GIN pending list cleanup during autoanalyze blocks cleanup by VACUUM
On Thu, Oct 7, 2021 at 10:34 PM Peter Geoghegan wrote: > This issue was brought to my attention by Nikolay Samokhvalov. He > reached out privately about it. He mentioned one problematic case > involving an ANALYZE lasting 45 minutes, or longer (per > log_autovacuum_min_duration output for the autoanalyze). That was > correlated with VACUUMs on other tables whose OldestXmin values were > all held back to the same old XID. I think that this issue ought to be > treated as a bug. It's hard to think of a non-invasive bug fix. The obvious approach is to move the index_vacuum_cleanup()/ginvacuumcleanup() calls in analyze.c to some earlier point in ANALYZE, in order to avoid doing lots of VACUUM-ish work while we hold an MVCC snapshot that blocks cleanup in other tables. The code in question is more or less supposed to be run during VACUUM already, and so the idea of moving it back to when the autoanalyze worker backend state "still looks like the usual autovacuum case" makes a certain amount of sense. But that doesn't work, at least not without lots of invasive changes. While I'm no closer to a backpatchable fix than I was on Thursday, I do have some more ideas about what to do on HEAD. I now lean towards completely ripping analyze_only calls out, there -- the whole idea of calling amvacuumcleanup() routines during autoanalyze (but not plain ANALYZE) seems bolted on. It's not just the risk of similar problems cropping up in the future -- it's that the whole approach seems obsolete. We now generally expect autovacuum to run against insert-only tables. That might not be a perfect fit for this, but it still seems far better. Does anyone have any ideas for a targeted fix? Here's why the "obvious" fix is impractical, at least for backpatch: To recap, a backend running VACUUM is generally able to avoid the need to be considered inside GetOldestNonRemovableTransactionId(), which is practically essential for any busy database -- without that, long running VACUUM operations would behave like conventional long running transactions, causing all sorts of chaos. The problem here is that we can have ginvacuumcleanup() calls that take place without avoiding the same kind of chaos, just because they happen to take place during autoanalyze. It seems like the whole GIN autoanalyze mechanism design was based on the assumption that it didn't make much difference *when* we reach ginvacuumcleanup(), as long as it happened regularly. But that's just not true. We go to a lot of trouble to make VACUUM have this property. This cannot easily be extended or generalized to cover this special case during ANALYZE. For one thing, the high level vacuum_rel() entry point sets things up carefully, using session-level locks for relations. For another, it relies on special PROC_IN_VACUUM flag logic -- that status is stored in MyProc->statusFlags. -- Peter Geoghegan
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
On Sat, Oct 09, 2021 at 04:34:46PM -0400, Tom Lane wrote: > Hah ... your backpan link led me to realize the actual problem with > Test::More. It got folded into Test::Simple at some point, and > evidently cpanm isn't smart enough to handle a request for a back > version in such cases. But this works: > > $ cpanm install Test::Simple@0.87_01 > ... > $ perl -MTest::More -e 'print $Test::More::VERSION, "\n";' > 0.8701 > > So we oughta recommend that instead. Now I'm wondering what > version of IPC::Run to recommend. You mentioned prairiedog uses IPC::Run 0.79. That's from 2005. (Perl 5.8.3 is from 2004, and Test::More 0.87 is from 2009.) I'd just use 0.79 in the README recipe. IPC::Run is easy to upgrade, so if we find cause to rely on a newer version, I'd be fine updating that requirement.
Re: Reword docs of feature "Remove temporary files after backend crash"
On 2021/10/10 1:25, Bharath Rupireddy wrote: On Sat, Oct 9, 2021 at 9:42 PM Justin Pryzby wrote: I doubt there's much confusion here - crashes are treated the same. I think the fix would be to say "server crash" rather than backend crash. IIUC, the "server crash" includes any backend, auxiliary process, postmaster crash i.e. database cluster/instance crash. The commit cd91de0d1 especially added the temp file cleanup support if any backend or auxiliary process (except syslogger and stats collector) Also the startup process should be in this exception list? crashes. The temp file cleanup in postmaster crash does exist prior to the commit cd91de0d1. When we add the description about the new GUC introduced by the commit cd91de0d1, let's be clear as to which crash triggers the new temp file cleanup path. If we really want to add this information, the description of restart_after_crash seems more proper place to do that in. remove_temp_files_after_crash works only when the processes are reinitialized because restart_after_crash is enabled. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: ldap/t/001_auth.pl fails with openldap 2.5
Andres Freund writes: > So, does anybody see a reason not to go for the trivial > [ patch ] I'd be happy to rely on the buildfarm's opinion here. > Although I'm mildly tempted to rewrap the parameters, it's kinda odd how the > trailing parameter on one line, has its value on the next line. I'm betting that perltidy did that. If you want to fix it so it stays fixed, maybe reordering the parameters could help. regards, tom lane
