Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
On Fri, Apr 19, 2024 at 12:47:43PM +0300, Aleksander Alekseev wrote: > Thanks. I see a few pieces of code that use special FOO_NUMBER enum > values instead of a macro. Should we refactor these pieces > accordingly? PFA another patch. I don't see why not for the places you are changing here, we can be more consistent. Now, such changes are material for v18. -- Michael signature.asc Description: PGP signature
Re: POC: GROUP BY optimization
On 4/12/24 06:44, Tom Lane wrote: If this patch were producing better results I'd be more excited about putting more work into it. But on the basis of what I'm seeing right now, I think maybe we ought to give up on it. Let me show current cases where users have a profit with this tiny improvement (see queries and execution results in query.sql): 1. 'Not optimised query text' — when we didn't consider group-by ordering during database development. 2. 'Accidental pathkeys' - we didn't see any explicit orderings, but accidentally, the planner used merge join that caused some orderings and we can utilise it. 3. 'Uncertain scan path' — We have options regarding which index to use, and because of that, we can't predict the optimal group-by ordering before the start of query planning. 4. 'HashAgg V/S GroupAgg' — sometimes, the GroupAgg strategy outperforms HashAgg just because we don't need any ordering procedure at all. And the last thing here — this code introduces the basics needed to add more sophisticated strategies, like ordering according to uniqueness or preferring to set constant-width columns first in grouping. -- regards, Andrei Lepikhov Postgres Professional query.sql Description: application/sql
Re: Assert failure in _bt_preprocess_array_keys
On Sun, Apr 21, 2024 at 10:36 PM Richard Guo wrote: > I didn't spend much time digging into it, but I wonder if this Assert is > sensible. I noticed that before commit 5bf748b86b, the two datatypes > were not equal to each other either (anyrange vs. int4range). The assertion is wrong. It is testing behavior that's much older than commit 5bf748b86b, though. We can just get rid of it, since all of the information that we'll actually apply when preprocessing scan keys comes from the operator class. Pushed a fix removing the assertion just now. Thanks for the report. -- Peter Geoghegan
Assert failure in _bt_preprocess_array_keys
I came across an assert failure in _bt_preprocess_array_keys regarding
the sanity check on the datatype of the array elements. It can be
reproduced with the query below.
create table t (c int4range);
create unique index on t (c);
select * from t where c in ('(1, 100]'::int4range, '(50, 300]'::int4range);
It fails on this Assert:
+ elemtype = cur->sk_subtype;
+ if (elemtype == InvalidOid)
+ elemtype = rel->rd_opcintype[cur->sk_attno - 1];
+ Assert(elemtype == ARR_ELEMTYPE(arrayval));
... which was introduced in 5bf748b86b.
I didn't spend much time digging into it, but I wonder if this Assert is
sensible. I noticed that before commit 5bf748b86b, the two datatypes
were not equal to each other either (anyrange vs. int4range).
Thanks
Richard
Re: pg_combinebackup does not detect missing files
On 4/20/24 01:47, Robert Haas wrote: On Thu, Apr 18, 2024 at 7:36 PM David Steele wrote: Sure -- pg_combinebackup would only need to verify the data that it uses. I'm not suggesting that it should do an exhaustive verify of every single backup in the chain. Though I can see how it sounded that way since with pg_verifybackup that would pretty much be your only choice. The beauty of doing verification in pg_combinebackup is that it can do a lot less than running pg_verifybackup against every backup but still get a valid result. All we care about is that the output is correct -- if there is corruption in an unused part of an earlier backup pg_combinebackup doesn't need to care about that. As far as I can see, pg_combinebackup already checks most of the boxes. The only thing I know that it can't do is detect missing files and that doesn't seem like too big a thing to handle. Hmm, that's an interesting perspective. I've always been very skeptical of doing verification only around missing files and not anything else. Yeah, me too. There should also be some verification of the file contents themselves but now I can see we don't have that. For instance, I can do something like this: cp test/backup/incr1/base/1/3598 test/backup/incr1/base/1/2336 And pg_combinebackup runs without complaint. Maybe missing files are more likely than corrupted files, but it would still be nice to check for both. I figured that wouldn't be particularly meaningful, and that's pretty much the only kind of validation that's even theoretically possible without a bunch of extra overhead, since we compute checksums on entire files rather than, say, individual blocks. And you could really only do it for the final backup in the chain, because you should end up accessing all of those files, but the same is not true for the predecessor backups. So it's a very weak form of verification. I don't think it is weak if you can verify that the output is exactly as expected, i.e. all files are present and have the correct contents. But in this case it would be nice to at least know that the source files on disk are valid (which pg_verifybackup does). Without block checksums it is hard to know if the final output is correct or not. But I looked into it and I think you're correct that, if you restrict the scope in the way that you suggest, we can do it without much additional code, or much additional run-time. The cost is basically that, instead of only looking for a backup_manifest entry when we think we can reuse its checksum, we need to do a lookup for every single file in the final input directory. Then, after processing all such files, we need to iterate over the hash table one more time and see what files were never touched. That seems like an acceptably low cost to me. So, here's a patch. I tested the patch and it works, but there is some noise from WAL files since they are not in the manifest: $ pg_combinebackup test/backup/full test/backup/incr1 -o test/backup/combine pg_combinebackup: warning: "pg_wal/00010008" is present on disk but not in the manifest pg_combinebackup: error: "base/1/3596" is present in the manifest but not on disk Maybe it would be better to omit this warning since it could get very noisy depending on the number of WAL segments generated during backup. Though I do find it a bit odd that WAL files are not in the source backup manifests but do end up in the manifest after a pg_combinebackup. It doesn't seem harmful, just odd. I do think there's some chance that this will encourage people to believe that pg_combinebackup is better at finding problems than it really is or ever will be, Given that pg_combinebackup is not verifying checksums, you are probably right. and I also question whether it's right to keep changing stuff after feature freeze. But I have a feeling most people here are going to think this is worth including in 17. Let's see what others say. I think it is a worthwhile change and we are still a month away from beta1. We'll see if anyone disagrees. Regards, -David
RE: promotion related handling in pg_sync_replication_slots()
On Friday, April 19, 2024 4:22 PM shveta malik wrote: > On Fri, Apr 19, 2024 at 11:37 AM shveta malik wrote: > > > > On Fri, Apr 19, 2024 at 10:53 AM Bertrand Drouvot > > wrote: > > > > > > Hi, > > > > > > On Thu, Apr 18, 2024 at 05:36:05PM +0530, shveta malik wrote: > > > > Please find v8 attached. Changes are: > > > > > > Thanks! > > > > > > A few comments: > > > > Thanks for reviewing. > > > > > 1 === > > > > > > @@ -1440,7 +1461,7 @@ ReplSlotSyncWorkerMain(char *startup_data, > size_t startup_data_len) > > > * slotsync_worker_onexit() but that will need the connection to > > > be > made > > > * global and we want to avoid introducing global for this > > > purpose. > > > */ > > > - before_shmem_exit(slotsync_failure_callback, > PointerGetDatum(wrconn)); > > > + before_shmem_exit(slotsync_worker_disconnect, > > > + PointerGetDatum(wrconn)); > > > > > > The comment above this change still states "Register the failure > > > callback once we have the connection", I think it has to be reworded > > > a bit now that v8 is making use of slotsync_worker_disconnect(). > > > > > > 2 === > > > > > > +* Register slotsync_worker_onexit() before we register > > > +* ReplicationSlotShmemExit() in BaseInit(), to ensure that during > exit of > > > +* slot sync worker, ReplicationSlotShmemExit() is called first, > followed > > > +* by slotsync_worker_onexit(). Startup process during > > > + promotion waits for > > > > > > Worth to mention in shmem_exit() (where it "while > (--before_shmem_exit_index >= 0)" > > > or before the shmem_exit() definition) that ReplSlotSyncWorkerMain() > > > relies on this LIFO behavior? (not sure if there is other "strong" > > > LIFO requirement in other part of the code). > > > > I see other modules as well relying on LIFO behavior. > > Please see applyparallelworker.c where > > 'before_shmem_exit(pa_shutdown)' is needed to be done after > > 'before_shmem_exit(logicalrep_worker_onexit)' (commit id 3d144c6). > > Also in postinit.c, I see such comments atop > > 'before_shmem_exit(ShutdownPostgres, 0)'. > > I feel we can skip adding this specific comment about > > ReplSlotSyncWorkerMain() in ipc.c, as none of the other modules has > > also not added any. I will address the rest of your comments in the > > next version. > > > > > 3 === > > > > > > +* Startup process during promotion waits for slot sync to finish > and it > > > +* does that by checking the 'syncing' flag. > > > > > > worth to mention ShutDownSlotSync()? > > > > > > 4 === > > > > > > I did a few tests manually (launching ShutDownSlotSync() through gdb > > > / with and without sync worker and with / without > > > pg_sync_replication_slots() running > > > concurrently) and it looks like it works as designed. > > > > Thanks for testing it. > > > > > Having said that, the logic that is in place to take care of the > > > corner cases described up-thread seems reasonable to me. > > Please find v9 with the above comments addressed. Thanks, the patch looks good to me. I also tested a few concurrent promotion/function execution cases and didn't find issues. Best Regards, Hou zj
RE: Disallow changing slot's failover option in transaction block
On Friday, April 19, 2024 10:54 AM Kuroda, Hayato/黒田 隼人 wrote: > In your patch, the pg_dump.c was updated. IIUC the main reason was that > pg_restore executes some queries as single transactions so that ALTER > SUBSCRIPTION cannot be done, right? Yes, and please see below for other reasons. > Also, failover was synchronized only when we were in the upgrade mode, but > your patch seems to remove the condition. Can you clarify the reason? We used ALTER SUBSCRIPTION in upgrade mode because it was not allowed to use connect=false and failover=true together when CREATE SUBSCRIPTION. But since we don't have this restriction anymore(we don't alter slot when creating sub anymore), we can directly specify failover in CREATE SUBSCRIPTION and do that in non-upgrade mode as well. Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments. [1] https://www.postgresql.org/message-id/CAJpy0uD3YOeDg-tTCUi3EZ8vznRDfDqO_k6LepJpXUV1Z_%3DgkA%40mail.gmail.com [2] https://www.postgresql.org/message-id/ZiIxuaiINsuaWuDK%40ip-10-97-1-34.eu-west-3.compute.internal Best Regards, Hou zj v3-0001-Fix-the-handling-of-failover-option-in-subscripti.patch Description: v3-0001-Fix-the-handling-of-failover-option-in-subscripti.patch
Re: allow changing autovacuum_max_workers without restarting
> Part of the underlying problem here is that, AFAIK, neither PostgreSQL > as a piece of software nor we as human beings who operate PostgreSQL > databases have much understanding of how autovacuum_max_workers should > be set. It's relatively easy to hose yourself by raising > autovacuum_max_workers to try to make things go faster, but produce > the exact opposite effect due to how the cost balancing stuff works. Yeah, this patch will not fix this problem. Anyone who raises av_max_workers should think about adjusting the av_cost_delay. This was discussed up the thread [4] and even without this patch, I think it's necessary to add more documentation on the relationship between workers and cost. > So I feel like what this proposal reveals is that we know that our > algorithm for ramping up the number of running workers doesn't really > work. And maybe that's just a consequence of the general problem that > we have no global information about how much vacuuming work there is > to be done at any given time, and therefore we cannot take any kind of > sensible guess about whether 1 more worker will help or hurt. Or, > maybe there's some way to do better than what we do today without a > big rewrite. I'm not sure. I don't think this patch should be burdened > with solving the general problem here. But I do think the general > problem is worth some discussion. This patch is only solving the operational problem of adjusting autovacuum_max_workers, and it does so without introducing complexity. A proposal that will alleviate the users from the burden of having to think about autovacuum_max_workers, cost_delay and cost_limit settings will be great. This patch may be the basis for such dynamic "auto-tuning" of autovacuum workers. Regards, Sami [4] https://www.postgresql.org/message-id/20240419154322.GA3988554%40nathanxps13
Re: fix tablespace handling in pg_combinebackup
On Mon, Apr 22, 2024 at 12:00 AM Alexander Lakhin wrote:
> From what I can see, the following condition (namely, -l):
> if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path")
> {
> push @tsoids, $1;
> return 0;
> }
>
> is false for junction points on Windows (cf [1]), but the target path is:
Ah, yes, right, -l doesn't like junction points. Well, we're already
using the Win32API::File package (it looks like a CPAN library, but I
guess the Windows perl distros like Strawberry are all including it
for free?). See PostgreSQL::Test::Utils::is_symlink(), attached.
That seems to work as expected, but someone who actually knows perl
can surely make it better. Then I hit the next problem:
readlink
C:\cirrus\build/testrun/pg_combinebackup/002_compare_backups\data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/16415:
Inappropriate I/O control operation at
C:/cirrus/src/test/perl/PostgreSQL/Test/Cluster.pm line 927.
https://cirrus-ci.com/task/5162332353986560
I don't know where exactly that error message is coming from, but
assuming that Strawberry Perl contains this code:
https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L2041
https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L1976
... then it's *very* similar to what we're doing in our own
pgreadlink() code. I wondered if the buffer size might be too small
for our path, but it doesn't seem so:
https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L1581C1-L1581C35
(I think MAX_PATH is 256 on Windows.)
If there is some unfixable problem with what they're doing in their
readlink(), then I guess it should be possible to read the junction
point directly in Perl using Win32API::File::DeviceIoControl()... but
I can't see what's wrong with it! Maybe it's not the same code?
Attached are the new test support functions, and the fixup to Robert's
6bf5c42b that uses them. To be clear, this doesn't work, yet. It has
got to be close though...
0001-More-Windows-pseudo-symlink-support-for-Perl-code.patch
Description: Binary data
0002-fixup.patch
Description: Binary data
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Daniel Gustafsson writes: >> On 6 Apr 2024, at 01:10, Tom Lane wrote: >> (So now I'm wondering why *only* copperhead has shown this so far. >> Are our other cross-version-upgrade testing animals AWOL?) > Clicking around searching for Xversion animals I didn't spot any, but without > access to the database it's nontrivial to know which animal does what. I believe I see why this is (or isn't) happening. The animals currently running xversion tests are copperhead, crake, drongo, and fairywren. copperhead is using the makefiles while the others are using meson. And I find this in src/test/modules/test_pg_dump/meson.build (from 3f0e786cc): # doesn't delete its user 'runningcheck': false, So the meson animals are not running the test that sets up the problematic data. I think we should remove the above, since (a) the reason to have it is gone, and (b) it seems really bad that the set of tests run by meson is different from that run by the makefiles. However, once we do that, those other three animals will presumably go red, greatly complicating detection of any Windows-specific problems. So I'm inclined to not do it till just before we intend to commit a fix for the underlying problem. (Enough before that we can confirm that they do go red.) Speaking of which ... >> I doubt this is something we'll have fixed by Monday, so I will >> go add an open item for it. > +1. Having opened the can of worms I'll have a look at it next week. ... were you going to look at it? I can take a whack if it's too far down your priority list. regards, tom lane
Re: createdb compares strategy as case-sensitive
On 4/21/24 17:10, Tom Lane wrote: > Tomas Vondra writes: >> On 4/21/24 00:19, Tom Lane wrote: >>> I'm not suggesting that this is an interesting security vulnerability, >>> because if you can control the arguments to createdb it's probably >>> game over long since. But wrapping the arguments is good for >>> delivering on-point error messages. So I'd add a fmtId() call to >>> LOCALE_PROVIDER too. > >> OK, the attached 0001 patch does these three things - adds the fmtId() >> for locale_provider, make the comparison case-insensitive for strategy >> and also removes the comma from the hint. > > LGTM. > Pushed, after tweaking the commit message a bit. >> The createdb vs. CREATE DATABASE difference made me look if we have any >> regression tests for CREATE DATABASE, and we don't. I guess it would be >> good to have some, so I added a couple, for some of the parameters, see >> 0002. But there's a problem with the locale stuff - this seems to work >> in plain "make check", but pg_upgrade creates the clusters with >> different providers etc. which changes the expected output. I'm not sure >> there's a good way to deal with this ... > > Probably not worth the trouble, really. > Agreed. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: createdb compares strategy as case-sensitive
Tomas Vondra writes: > On 4/21/24 00:19, Tom Lane wrote: >> I'm not suggesting that this is an interesting security vulnerability, >> because if you can control the arguments to createdb it's probably >> game over long since. But wrapping the arguments is good for >> delivering on-point error messages. So I'd add a fmtId() call to >> LOCALE_PROVIDER too. > OK, the attached 0001 patch does these three things - adds the fmtId() > for locale_provider, make the comparison case-insensitive for strategy > and also removes the comma from the hint. LGTM. > The createdb vs. CREATE DATABASE difference made me look if we have any > regression tests for CREATE DATABASE, and we don't. I guess it would be > good to have some, so I added a couple, for some of the parameters, see > 0002. But there's a problem with the locale stuff - this seems to work > in plain "make check", but pg_upgrade creates the clusters with > different providers etc. which changes the expected output. I'm not sure > there's a good way to deal with this ... Probably not worth the trouble, really. regards, tom lane
Re: createdb compares strategy as case-sensitive
On 4/21/24 00:19, Tom Lane wrote:
> Tomas Vondra writes:
>> On 4/20/24 22:40, Tom Lane wrote:
>>> Seems reasonable. The alternative could be to remove createdb.c's use
>>> of fmtId() here, but I don't think that's actually better.
>
>> Why? It seems to me this is quite close to e.g. LOCALE_PROVIDER, and we
>> don't do fmtId() for that. So why should we do that for STRATEGY?
>
> Hah, nothing like being randomly inconsistent with adjacent code.
> Every other argument handled by createdb gets wrapped by either
> fmtId or appendStringLiteralConn.
>
> I think the argument for this is it ensures that the switch value as
> accepted by createdb is the argument that CREATE DATABASE will see.
> Compare
>
> $ createdb --strategy="foo bar" mydb
> createdb: error: database creation failed: ERROR: invalid create database
> strategy "foo bar"
> HINT: Valid strategies are "wal_log", and "file_copy".
>
> $ createdb --locale-provider="foo bar" mydb
> createdb: error: database creation failed: ERROR: syntax error at or near ";"
> LINE 1: CREATE DATABASE mydb LOCALE_PROVIDER foo bar;
> ^
>
> I'm not suggesting that this is an interesting security vulnerability,
> because if you can control the arguments to createdb it's probably
> game over long since. But wrapping the arguments is good for
> delivering on-point error messages. So I'd add a fmtId() call to
> LOCALE_PROVIDER too.
>
> BTW, someone's taken the Oxford comma too far in that HINT.
> Nobody writes a comma when there are only two alternatives.
>
OK, the attached 0001 patch does these three things - adds the fmtId()
for locale_provider, make the comparison case-insensitive for strategy
and also removes the comma from the hint.
The createdb vs. CREATE DATABASE difference made me look if we have any
regression tests for CREATE DATABASE, and we don't. I guess it would be
good to have some, so I added a couple, for some of the parameters, see
0002. But there's a problem with the locale stuff - this seems to work
in plain "make check", but pg_upgrade creates the clusters with
different providers etc. which changes the expected output. I'm not sure
there's a good way to deal with this ...
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom e7a18c780591673592b2a5eb1e129fcceb17f0fe Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sun, 21 Apr 2024 14:19:32 +0200
Subject: [PATCH 1/2] createdb: compare strategy case-insensitive
When specifying the createdb strategy, the documentation suggests valid
options are FILE_COPY and WAL_LOG, but the code does case-sensitive
comparison and accepts only "file_copy" and "wal_log" as valid.
Fixed by doing a case-insensitive comparison using pg_strcasecmp(), same
as for other string parameters.
While at it, apply fmtId() to a nearby "locale_provider". This already
did the comparison in case-insensitive way, but the value would not be
double-quoted, confusing the parser and the error message.
Backpatch to 15, where the strategy was introduced.
Backpatch-through: 15
---
src/backend/commands/dbcommands.c | 6 +++---
src/bin/scripts/createdb.c| 2 +-
src/bin/scripts/t/020_createdb.pl | 10 ++
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 8229dfa1f22..cd06d1270c5 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1018,15 +1018,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
char *strategy;
strategy = defGetString(dstrategy);
- if (strcmp(strategy, "wal_log") == 0)
+ if (pg_strcasecmp(strategy, "wal_log") == 0)
dbstrategy = CREATEDB_WAL_LOG;
- else if (strcmp(strategy, "file_copy") == 0)
+ else if (pg_strcasecmp(strategy, "file_copy") == 0)
dbstrategy = CREATEDB_FILE_COPY;
else
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid create database strategy \"%s\"", strategy),
- errhint("Valid strategies are \"wal_log\", and \"file_copy\".")));
+ errhint("Valid strategies are \"wal_log\" and \"file_copy\".")));
}
/* If encoding or locales are defaulted, use source's setting */
diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c
index 007061e756f..30426860efa 100644
--- a/src/bin/scripts/createdb.c
+++ b/src/bin/scripts/createdb.c
@@ -237,7 +237,7 @@ main(int argc, char *argv[])
appendStringLiteralConn(, lc_ctype, conn);
}
if (locale_provider)
- appendPQExpBuffer(, " LOCALE_PROVIDER %s", locale_provider);
+ appendPQExpBuffer(, " LOCALE_PROVIDER %s", fmtId(locale_provider));
if (icu_locale)
{
appendPQExpBufferStr(, " ICU_LOCALE ");
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index 512b55c48a9..4a0e2c883a1 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -255,11 +255,21
Re: fix tablespace handling in pg_combinebackup
Hello Thomas and Robert,
20.04.2024 08:56, Thomas Munro wrote:
... it still broke[4]. So I'm not sure what's going on...
From what I can see, the following condition (namely, -l):
if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path")
{
push @tsoids, $1;
return 0;
}
is false for junction points on Windows (cf [1]), but the target path is:
Directory of
C:\src\postgresql\build\testrun\pg_combinebackup\002_compare_backups\data\t_002_compare_backups_primary_data\backup\backup1\pg_tblspc
04/21/2024 02:05 PM .
04/21/2024 02:05 PM ..
04/21/2024 02:05 PM 16415
[\??\C:\Users\ADMINI~1\AppData\Local\Temp\xXMfNDMCot\ts1backup]
[1] https://www.perlmonks.org/?node_id=1223819
Best regards,
Alexander
Re: Support a wildcard in backtrace_functions
On Sat, 20 Apr 2024 at 01:19, Michael Paquier wrote: > Removing this GUC and making the backend react by default the same way > as when this GUC was enabled sounds like a sensible route here. This > stuff is useful. I definitely agree it's useful. But I feel like changing the default of the GUC and removing the ability to disable it at the same time are pretty radical changes that we should not be doing after a feature freeze. I think we should at least have a way to turn this feature off to be able to avoid log spam. Especially given the fact that extensions use elog much more freely than core. Which afaict from other threads[1] Tom also thinks we should normally be careful about. Of the options to resolve the open item so far, I think there are only three somewhat reasonable to do after the feature freeze: 1. Rename the GUC to something else (but keep behaviour the same) 2. Decide to keep the GUC as is 3. Revert a740b213d4 I hoped 1 was possible, but based on the discussion so far it doesn't seem like we'll be able to get a quick agreement on a name. IMHO 2 is just a version of 1, but with a GUC name that no-one thinks is a good one. So I think we are left with option 3. [1]: https://www.postgresql.org/message-id/flat/524751.1707240550%40sss.pgh.pa.us#59710fd4f3f186e642b8e6b886b2fdff
Re: Support a wildcard in backtrace_functions
On 19.04.24 21:24, Tom Lane wrote: But on the other hand, in my personal experience, backtrace_on_internal_error would be the right thing in a really large number of cases. That's why I thought we could get away with doing it automatically. Sure, more control would be better. But if we just hard-wire it for the moment then we aren't locking in what the eventual design for that control will be. Note that a standard test run produces a number of internal errors. I haven't checked how likely these are in production, but one might want to consider that before starting to dump backtraces in routine situations. For example, $ PG_TEST_INITDB_EXTRA_OPTS='-c backtrace_on_internal_error=on' meson test -C build $ grep -r 'BACKTRACE:' build/testrun | wc -l 85
