Re: [PATCH] Fix build with LLVM 15 or above
On Mon, Oct 3, 2022 at 4:56 PM Po-Chuan Hsieh wrote: > While building PostgreSQL 15 RC 1 with LLVM 15, I got a build failure as > follows: > > cc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement > -Werror=vla -Werror=unguarded-availability-new -Wendif-labels > -Wmissing-format-attribute -Wcast-function-type -Wformat-security > -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument > -Wno-compound-token-split-by-macro -O2 -pipe -O3 -funroll-loops > -fstack-protector-strong -fno-strict-aliasing -Wno-deprecated-declarations > -fPIC -DPIC -D__STDC_LIMIT_MACROS -D__STDC_FORMAT_MACROS > -D__STDC_CONSTANT_MACROS -I/usr/local/llvm15/include > -I../../../../src/include -I/usr/local/include -I/usr/local/include > -I/usr/local/include/libxml2 -I/usr/local/include -I/usr/local/include > -I/usr/local/include -I/usr/local/include -c -o llvmjit.o llvmjit.c > llvmjit.c:1115:50: error: use of undeclared identifier 'LLVMJITCSymbolMapPair' > LLVMOrcCSymbolMapPairs symbols = > palloc0(sizeof(LLVMJITCSymbolMapPair) * LookupSetSize); > ^ > llvmjit.c:1233:81: error: too few arguments to function call, expected 3, > have 2 > ref_gen = > LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL); > ~~ > ^ > /usr/local/llvm15/include/llvm-c/Orc.h:997:31: note: > 'LLVMOrcCreateCustomCAPIDefinitionGenerator' declared here > LLVMOrcDefinitionGeneratorRef LLVMOrcCreateCustomCAPIDefinitionGenerator( > ^ > 2 errors generated. > gmake: *** [: llvmjit.o] Error 1 > *** Error code 2 > > I've prepared a patch (attached) to fix the build issue with LLVM 15 or > above. It is also available at > https://people.FreeBSD.org/~sunpoet/patch/postgres/0001-Fix-build-with-LLVM-15-or-above.patch Hi, Unfortunately that is only the tip of a mini iceberg. While that change makes it compile, there are other API changes that are required to make our use of LLVM ORC actually work. We can't get through 'make check', because various code paths in LLVM 15 abort, because we're using a bunch of APIs from before the big change to "opaque pointers" https://llvm.org/docs/OpaquePointers.html. I've been trying to get to a patch to fix that -- basically a few simple-looking changes like LLVMBuildLoad() to LLVMBuildLoad2() as described there -- that gain an argument where you have to tell it the type of the pointer (whereas before it knew the type of pointers automatically). Unfortunately I had to work on other problems that came up recently and it's probably going to be at least a week before I can get back to this and post a patch. One option I thought about as a stopgap measure is to use LLVMContextSetOpaquePointers(context, false) to turn the new code paths off, but it doesn't seem to work for me and I couldn't figure out why yet (it still aborts -- probably there are more 'contexts' around that I didn't handle, something like that). That option is available for LLVM 15 but will be taken out in LLVM 16, so that's supposed to be the last chance to stop using pre-opaque pointers; see the bottom of the page I linked above for that, where they call it setOpaquePointers(false) (the C++ version of LLVMContextSetOpaquePointers()). I don't really want to go with that if we can avoid it, though, because it says "Opaque pointers are enabled by default. Typed pointers are still available, but only supported on a best-effort basis and may be untested" so I expect it to be blighted with problems. Here's my attempt at that minimal change, which is apparently still missing something (if you can get this to build and pass all tests against LLVM 15 then it might still be interesting to know about): https://github.com/macdice/postgres/tree/llvm15-min Here's my WIP unfinished branch where I'm trying to get the real code change done. It needs more work on function pointer types, which are a bit tedious to deal with and I haven't got it all right in here yet as you can see from failures if you build against 15: https://github.com/macdice/postgres/tree/llvm15 Hopefully more next week...
Re: proposal: possibility to read dumped table's name from file
Hi, On Mon, Oct 03, 2022 at 06:00:12AM +0200, Pavel Stehule wrote: > ne 2. 10. 2022 v 22:52 odesílatel Daniel Gustafsson > napsal: > > > > On 2 Oct 2022, at 18:04, Andres Freund wrote: > > > On 2022-10-02 00:19:59 -0700, Andres Freund wrote: > > >> On 2022-10-01 23:56:59 -0700, Andres Freund wrote: > > >>> On 2022-09-12 09:58:37 +0200, Daniel Gustafsson wrote: > > Correct, fixed in the attached. > > >>> > > >>> Updated patch adding meson compatibility attached. > > >> > > >> Err, forgot to amend one hunk :( > > > > > > That fixed it on all platforms but windows, due to copy-pasto. I really > > should > > > have stopped earlier yesterday... > > > > Thanks for updating the patch! > > > > The parser in the original submission was -1'd by me, and the current > > version > > proposed as an alternative. This was subsequently -1'd as well but no > > updated > > patch with a rewritten parser has been posted. So this is now stalled > > again. > > > > You started rewriting it, but you didn't finish it. > > Unfortunately, there is not a clean opinion on using bison's parser for > this purpose. I understand that the complexity of this language is too low, > so the benefit of using bison's gramatic is low too. Personally, I have not > any problem using bison for this purpose. For this case, I think we compare > two similarly long ways, but unfortunately, customers that have a problem > with long command lines still have this problem. > > Can we go forward? Daniel is strongly against handwritten parser. Is there > somebody strongly against bison's based parser? There is not any other way. I don't have a strong opinion either, but it seems that 2 people argued against a bison parser (vs only 1 arguing for) and the fact that the current habit is to rely on hand written parsers for simple cases (e.g. jsonapi.c / pg_parse_json()), it seems that we should go back to Pavel's original parser. I only had a quick look but it indeed seems trivial, it just maybe need a bit of refactoring to avoid some code duplication (getFiltersFromFile is duplicated, and getDatabaseExcludeFiltersFromFile could be removed if getFiltersFromFile knew about the 2 patterns).
Re: proposal: possibility to read dumped table's name from file
ne 2. 10. 2022 v 22:52 odesílatel Daniel Gustafsson napsal: > > On 2 Oct 2022, at 18:04, Andres Freund wrote: > > On 2022-10-02 00:19:59 -0700, Andres Freund wrote: > >> On 2022-10-01 23:56:59 -0700, Andres Freund wrote: > >>> On 2022-09-12 09:58:37 +0200, Daniel Gustafsson wrote: > Correct, fixed in the attached. > >>> > >>> Updated patch adding meson compatibility attached. > >> > >> Err, forgot to amend one hunk :( > > > > That fixed it on all platforms but windows, due to copy-pasto. I really > should > > have stopped earlier yesterday... > > Thanks for updating the patch! > > The parser in the original submission was -1'd by me, and the current > version > proposed as an alternative. This was subsequently -1'd as well but no > updated > patch with a rewritten parser has been posted. So this is now stalled > again. > You started rewriting it, but you didn't finish it. Unfortunately, there is not a clean opinion on using bison's parser for this purpose. I understand that the complexity of this language is too low, so the benefit of using bison's gramatic is low too. Personally, I have not any problem using bison for this purpose. For this case, I think we compare two similarly long ways, but unfortunately, customers that have a problem with long command lines still have this problem. Can we go forward? Daniel is strongly against handwritten parser. Is there somebody strongly against bison's based parser? There is not any other way. I am able to complete Daniel's patch, if there will not be objections. Regards Pavel > Having been around in 12 commitfests without a committer feeling confident > about pushing this I plan to mark it returned with feedback, and if a new > parser materializes itc can be readded instead of being dragged along. > > > c.h (and postgres.h, postgres_fe.h) shouldn't be included in headers. > > > > This is a common enough mistake that I'm wondering if we could automate > > warning about it somehow. > > Maybe we can add a simple git grep invocation in the CompilerWarnings CI > job to > catch this in the CFBot? If something like the below sketch matches then > we > can throw an error. (only for illustration, all three files needs to > checked). > > git grep "\"c\.h" -- *.h :^src/include/postgres*.h; > > -- > Daniel Gustafsson https://vmware.com/ > >
[PATCH] Fix build with LLVM 15 or above
Hello,
While building PostgreSQL 15 RC 1 with LLVM 15, I got a build failure as
follows:
cc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement
-Werror=vla -Werror=unguarded-availability-new -Wendif-labels
-Wmissing-format-attribute -Wcast-function-type -Wformat-security
-fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument
-Wno-compound-token-split-by-macro -O2 -pipe -O3 -funroll-loops
-fstack-protector-strong -fno-strict-aliasing -Wno-deprecated-declarations
-fPIC -DPIC -D__STDC_LIMIT_MACROS -D__STDC_FORMAT_MACROS
-D__STDC_CONSTANT_MACROS -I/usr/local/llvm15/include
-I../../../../src/include -I/usr/local/include -I/usr/local/include
-I/usr/local/include/libxml2 -I/usr/local/include -I/usr/local/include
-I/usr/local/include -I/usr/local/include -c -o llvmjit.o llvmjit.c
llvmjit.c:1115:50: error: use of undeclared identifier
'LLVMJITCSymbolMapPair'
LLVMOrcCSymbolMapPairs symbols =
palloc0(sizeof(LLVMJITCSymbolMapPair) * LookupSetSize);
^
llvmjit.c:1233:81: error: too few arguments to function call, expected 3,
have 2
ref_gen =
LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL);
~~
^
/usr/local/llvm15/include/llvm-c/Orc.h:997:31: note:
'LLVMOrcCreateCustomCAPIDefinitionGenerator' declared here
LLVMOrcDefinitionGeneratorRef LLVMOrcCreateCustomCAPIDefinitionGenerator(
^
2 errors generated.
gmake: *** [: llvmjit.o] Error 1
*** Error code 2
I've prepared a patch (attached) to fix the build issue with LLVM 15 or
above. It is also available at
https://people.FreeBSD.org/~sunpoet/patch/postgres/0001-Fix-build-with-LLVM-15-or-above.patch
Thanks.
Regards,
sunpoet
From bd2f6d2ec5369b5e33c6bf41560cdae3f8088f07 Mon Sep 17 00:00:00 2001
From: Po-Chuan Hsieh
Date: Sat, 1 Oct 2022 22:23:41 +0800
Subject: [PATCH] Fix build with LLVM 15 or above
- Upstream has fixed the struct name prefix from LLVMJIT to LLVMOrc [1].
- Upstream has changed the API of LLVMOrcCreateCustomCAPIDefinitionGenerator [2].
Reference: https://github.com/llvm/llvm-project/commit/b425f556935c1105dea59871a46caa7af2d378ad [1]
https://github.com/llvm/llvm-project/commit/14b7c108a2bf46541efc3a5c9cbd589b3afc18e6 [2]
---
src/backend/jit/llvm/llvmjit.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index fd3eecf27d..731b28a4c9 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -1112,7 +1112,11 @@ llvm_resolve_symbols(LLVMOrcDefinitionGeneratorRef GeneratorObj, void *Ctx,
LLVMOrcJITDylibRef JD, LLVMOrcJITDylibLookupFlags JDLookupFlags,
LLVMOrcCLookupSet LookupSet, size_t LookupSetSize)
{
+#if LLVM_VERSION_MAJOR >= 15
+ LLVMOrcCSymbolMapPairs symbols = palloc0(sizeof(LLVMOrcCSymbolMapPair) * LookupSetSize);
+#else
LLVMOrcCSymbolMapPairs symbols = palloc0(sizeof(LLVMJITCSymbolMapPair) * LookupSetSize);
+#endif
LLVMErrorRef error;
LLVMOrcMaterializationUnitRef mu;
@@ -1230,7 +1234,11 @@ llvm_create_jit_instance(LLVMTargetMachineRef tm)
* Symbol resolution support for "special" functions, e.g. a call into an
* SQL callable function.
*/
+#if LLVM_VERSION_MAJOR >= 15
+ ref_gen = LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL, NULL);
+#else
ref_gen = LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL);
+#endif
LLVMOrcJITDylibAddGenerator(LLVMOrcLLJITGetMainJITDylib(lljit), ref_gen);
return lljit;
--
2.35.2
Re: pg_upgrade test failure
On Mon, Oct 3, 2022 at 1:40 PM Michael Paquier wrote:
> On Mon, Oct 03, 2022 at 12:10:06PM +1300, Thomas Munro wrote:
> > I think something like the attached should do the right thing for
> > STATUS_DELETE_PENDING (sort of "ENOENT-in-progress"). unlink() goes
> > back to being blocking (sleep+retry until eventually we reach ENOENT
> > or we time out and give up with EACCES), but we still distinguish it
> > from true ENOENT so we have a fast exit in that case. This is passing
> > CI, but not tested yet.
>
> if (lstat(path, ) < 0)
> - return -1;
> + {
> + if (lstat_error_was_status_delete_pending())
> + is_lnk = false;
> + else
> + return -1;
> + }
> + else
> + is_lnk = S_ISLNK(st.st_mode);
> Sorry, I don't remember all the details in this area, but a directory
> can never be marked as STATUS_DELETE_PENDING with some of its contents
> still inside, right?
That's my understanding, yes: just like Unix, you can't remove a
directory with something in it. Unlike Unix, that includes files that
have been unlinked but are still open somewhere. (Note that in this
case it's not exactly a real directory, it's a junction point, which
is a directory but it doesn't have contents, it is a reparse point
pointing somewhere else, so I suspect that it can't really suffer from
ENOTEMPTY, but it probably can suffer from 'someone has it open for a
short time because they are concurrently stat-ing it'.)
> If it has some contents, forcing unlink() all
> the time would be fine?
Here's why I think it's probably OK to use unlink() unconditionally
after detecting STATUS_DELETE_PENDING. AFAICT there is no way to even
find out if it's a file or a junction in this state, but it doesn't
matter: we are not waiting for rmdir() or unlink() to succeed, we are
waiting for it to fail with an error other than EACCES, most likely
ENOENT (or to time out, perhaps because someone held the file open for
11 seconds, or because EACCES was actually telling us about a
permissions problem). EACCES is the errno for many things including
STATUS_DELETE_PENDING and also "you called unlink() but it's a
directory" (should be EPERM according to POSIX, or EISDIR according
to Linux). Both of those reasons imply that the zombie directory
entry still exists, and we don't care which of those reasons triggered
it.So I think that setting is_lnk = false is good enough here. Do
you see a hole in it?
Re: [PATCH] Log details for client certificate failures
On Sat, Oct 1, 2022 at 7:53 PM Peter Eisentraut wrote: > > On 29.09.22 06:52, Masahiko Sawada wrote: > > While this seems a future-proof idea, I wonder if it might be overkill > > since we don't need to worry about accumulation of leaked memory in > > this case. Given that only check_cluter_name is the case where we > > found a small memory leak, I think it's adequate to fix it. > > > > Fixing this issue suppresses the valgrind's complaint but since the > > boot value of cluster_name is "" the memory leak we can avoid is only > > 1 byte. > > I have committed this. I think it's better to keep the code locally > robust and not to have to rely on complex analysis of how GUC memory > management works. Thanks! Agreed. Regards, -- Masahiko Sawada PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: [PoC] Improve dead tuple storage for lazy vacuum
On Mon, Oct 3, 2022 at 2:04 AM Andres Freund wrote: > > Hi, > > On 2022-09-16 15:00:31 +0900, Masahiko Sawada wrote: > > I've updated the radix tree patch. It's now separated into two patches. > > cfbot notices a compiler warning: > https://cirrus-ci.com/task/6247907681632256?logs=gcc_warning#L446 > > [11:03:05.343] radixtree.c: In function ‘rt_iterate_next’: > [11:03:05.343] radixtree.c:1758:15: error: ‘slot’ may be used uninitialized > in this function [-Werror=maybe-uninitialized] > [11:03:05.343] 1758 |*value_p = *((uint64 *) slot); > [11:03:05.343] | ^~ > Thanks, I'll fix it in the next version patch. Regards, -- Masahiko Sawada PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Improve TAP tests of pg_upgrade for cross-version tests
On Sun, Oct 02, 2022 at 10:02:37AM -0700, Andres Freund wrote:
> This fails tests widely, and has so for a while:
> https://cirrus-ci.com/build/4862820121575424
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3649
>
> Note that it causes timeouts, which end up chewing up a cfbot "slot" for an
> hour...
Sorry for kicking the can down the road for a too-long time. Attached
is an updated patch that I have strimmed down to the minimum that
addresses all the issues I want fixed as of the scope of this thread:
- The filtering of the dumps is reduced to a minimum, removing only
comments and empty lines.
- The configuration of the old and new nodes is tweaked so as it is
abvle to handle upgrade from nodes older than 10.
- pg_dumpall is tweaked to use --extra-float-digits=0 for the old
nodes older than 11 to minimize the amount of diffs generated.
That's quite nice in itself, as it becomes possible to use much more
dump patterns loaded as part of the tests. More filtering rules could
be used, like the part about procedures and functions that I have sent
in the previous versions, but what I have here is enough to make the
test complete with all the versions supported by Cluster.pm. I am
thinking to get this stuff applied soon before moving on to the PATH
issue.
There is still one issue reported upthread by Justin about the fact
that we can use unexpected commands depending on the node involved.
For example, something like that would build a PATH so as psql from
the old node is used, not from the new node:
$newnode->command_ok(['psql', '-X', '-f', 'blah.sql']);
So with the current Cluster.pm, we could fail upgrade_adapt.sql if the
version upgraded from does not support psql's \if, and I don't think
that we should use a full path to the binary either. I am not
completely done analyzing that and this deserves a separate thread, as
it impacts all the commands used in TAP tests manipulating nodes from
multiple versions.
--
Michael
From cfbf5133835d2de963156231521993900f963dc1 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 3 Oct 2022 10:51:24 +0900
Subject: [PATCH v4] Add more filtering capabilities in the dumps.
This adds support for some filtering capabilities, for some tests done
down to v9.5. This allows the tests to pass with v14, while v9.5~13
still generate a few diffs, but these are minimal compared to what
happened before this commit.
---
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 100 +
1 file changed, 69 insertions(+), 31 deletions(-)
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 2f9b13bf0a..ac03ef5734 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -30,6 +30,27 @@ sub generate_db
"created database with ASCII characters from $from_char to
$to_char");
}
+# Filter the contents of a dump before its use in a content comparison.
+# This returns the path to the filtered dump.
+sub filter_dump
+{
+ my ($node, $dump_file) = @_;
+ my $dump_contents = slurp_file($dump_file);
+
+ # Remove the comments.
+ $dump_contents =~ s/^\-\-.*//mgx;
+ # Remove empty lines.
+ $dump_contents =~ s/^\n//mgx;
+
+ my $dump_file_filtered = "${dump_file}_filtered";
+ open(my $dh, '>', $dump_file_filtered)
+ || die "opening $dump_file_filtered";
+ print $dh $dump_contents;
+ close($dh);
+
+ return $dump_file_filtered;
+}
+
# The test of pg_upgrade requires two clusters, an old one and a new one
# that gets upgraded. Before running the upgrade, a logical dump of the
# old cluster is taken, and a second logical dump of the new one is taken
@@ -49,8 +70,10 @@ if ( (defined($ENV{olddump}) && !defined($ENV{oldinstall}))
die "olddump or oldinstall is undefined";
}
-# Temporary location for the dumps taken
+# Paths to the dumps taken during the tests.
my $tempdir = PostgreSQL::Test::Utils::tempdir;
+my $dump1_file = "$tempdir/dump1.sql";
+my $dump2_file = "$tempdir/dump2.sql";
# Initialize node to upgrade
my $oldnode =
@@ -60,7 +83,10 @@ my $oldnode =
# To increase coverage of non-standard segment size and group access without
# increasing test runtime, run these tests with a custom setting.
# --allow-group-access and --wal-segsize have been added in v11.
-$oldnode->init(extra => [ '--wal-segsize', '1', '--allow-group-access' ]);
+my %node_params = ();
+$node_params{extra} = [ '--wal-segsize', '1', '--allow-group-access' ]
+if $oldnode->pg_version >= 11;
+$oldnode->init(%node_params);
$oldnode->start;
# The default location of the source code is the root of this directory.
@@ -129,37 +155,39 @@ else
is($rc, 0, 'regression tests pass');
}
+# Initialize a new node for the upgrade.
+# Initialize a new node for the upgrade.
+my $newnode = PostgreSQL::Test::Cluster->new('new_node');
+$newnode->init(%node_params);
+
+my $newbindir =
Re: CI and test improvements
On Sun, Oct 02, 2022 at 02:54:21PM -0700, Andres Freund wrote: > the clang-only memory sanitizer there (if it works on freebsd)... Have you looked at this much ? I think it'll require a bunch of exclusions, right ? -- Justin
Re: Question: test "aggregates" failed in 32-bit machine
On Sun, Oct 02, 2022 at 02:11:12PM -0400, Tom Lane wrote: > "Jonathan S. Katz" writes: >> OK. For v15 I am heavily in favor for the least risky approach given the >> point we are at in the release cycle. The RMT hasn’t met yet to discuss, >> but from re-reading this thread again, I would recommend to revert >> (i.e. the “straight up revert”). > > OK by me. I don't quite see why it would be to let this code live on HEAD if it is not ready to be merged as there is a risk of creating side issues with things tied to the costing still ready to be merged, so I agree that the reversion done on both branches is the way to go for now. This could always be reworked and reproposed in the future. > I'm just about to throw up my hands and go for reversion in both branches, > because I'm now discovering that the code I'd hoped to salvage in > pathkeys.c (get_useful_group_keys_orderings and related) has its very own > bugs. It's imagining that it can rearrange a PathKeys list arbitrarily > and then rearrange the GROUP BY SortGroupClause list to match, but that's > easier said than done, for a couple of different reasons. (I now > understand why db0d67db2 made a cowboy hack in get_eclass_for_sort_expr ... > but it's still a cowboy hack with difficult-to-foresee side effects.) > There are other things in there that make it painfully obvious that > this code wasn't very carefully reviewed, eg XXX comments that should > have been followed up and were not, or a reference to a nonexistent > "debug_group_by_match_order_by" flag (maybe that was a GUC at some point?). Okay. Ugh. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade test failure
On Mon, Oct 03, 2022 at 12:10:06PM +1300, Thomas Munro wrote:
> I think something like the attached should do the right thing for
> STATUS_DELETE_PENDING (sort of "ENOENT-in-progress"). unlink() goes
> back to being blocking (sleep+retry until eventually we reach ENOENT
> or we time out and give up with EACCES), but we still distinguish it
> from true ENOENT so we have a fast exit in that case. This is passing
> CI, but not tested yet.
if (lstat(path, ) < 0)
- return -1;
+ {
+ if (lstat_error_was_status_delete_pending())
+ is_lnk = false;
+ else
+ return -1;
+ }
+ else
+ is_lnk = S_ISLNK(st.st_mode);
Sorry, I don't remember all the details in this area, but a directory
can never be marked as STATUS_DELETE_PENDING with some of its contents
still inside, right? If it has some contents, forcing unlink() all
the time would be fine?
> One ugly thing in this patch is that it has to deal with our
> historical mistake (?) of including Windows headers in this file in
> Cygwin builds for no reason and thus getting WIN32 defined on a
> non-WIN32 build, as I've complained about before[1] but not yet tidied
> up.
Your proposal remains local to dirmod.c, so that does not sound like a
big deal to me for the time being.
--
Michael
signature.asc
Description: PGP signature
Re: [RFC] building postgres with meson - v13
On Sun, Oct 02, 2022 at 01:38:37PM -0500, Justin Pryzby wrote: > On Sun, Oct 02, 2022 at 11:05:30AM -0700, Andres Freund wrote: > > > Also, you wrote "rm -fr build" between building for gcc and clang, but > > > since they run in an "always" block, it'd be better to use separate > > > dirs, to allow seeing logs for the the all (failed) tasks, in case the > > > last one succeeds. > > > > Hm, when are logs important for CompilerWarnings? I don't think we even > > collect any? Using a different builddir for the "sibling" tests (i.e. the > > two > > gcc and the two clang tests) would increase the times a bit because we'd > > regenerate the bison files etc. > > > > I guess it'll look a bit cleaner to use a build-gcc and a build-clang, just > > to > > get rid of the irregularity of needing that rm -rf. > > The build logs are important when hacking on .cirrus.yml itself. > > You're right that we don't normally save logs for CompilerWarnings; one or > another (unpublished) patch of mine adds that, and then also needed to change > to use separate dirs in order to debug building while experimenting with your > patch to use meson. FYI, this is what led me to make that suggestion. https://cirrus-ci.com/task/5920691940753408 I had a patch laying around to change the "compiler warnings" task to use debian "testing", which seems to have added some new flags in -Wall, which caused me to add (for now) some compiler flags like -Wno-error=... But when I added them to the task's CFLAGS, it broke "clang" (which doesn't support the warnings) in an obscure way[0], and no logs available to show why. [0] Header "uuid/uuid.h" has symbol "uuid_generate" with dependency uuid: NO So, I think it's worth reporting meson's build logs, even though no tests are run here. -- Justin
Re: Reducing the chunk header sizes on all memory context types
On Thu, 29 Sept 2022 at 18:30, David Rowley wrote: > Does anyone have any opinions on this? I by no means think I've nailed the fix in other_ideas_to_fix_MemoryContextContains.patch, so it would be good to see if anyone else has any new ideas on how to solve this issue. Andres did mention to me off-list about perhaps adding a boolean field to FunctionCallInfoBaseData to indicate if the return value can be assumed to be in CurrentMemoryContext. I feel like that might be quite a bit of work to go and change all functions to ensure that that's properly populated. For example, look at split_part() in varlena.c, it's going to be a little tedious to ensure we set that field correctly there as that function sometimes returns it's input, sometimes returns a string constant and sometimes allocates new memory. I feel fixing it this way will be error-prone and cause lots of future bugs. I'm also aware that the change made in b76fb6c2a becomes less temporary with each day that passes, so I really would like to find a solution to the MemoryContextContains issue. I'll reiterate that I don't think reverting c6e0fe1f2 fixes MemoryContextContains. That would just put back the behaviour of it returning true based on the owning MemoryContext and/or the direction that the wind is coming from on the particular day the function is called. Although I do think other_ideas_to_fix_MemoryContextContains.patch does fix the problem. I also fear a few people would be reaching for their pitchforks if I was to go and commit it. However, as of now, I'm starting to look more favourably at it as more time passes. David
Re: pg_upgrade test failure
On Mon, Oct 3, 2022 at 9:07 AM Thomas Munro wrote:
> On Tue, Sep 20, 2022 at 1:31 PM Justin Pryzby wrote:
> > I suspect that rmtree() was looping in pgunlink(), and got ENOENT, so
> > didn't warn about the file itself, but then failed one moment later in
> > rmdir.
>
> Yeah, I think this is my fault. In commit f357233c the new lstat()
> call might return ENOENT for STATUS_DELETE_PENDING, and then we don't
> enter pgunlink()'s 10 second sleep-retry loop. Let me think about how
> best to fix that, and how to write a regression test program that
> would exercise stuff like this. Might take a couple of days as I am
> away from computers until mid-week.
I think something like the attached should do the right thing for
STATUS_DELETE_PENDING (sort of "ENOENT-in-progress"). unlink() goes
back to being blocking (sleep+retry until eventually we reach ENOENT
or we time out and give up with EACCES), but we still distinguish it
from true ENOENT so we have a fast exit in that case. This is passing
CI, but not tested yet.
One ugly thing in this patch is that it has to deal with our
historical mistake (?) of including Windows headers in this file in
Cygwin builds for no reason and thus getting WIN32 defined on a
non-WIN32 build, as I've complained about before[1] but not yet tidied
up.
Remembering why we do any of this weird looking stuff that we don't
need on Unix, the general idea is that things that scan directories to
unlink everything before unlinking the parent directory need to block
for a while on STATUS_DELETE_PENDING to increase their probability of
success, while things that do anything else probably just want to skip
such zombie files completely. To recap, we have:
* readdir() sees files that are ENOENT-in-progress (so recursive
unlinks can see them)
* unlink() waits for ENOENT-in-progress to reach ENOENT (what broke here)
* stat() and lstat() report ENOENT-in-progress as ENOENT (done to fix
eg pg_basebackup, which used to fail at random on Windows)
* open() reports ENOENT-in-progress as either ENOENT or EEXIST
depending on O_CREAT (because used by stat())
Clearly this set of kludges isn't perfect and other kludge-sets would
be possible too. One thought is that we could hide ENOENT-in-progress
from readdir(), and add a new rmdir() wrapper instead. If it gets a
directory-not-empty error from the kernel, it could at that point wait
for zombie files to go away (perhaps registering for file system
events with some local equivalent of KQ_FILTER_VNODE if there is one,
to be less sloppy that the current sleep() nonsense, but sleep would
work too).
When I'm back at my real keyboard I'll try to come up with tests for
this stuff, but I'm not sure how solid we can really make a test for
this particular case -- I think you'd need to have another thread open
the file and then close it after different periods of time, to
demonstrate that the retry loop works but also gives up, and that's
exactly the sort of timing-dependent stuff we try to avoid. But I
think I'll try that anyway, because it's essential infrastructure to
allow Unix-only hackers to work only this stuff. Once we have that,
we might be able to make some more progress with the various
FILE_DISPOSITION_POSIX_SEMANTICS proposals, if it helps, because we'll
have reproducible evidence for what it really does.
[1] https://commitfest.postgresql.org/39/3781/
From 0fa290b4d3597c843804d3ee35559074864424aa Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Mon, 3 Oct 2022 09:15:02 +1300
Subject: [PATCH] Fix STATUS_DELETE_PENDING handling in pgunlink().
Commit c5cb8f3b didn't handle STATUS_DELETE_PENDING correctly, and would
report ENOENT immediately without waiting. In order for simple recursive
unlink algorithms to have a chance of succeeding while other backends
might not have closed them yet, we rely on the 10-seconds-of-retries
behavior present in pgunlink().
Diagnosed-by: Justin Pryzby
Discussion: https://postgr.es/m/20220920013122.GA31833%40telsasoft.com
---
src/port/dirmod.c | 38 --
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index ae6301dd6c..aae81228ad 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -39,6 +39,10 @@
#endif
#endif
+#if defined(WIN32) && !defined(__CYGWIN__)
+#include "port/win32ntdll.h"
+#endif
+
#if defined(WIN32) || defined(__CYGWIN__)
/*
@@ -91,6 +95,22 @@ pgrename(const char *from, const char *to)
return 0;
}
+/*
+ * Check if _pglstat64()'s reason for failure was STATUS_DELETE_PENDING.
+ * This doesn't apply to Cygwin, which has its own lstat() that would report
+ * the case as EACCES.
+*/
+static bool
+lstat_error_was_status_delete_pending(void)
+{
+ if (errno != ENOENT)
+ return false;
+#if defined(WIN32) && !defined(__CYGWIN__)
+ if (pg_RtlGetLastNtStatus() == STATUS_DELETE_PENDING)
+ return true;
+#endif
+ return false;
+}
/*
* pgunlink
@@ -98,6 +118,7 @@ pgrename(const char *from, const
Re: missing indexes in indexlist with partitioned tables
David Rowley writes: > * I can't quite figure out why you're doing "DROP TABLE a CASCADE;" in > inherits.sql. You've not changed anything else in that file. Did you > mean to do this in join.sql? Doing that would be a bad idea no matter where it's done. IIRC, those tables are intentionally set up to stress later dump/restore tests (with issues like inheritance children having column order different from their parents). regards, tom lane
Re: missing indexes in indexlist with partitioned tables
On Sun, 2 Oct 2022 at 05:34, Arne Roland wrote: > > On Tue, Sep 20, 2022 at 4:53 PM David Rowley wrote: > > Arne sent me an off-list > > message to say he's planning on working on the patch that uses the > > existing field instead of the new one he originally added. Let's hold > > off for that patch. > > I wouldn't say, I explicitly stated that. But I ended up doing something, > that resulted in the attached patch. :) I stand corrected. You said you'd think about it, not do it. Anyway, thanks for doing it :) > Further feedback would be appreciated! I had a quick look through the patch. Here are a few things that would be good to adjust. I understand that some of these things were how I left them in the patch I sent. In my defence, I mostly did that very quickly just so I could see if there was some issue with having the partitioned indexes in indexlist. I didn't actually put much effort into addressing the fine details of how that should be done. * In the header comment in get_relation_info(), I don't think we need to mention join removals explicitly. At a stretch, maybe mentioning "unique proofs" might be ok, but "various optimizations" might be better. If you mention "join removal", I fear that will just become outdated too quickly as further optimisations are added. Likewise for the comment about "join pruning" you've added in the function body. FWIW, we call these "join removals" anyway. * I think we should put RelationGetNumberOfBlocks() back to what it was and just ensure we don't call that for partitioned indexes in get_relation_info(). (Yes, I know that was my change) * I can't quite figure out why you're doing "DROP TABLE a CASCADE;" in inherits.sql. You've not changed anything else in that file. Did you mean to do this in join.sql? * The new test in join.sql. I understand that you've mostly copied the test from another place in the file and adjusted it to use a partitioned table. However, I don't really think you need to INSERT any data into those tables. I also think that using the table name of "a" is dangerous as it could conflict with another table by the same name in a parallel run of the tests. The non-partitioned version is a TEMP table. Also, it's slightly painful to look at the inconsistent capitalisation of SQL keywords in those queries you've added, again, I understand those are copied from above, but I see no need to duplicate the inconsistencies. Perhaps it's fine to copy the upper case keywords in the DDL and keep all lowercase in the queries. Also, I think you probably should just add a single simple join removal test for partitioned tables. You're not exercising any code that the non-partitioned case isn't by adding any additional tests. All that I think is worth testing here is ensuring nobody thinks that partitioned tables can get away with an empty indexlist again. * I had a bit of a 2nd thought on the test change in partition_join.sql. I know I added the "c" column so that join removals didn't apply. I'm now thinking it's probably better to just change the queries to use JOIN ON rather than JOIN USING. Also, apply the correct alias to the ORDER BY. This method should save from slowing the test down due to the additional column. We have some pretty slow buildfarm machines that this might actually make a meaningful difference to. Thanks again for working on this. David
Re: CI and test improvements
Hi, On 2022-10-02 16:35:06 -0500, Justin Pryzby wrote: > On Sun, Oct 02, 2022 at 01:52:01PM -0700, Andres Freund wrote: > > On 2022-10-01 18:36:41 -0700, Andres Freund wrote: > > > I am wondering if we should instead introduce a new "quickcheck" task that > > > just compiles and runs maybe one test and have *all* other tests depend on > > > that. Wasting a precious available windows instance to just fail to > > > build or > > > immediately fail during tests doesn't really make sense. > > > With a primed cache this takes ~32s, not too bad imo. 12s of that is > > cloning the repo. > > Maybe - that would avoid waiting 4 minutes for a windows instance to > start in the (hopefully atypical) case of a patch that fails in 1-2 > minutes under linux/freebsd. > > If the patch were completely broken, the windows task would take ~4min > to start, plus up to ~4min before failing to compile or failing an early > test. 6-8 minutes isn't nothing, but doesn't seem worth the added > complexity. Btw, the motivation to work on this just now was that I'd like to enable more sanitizers (undefined,alignment for linux-meson, address for linux-autoconf). Yes, we could make the dependency on freebsd instead, but I'd like to try to enable the clang-only memory sanitizer there (if it works on freebsd)... Greetings, Andres Freund
Re: CI and test improvements
Hi, On 2022-10-02 16:35:06 -0500, Justin Pryzby wrote: > Maybe - that would avoid waiting 4 minutes for a windows instance to > start in the (hopefully atypical) case of a patch that fails in 1-2 > minutes under linux/freebsd. > > If the patch were completely broken, the windows task would take ~4min > to start, plus up to ~4min before failing to compile or failing an early > test. 6-8 minutes isn't nothing, but doesn't seem worth the added > complexity. Avoiding 6-8mins of wasted windows time would, I think, allow us to crank cfbot's concurrency up a notch or two. > Also, this would mean that in the common case, the slowest task would be > delayed until after the SanityCheck task instance starts, compiles, and > runs some test :( Your best case is 32sec, but I doubt that's going to > be typical. Even the worst case isn't that bad, the uncached minimal build is 67s. > I was thinking about the idea of cfbot handling "tasks" separately, > similar to what it used to do with travis/appveyor. The logic for > "windows tasks are only run if linux passes tests" could live there. I don't really see the advantage of doing that over just increasing concurrency by a bit. > > +# no options enabled, should be small > > +CCACHE_MAXSIZE: "150M" > > Actually, tasks can share caches if the "cache key" is set. > If there was a separate "Sanity" task, I think it should use whatever > flags linux (or freebsd) use to avoid doing two compilations (with lots > of cache misses for patches which modify *.h files, which would then > happen twice, in serial). I think the price of using exactly the same flags is higher than the gain. And it'll rarely work if we use the container task for the sanity check, as the timestamps of the compiler, system headers etc will be different. > > + # use always: to continue after failures. Task that did not run count as > > a > > + # success, so we need to recheck SanityChecks's condition here ... > > > - # task that did not run, count as a success, so we need to recheck Linux' > > - # condition here ... > > Another/better justification/description is that "cirrus warns if the > depending task has different only_if conditions than the dependant task". That doesn't really seem easier to understand to me. Greetings, Andres Freund
Re: Question: test "aggregates" failed in 32-bit machine
David Rowley writes: > As for the slight misuse of group_pathkeys, I guess since there are no > users that require just the plain pathkeys belonging to the GROUP BY, > then likely the best thing would be just to rename that field to > something like groupagg_pathkeys. Maintaining two separate fields and > concatenating them every time we want group_pathkeys does not seem > that appealing to me. Seems like a waste of memory and effort. I don't > want to hi-jack this thread to discuss that, but if you have a > preferred course of action, then I'm happy to kick off a discussion on > a new thread. I don't feel any great urgency to resolve this. Let's wait and see what comes out of the other thread. regards, tom lane
Re: CI and test improvements
On Sun, Oct 02, 2022 at 01:52:01PM -0700, Andres Freund wrote: > Hi, > > On 2022-10-01 18:36:41 -0700, Andres Freund wrote: > > I am wondering if we should instead introduce a new "quickcheck" task that > > just compiles and runs maybe one test and have *all* other tests depend on > > that. Wasting a precious available windows instance to just fail to build > > or > > immediately fail during tests doesn't really make sense. > With a primed cache this takes ~32s, not too bad imo. 12s of that is > cloning the repo. Maybe - that would avoid waiting 4 minutes for a windows instance to start in the (hopefully atypical) case of a patch that fails in 1-2 minutes under linux/freebsd. If the patch were completely broken, the windows task would take ~4min to start, plus up to ~4min before failing to compile or failing an early test. 6-8 minutes isn't nothing, but doesn't seem worth the added complexity. Also, this would mean that in the common case, the slowest task would be delayed until after the SanityCheck task instance starts, compiles, and runs some test :( Your best case is 32sec, but I doubt that's going to be typical. I was thinking about the idea of cfbot handling "tasks" separately, similar to what it used to do with travis/appveyor. The logic for "windows tasks are only run if linux passes tests" could live there. That could also be useful if there's ever the possibility of running an additional OS on another CI provider, or if another provider can run windows tasks faster, or if we need to reduce our load/dependency on cirrus. I realized that goes backwards in some ways to the direction we've gone with cirrus, and I'm not sure how exactly it would do that (I suppose it might add ci-os-only tags to its commit message). > +# no options enabled, should be small > +CCACHE_MAXSIZE: "150M" Actually, tasks can share caches if the "cache key" is set. If there was a separate "Sanity" task, I think it should use whatever flags linux (or freebsd) use to avoid doing two compilations (with lots of cache misses for patches which modify *.h files, which would then happen twice, in serial). > + # use always: to continue after failures. Task that did not run count as a > + # success, so we need to recheck SanityChecks's condition here ... > - # task that did not run, count as a success, so we need to recheck Linux' > - # condition here ... Another/better justification/description is that "cirrus warns if the depending task has different only_if conditions than the dependant task". -- Justin
Re: Question: test "aggregates" failed in 32-bit machine
On Mon, 3 Oct 2022 at 09:59, Tom Lane wrote: > > David Rowley writes: > > For the master version, I think it's safe just to get rid of > > PlannerInfo.num_groupby_pathkeys now. I only added that so I could > > strip off the ORDER BY / DISTINCT aggregate PathKeys from the group by > > pathkeys before passing to the functions that rearranged the GROUP BY > > clause. > > I was kind of unhappy with that data structure too, but from the > other direction: I didn't like that you were folding aggregate-derived > pathkeys into root->group_pathkeys in the first place. That seems like > a kluge that might work all right for the moment but will cause problems > down the road. (Despite the issues with the patch at hand, I don't > think it's unreasonable to suppose that somebody will have a more > successful go at optimizing GROUP BY sorting later.) If we keep the > data structure like this, I think we absolutely need num_groupby_pathkeys, > or some other way of recording which pathkeys came from what source. Ok, I don't feel too strongly about removing num_groupby_pathkeys. I'm fine to leave it there. However, I'll reserve slight concerns that we'll likely receive sporadic submissions of cleanup patches that remove the unused field over the course of the next few years and that dealing with those might take up more time than just removing it now and putting it back when we need it. We have been receiving quite a few patches along those lines lately. As for the slight misuse of group_pathkeys, I guess since there are no users that require just the plain pathkeys belonging to the GROUP BY, then likely the best thing would be just to rename that field to something like groupagg_pathkeys. Maintaining two separate fields and concatenating them every time we want group_pathkeys does not seem that appealing to me. Seems like a waste of memory and effort. I don't want to hi-jack this thread to discuss that, but if you have a preferred course of action, then I'm happy to kick off a discussion on a new thread. David
Re: proposal: possibility to read dumped table's name from file
On 2022-10-02 22:52:33 +0200, Daniel Gustafsson wrote: > The parser in the original submission was -1'd by me, and the current version > proposed as an alternative. This was subsequently -1'd as well but no updated > patch with a rewritten parser has been posted. So this is now stalled again. > > Having been around in 12 commitfests without a committer feeling confident > about pushing this I plan to mark it returned with feedback, and if a new > parser materializes itc can be readded instead of being dragged along. Makes sense to me.
Re: proposal: possibility to read dumped table's name from file
Daniel Gustafsson writes: > On 2 Oct 2022, at 18:04, Andres Freund wrote: >> c.h (and postgres.h, postgres_fe.h) shouldn't be included in headers. >> This is a common enough mistake that I'm wondering if we could automate >> warning about it somehow. > Maybe we can add a simple git grep invocation in the CompilerWarnings CI job > to > catch this in the CFBot? I'd be inclined to teach headerscheck or cpluspluscheck about it. regards, tom lane
Re: Documentation building fails on HTTPS redirect (again)
> On 2 Oct 2022, at 03:49, Andres Freund wrote: > On 2022-09-30 11:35:36 +0200, Daniel Gustafsson wrote: >> Installing the stylesheets locally as we document solves the issue of course, >> but maybe it's time to move to using --nonet as we discussed in [0] and >> require >> the stylesheets locally? It's a shame that casual contributions require a >> big >> investment in installation, but it seems hard to get around. > > docbooks-xml and docbooks-xsl aren't that big (adding 8MB to a minimal debian > install). Thats true, size wise they are trivial, but it's a shame we seemingly need to move away from "you don't have to do anything" which worked for years, to "you need to install X which you are unlikely to need for anything else". Especially when the failure stems from such a silly limitation. But, that's out of our hands, and we can only work on making it better for our contributors. > However a) we document installing fop as well, even though it's not needed for > the html docs build b) the dependencies recommended by the debian packages > increase the size a lot. Just using our documented line ends up with 550MB. We have this in the documentation today, but it's not especially visible and well below where we list the packages: "If xmllint or xsltproc is not found, you will not be able to build any of the documentation. fop is only needed to build the documentation in PDF format." I think we should make it a lot more visible. > Perhaps separating out fop and using --no-install-recommends (and other > similar flags) makes it less of an issue? We probably should work to deliver > a more usable error than what just using --nonet gives you... I agree with that. -- Daniel Gustafsson https://vmware.com/
Re: Question: test "aggregates" failed in 32-bit machine
David Rowley writes: > For the master version, I think it's safe just to get rid of > PlannerInfo.num_groupby_pathkeys now. I only added that so I could > strip off the ORDER BY / DISTINCT aggregate PathKeys from the group by > pathkeys before passing to the functions that rearranged the GROUP BY > clause. I was kind of unhappy with that data structure too, but from the other direction: I didn't like that you were folding aggregate-derived pathkeys into root->group_pathkeys in the first place. That seems like a kluge that might work all right for the moment but will cause problems down the road. (Despite the issues with the patch at hand, I don't think it's unreasonable to suppose that somebody will have a more successful go at optimizing GROUP BY sorting later.) If we keep the data structure like this, I think we absolutely need num_groupby_pathkeys, or some other way of recording which pathkeys came from what source. One way to manage that would be to insist that the length of root->group_clauses should indicate the number of associated grouping pathkeys. Right now they might not be the same because we might discover some of the pathkeys to be redundant --- but if we do, ISTM that the corresponding GROUP BY clauses are also redundant and could get dropped. That ties into the stuff I was worried about in [1], though. I'll keep this in mind when I get back to messing with that. regards, tom lane [1] https://www.postgresql.org/message-id/flat/1657885.1657647073%40sss.pgh.pa.us
Re: proposal: possibility to read dumped table's name from file
> On 2 Oct 2022, at 18:04, Andres Freund wrote: > On 2022-10-02 00:19:59 -0700, Andres Freund wrote: >> On 2022-10-01 23:56:59 -0700, Andres Freund wrote: >>> On 2022-09-12 09:58:37 +0200, Daniel Gustafsson wrote: Correct, fixed in the attached. >>> >>> Updated patch adding meson compatibility attached. >> >> Err, forgot to amend one hunk :( > > That fixed it on all platforms but windows, due to copy-pasto. I really should > have stopped earlier yesterday... Thanks for updating the patch! The parser in the original submission was -1'd by me, and the current version proposed as an alternative. This was subsequently -1'd as well but no updated patch with a rewritten parser has been posted. So this is now stalled again. Having been around in 12 commitfests without a committer feeling confident about pushing this I plan to mark it returned with feedback, and if a new parser materializes itc can be readded instead of being dragged along. > c.h (and postgres.h, postgres_fe.h) shouldn't be included in headers. > > This is a common enough mistake that I'm wondering if we could automate > warning about it somehow. Maybe we can add a simple git grep invocation in the CompilerWarnings CI job to catch this in the CFBot? If something like the below sketch matches then we can throw an error. (only for illustration, all three files needs to checked). git grep "\"c\.h" -- *.h :^src/include/postgres*.h; -- Daniel Gustafsson https://vmware.com/
Re: CI and test improvements
Hi,
On 2022-10-01 18:36:41 -0700, Andres Freund wrote:
> I am wondering if we should instead introduce a new "quickcheck" task that
> just compiles and runs maybe one test and have *all* other tests depend on
> that. Wasting a precious available windows instance to just fail to build or
> immediately fail during tests doesn't really make sense.
Attached is an implementation of that idea.
I fairly randomly chose two quick tests to execute as part of the sanity
check, cube/regress pg_ctl/001_start_stop. I wanted to have coverage for
initdb, a pg_regress style test, a tap test, some other client binary.
With a primed cache this takes ~32s, not too bad imo. 12s of that is cloning
the repo.
What do you think?
We could bake a bare repo into the images to make the clone step in faster,
but that'd be for later anyway.
set -e
rm -rf /tmp/pg-clone-better
mkdir /tmp/pg-clone-better
cd /tmp/pg-clone-better
git init --bare
git remote add origin https://github.com/postgres/postgres.git --no-tags -t
'REL_*' -t master
git fetch -v
git repack -ad -f
du -sh
results in a 227MB repo.
git clone https://github.com/anarazel/postgres.git -v --depth 1000 -b
ci-sanitycheck --reference /tmp/pg-clone-better /tmp/pg-clone-better-clone
clones an example branch in ~1.35s.
Greetings,
Andres Freund
>From fb7f8413a9efa136e73911e56f9970d8289ee178 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sun, 2 Oct 2022 11:26:19 -0700
Subject: [PATCH v1] ci: Introduce a SanityCheck task that other tasks depend
on
---
.cirrus.yml | 94 +
1 file changed, 87 insertions(+), 7 deletions(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index 531cfe96f65..87dc7025eaf 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -55,6 +55,83 @@ on_failure_meson: _failure_meson
type: text/plain
+# To avoid unnecessarily spinning up a lot of VMs / containers for entirely
+# broken commits, have a very minimal test that all others depend on.
+task:
+ name: SanityCheck
+
+ # If a specific OS is requested, don't run the sanity check. This shortens
+ # push-wait-for-ci cycles a bit when debugging operating system specific
+ # failures.
+ only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*'
+
+ env:
+CPUS: 4
+BUILD_JOBS: 8
+TEST_JOBS: 8
+CCACHE_DIR: ${CIRRUS_WORKING_DIR}/ccache_dir
+# no options enabled, should be small
+CCACHE_MAXSIZE: "150M"
+
+ container:
+image: $CONTAINER_REPO/linux_debian_bullseye_ci:latest
+cpu: $CPUS
+
+ ccache_cache:
+folder: $CCACHE_DIR
+
+ create_user_script: |
+useradd -m postgres
+chown -R postgres:postgres .
+mkdir -p ${CCACHE_DIR}
+chown -R postgres:postgres ${CCACHE_DIR}
+echo '* - memlock 134217728' > /etc/security/limits.d/postgres.conf
+su postgres -c "ulimit -l -H && ulimit -l -S"
+# Can't change container's kernel.core_pattern. Postgres user can't write
+# to / normally. Change that.
+chown root:postgres /
+chmod g+rwx /
+
+ configure_script: |
+su postgres <<-EOF
+ meson setup \
+--buildtype=debug \
+--auto-features=disabled \
+-Dtap_tests=enabled \
+build
+EOF
+ build_script: |
+su postgres <<-EOF
+ set -e
+ ccache -s
+ ccache -z
+ ninja -C build -j${BUILD_JOBS}
+ ccache -s
+EOF
+ upload_caches: ccache
+
+ # Run a minimal set of tests. The main regression tests take too long for
+ # this purpose. I chose a random pg_regress style test, and a tap test that
+ # exercises both a frontend binary and the backend.
+ test_minimal_script: |
+su postgres <<-EOF
+ ulimit -c unlimited
+ meson test $MTEST_ARGS --num-processes ${TEST_JOBS} \
+tmp_install cube/regress pg_ctl/001_start_stop
+EOF
+
+ on_failure:
+<<: *on_failure_meson
+cores_script: |
+ shopt -s nullglob
+ mkdir -m 770 /tmp/cores
+ ls -l /
+ if [ -n "$(echo /core*)" ] ; then
+mv /core* /tmp/cores/
+src/tools/ci/cores_backtrace.sh linux /tmp/cores
+ fi
+
+
task:
name: FreeBSD - 13 - Meson
@@ -69,6 +146,7 @@ task:
CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
CFLAGS: -Og -ggdb
+ depends_on: SanityCheck
only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*freebsd.*'
compute_engine_instance:
@@ -170,6 +248,7 @@ task:
LINUX_CONFIGURE_FEATURES: *LINUX_CONFIGURE_FEATURES
LINUX_MESON_FEATURES: *LINUX_MESON_FEATURES
+ depends_on: SanityCheck
only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*linux.*'
compute_engine_instance:
@@ -311,6 +390,7 @@ task:
CFLAGS: -Og -ggdb
CXXFLAGS: -Og -ggdb
+ depends_on: SanityCheck
only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~
Re: Question: test "aggregates" failed in 32-bit machine
On Mon, 3 Oct 2022 at 08:10, Tom Lane wrote: > As attached. For the master version, I think it's safe just to get rid of PlannerInfo.num_groupby_pathkeys now. I only added that so I could strip off the ORDER BY / DISTINCT aggregate PathKeys from the group by pathkeys before passing to the functions that rearranged the GROUP BY clause. David
Re: disfavoring unparameterized nested loops
On Fri, Sep 30, 2022 at 3:19 PM Benjamin Coutu wrote: > > For all I know you might be onto something. But it really seems > > independent to me. > > Yeah, I‘m sorry if I highjacked this thread for something related but > technically different. I certainly wouldn't say that you hijacked the thread. I'm glad that you revived the discussion, in fact. The way that I've framed the problem is probably at least somewhat controversial. In fact I'm almost certain that at least one or two people will flat out disagree with me. But even if everybody else already thought about unparameterized nested loop joins in the same terms, it might still be useful to make the arguments that you've made. What I'm saying is that the probability of "getting it right" is virtually irrelevant in the case of these unparameterized nested loop join plans specifically. Any probability that's less than 1.0 is already unacceptable, more or less. A probability of 1.0 is never unattainable in the real world, no matter what, so why should the true probability (whatever that means) matter at all? The appropriate course of action will still be "just don't do that, ever". To me this dynamic seems qualitatively different to other cases, where we might want to give some weight to uncertainty. Understanding where the boundaries lie between those trickier cases and this simpler case seems important and relevant to me. -- Peter Geoghegan
Re: pg_upgrade test failure
On Tue, Sep 20, 2022 at 1:31 PM Justin Pryzby wrote: > I suspect that rmtree() was looping in pgunlink(), and got ENOENT, so > didn't warn about the file itself, but then failed one moment later in > rmdir. Yeah, I think this is my fault. In commit f357233c the new lstat() call might return ENOENT for STATUS_DELETE_PENDING, and then we don't enter pgunlink()'s 10 second sleep-retry loop. Let me think about how best to fix that, and how to write a regression test program that would exercise stuff like this. Might take a couple of days as I am away from computers until mid-week.
Re: disfavoring unparameterized nested loops
On Sun, Oct 2, 2022 at 3:43 AM Robert Haas wrote: > On Fri, Sep 30, 2022 at 2:24 PM Peter Geoghegan wrote: > > It's not just that the risks are ludicrously high, of course. The > > potential benefits must *also* be very low. It's both factors, > > together. > > Hmm, maybe. But it also wouldn't surprise me very much if someone can > come up with a test case where a nested loop with a single row (or > maybe no rows) on one side or the other and it's significantly faster > than any alternative plan. That's certainly possible, but wouldn't the difference all come from fixed startup costs? If we're talking about a single row, with a minimal test case, then the potential downside of this more conservative strategy might indeed amount to something like a 2x or 3x slowdown, if we look at it in isolation. But why measure it that way? I think that absolute differences like milliseconds of execution time are much more relevant. Real production databases have many queries with very diverse characteristics -- there is a lot going on at any given moment. The proportion of queries that will be affected either way by avoiding unparamaterized nested loop joins is probably going to be very small. Nobody is going to notice if only a small subset or all queries are maybe 1 ms or 2 ms slower. As long as it's well within the margin of noise in 100% of all cases, it really shouldn't matter. AFAICT the choice is one of "bounded, low upside versus unbounded, high downside". > I believe, though, that even if such cases > exist, they are probably relatively few in number compared to the > cases where parameterized nested loops hurt, and the savings are > probably small compared to the multiple-orders-of-magnitude slowdowns > that you can get when a nested loop goes bad. But they might still be > relatively large -- 2x, 3x? -- in absolute terms. I suspect it won't even matter if disallowing unparamaterized nested loop joins loses on average. I am reminded of this: https://en.wikipedia.org/wiki/St._Petersburg_paradox#Expected_utility_theory -- Peter Geoghegan
Re: [RFC] building postgres with meson - v13
On Sun, Oct 02, 2022 at 11:05:30AM -0700, Andres Freund wrote: > > Also, you wrote "rm -fr build" between building for gcc and clang, but > > since they run in an "always" block, it'd be better to use separate > > dirs, to allow seeing logs for the the all (failed) tasks, in case the > > last one succeeds. > > Hm, when are logs important for CompilerWarnings? I don't think we even > collect any? Using a different builddir for the "sibling" tests (i.e. the two > gcc and the two clang tests) would increase the times a bit because we'd > regenerate the bison files etc. > > I guess it'll look a bit cleaner to use a build-gcc and a build-clang, just to > get rid of the irregularity of needing that rm -rf. The build logs are important when hacking on .cirrus.yml itself. You're right that we don't normally save logs for CompilerWarnings; one or another (unpublished) patch of mine adds that, and then also needed to change to use separate dirs in order to debug building while experimenting with your patch to use meson. -- Justin
Re: Question: test "aggregates" failed in 32-bit machine
"Jonathan S. Katz" writes: > OK. For v15 I am heavily in favor for the least risky approach given the > point we are at in the release cycle. The RMT hasn’t met yet to discuss, > but from re-reading this thread again, I would recommend to revert > (i.e. the “straight up revert”). OK by me. > I’m less opinionated on the approach for what’s in HEAD, but the rewrite > you suggest sounds promising. I'm just about to throw up my hands and go for reversion in both branches, because I'm now discovering that the code I'd hoped to salvage in pathkeys.c (get_useful_group_keys_orderings and related) has its very own bugs. It's imagining that it can rearrange a PathKeys list arbitrarily and then rearrange the GROUP BY SortGroupClause list to match, but that's easier said than done, for a couple of different reasons. (I now understand why db0d67db2 made a cowboy hack in get_eclass_for_sort_expr ... but it's still a cowboy hack with difficult-to-foresee side effects.) There are other things in there that make it painfully obvious that this code wasn't very carefully reviewed, eg XXX comments that should have been followed up and were not, or a reference to a nonexistent "debug_group_by_match_order_by" flag (maybe that was a GUC at some point?). On top of that, it's producing several distinct pathkey orderings for the caller to try, but it's completely unclear to me that the subsequent choice of cheapest path isn't going to largely reduce to the question of whether we can accurately estimate the relative costs of different sort-column orders. Which is exactly what we're finding we can't do. So that end of it seems to need a good deal of rethinking as well. In short, this needs a whole lotta work, and I'm not volunteering. regards, tom lane
Re: [RFC] building postgres with meson - v13
Hi,
On 2022-10-02 12:25:20 -0500, Justin Pryzby wrote:
> On Mon, Sep 26, 2022 at 06:19:51PM -0700, Andres Freund wrote:
> > From 680ff3f7b4da1dbf21d0c7cd87af9bb5ee8b230c Mon Sep 17 00:00:00 2001
> > From: Andres Freund
> > Date: Wed, 21 Sep 2022 20:36:36 -0700
> > Subject: [PATCH v17 01/23] meson: ci: wip: move compilerwarnings task to
> > meson
>
> This patch isn't finished, but this part looks like a rebase conflict:
>
> - make -s -j${BUILD_JOBS} clean
> + make -s -j${BUILD_JOBS} world-bin
I don't think so - it's the first task building with autoconf / in-tree. I
however shouldn't added ccache to CC, that was an accident. I think I'll
convert it to a vpath build, seems cleaner.
> Also, you wrote "rm -fr build" between building for gcc and clang, but
> since they run in an "always" block, it'd be better to use separate
> dirs, to allow seeing logs for the the all (failed) tasks, in case the
> last one succeeds.
Hm, when are logs important for CompilerWarnings? I don't think we even
collect any? Using a different builddir for the "sibling" tests (i.e. the two
gcc and the two clang tests) would increase the times a bit because we'd
regenerate the bison files etc.
I guess it'll look a bit cleaner to use a build-gcc and a build-clang, just to
get rid of the irregularity of needing that rm -rf.
> On Mon, Sep 26, 2022 at 06:19:51PM -0700, Andres Freund wrote:
> > From 6025cb80d65fd7a8414241931df9f003a292052f Mon Sep 17 00:00:00 2001
> > From: Andres Freund
> > Date: Sun, 25 Sep 2022 12:07:29 -0700
> > Subject: [PATCH v17 16/23] windows: adjust FD_SETSIZE via commandline
> > define
>
> > +++ b/src/bin/pgbench/meson.build
> > @@ -27,6 +27,8 @@ pgbench = executable('pgbench',
> >pgbench_sources,
> >dependencies: [frontend_code, libpq, thread_dep],
> >include_directories: include_directories('.'),
> > + c_pch: pch_postgres_fe_h,
> > + c_args: host_system == 'windows' ? ['-DFD_SETSIZE=1024'] : [],
> >kwargs: default_bin_args,
> > )
>
> This puts PCH into the preparatory commit.
> Also, src/tools/msvc/Mkvcbuild.pm seems to use spaces rather than tabs.
Oops, will fix.
Greetings,
Andres Freund
Re: Making pg_rewind faster
Hi,
On 2022-09-13 20:50:20 +0300, Alexander Korotkov wrote:
> On Fri, Jul 29, 2022 at 1:05 PM Justin Kwan wrote:
> > Not sure if this email went through previously but thank you for your
> > feedback, I've incorporated your suggestions by scanning the logs produced
> > from pg_rewind when asserting that certain WAL segment files were skipped
> > from being copied over to the target server.
> >
> > I've also updated the pg_rewind patch file to target the Postgres master
> > branch (version 16 to be). Please see attached.
>
> Thank you for the revision.
>
> I've taken a look at this patch. Overall it looks good to me. I also
> don't see any design objections in the thread.
>
> A couple of points from me:
> 1) I would prefer to evade hard-coded names for WAL segments in the
> tap tests. Could we calculate the names in the tap tests based on the
> diverge point, etc.?
> 2) Patch contains some indentation with spaces, which should be done
> in tabs. Please consider either manually fixing this or running
> pgindent over modified files.
This patch currently fails because it hasn't been adjusted for
commit c47885bd8b6
Author: Andres Freund
Date: 2022-09-19 18:03:17 -0700
Split TESTDIR into TESTLOGDIR and TESTDATADIR
The adjustment is trivial. Attached, together with also producing an error
message rather than just dying wordlessly.
It doesn't seem quite right to read pg_rewind's logs by reading
regress_log_001_basic. Too easy to confuse different runs of pg_rewind
etc. I'd suggest trying to redirect the log to a different file.
With regard to Alexander's point about whitespace:
.git/rebase-apply/patch:25: indent with spaces.
/* Handle WAL segment file. */
.git/rebase-apply/patch:26: indent with spaces.
const char *fname;
.git/rebase-apply/patch:27: indent with spaces.
char*slash;
.git/rebase-apply/patch:29: indent with spaces.
/* Split filepath into directory & filename. */
.git/rebase-apply/patch:30: indent with spaces.
slash = strrchr(path, '/');
warning: squelched 29 whitespace errors
warning: 34 lines add whitespace errors.
Greetings,
Andres Freund
>From 44b987f4f21d4ff6c898b085db3c6c1094766f97 Mon Sep 17 00:00:00 2001
From: Justin Kwan
Date: Tue, 19 Jul 2022 22:15:36 -0700
Subject: [PATCH v3 1/2] Avoid copying WAL segments before divergence to speed
up pg_rewind
"Optimize pg_rewind to Skip Copying Common WAL Files". It adds a conditional
check to avoid uncessesarily copying any WAL segment files from source to
target if they are common between both servers, before the point of WAL
divergence during pg_rewind. On the source server, each WAL file's
corresponding segment number is computed and compared against the segement
number of the first diverged LSN. All WAL files which fall before the segment
of the first diverged LSN can safely be skipped from copying to the target.
The reduction in WAL segment files transmitted over the network from source to
target server massively reduces overall pg_rewind execution time, when a large
amount of WAL segment files are retained.
This patch is intended for immediate application into the Postgres master
branch on version 14.4.
Regression tests are included to verify that WAL segment files prior to WAL
diverge are not copied. The source code successfully compiles, and all tests
successfully pass.
Author: Justin Kwan, Vignesh Ravichandran
---
src/bin/pg_rewind/filemap.c | 15 +
src/bin/pg_rewind/pg_rewind.c | 39 ++-
src/bin/pg_rewind/pg_rewind.h | 2 +
src/bin/pg_rewind/t/001_basic.pl | 65 +++
src/bin/pg_rewind/t/008_min_recovery_point.pl | 64 ++
5 files changed, 183 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 269ed6446e6..2fd0523ce5b 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -26,6 +26,7 @@
#include
#include
+#include "access/xlog_internal.h"
#include "catalog/pg_tablespace_d.h"
#include "common/hashfn.h"
#include "common/string.h"
@@ -728,6 +729,20 @@ decide_file_action(file_entry_t *entry)
case FILE_TYPE_REGULAR:
if (!entry->isrelfile)
{
+/* Handle WAL segment file. */
+const char *fname;
+char*slash;
+
+/* Split filepath into directory & filename. */
+slash = strrchr(path, '/');
+if (slash)
+fname = slash + 1;
+else
+fname = path;
+
+if (IsXLogFileName(fname))
+return decide_wal_file_action(fname);
+
/*
* It's a non-data file that we have no special processing
* for. Copy it in toto.
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 3cd77c09b1a..4ac38427b38 100644
---
Re: Question: test "aggregates" failed in 32-bit machine
> On Oct 2, 2022, at 1:12 PM, Tom Lane wrote: > > "Jonathan S. Katz" writes: >>> On 10/1/22 6:57 PM, Tom Lane wrote: >>> I plan to have a look tomorrow at the idea of reverting only the cost_sort >>> changes, and rewriting get_cheapest_group_keys_order() to just sort the >>> keys by decreasing numgroups estimates as I suggested upthread. That >>> might be substantially less messy, because of fewer interactions with >>> 1349d2790. > >> Maybe this leads to a follow-up question of do we continue to improve >> what is in HEAD while reverting the code in v15 (particularly if it's >> easier to do it that way)? > > No. I see no prospect that the cost_sort code currently in HEAD is going > to become shippable in the near future. Quite aside from the plain bugs, > I think it's based on untenable assumptions about how accurately we can > estimate the CPU costs associated with different sort-column orders. OK. > Having said that, it's certainly possible that we should do something > different in HEAD than in v15. We could do the rewrite I suggest above > in HEAD while doing a straight-up revert in v15. I've been finding that > 1349d2790 is sufficiently entwined with this code that the patches would > look significantly different in any case, so that might be the most > reliable way to proceed in v15. OK. For v15 I am heavily in favor for the least risky approach given the point we are at in the release cycle. The RMT hasn’t met yet to discuss, but from re-reading this thread again, I would recommend to revert (i.e. the “straight up revert”). I’m less opinionated on the approach for what’s in HEAD, but the rewrite you suggest sounds promising. Thanks, Jonathan
Re: [RFC] building postgres with meson - v13
On Mon, Sep 26, 2022 at 06:19:51PM -0700, Andres Freund wrote:
> From 680ff3f7b4da1dbf21d0c7cd87af9bb5ee8b230c Mon Sep 17 00:00:00 2001
> From: Andres Freund
> Date: Wed, 21 Sep 2022 20:36:36 -0700
> Subject: [PATCH v17 01/23] meson: ci: wip: move compilerwarnings task to meson
This patch isn't finished, but this part looks like a rebase conflict:
- make -s -j${BUILD_JOBS} clean
+ make -s -j${BUILD_JOBS} world-bin
Also, you wrote "rm -fr build" between building for gcc and clang, but
since they run in an "always" block, it'd be better to use separate
dirs, to allow seeing logs for the the all (failed) tasks, in case the
last one succeeds.
On Mon, Sep 26, 2022 at 06:19:51PM -0700, Andres Freund wrote:
> From 6025cb80d65fd7a8414241931df9f003a292052f Mon Sep 17 00:00:00 2001
> From: Andres Freund
> Date: Sun, 25 Sep 2022 12:07:29 -0700
> Subject: [PATCH v17 16/23] windows: adjust FD_SETSIZE via commandline
> define
> +++ b/src/bin/pgbench/meson.build
> @@ -27,6 +27,8 @@ pgbench = executable('pgbench',
>pgbench_sources,
>dependencies: [frontend_code, libpq, thread_dep],
>include_directories: include_directories('.'),
> + c_pch: pch_postgres_fe_h,
> + c_args: host_system == 'windows' ? ['-DFD_SETSIZE=1024'] : [],
>kwargs: default_bin_args,
> )
This puts PCH into the preparatory commit.
Also, src/tools/msvc/Mkvcbuild.pm seems to use spaces rather than tabs.
--
Justin
Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
Hi, On 2022-09-27 14:20:44 -0400, Melanie Plageman wrote: > v30 attached > rebased and pgstat_io_ops.c builds with meson now > also, I tested with pgstat_report_stat() only flushing when forced and > tests still pass Unfortunately tests fail in CI / cfbot. E.g., https://cirrus-ci.com/task/5816109319323648 https://api.cirrus-ci.com/v1/artifact/task/5816109319323648/testrun/build/testrun/main/regress/regression.diffs diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/stats.out /tmp/cirrus-ci-build/build/testrun/main/regress/results/stats.out --- /tmp/cirrus-ci-build/src/test/regress/expected/stats.out2022-10-01 12:07:47.779183501 + +++ /tmp/cirrus-ci-build/build/testrun/main/regress/results/stats.out 2022-10-01 12:11:38.686433303 + @@ -997,6 +997,8 @@ -- Set temp_buffers to a low value so that we can trigger writes with fewer -- inserted tuples. SET temp_buffers TO '1MB'; +ERROR: invalid value for parameter "temp_buffers": 128 +DETAIL: "temp_buffers" cannot be changed after any temporary tables have been accessed in the session. CREATE TEMPORARY TABLE test_io_local(a int, b TEXT); SELECT sum(alloc) AS io_sum_local_allocs_before FROM pg_stat_io WHERE io_context = 'local' \gset SELECT sum(read) AS io_sum_local_reads_before FROM pg_stat_io WHERE io_context = 'local' \gset @@ -1037,7 +1039,7 @@ SELECT :io_sum_local_writes_after > :io_sum_local_writes_before; ?column? -- - t + f (1 row) SELECT :io_sum_local_extends_after > :io_sum_local_extends_before; So the problem is just that something else accesses temp buffers earlier in the same test. That's likely because since you sent your email commit d7e39d72ca1c6f188b400d7d58813ff5b5b79064 Author: Tom Lane Date: 2022-09-29 12:14:39 -0400 Use actual backend IDs in pg_stat_get_backend_idset() and friends. was applied, which adds a temp table earlier in the same session. I think the easiest way to make this robust would be to just add a reconnect before the place you need to set temp_buffers, that way additional temp tables won't cause a problem. Setting the patch to waiting-for-author for now. Greetings, Andres Freund
Re: Add support for DEFAULT specification in COPY FROM
Hi,
On 2022-09-26 12:12:15 -0300, Israel Barth Rubio wrote:
> Thanks for your review! I have applied the suggested changes, and I'm
> submitting the new patch version.
cfbot shows that tests started failing with this version:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3822
https://cirrus-ci.com/task/5354378189078528?logs=test_world#L267
[11:03:09.595] == running regression test queries
==
[11:03:09.595] test file_fdw ... FAILED (test process
exited with exit code 2) 441 ms
[11:03:09.595] == shutting down postmaster
==
[11:03:09.595]
[11:03:09.595] ==
[11:03:09.595] 1 of 1 tests failed.
[11:03:09.595] ==
[11:03:09.595]
[11:03:09.595] The differences that caused some tests to fail can be viewed in
the
[11:03:09.595] file
"/tmp/cirrus-ci-build/build/testrun/file_fdw/regress/regression.diffs". A copy
of the test summary that you see
[11:03:09.595] above is saved in the file
"/tmp/cirrus-ci-build/build/testrun/file_fdw/regress/regression.out".
[11:03:09.595]
[11:03:09.595] # test failed
The reason for the failure is a crash:
https://api.cirrus-ci.com/v1/artifact/task/5354378189078528/testrun/build/testrun/file_fdw/regress/log/postmaster.log
2022-09-30 11:01:29.228 UTC client backend[26885] pg_regress/file_fdw ERROR:
cannot insert into foreign table "p1"
2022-09-30 11:01:29.228 UTC client backend[26885] pg_regress/file_fdw
STATEMENT: UPDATE pt set a = 1 where a = 2;
TRAP: FailedAssertion("CurrentMemoryContext ==
econtext->ecxt_per_tuple_memory", File:
"../src/backend/commands/copyfromparse.c", Line: 956, PID: 26885)
postgres: postgres regression [local]
SELECT(ExceptionalCondition+0x8d)[0x559ed2fdf600]
postgres: postgres regression [local] SELECT(NextCopyFrom+0x3e4)[0x559ed2c4e3cb]
/tmp/cirrus-ci-build/build/tmp_install/usr/local/lib/x86_64-linux-gnu/postgresql/file_fdw.so(+0x2eef)[0x7ff42d072eef]
postgres: postgres regression [local] SELECT(+0x2cc400)[0x559ed2cff400]
postgres: postgres regression [local] SELECT(+0x2ba0eb)[0x559ed2ced0eb]
postgres: postgres regression [local] SELECT(ExecScan+0x6d)[0x559ed2ced178]
postgres: postgres regression [local] SELECT(+0x2cc43e)[0x559ed2cff43e]
postgres: postgres regression [local] SELECT(+0x2af6d5)[0x559ed2ce26d5]
postgres: postgres regression [local]
SELECT(standard_ExecutorRun+0x15f)[0x559ed2ce28b0]
postgres: postgres regression [local] SELECT(ExecutorRun+0x25)[0x559ed2ce297e]
postgres: postgres regression [local] SELECT(+0x47275b)[0x559ed2ea575b]
postgres: postgres regression [local] SELECT(PortalRun+0x307)[0x559ed2ea71af]
postgres: postgres regression [local] SELECT(+0x47013a)[0x559ed2ea313a]
postgres: postgres regression [local] SELECT(PostgresMain+0x774)[0x559ed2ea5054]
postgres: postgres regression [local] SELECT(+0x3d41f4)[0x559ed2e071f4]
postgres: postgres regression [local] SELECT(+0x3d73a5)[0x559ed2e0a3a5]
postgres: postgres regression [local] SELECT(+0x3d75b7)[0x559ed2e0a5b7]
postgres: postgres regression [local]
SELECT(PostmasterMain+0x1215)[0x559ed2e0bc52]
postgres: postgres regression [local] SELECT(main+0x231)[0x559ed2d46f17]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xea)[0x7ff43892dd0a]
postgres: postgres regression [local] SELECT(_start+0x2a)[0x559ed2b0204a]
A full backtrace is at
https://api.cirrus-ci.com/v1/task/5354378189078528/logs/cores.log
Regards,
Andres Freund
Re: Question: test "aggregates" failed in 32-bit machine
"Jonathan S. Katz" writes: > On 10/1/22 6:57 PM, Tom Lane wrote: >> I plan to have a look tomorrow at the idea of reverting only the cost_sort >> changes, and rewriting get_cheapest_group_keys_order() to just sort the >> keys by decreasing numgroups estimates as I suggested upthread. That >> might be substantially less messy, because of fewer interactions with >> 1349d2790. > Maybe this leads to a follow-up question of do we continue to improve > what is in HEAD while reverting the code in v15 (particularly if it's > easier to do it that way)? No. I see no prospect that the cost_sort code currently in HEAD is going to become shippable in the near future. Quite aside from the plain bugs, I think it's based on untenable assumptions about how accurately we can estimate the CPU costs associated with different sort-column orders. Having said that, it's certainly possible that we should do something different in HEAD than in v15. We could do the rewrite I suggest above in HEAD while doing a straight-up revert in v15. I've been finding that 1349d2790 is sufficiently entwined with this code that the patches would look significantly different in any case, so that might be the most reliable way to proceed in v15. regards, tom lane
Re: ExecRTCheckPerms() and many prunable partitions
Hi, On 2022-09-07 18:23:06 +0900, Amit Langote wrote: > Attached updated patches. Thanks to Justin's recent patch (89d16b63527) to add -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST to the FreeBSD ci task we now see the following: https://cirrus-ci.com/task/4772259058417664 https://api.cirrus-ci.com/v1/artifact/task/4772259058417664/testrun/build/testrun/main/regress/regression.diffs diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/updatable_views.out /tmp/cirrus-ci-build/build/testrun/main/regress/results/updatable_views.out --- /tmp/cirrus-ci-build/src/test/regress/expected/updatable_views.out 2022-10-02 10:37:08.888945000 + +++ /tmp/cirrus-ci-build/build/testrun/main/regress/results/updatable_views.out 2022-10-02 10:40:26.947887000 + @@ -1727,14 +1727,16 @@ (4 rows) UPDATE base_tbl SET id = 2000 WHERE id = 2; +WARNING: outfuncs/readfuncs failed to produce an equal rewritten parse tree UPDATE rw_view1 SET id = 3000 WHERE id = 3; +WARNING: outfuncs/readfuncs failed to produce an equal rewritten parse tree SELECT * FROM base_tbl; id | idplus1 --+- 1 | 2 4 | 5 - 2000 |2001 - 3000 |3001 + 2000 | 3 + 3000 | 4 (4 rows) DROP TABLE base_tbl CASCADE; and many more. Greetings, Andres Freund
Re: [PoC] Improve dead tuple storage for lazy vacuum
Hi, On 2022-09-16 15:00:31 +0900, Masahiko Sawada wrote: > I've updated the radix tree patch. It's now separated into two patches. cfbot notices a compiler warning: https://cirrus-ci.com/task/6247907681632256?logs=gcc_warning#L446 [11:03:05.343] radixtree.c: In function ‘rt_iterate_next’: [11:03:05.343] radixtree.c:1758:15: error: ‘slot’ may be used uninitialized in this function [-Werror=maybe-uninitialized] [11:03:05.343] 1758 |*value_p = *((uint64 *) slot); [11:03:05.343] | ^~ Greetings, Andres Freund
Re: Improve TAP tests of pg_upgrade for cross-version tests
Hi, On 2022-07-29 16:15:26 -0500, Justin Pryzby wrote: > This was using the old psql rather than the new one. > Before v10, psql didn't have \if. > > I think Cluster.pm should be updated to support the upgrades that upgrade.sh > supported. I guess it ought to be fixed in v15. This fails tests widely, and has so for a while: https://cirrus-ci.com/build/4862820121575424 https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3649 Note that it causes timeouts, which end up chewing up a cfbot "slot" for an hour... Greetings, Andres Freund
Re: How to retain lesser paths at add_path()?
Hi, On 2022-07-31 16:05:20 -0400, Tom Lane wrote: > Thoughts? As the patch got some feedback ~2 months ago, I'm updating the status to waiting-for-author. Minor note: cfbot complains about a cpluspluscheck violation: [12:24:50.124] time make -s cpluspluscheck EXTRAFLAGS='-fmax-errors=10' [12:25:17.757] In file included from /tmp/cpluspluscheck.AoEDdi/test.cpp:3: [12:25:17.757] /tmp/cirrus-ci-build/src/include/optimizer/pathnode.h: In function ‘PathComparison path_comparison_combine(PathComparison, PathComparison)’: [12:25:17.757] /tmp/cirrus-ci-build/src/include/optimizer/pathnode.h:39:19: error: invalid conversion from ‘int’ to ‘PathComparison’ [-fpermissive] [12:25:17.757]39 | return (c1 | c2) & PATH_COMPARISON_MASK; [12:25:17.757] | ^ [12:25:17.757] | | [12:25:17.757] | int [12:25:33.857] make: *** [GNUmakefile:141: cpluspluscheck] Error 1 Greetings, Andres Freund
Re: Proposal for internal Numeric to Uint64 conversion function.
Hi, The patch for this CF entry doesn't apply currently, and it looks like that's been the case for quite a while... Greetings, Andres Freund
Re: Table AM and DROP TABLE [ Was: Table AM and DDLs]
Hi, The CF entry for this patch doesn't currently apply and there has been a bunch of feedback on the approach. Mats, are you actually waiting for further feedback right now? Greetings, Andres Freund
ssl tests aren't concurrency safe due to get_free_port()
Hi, See e.g. https://cirrus-ci.com/task/4682373060100096 2022-10-01 15:15:21.849 UTC [41962][postmaster] LOG: could not bind IPv4 address "127.0.0.1": Address already in use 2022-10-01 15:15:21.849 UTC [41962][postmaster] HINT: Is another postmaster already running on port 57003? If not, wait a few seconds and retry. 2022-10-01 15:15:21.849 UTC [41962][postmaster] WARNING: could not create listen socket for "127.0.0.1" I downloaded all test logs and grepping for 57003 shows the problem: build/testrun/ldap/001_auth/log/regress_log_001_auth 3:# Checking port 57003 4:# Found port 57003 22:# Running: /usr/sbin/slapd -f /tmp/cirrus-ci-build/build/testrun/ldap/001_auth/data/slapd.conf -h ldap://localhost:57002 ldaps://localhost:57003 build/testrun/ldap/001_auth/log/001_auth_node.log 253:2022-10-01 15:15:25.103 UTC [42574][client backend] [[unknown]][3/1:0] DETAIL: Connection matched pg_hba.conf line 1: "local all all ldap ldapurl="ldaps://localhost:57003/dc=example,dc=net??sub?(uid=$username)" ldaptls=1" build/testrun/ssl/001_ssltests/log/regress_log_001_ssltests 2:# Checking port 57003 3:# Found port 57003 8:Connection string: port=57003 host=/tmp/1k5yhaWLQ1 build/testrun/ssl/001_ssltests/log/001_ssltests_primary.log 2:2022-10-01 15:15:20.668 UTC [41740][postmaster] LOG: listening on Unix socket "/tmp/1k5yhaWLQ1/.s.PGSQL.57003" 58:2022-10-01 15:15:21.849 UTC [41962][postmaster] HINT: Is another postmaster already running on port 57003? If not, wait a few seconds and retry. I.e. we chose the same port for slapd as part of ldap's 001_auth.pl as for the postgres instance of ssl's 001_ssltests.pl. I don't think get_free_port() has any chance of being reliable as is. It's fundamentally racy just among concurrently running tests, without even considering things external to the tests (given it's using the range of ports auto-assigned for client tcp ports...). The current code is from 803466b6ffa, which said: This isn't 100% bulletproof, since conceivably something else on the machine could grab the port between the time we check and the time we actually start the server. But that's a pretty short window, so in practice this should be good enough. but I've seen this fail a couple times, so I suspect it's unfortunately not good enough. I can see a few potential ways of improving the situation: 1) Improve the port we start the search for a unique port from. We use the following bit right now: # Tracking of last port value assigned to accelerate free port lookup. $last_port_assigned = int(rand() * 16384) + 49152; It *might* be less likely that we hit conflicts if we start the search on a port based on the pid, rather than rand(). We e.g., could start at something like (($$ * 16) % 16384 + 49152), giving a decent likelihood that each test has 16 free ports. Perhaps also worth to increase the range of ports searched? 2) Use a lockfile containing a pid to protect the choice of a port within a build directory. Before accepting a port get_free_port() would check if the a lockfile exists for the port and if so, if the test using it is still alive. That will protect against racyness between multiple tests inside a build directory, but won't protect against races between multiple builds running concurrently on a machine (e.g. on a buildfarm host) 3) We could generate unique port ranges for each test and thus remove the chance of conflicts within a builddir and substantially reduce the likelihood of conflicts between build directories (as the conflict would be between two tests in different build directories, rather than all tests). This would be easy to do in the meson build, but nontrivial in autoconf afaics. 4) Add retries to the tests that need get_free_port(). That seems hard to get right, because every single restart of postgres (or some other service that we used get_free_port()) provides the potential for a new conflict. Greetings, Andres Freund
Re: Question: test "aggregates" failed in 32-bit machine
On 10/1/22 6:57 PM, Tom Lane wrote:
"Jonathan S. Katz" writes:
On 10/1/22 3:13 PM, Tom Lane wrote:
I'm still of the opinion that we need to revert this code for now.
[RMT hat, but speaking just for me] reading through Tom's analysis, this
seems to be the safest path forward. I have a few questions to better
understand:
1. How invasive would the revert be?
I've just finished constructing a draft full-reversion patch. I'm not
confident in this yet; in particular, teasing it apart from 1349d2790
("Improve performance of ORDER BY / DISTINCT aggregates") was fairly
messy. I need to look through the regression test changes and make
sure that none are surprising. But this is approximately the right
scope if we rip it out entirely.
I plan to have a look tomorrow at the idea of reverting only the cost_sort
changes, and rewriting get_cheapest_group_keys_order() to just sort the
keys by decreasing numgroups estimates as I suggested upthread. That
might be substantially less messy, because of fewer interactions with
1349d2790.
Maybe this leads to a follow-up question of do we continue to improve
what is in HEAD while reverting the code in v15 (particularly if it's
easier to do it that way)?
I know we're generally not in favor of that approach, but wanted to ask.
2. Are the other user-visible items that would be impacted?
See above. (But note that 1349d2790 is HEAD-only, not in v15.)
With the RMT hat, I'm hyperfocused on PG15 stability. We have plenty of
time time to stabilize head for v16 :)
3. Is there an option of disabling the feature by default viable?
Not one that usefully addresses my concerns. The patch did add an
enable_group_by_reordering GUC which we could change to default-off,
but it does nothing about the cost_sort behavioral changes. I would
be a little inclined to rip out that GUC in either case, because
I doubt that we need it with the more restricted change.
Understood.
I'll wait for your analysis of reverting only the cost_sort changes etc.
mentioned above.
Thanks,
Jonathan
OpenPGP_signature
Description: OpenPGP digital signature
Re: proposal: possibility to read dumped table's name from file
Hi, On 2022-10-02 00:19:59 -0700, Andres Freund wrote: > On 2022-10-01 23:56:59 -0700, Andres Freund wrote: > > On 2022-09-12 09:58:37 +0200, Daniel Gustafsson wrote: > > > Correct, fixed in the attached. > > > > Updated patch adding meson compatibility attached. > > Err, forgot to amend one hunk :( That fixed it on all platforms but windows, due to copy-pasto. I really should have stopped earlier yesterday... > +/*- > + * > + * filter.h > + * Common header file for the parser of filter file > + * > + * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group > + * Portions Copyright (c) 1994, Regents of the University of California > + * > + * src/bin/pg_dump/filter.h > + * > + *- > + */ > +#ifndef FILTER_H > +#define FILTER_H > +#include "c.h" c.h (and postgres.h, postgres_fe.h) shouldn't be included in headers. This is a common enough mistake that I'm wondering if we could automate warning about it somehow. Greetings, Andres Freund >From e0dff3c3275a69b53fdf1c628c204365bb96852f Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Wed, 7 Sep 2022 15:22:05 +0200 Subject: [PATCH v7] Add include/exclude filtering via file in pg_dump Author: Pavel Stehule Discussion: https://postgr.es/m/CAFj8pRB10wvW0CC9Xq=1XDs=zcqxer3cblcnza+qix4cuh-...@mail.gmail.com --- src/bin/pg_dump/.gitignore| 4 + src/bin/pg_dump/Makefile | 17 +++- src/bin/pg_dump/filter.h | 44 ++ src/bin/pg_dump/filterparse.y | 64 ++ src/bin/pg_dump/filterscan.l | 139 ++ src/bin/pg_dump/meson.build | 17 src/bin/pg_dump/pg_backup_utils.c | 33 +++ src/bin/pg_dump/pg_backup_utils.h | 1 + src/bin/pg_dump/pg_dump.c | 56 src/bin/pg_dump/pg_dumpall.c | 65 +- src/bin/pg_dump/pg_restore.c | 58 + doc/src/sgml/ref/pg_dump.sgml | 88 +++ doc/src/sgml/ref/pg_dumpall.sgml | 22 + doc/src/sgml/ref/pg_restore.sgml | 25 ++ src/tools/msvc/Mkvcbuild.pm | 4 +- 15 files changed, 633 insertions(+), 4 deletions(-) create mode 100644 src/bin/pg_dump/filter.h create mode 100644 src/bin/pg_dump/filterparse.y create mode 100644 src/bin/pg_dump/filterscan.l diff --git a/src/bin/pg_dump/.gitignore b/src/bin/pg_dump/.gitignore index e6d78127793..11f2d68bea0 100644 --- a/src/bin/pg_dump/.gitignore +++ b/src/bin/pg_dump/.gitignore @@ -2,4 +2,8 @@ /pg_dumpall /pg_restore +# Local generated source files +/filterparse.c +/filterscan.c + /tmp_check/ diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile index 9dc5a784dd2..e3befdc9b1f 100644 --- a/src/bin/pg_dump/Makefile +++ b/src/bin/pg_dump/Makefile @@ -26,6 +26,8 @@ OBJS = \ $(WIN32RES) \ compress_io.o \ dumputils.o \ + filterparse.o \ + filterscan.o \ parallel.o \ pg_backup_archiver.o \ pg_backup_custom.o \ @@ -37,14 +39,23 @@ OBJS = \ all: pg_dump pg_restore pg_dumpall +# See notes in src/backend/parser/Makefile about the following two rules +filterparse.h: filterparse.c + touch $@ + +filterparse.c: BISONFLAGS += -d + +# Force these dependencies to be known even without dependency info built: +filterparse.o filterscan.o: filterparse.h + pg_dump: pg_dump.o common.o pg_dump_sort.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils $(CC) $(CFLAGS) pg_dump.o common.o pg_dump_sort.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) pg_restore: pg_restore.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils $(CC) $(CFLAGS) pg_restore.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) -pg_dumpall: pg_dumpall.o dumputils.o $(WIN32RES) | submake-libpq submake-libpgport submake-libpgfeutils - $(CC) $(CFLAGS) pg_dumpall.o dumputils.o $(WIN32RES) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) +pg_dumpall: pg_dumpall.o dumputils.o filterparse.o filterscan.o $(WIN32RES) | submake-libpq submake-libpgport submake-libpgfeutils + $(CC) $(CFLAGS) pg_dumpall.o dumputils.o filterparse.o filterscan.o $(WIN32RES) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X) install: all installdirs $(INSTALL_PROGRAM) pg_dump$(X) '$(DESTDIR)$(bindir)'/pg_dump$(X) @@ -63,6 +74,8 @@ installcheck: uninstall: rm -f $(addprefix '$(DESTDIR)$(bindir)'/, pg_dump$(X) pg_restore$(X) pg_dumpall$(X)) +distprep: filterparse.c filterscan.c + clean distclean maintainer-clean: rm -f pg_dump$(X) pg_restore$(X) pg_dumpall$(X) $(OBJS) pg_dump.o common.o pg_dump_sort.o pg_restore.o pg_dumpall.o rm -rf tmp_check diff --git a/src/bin/pg_dump/filter.h b/src/bin/pg_dump/filter.h new file mode 100644 index 000..5dff4161f02 --- /dev/null +++ b/src/bin/pg_dump/filter.h @@ -0,0 +1,44 @@ +/*- + * + * filter.h + * Common header file for the parser
Re: pg_upgrade test failure
Hi, On 2022-09-27 11:47:37 +0530, Bharath Rupireddy wrote: > Just for the records - the same issue was also seen here [1], [2]. > > [1] https://cirrus-ci.com/task/5709014662119424?logs=check_world#L82 > [2] > https://api.cirrus-ci.com/v1/artifact/task/5709014662119424/testrun/build/testrun/pg_upgrade/002_pg_upgrade/log/regress_log_002_pg_upgrade Yea, this is at the moment one of the top sources of spurious test failures for cfbot. Just manually looking at http://cfbot.cputube.org/ for tasks that recently changed state on windows: https://cirrus-ci.com/task/6422687231770624?logs=check_world#L60 https://cirrus-ci.com/task/6408332243107840?logs=check_world#L60 https://cirrus-ci.com/task/6202259712245760?logs=check_world#L60 https://cirrus-ci.com/task/6150885981028352?logs=check_world#L60 https://cirrus-ci.com/task/5361597290905600?logs=check_world#L60 https://cirrus-ci.com/task/5177327624650752?logs=check_world#L60 https://cirrus-ci.com/task/4862503887831040?logs=check_world#L60 https://cirrus-ci.com/task/4576362479484928?logs=check_world#L60 Something needs to happen here. Greetings, Andres Freund
Re: pgstattuple: add test for coverage
> Which indeed is the case, e.g. on 32bit systems it fails: > > https://cirrus-ci.com/task/4619535222308864?logs=test_world_32#L253 > > https://api.cirrus-ci.com/v1/artifact/task/4619535222308864/testrun/build-32/testrun/pgstattuple/regress/regression.diffs > > table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | > dead_tuple_len | dead_tuple_percent | free_space | free_percent > > ---+-+---+---+--++++-- > - 1171456 |5000 |56 | 47.8 | 5000 | > 56 | 47.8 | 7452 | 0.64 > + 1138688 |5000 |54 | 47.42 | 5000 | > 54 | 47.42 | 14796 | 1.3 > (1 row) > > ... > > > You definitely can't rely on such details not to change across platforms. Thank you for letting me know I'll fix it and check if there's any problem.
Re: [Proposal] Add foreign-server health checks infrastructure
Hi, On 2022-09-21 11:56:56 +, kuroda.hay...@fujitsu.com wrote: > PSA rebased patches. I reviewed my myself and they contain changes. > E.g., move GUC-related code to option.c. This seems to reliably fail on windows. See https://cirrus-ci.com/task/6454408568373248 https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3388 https://api.cirrus-ci.com/v1/artifact/task/6454408568373248/testrun/build/testrun/postgres_fdw/regress/regression.diffs diff -w -U3 C:/cirrus/contrib/postgres_fdw/expected/postgres_fdw.out C:/cirrus/build/testrun/postgres_fdw/regress/results/postgres_fdw.out --- C:/cirrus/contrib/postgres_fdw/expected/postgres_fdw.out2022-10-02 14:47:24.486355800 + +++ C:/cirrus/build/testrun/postgres_fdw/regress/results/postgres_fdw.out 2022-10-02 15:02:03.039752800 + @@ -11478,6 +11478,8 @@ ALTER SERVER loopback OPTIONS (SET application_name 'healthcheck'); -- Set GUC for checking the health of remote servers SET postgres_fdw.health_check_interval TO '1s'; +ERROR: invalid value for parameter "postgres_fdw.health_check_interval": 1000 +DETAIL: postgres_fdw.health_check_interval must be set to 0 on this platform BEGIN; SELECT 1 FROM ft1 LIMIT 1; ?column? @@ -11495,7 +11497,15 @@ -- While sleeping the process down will be detected. SELECT pg_sleep(3); -ERROR: Foreign Server loopback might be down. + pg_sleep +-- + +(1 row) + COMMIT; +ERROR: server closed the connection unexpectedly + This probably means the server terminated abnormally + before or while processing the request. +CONTEXT: remote SQL command: COMMIT TRANSACTION -- Clean up RESET debug_discard_caches; Greetings, Andres Freund
Re: Bloom filter Pushdown Optimization for Merge Join
On Sun, Oct 2, 2022 at 6:40 AM Zhihong Yu wrote: > > > On Sat, Oct 1, 2022 at 12:45 AM Zhihong Yu wrote: > >> >> >> On Fri, Sep 30, 2022 at 9:20 PM Zhihong Yu wrote: >> >>> >>> >>> On Fri, Sep 30, 2022 at 8:40 PM Zhihong Yu wrote: >>> On Fri, Sep 30, 2022 at 3:44 PM Zheng Li wrote: > Hello, > > A bloom filter provides early filtering of rows that cannot be joined > before they would reach the join operator, the optimization is also > called a semi join filter (SJF) pushdown. Such a filter can be created > when one child of the join operator must materialize its derived table > before the other child is evaluated. > > For example, a bloom filter can be created using the the join keys for > the build side/inner side of a hash join or the outer side of a merge > join, the bloom filter can then be used to pre-filter rows on the > other side of the join operator during the scan of the base relation. > The thread about “Hash Joins vs. Bloom Filters / take 2” [1] is good > discussion on using such optimization for hash join without going into > the pushdown of the filter where its performance gain could be further > increased. > > We worked on prototyping bloom filter pushdown for both hash join and > merge join. Attached is a patch set for bloom filter pushdown for > merge join. We also plan to send the patch for hash join once we have > it rebased. > > Here is a summary of the patch set: > 1. Bloom Filter Pushdown optimizes Merge Join by filtering rows early > during the table scan instead of later on. > -The bloom filter is pushed down along the execution tree to > the target SeqScan nodes. > -Experiments show that this optimization can speed up Merge > Join by up to 36%. > > 2. The planner makes the decision to use the bloom filter based on the > estimated filtering rate and the expected performance gain. > -The planner accomplishes this by estimating four numbers per > variable - the total number of rows of the relation, the number of > distinct values for a given variable, and the minimum and maximum > value of the variable (when applicable). Using these numbers, the > planner estimates a filtering rate of a potential filter. > -Because actually creating and implementing the filter adds > more operations, there is a minimum threshold of filtering where the > filter would actually be useful. Based on testing, we query to see if > the estimated filtering rate is higher than 35%, and that informs our > decision to use a filter or not. > > 3. If using a bloom filter, the planner also adjusts the expected cost > of Merge Join based on expected performance gain. > > 4. Capability to build the bloom filter in parallel in case of > parallel SeqScan. This is done efficiently by populating a local bloom > filter for each parallel worker and then taking a bitwise OR over all > the local bloom filters to form a shared bloom filter at the end of > the parallel SeqScan. > > 5. The optimization is GUC controlled, with settings of > enable_mergejoin_semijoin_filter and force_mergejoin_semijoin_filter. > > We found in experiments that there is a significant improvement > when using the bloom filter during Merge Join. One experiment involved > joining two large tables while varying the theoretical filtering rate > (TFR) between the two tables, the TFR is defined as the percentage > that the two datasets are disjoint. Both tables in the merge join were > the same size. We tested changing the TFR to see the change in > filtering optimization. > > For example, let’s imagine t0 has 10 million rows, which contain the > numbers 1 through 10 million randomly shuffled. Also, t1 has the > numbers 4 million through 14 million randomly shuffled. Then the TFR > for a join of these two tables is 40%, since 40% of the tables are > disjoint from the other table (1 through 4 million for t0, 10 million > through 14 million for t4). > > Here is the performance test result joining two tables: > TFR: theoretical filtering rate > EFR: estimated filtering rate > AFR: actual filtering rate > HJ: hash join > MJ Default: default merge join > MJ Filter: merge join with bloom filter optimization enabled > MJ Filter Forced: merge join with bloom filter optimization forced > > TFR EFR AFR HJ MJ Default MJ Filter MJ Filter Forced > > - > 10 33.46 7.416529 226382194923160 > 20 37.27 14.85 6483 222902192821930 > 30 41.32 22.25 6395 223742071820794 > 40 45.67 29.76272
Re: Bloom filter Pushdown Optimization for Merge Join
On Sat, Oct 1, 2022 at 12:45 AM Zhihong Yu wrote: > > > On Fri, Sep 30, 2022 at 9:20 PM Zhihong Yu wrote: > >> >> >> On Fri, Sep 30, 2022 at 8:40 PM Zhihong Yu wrote: >> >>> >>> >>> On Fri, Sep 30, 2022 at 3:44 PM Zheng Li wrote: >>> Hello, A bloom filter provides early filtering of rows that cannot be joined before they would reach the join operator, the optimization is also called a semi join filter (SJF) pushdown. Such a filter can be created when one child of the join operator must materialize its derived table before the other child is evaluated. For example, a bloom filter can be created using the the join keys for the build side/inner side of a hash join or the outer side of a merge join, the bloom filter can then be used to pre-filter rows on the other side of the join operator during the scan of the base relation. The thread about “Hash Joins vs. Bloom Filters / take 2” [1] is good discussion on using such optimization for hash join without going into the pushdown of the filter where its performance gain could be further increased. We worked on prototyping bloom filter pushdown for both hash join and merge join. Attached is a patch set for bloom filter pushdown for merge join. We also plan to send the patch for hash join once we have it rebased. Here is a summary of the patch set: 1. Bloom Filter Pushdown optimizes Merge Join by filtering rows early during the table scan instead of later on. -The bloom filter is pushed down along the execution tree to the target SeqScan nodes. -Experiments show that this optimization can speed up Merge Join by up to 36%. 2. The planner makes the decision to use the bloom filter based on the estimated filtering rate and the expected performance gain. -The planner accomplishes this by estimating four numbers per variable - the total number of rows of the relation, the number of distinct values for a given variable, and the minimum and maximum value of the variable (when applicable). Using these numbers, the planner estimates a filtering rate of a potential filter. -Because actually creating and implementing the filter adds more operations, there is a minimum threshold of filtering where the filter would actually be useful. Based on testing, we query to see if the estimated filtering rate is higher than 35%, and that informs our decision to use a filter or not. 3. If using a bloom filter, the planner also adjusts the expected cost of Merge Join based on expected performance gain. 4. Capability to build the bloom filter in parallel in case of parallel SeqScan. This is done efficiently by populating a local bloom filter for each parallel worker and then taking a bitwise OR over all the local bloom filters to form a shared bloom filter at the end of the parallel SeqScan. 5. The optimization is GUC controlled, with settings of enable_mergejoin_semijoin_filter and force_mergejoin_semijoin_filter. We found in experiments that there is a significant improvement when using the bloom filter during Merge Join. One experiment involved joining two large tables while varying the theoretical filtering rate (TFR) between the two tables, the TFR is defined as the percentage that the two datasets are disjoint. Both tables in the merge join were the same size. We tested changing the TFR to see the change in filtering optimization. For example, let’s imagine t0 has 10 million rows, which contain the numbers 1 through 10 million randomly shuffled. Also, t1 has the numbers 4 million through 14 million randomly shuffled. Then the TFR for a join of these two tables is 40%, since 40% of the tables are disjoint from the other table (1 through 4 million for t0, 10 million through 14 million for t4). Here is the performance test result joining two tables: TFR: theoretical filtering rate EFR: estimated filtering rate AFR: actual filtering rate HJ: hash join MJ Default: default merge join MJ Filter: merge join with bloom filter optimization enabled MJ Filter Forced: merge join with bloom filter optimization forced TFR EFR AFR HJ MJ Default MJ Filter MJ Filter Forced - 10 33.46 7.416529 226382194923160 20 37.27 14.85 6483 222902192821930 30 41.32 22.25 6395 223742071820794 40 45.67 29.76272 219691944919410 50 50.41 37.16210 214121822218224 60 55.64 44.51 6052 21108
Re: disfavoring unparameterized nested loops
On Fri, Sep 30, 2022 at 2:24 PM Peter Geoghegan wrote: > It's not just that the risks are ludicrously high, of course. The > potential benefits must *also* be very low. It's both factors, > together. Hmm, maybe. But it also wouldn't surprise me very much if someone can come up with a test case where a nested loop with a single row (or maybe no rows) on one side or the other and it's significantly faster than any alternative plan. I believe, though, that even if such cases exist, they are probably relatively few in number compared to the cases where parameterized nested loops hurt, and the savings are probably small compared to the multiple-orders-of-magnitude slowdowns that you can get when a nested loop goes bad. But they might still be relatively large -- 2x, 3x? -- in absolute terms. In the prior discussion, the only person who showed a case in which he thought that an unparameterized nested loop might be a clear winner was Tom, but it was just sort of a general "this kind of case might be a problem" thing rather than a fully worked out example with real timings. Perhaps someone ought to try to characterize the kinds of cases he mentioned, to help us get a clearer feeling about what, if anything, we're gaining from the current scheme. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [PATCH] Add sortsupport for range types and btree_gist
On 2022-10-02 00:23:32 -0700, Andres Freund wrote:
> Updated the patch to add the minimal change for meson compat.
Now I made the same mistake of not adding the change... Clearly I need to stop
for tonight. Either way, here's the hopefully correct change.
>From f355228462b4942bec2a7e0a663331a7ab626591 Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Wed, 31 Aug 2022 19:20:43 +0200
Subject: [PATCH v4] Add sortsupport for range types and btree_gist
Incrementally building up a GiST index can result in a sub-optimal index
structure for range types.
By sorting the data before inserting it into the index will result in a
much better index.
This can provide sizeable improvements in query execution time, I/O read
time and shared block hits/reads.
Signed-off-by: Christoph Heiss
---
src/include/catalog/pg_amproc.dat | 3 +
src/include/catalog/pg_proc.dat | 3 +
src/backend/utils/adt/rangetypes_gist.c | 70 +
contrib/btree_gist/Makefile | 3 +-
contrib/btree_gist/btree_bit.c | 19
contrib/btree_gist/btree_bool.c | 22
contrib/btree_gist/btree_cash.c | 22
contrib/btree_gist/btree_enum.c | 26 +
contrib/btree_gist/btree_gist--1.7--1.8.sql | 110
contrib/btree_gist/btree_gist.control | 2 +-
contrib/btree_gist/btree_inet.c | 19
contrib/btree_gist/btree_interval.c | 19
contrib/btree_gist/btree_macaddr8.c | 19
contrib/btree_gist/btree_time.c | 19
contrib/btree_gist/meson.build | 1 +
15 files changed, 355 insertions(+), 2 deletions(-)
create mode 100644 contrib/btree_gist/btree_gist--1.7--1.8.sql
diff --git a/src/include/catalog/pg_amproc.dat b/src/include/catalog/pg_amproc.dat
index 4cc129bebd8..9318ad5fd84 100644
--- a/src/include/catalog/pg_amproc.dat
+++ b/src/include/catalog/pg_amproc.dat
@@ -600,6 +600,9 @@
{ amprocfamily => 'gist/range_ops', amproclefttype => 'anyrange',
amprocrighttype => 'anyrange', amprocnum => '7',
amproc => 'range_gist_same' },
+{ amprocfamily => 'gist/range_ops', amproclefttype => 'anyrange',
+ amprocrighttype => 'anyrange', amprocnum => '11',
+ amproc => 'range_gist_sortsupport' },
{ amprocfamily => 'gist/network_ops', amproclefttype => 'inet',
amprocrighttype => 'inet', amprocnum => '1',
amproc => 'inet_gist_consistent' },
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 68bb032d3ea..951d864f3bb 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10313,6 +10313,9 @@
{ oid => '3881', descr => 'GiST support',
proname => 'range_gist_same', prorettype => 'internal',
proargtypes => 'anyrange anyrange internal', prosrc => 'range_gist_same' },
+{ oid => '8849', descr => 'GiST support',
+ proname => 'range_gist_sortsupport', prorettype => 'void',
+ proargtypes => 'internal', prosrc => 'range_gist_sortsupport' },
{ oid => '6154', descr => 'GiST support',
proname => 'multirange_gist_consistent', prorettype => 'bool',
proargtypes => 'internal anymultirange int2 oid internal',
diff --git a/src/backend/utils/adt/rangetypes_gist.c b/src/backend/utils/adt/rangetypes_gist.c
index 777fdf0e2e9..f77fc213f83 100644
--- a/src/backend/utils/adt/rangetypes_gist.c
+++ b/src/backend/utils/adt/rangetypes_gist.c
@@ -21,6 +21,7 @@
#include "utils/fmgrprotos.h"
#include "utils/multirangetypes.h"
#include "utils/rangetypes.h"
+#include "utils/sortsupport.h"
/*
* Range class properties used to segregate different classes of ranges in
@@ -177,6 +178,7 @@ static void range_gist_double_sorting_split(TypeCacheEntry *typcache,
static void range_gist_consider_split(ConsiderSplitContext *context,
RangeBound *right_lower, int min_left_count,
RangeBound *left_upper, int max_left_count);
+static int range_gist_cmp(Datum a, Datum b, SortSupport ssup);
static int get_gist_range_class(RangeType *range);
static int single_bound_cmp(const void *a, const void *b, void *arg);
static int interval_cmp_lower(const void *a, const void *b, void *arg);
@@ -773,6 +775,20 @@ range_gist_picksplit(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(v);
}
+/*
+ * Sort support routine for fast GiST index build by sorting.
+ */
+Datum
+range_gist_sortsupport(PG_FUNCTION_ARGS)
+{
+ SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
+
+ ssup->comparator = range_gist_cmp;
+ ssup->ssup_extra = NULL;
+
+ PG_RETURN_VOID();
+}
+
/* equality comparator for GiST */
Datum
range_gist_same(PG_FUNCTION_ARGS)
@@ -1693,6 +1709,60 @@ range_gist_consider_split(ConsiderSplitContext *context,
}
}
+/*
+ * GiST sortsupport comparator for ranges.
+ *
+ * Operates solely on the lower bounds of the ranges, comparing them using
+ * range_cmp_bounds().
+ * Empty ranges are sorted before non-empty ones.
+ */
+static int
+range_gist_cmp(Datum a, Datum b, SortSupport ssup)
+{
+
Re: [PATCH] Add sortsupport for range types and btree_gist
Hi,
On 2022-08-31 21:15:40 +0200, Christoph Heiss wrote:
> Notable changes from v1:
> - gbt_enum_sortsupport() now passes on fcinfo->flinfo
> enum_cmp_internal() needs a place to cache the typcache entry.
> - inet sortsupport now uses network_cmp() directly
Updated the patch to add the minimal change for meson compat.
Greetings,
Andres Freund
>From 4f3a9a84c267ce693738cdbe3b65dc16fcf5e882 Mon Sep 17 00:00:00 2001
From: Christoph Heiss
Date: Wed, 31 Aug 2022 19:20:43 +0200
Subject: [PATCH v3] Add sortsupport for range types and btree_gist
Incrementally building up a GiST index can result in a sub-optimal index
structure for range types.
By sorting the data before inserting it into the index will result in a
much better index.
This can provide sizeable improvements in query execution time, I/O read
time and shared block hits/reads.
Signed-off-by: Christoph Heiss
---
src/include/catalog/pg_amproc.dat | 3 +
src/include/catalog/pg_proc.dat | 3 +
src/backend/utils/adt/rangetypes_gist.c | 70 +
contrib/btree_gist/Makefile | 3 +-
contrib/btree_gist/btree_bit.c | 19
contrib/btree_gist/btree_bool.c | 22
contrib/btree_gist/btree_cash.c | 22
contrib/btree_gist/btree_enum.c | 26 +
contrib/btree_gist/btree_gist--1.7--1.8.sql | 110
contrib/btree_gist/btree_gist.control | 2 +-
contrib/btree_gist/btree_inet.c | 19
contrib/btree_gist/btree_interval.c | 19
contrib/btree_gist/btree_macaddr8.c | 19
contrib/btree_gist/btree_time.c | 19
14 files changed, 354 insertions(+), 2 deletions(-)
create mode 100644 contrib/btree_gist/btree_gist--1.7--1.8.sql
diff --git a/src/include/catalog/pg_amproc.dat b/src/include/catalog/pg_amproc.dat
index 4cc129bebd8..9318ad5fd84 100644
--- a/src/include/catalog/pg_amproc.dat
+++ b/src/include/catalog/pg_amproc.dat
@@ -600,6 +600,9 @@
{ amprocfamily => 'gist/range_ops', amproclefttype => 'anyrange',
amprocrighttype => 'anyrange', amprocnum => '7',
amproc => 'range_gist_same' },
+{ amprocfamily => 'gist/range_ops', amproclefttype => 'anyrange',
+ amprocrighttype => 'anyrange', amprocnum => '11',
+ amproc => 'range_gist_sortsupport' },
{ amprocfamily => 'gist/network_ops', amproclefttype => 'inet',
amprocrighttype => 'inet', amprocnum => '1',
amproc => 'inet_gist_consistent' },
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 68bb032d3ea..951d864f3bb 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10313,6 +10313,9 @@
{ oid => '3881', descr => 'GiST support',
proname => 'range_gist_same', prorettype => 'internal',
proargtypes => 'anyrange anyrange internal', prosrc => 'range_gist_same' },
+{ oid => '8849', descr => 'GiST support',
+ proname => 'range_gist_sortsupport', prorettype => 'void',
+ proargtypes => 'internal', prosrc => 'range_gist_sortsupport' },
{ oid => '6154', descr => 'GiST support',
proname => 'multirange_gist_consistent', prorettype => 'bool',
proargtypes => 'internal anymultirange int2 oid internal',
diff --git a/src/backend/utils/adt/rangetypes_gist.c b/src/backend/utils/adt/rangetypes_gist.c
index 777fdf0e2e9..f77fc213f83 100644
--- a/src/backend/utils/adt/rangetypes_gist.c
+++ b/src/backend/utils/adt/rangetypes_gist.c
@@ -21,6 +21,7 @@
#include "utils/fmgrprotos.h"
#include "utils/multirangetypes.h"
#include "utils/rangetypes.h"
+#include "utils/sortsupport.h"
/*
* Range class properties used to segregate different classes of ranges in
@@ -177,6 +178,7 @@ static void range_gist_double_sorting_split(TypeCacheEntry *typcache,
static void range_gist_consider_split(ConsiderSplitContext *context,
RangeBound *right_lower, int min_left_count,
RangeBound *left_upper, int max_left_count);
+static int range_gist_cmp(Datum a, Datum b, SortSupport ssup);
static int get_gist_range_class(RangeType *range);
static int single_bound_cmp(const void *a, const void *b, void *arg);
static int interval_cmp_lower(const void *a, const void *b, void *arg);
@@ -773,6 +775,20 @@ range_gist_picksplit(PG_FUNCTION_ARGS)
PG_RETURN_POINTER(v);
}
+/*
+ * Sort support routine for fast GiST index build by sorting.
+ */
+Datum
+range_gist_sortsupport(PG_FUNCTION_ARGS)
+{
+ SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
+
+ ssup->comparator = range_gist_cmp;
+ ssup->ssup_extra = NULL;
+
+ PG_RETURN_VOID();
+}
+
/* equality comparator for GiST */
Datum
range_gist_same(PG_FUNCTION_ARGS)
@@ -1693,6 +1709,60 @@ range_gist_consider_split(ConsiderSplitContext *context,
}
}
+/*
+ * GiST sortsupport comparator for ranges.
+ *
+ * Operates solely on the lower bounds of the ranges, comparing them using
+ * range_cmp_bounds().
+ * Empty ranges are sorted before non-empty ones.
+ */
+static int
+range_gist_cmp(Datum a,
Re: proposal: possibility to read dumped table's name from file
Hi,
On 2022-10-01 23:56:59 -0700, Andres Freund wrote:
> On 2022-09-12 09:58:37 +0200, Daniel Gustafsson wrote:
> > Correct, fixed in the attached.
>
> Updated patch adding meson compatibility attached.
Err, forgot to amend one hunk :(
Greetings,
Andres Freund
>From fe2926ccd49a460cbaa39a8916a4dd097463b294 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson
Date: Wed, 7 Sep 2022 15:22:05 +0200
Subject: [PATCH v6] Add include/exclude filtering via file in pg_dump
Author: Pavel Stehule
Discussion: https://postgr.es/m/CAFj8pRB10wvW0CC9Xq=1XDs=zcqxer3cblcnza+qix4cuh-...@mail.gmail.com
---
src/bin/pg_dump/.gitignore| 4 +
src/bin/pg_dump/Makefile | 17 +++-
src/bin/pg_dump/filter.h | 44 ++
src/bin/pg_dump/filterparse.y | 64 ++
src/bin/pg_dump/filterscan.l | 139 ++
src/bin/pg_dump/meson.build | 18
src/bin/pg_dump/pg_backup_utils.c | 33 +++
src/bin/pg_dump/pg_backup_utils.h | 1 +
src/bin/pg_dump/pg_dump.c | 56
src/bin/pg_dump/pg_dumpall.c | 65 +-
src/bin/pg_dump/pg_restore.c | 58 +
doc/src/sgml/ref/pg_dump.sgml | 88 +++
doc/src/sgml/ref/pg_dumpall.sgml | 22 +
doc/src/sgml/ref/pg_restore.sgml | 25 ++
src/tools/msvc/Mkvcbuild.pm | 4 +-
15 files changed, 634 insertions(+), 4 deletions(-)
create mode 100644 src/bin/pg_dump/filter.h
create mode 100644 src/bin/pg_dump/filterparse.y
create mode 100644 src/bin/pg_dump/filterscan.l
diff --git a/src/bin/pg_dump/.gitignore b/src/bin/pg_dump/.gitignore
index e6d78127793..11f2d68bea0 100644
--- a/src/bin/pg_dump/.gitignore
+++ b/src/bin/pg_dump/.gitignore
@@ -2,4 +2,8 @@
/pg_dumpall
/pg_restore
+# Local generated source files
+/filterparse.c
+/filterscan.c
+
/tmp_check/
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index 9dc5a784dd2..e3befdc9b1f 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -26,6 +26,8 @@ OBJS = \
$(WIN32RES) \
compress_io.o \
dumputils.o \
+ filterparse.o \
+ filterscan.o \
parallel.o \
pg_backup_archiver.o \
pg_backup_custom.o \
@@ -37,14 +39,23 @@ OBJS = \
all: pg_dump pg_restore pg_dumpall
+# See notes in src/backend/parser/Makefile about the following two rules
+filterparse.h: filterparse.c
+ touch $@
+
+filterparse.c: BISONFLAGS += -d
+
+# Force these dependencies to be known even without dependency info built:
+filterparse.o filterscan.o: filterparse.h
+
pg_dump: pg_dump.o common.o pg_dump_sort.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) pg_dump.o common.o pg_dump_sort.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
pg_restore: pg_restore.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) pg_restore.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
-pg_dumpall: pg_dumpall.o dumputils.o $(WIN32RES) | submake-libpq submake-libpgport submake-libpgfeutils
- $(CC) $(CFLAGS) pg_dumpall.o dumputils.o $(WIN32RES) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
+pg_dumpall: pg_dumpall.o dumputils.o filterparse.o filterscan.o $(WIN32RES) | submake-libpq submake-libpgport submake-libpgfeutils
+ $(CC) $(CFLAGS) pg_dumpall.o dumputils.o filterparse.o filterscan.o $(WIN32RES) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
install: all installdirs
$(INSTALL_PROGRAM) pg_dump$(X) '$(DESTDIR)$(bindir)'/pg_dump$(X)
@@ -63,6 +74,8 @@ installcheck:
uninstall:
rm -f $(addprefix '$(DESTDIR)$(bindir)'/, pg_dump$(X) pg_restore$(X) pg_dumpall$(X))
+distprep: filterparse.c filterscan.c
+
clean distclean maintainer-clean:
rm -f pg_dump$(X) pg_restore$(X) pg_dumpall$(X) $(OBJS) pg_dump.o common.o pg_dump_sort.o pg_restore.o pg_dumpall.o
rm -rf tmp_check
diff --git a/src/bin/pg_dump/filter.h b/src/bin/pg_dump/filter.h
new file mode 100644
index 000..5dff4161f02
--- /dev/null
+++ b/src/bin/pg_dump/filter.h
@@ -0,0 +1,44 @@
+/*-
+ *
+ * filter.h
+ * Common header file for the parser of filter file
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/bin/pg_dump/filter.h
+ *
+ *-
+ */
+#ifndef FILTER_H
+#define FILTER_H
+#include "c.h"
+
+/*
+ * List of objects that can be specified in filter file
+ */
+typedef enum
+{
+ FILTER_OBJECT_TYPE_NONE,
+ FILTER_OBJECT_TYPE_DATA,
+ FILTER_OBJECT_TYPE_DATABASE,
+ FILTER_OBJECT_TYPE_FOREIGN_DATA,
+ FILTER_OBJECT_TYPE_FUNCTION,
+ FILTER_OBJECT_TYPE_INDEX,
+ FILTER_OBJECT_TYPE_SCHEMA,
+ FILTER_OBJECT_TYPE_TABLE,
+ FILTER_OBJECT_TYPE_TRIGGER,
+} FilterObjectType;
+
+extern FILE *filter_yyin;
+
+extern int filter_yylex(void);
+extern void filter_yyerror(void *priv, const char *msg);
+extern void
Re: Transparent column encryption
Hi, On 2022-09-27 15:51:25 +0200, Peter Eisentraut wrote: > Updated version with meson build system support added (for added files and > new tests). This fails on windows: https://cirrus-ci.com/task/6151847080624128 https://api.cirrus-ci.com/v1/artifact/task/6151847080624128/testrun/build/testrun/column_encryption/001_column_encryption/log/regress_log_001_column_encryption psql error: stderr: 'OPENSSL_Uplink(7FFC165CBD50,08): no OPENSSL_Applink' Greetings, Andres Freund
Re: pgstattuple: add test for coverage
Hi, On 2022-08-03 11:19:59 +0900, Dong Wook Lee wrote: > Is there no problem with selecting all the columns during SELECT statements? > I thought there might be a problem where the test results could change easily. Which indeed is the case, e.g. on 32bit systems it fails: https://cirrus-ci.com/task/4619535222308864?logs=test_world_32#L253 https://api.cirrus-ci.com/v1/artifact/task/4619535222308864/testrun/build-32/testrun/pgstattuple/regress/regression.diffs table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent ---+-+---+---+--++++-- - 1171456 |5000 |56 | 47.8 | 5000 | 56 | 47.8 | 7452 | 0.64 + 1138688 |5000 |54 | 47.42 | 5000 | 54 | 47.42 | 14796 | 1.3 (1 row) ... You definitely can't rely on such details not to change across platforms. Greetings, Andres Freund
Re: Amcheck verification of GiST and GIN
Hi,
On 2022-09-22 08:19:09 -0700, Andres Freund wrote:
> Hi,
>
> On 2022-08-17 17:28:02 +0500, Andrey Borodin wrote:
> > Here's v13. Changes:
> > 1. Fixed passing through downlink in GIN index
> > 2. Fixed GIN tests (one test case was not working)
> >
> > Thanks to Vitaliy Kukharik for trying this patches.
>
> Due to the merge of the meson based build, this patch needs to be
> adjusted. See
> https://cirrus-ci.com/build/6637154947301376
>
> The changes should be fairly simple, just mirroring the Makefile ones.
Here's an updated patch adding meson compat.
I didn't fix the following warnings:
[25/28 3 89%] Compiling C object contrib/amcheck/amcheck.dll.p/amcheck.c.obj
../../home/andres/src/postgresql/contrib/amcheck/amcheck.c: In function
‘amcheck_lock_relation_and_check’:
../../home/andres/src/postgresql/contrib/amcheck/amcheck.c:81:20: warning:
implicit declaration of function ‘NewGUCNestLevel’
[-Wimplicit-function-declaration]
81 | save_nestlevel = NewGUCNestLevel();
|^~~
../../home/andres/src/postgresql/contrib/amcheck/amcheck.c:124:2: warning:
implicit declaration of function ‘AtEOXact_GUC’; did you mean ‘AtEOXact_SMgr’?
[-Wimplicit-function-declaration]
124 | AtEOXact_GUC(false, save_nestlevel);
| ^~~~
| AtEOXact_SMgr
[26/28 2 92%] Compiling C object contrib/amcheck/amcheck.dll.p/verify_gin.c.obj
../../home/andres/src/postgresql/contrib/amcheck/verify_gin.c: In function
‘gin_check_parent_keys_consistency’:
../../home/andres/src/postgresql/contrib/amcheck/verify_gin.c:423:8: warning:
unused variable ‘heapallindexed’ [-Wunused-variable]
423 | bool heapallindexed = *((bool*)callback_state);
|^~
[28/28 1 100%] Linking target contrib/amcheck/amcheck.dll
Greetings,
Andres Freund
>From 9c2919df1419216e596819e9a3e23616d431d8d0 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin"
Date: Sat, 23 Jul 2022 14:08:10 +0500
Subject: [PATCH v14 1/3] Refactor amcheck to extract common locking routines
---
contrib/amcheck/Makefile| 2 +
contrib/amcheck/amcheck.c | 188 +++
contrib/amcheck/amcheck.h | 27 +++
contrib/amcheck/meson.build | 1 +
contrib/amcheck/verify_nbtree.c | 308
5 files changed, 297 insertions(+), 229 deletions(-)
create mode 100644 contrib/amcheck/amcheck.c
create mode 100644 contrib/amcheck/amcheck.h
diff --git a/contrib/amcheck/Makefile b/contrib/amcheck/Makefile
index b82f221e50b..f10fd9d89d5 100644
--- a/contrib/amcheck/Makefile
+++ b/contrib/amcheck/Makefile
@@ -3,11 +3,13 @@
MODULE_big = amcheck
OBJS = \
$(WIN32RES) \
+ amcheck.o \
verify_heapam.o \
verify_nbtree.o
EXTENSION = amcheck
DATA = amcheck--1.2--1.3.sql amcheck--1.1--1.2.sql amcheck--1.0--1.1.sql amcheck--1.0.sql
+
PGFILEDESC = "amcheck - function for verifying relation integrity"
REGRESS = check check_btree check_heap
diff --git a/contrib/amcheck/amcheck.c b/contrib/amcheck/amcheck.c
new file mode 100644
index 000..0194ef0d7a2
--- /dev/null
+++ b/contrib/amcheck/amcheck.c
@@ -0,0 +1,188 @@
+/*-
+ *
+ * amcheck.c
+ * Utility functions common to all access methods.
+ *
+ * Copyright (c) 2017-2019, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/amcheck/amcheck.c
+ *
+ *-
+ */
+#include "postgres.h"
+
+#include "access/genam.h"
+#include "access/table.h"
+#include "access/tableam.h"
+#include "amcheck.h"
+#include "catalog/index.h"
+#include "commands/tablecmds.h"
+
+
+static bool
+amcheck_index_mainfork_expected(Relation rel);
+
+/*
+ * Check if index relation should have a file for its main relation
+ * fork. Verification uses this to skip unlogged indexes when in hot standby
+ * mode, where there is simply nothing to verify.
+ *
+ * NB: Caller should call index_checkable()
+ * before calling here.
+ */
+static bool
+amcheck_index_mainfork_expected(Relation rel)
+{
+ if (rel->rd_rel->relpersistence != RELPERSISTENCE_UNLOGGED ||
+ !RecoveryInProgress())
+ return true;
+
+ ereport(NOTICE,
+ (errcode(ERRCODE_READ_ONLY_SQL_TRANSACTION),
+ errmsg("cannot verify unlogged index \"%s\" during recovery, skipping",
+ RelationGetRelationName(rel;
+
+ return false;
+}
+
+void
+amcheck_lock_relation_and_check(Oid indrelid, IndexCheckableCallback checkable,
+IndexDoCheckCallback check, LOCKMODE lockmode, void *state)
+{
+ Oid heapid;
+ Relation indrel;
+ Relation heaprel;
+ Oid save_userid;
+ int save_sec_context;
+ int save_nestlevel;
+
+ /*
+ * We must lock table before index to avoid deadlocks. However, if the
+ * passed indrelid isn't an index then IndexGetRelation() will fail.
+ * Rather than emitting a not-very-helpful error message, postpone
+ * complaining, expecting that the is-it-an-index test below will fail.
+
Re: proposal: possibility to read dumped table's name from file
Hi,
On 2022-09-12 09:58:37 +0200, Daniel Gustafsson wrote:
> Correct, fixed in the attached.
Updated patch adding meson compatibility attached.
Greetings,
Andres Freund
>From 5d3ba4d2d6567626ccc0019208ea4c0ea91ac866 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson
Date: Wed, 7 Sep 2022 15:22:05 +0200
Subject: [PATCH v5] Add include/exclude filtering via file in pg_dump
Author: Pavel Stehule
Discussion: https://postgr.es/m/CAFj8pRB10wvW0CC9Xq=1XDs=zcqxer3cblcnza+qix4cuh-...@mail.gmail.com
---
src/bin/pg_dump/.gitignore| 4 +
src/bin/pg_dump/Makefile | 17 +++-
src/bin/pg_dump/filter.h | 44 ++
src/bin/pg_dump/filterparse.y | 64 ++
src/bin/pg_dump/filterscan.l | 139 ++
src/bin/pg_dump/meson.build | 18
src/bin/pg_dump/pg_backup_utils.c | 33 +++
src/bin/pg_dump/pg_backup_utils.h | 1 +
src/bin/pg_dump/pg_dump.c | 56
src/bin/pg_dump/pg_dumpall.c | 65 +-
src/bin/pg_dump/pg_restore.c | 58 +
doc/src/sgml/ref/pg_dump.sgml | 88 +++
doc/src/sgml/ref/pg_dumpall.sgml | 22 +
doc/src/sgml/ref/pg_restore.sgml | 25 ++
src/tools/msvc/Mkvcbuild.pm | 4 +-
15 files changed, 634 insertions(+), 4 deletions(-)
create mode 100644 src/bin/pg_dump/filter.h
create mode 100644 src/bin/pg_dump/filterparse.y
create mode 100644 src/bin/pg_dump/filterscan.l
diff --git a/src/bin/pg_dump/.gitignore b/src/bin/pg_dump/.gitignore
index e6d78127793..11f2d68bea0 100644
--- a/src/bin/pg_dump/.gitignore
+++ b/src/bin/pg_dump/.gitignore
@@ -2,4 +2,8 @@
/pg_dumpall
/pg_restore
+# Local generated source files
+/filterparse.c
+/filterscan.c
+
/tmp_check/
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index 9dc5a784dd2..e3befdc9b1f 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -26,6 +26,8 @@ OBJS = \
$(WIN32RES) \
compress_io.o \
dumputils.o \
+ filterparse.o \
+ filterscan.o \
parallel.o \
pg_backup_archiver.o \
pg_backup_custom.o \
@@ -37,14 +39,23 @@ OBJS = \
all: pg_dump pg_restore pg_dumpall
+# See notes in src/backend/parser/Makefile about the following two rules
+filterparse.h: filterparse.c
+ touch $@
+
+filterparse.c: BISONFLAGS += -d
+
+# Force these dependencies to be known even without dependency info built:
+filterparse.o filterscan.o: filterparse.h
+
pg_dump: pg_dump.o common.o pg_dump_sort.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) pg_dump.o common.o pg_dump_sort.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
pg_restore: pg_restore.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) pg_restore.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
-pg_dumpall: pg_dumpall.o dumputils.o $(WIN32RES) | submake-libpq submake-libpgport submake-libpgfeutils
- $(CC) $(CFLAGS) pg_dumpall.o dumputils.o $(WIN32RES) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
+pg_dumpall: pg_dumpall.o dumputils.o filterparse.o filterscan.o $(WIN32RES) | submake-libpq submake-libpgport submake-libpgfeutils
+ $(CC) $(CFLAGS) pg_dumpall.o dumputils.o filterparse.o filterscan.o $(WIN32RES) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
install: all installdirs
$(INSTALL_PROGRAM) pg_dump$(X) '$(DESTDIR)$(bindir)'/pg_dump$(X)
@@ -63,6 +74,8 @@ installcheck:
uninstall:
rm -f $(addprefix '$(DESTDIR)$(bindir)'/, pg_dump$(X) pg_restore$(X) pg_dumpall$(X))
+distprep: filterparse.c filterscan.c
+
clean distclean maintainer-clean:
rm -f pg_dump$(X) pg_restore$(X) pg_dumpall$(X) $(OBJS) pg_dump.o common.o pg_dump_sort.o pg_restore.o pg_dumpall.o
rm -rf tmp_check
diff --git a/src/bin/pg_dump/filter.h b/src/bin/pg_dump/filter.h
new file mode 100644
index 000..5dff4161f02
--- /dev/null
+++ b/src/bin/pg_dump/filter.h
@@ -0,0 +1,44 @@
+/*-
+ *
+ * filter.h
+ * Common header file for the parser of filter file
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/bin/pg_dump/filter.h
+ *
+ *-
+ */
+#ifndef FILTER_H
+#define FILTER_H
+#include "c.h"
+
+/*
+ * List of objects that can be specified in filter file
+ */
+typedef enum
+{
+ FILTER_OBJECT_TYPE_NONE,
+ FILTER_OBJECT_TYPE_DATA,
+ FILTER_OBJECT_TYPE_DATABASE,
+ FILTER_OBJECT_TYPE_FOREIGN_DATA,
+ FILTER_OBJECT_TYPE_FUNCTION,
+ FILTER_OBJECT_TYPE_INDEX,
+ FILTER_OBJECT_TYPE_SCHEMA,
+ FILTER_OBJECT_TYPE_TABLE,
+ FILTER_OBJECT_TYPE_TRIGGER,
+} FilterObjectType;
+
+extern FILE *filter_yyin;
+
+extern int filter_yylex(void);
+extern void filter_yyerror(void *priv, const char *msg);
+extern void filter_scanner_init(void);
+extern void filter_scanner_finish(void);
+extern int filter_yyparse(void
