Re: Transactions involving multiple postgres foreign servers, take 2
Fujii-san, On Thu, Oct 7, 2021 at 11:37 PM Fujii Masao wrote: > On 2021/10/07 19:47, Etsuro Fujita wrote: > > On Thu, Oct 7, 2021 at 1:29 PM k.jami...@fujitsu.com > > wrote: > >> and prove that commit performance can be improved, > >> e.g. if we can commit not in serial but also possible in parallel. > > > > If it's ok with you, I'd like to work on the performance issue. What > > I have in mind is commit all remote transactions in parallel instead > > of sequentially in the postgres_fdw transaction callback, as mentioned > > above, but I think that would improve the performance even for > > one-phase commit that we already have. > > +100 I’ve started working on this. Once I have a (POC) patch, I’ll post it in a new thread, as I think it can be discussed separately. Thanks! Best regards, Etsuro Fujita
Re: More business with $Test::Builder::Level in the TAP tests
On Fri, Oct 08, 2021 at 12:14:57PM -0400, Andrew Dunstan wrote: > I think we need to be more explicit about it, especially w.r.t. indirect > calls. Every subroutine in the call stack below where you want to error > reported as coming from should contain this line. Hmm. I got to think about that for a couple of days, and the simplest, still the cleanest, phrasing I can come up with is that: This should be incremented by any subroutine part of a stack calling test routines from Test::More, like ok() or is(). Perhaps you have a better suggestion? -- Michael signature.asc Description: PGP signature
Re: Time to upgrade buildfarm coverage for some EOL'd OSes?
On 08.10.21 00:40, Tom Lane wrote: As far as that goes, I thought we had a policy against auto-detecting user-visible features. From memory, the rationale goes like "if you want feature X you should say so, so that the build will fail if we can't provide it". Thus we make you say something like --with-openssl even though it wouldn't be particularly hard to auto-detect. Peter E. can probably advocate more strongly for this approach. Yeah, there used to be RPMs shipped that accidentally omitted readline support.
Re: Reword docs of feature "Remove temporary files after backend crash"
On Sun, Oct 10, 2021 at 9:12 AM Fujii Masao wrote: > > On 2021/10/10 1:25, Bharath Rupireddy wrote: > > On Sat, Oct 9, 2021 at 9:42 PM Justin Pryzby wrote: > >> > >> I doubt there's much confusion here - crashes are treated the same. I > >> think > >> the fix would be to say "server crash" rather than backend crash. > > > > IIUC, the "server crash" includes any backend, auxiliary process, > > postmaster crash i.e. database cluster/instance crash. The commit > > cd91de0d1 especially added the temp file cleanup support if any > > backend or auxiliary process (except syslogger and stats collector) > > Also the startup process should be in this exception list? Yes, if the startup process fails, neither restart_after_crash nor remove_temp_files_after_crash code path is hit. > > crashes. The temp file cleanup in postmaster crash does exist prior to > > the commit cd91de0d1. > > > > When we add the description about the new GUC introduced by the commit > > cd91de0d1, let's be clear as to which crash triggers the new temp file > > cleanup path. > > If we really want to add this information, the description of > restart_after_crash seems more proper place to do that in. > remove_temp_files_after_crash works only when the processes are > reinitialized because restart_after_crash is enabled. IMO, we can add the new description as proposed in my v1 patch (after adding startup process to the exception list) to both the GUCs restart_after_crash and remove_temp_files_after_crash. And, in remove_temp_files_after_crash GUC description we can just add a note saying "this GUC is effective only when restart_after_crash is on". Also, I see that the restart_after_crash and remove_temp_files_after_crash descriptions in guc.c say "Remove temporary files after backend crash.". I think we can also modify them to "Remove temporary files after the backend or auxiliary process (except startup, syslogger and stats collector) crash. Thoughts? Regards, Bharath Rupireddy.
Re: Skipping logical replication transactions on subscriber side
On 04.10.21 02:31, Masahiko Sawada wrote: I guess disabling subscriptions on error/conflict and skipping the particular transactions are somewhat different types of functions. Disabling subscriptions on error/conflict seems likes a setting parameter of subscriptions. The users might want to specify this option at creation time. Whereas, skipping the particular transaction is a repair function that the user might want to use on the spot in case of a failure. I’m concerned a bit that combining these functions to one syntax could confuse the users. Also, would the skip option be dumped and restored using pg_dump? Maybe there is an argument for yes, but if not, then we probably need a different path of handling it separate from the more permanent options.
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
On 03.10.21 09:03, Michael Paquier wrote: On Sat, Oct 02, 2021 at 11:34:38PM -0400, Tom Lane wrote: Maybe we could leave test.sh in place for awhile? I'd rather not cause a flag day for buildfarm owners. (Also, how do we see this working in the back branches?) I would be fine with test.sh staying around for now. test.sh could be changed to invoke the TAP test.
Re: Bug in DefineRange() with multiranges
On 04.10.21 19:09, Sergey Shinderuk wrote: I wonder what is the proper fix. Just drop pfree() altogether or add pstrdup() instead? I see that makeMultirangeTypeName() doesn't bother freeing its buf. I think removing the pfree()s is a correct fix.
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
Noah Misch writes: > On Sat, Oct 09, 2021 at 04:34:46PM -0400, Tom Lane wrote: >> ... Now I'm wondering what >> version of IPC::Run to recommend. > You mentioned prairiedog uses IPC::Run 0.79. That's from 2005. (Perl 5.8.3 > is from 2004, and Test::More 0.87 is from 2009.) I'd just use 0.79 in the > README recipe. IPC::Run is easy to upgrade, so if we find cause to rely on a > newer version, I'd be fine updating that requirement. Yeah, since we know 0.79 works, there seems no reason to suggest a later version. The only reason to suggest an earlier version would be if some other buildfarm critter is using something older than 0.79. I'm tempted to propose adjusting configure to require IPC::Run >= 0.79, so that we can find out if that's true. If it isn't, that's still a good change to codify what our minimum expectation is. As you say, we can always move that goalpost in future if we find it necessary. However, back to the matter of the recipe. I'm feeling discouraged again because experimentation shows that cpanm insists on updating the ExtUtils suite to current while installing Test::Simple. You can then downgrade that, but it's not a complete fix, because there are some new ExtUtils modules that don't get uninstalled. There's also assorted CPAN infrastructure left behind. The closest I can get to what we want using cpanm is with this recipe: cpanm install Test::Simple@0.87_01 cpanm install IPC::Run@0.79 cpanm install ExtUtils::MakeMaker@6.50 # downgrade (Note: the actual prerequisite of this release of Test::Simple seems to be "> 6.30", but the first such version that actually passes its own tests for me is 6.50. FWIW, prairiedog currently has 6.59.) Attached is the diff of module manifests between a raw perl 5.8.3 installation and what this results in. Probably the added CPAN::Meta modules are mostly harmless, but the forced addition of JSON::PP seems annoying. AFAICT the only way to get to precisely the minimum configuration is to do the extra module installs by hand, without using cpan or cpanm. I'm probably going to go and re-set-up prairiedog that way, but it seems like a bit too much trouble to ask of most developers. Thoughts? regards, tom lane --- perlmodules.raw.583 2021-10-10 12:24:12.700060319 -0400 +++ perlmodules.makemaker650.583 2021-10-10 12:54:29.871522532 -0400 @@ -35,6 +35,16 @@ CGI::Util 1.4 CPAN 1.76_01 CPAN::FirstTime 1.60 +CPAN::Meta 2.143240 +CPAN::Meta::Converter 2.143240 +CPAN::Meta::Feature 2.143240 +CPAN::Meta::History 2.143240 +CPAN::Meta::Merge 2.143240 +CPAN::Meta::Prereqs 2.143240 +CPAN::Meta::Requirements 2.131 +CPAN::Meta::Spec 2.143240 +CPAN::Meta::Validator 2.143240 +CPAN::Meta::YAML 0.011 CPAN::Nox 1.03 Carp 1.01 Carp::Heavy 1.01 @@ -80,37 +90,47 @@ Errno 1.09_00 Exporter 5.57 Exporter::Heavy 5.57 -ExtUtils::Command 1.05 -ExtUtils::Command::MM 0.03 +ExtUtils::Command 7.62 +ExtUtils::Command::MM 6.50 ExtUtils::Constant 0.14 ExtUtils::Embed 1.250601 -ExtUtils::Install 1.32 -ExtUtils::Installed 0.08 -ExtUtils::Liblist 1.01 -ExtUtils::Liblist::Kid 1.3 -ExtUtils::MM 0.04 -ExtUtils::MM_Any 0.07 -ExtUtils::MM_BeOS 1.04 -ExtUtils::MM_Cygwin 1.06 -ExtUtils::MM_DOS 0.02 -ExtUtils::MM_MacOS 1.07 -ExtUtils::MM_NW5 2.06 -ExtUtils::MM_OS2 1.04 -ExtUtils::MM_UWIN 0.02 -ExtUtils::MM_Unix 1.42 -ExtUtils::MM_VMS 5.70 -ExtUtils::MM_Win32 1.09 -ExtUtils::MM_Win95 0.03 -ExtUtils::MY 0.01 -ExtUtils::MakeMaker 6.17 -ExtUtils::MakeMaker::bytes 0.01 -ExtUtils::MakeMaker::vmsish 0.01 -ExtUtils::Manifest 1.42 +ExtUtils::Install 2.06 +ExtUtils::Installed 2.06 +ExtUtils::Liblist 6.50 +ExtUtils::Liblist::Kid 6.5 +ExtUtils::MM 6.50 +ExtUtils::MM_AIX 6.50 +ExtUtils::MM_Any 6.50 +ExtUtils::MM_BeOS 6.50 +ExtUtils::MM_Cygwin 6.50 +ExtUtils::MM_DOS 6.5 +ExtUtils::MM_Darwin 6.50 +ExtUtils::MM_MacOS 6.5 +ExtUtils::MM_NW5 6.50 +ExtUtils::MM_OS2 6.50 +ExtUtils::MM_OS390 7.62 +ExtUtils::MM_QNX 6.50 +ExtUtils::MM_UWIN 6.5 +ExtUtils::MM_Unix 6.50 +ExtUtils::MM_VMS 6.50 +ExtUtils::MM_VOS 6.50 +ExtUtils::MM_Win32 6.50 +ExtUtils::MM_Win95 6.50 +ExtUtils::MY 6.5 +ExtUtils::MakeMaker 6.50 +ExtUtils::MakeMaker::Config 6.50 +ExtUtils::MakeMaker::Locale 7.62 +ExtUtils::MakeMaker::bytes 6.5 +ExtUtils::MakeMaker::version 7.62 +ExtUtils::MakeMaker::version::regex 7.62 +ExtUtils::MakeMaker::version::vpp 7.62 +ExtUtils::MakeMaker::vmsish 6.5 +ExtUtils::Manifest 1.70 ExtUtils::Miniperl undef -ExtUtils::Mkbootstrap 1.15 -ExtUtils::Mksymlists 1.19 -ExtUtils::Packlist 0.04 -ExtUtils::testlib 1.15 +ExtUtils::Mkbootstrap 6.50 +ExtUtils::Mksymlists 6.50 +ExtUtils::Packlist 2.06 +ExtUtils::testlib 6.5 Fatal 1.03 Fcntl 1.05 File::Basename 2.72 @@ -130,7 +150,7 @@ File::Spec::Unix 1.5 File::Spec::VMS 1.4 File::Spec::Win32 1.4 -File::Temp 0.14 +File::Temp 0.22 File::stat 1.00 FileCache 1.03 FileHandle 2.01 @@ -158,8 +178,17 @@ IPC::Msg 1.02 IPC::Open2 1.01 IPC::Open3 1.0105 +IPC::Run 0.79 +IPC::Run::Debug undef +IPC::Run::IO undef +IPC::Run::Timer u
Re: Time to upgrade buildfarm coverage for some EOL'd OSes?
On 2021-10-08 21:40, Thomas Munro wrote: Sure I can keep curculio as is. Will just upgrade morepork to OpenBSD 6.9 then. Thanks very much for doing all these upgrades! No problem. Current status is: loach: Upgraded to FreeBSD 12.2 morepork: Upgraded to OpenBSD 6.9 conchuela: Upgraded to DragonFly BSD 6.0 sidewinder: Upgraded to NetBSD 9.2 curculio: Is not able to connect to https://git.postgresql.org due to the Let's Encrypt expired CA. Without doing anything: $ git clone https://git.postgresql.org Cloning into 'git.postgresql.org'... fatal: unable to access 'https://git.postgresql.org/': SSL certificate problem: certificate has expired Modifying /etc/ssl/certs.pem by removing expired DST Root CA X3: $ git clone https://git.postgresql.org Cloning into 'git.postgresql.org'... fatal: unable to access 'https://git.postgresql.org/': SSL certificate problem: unable to get local issuer certificate Then I tried to download the new CA and Intermediate from: https://letsencrypt.org/certificates/ and adding them manually to /etc/ssl/cert.pem but no dice. Only getting: $ git clone https://git.postgresql.org Cloning into 'git.postgresql.org'... fatal: unable to access 'https://git.postgresql.org/': SSL certificate problem: unable to get local issuer certificate If anybody have any tips about how to get SSL-working again, I'll gladly take it. /Mikael
Re: Time to upgrade buildfarm coverage for some EOL'd OSes?
=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: > curculio: Is not able to connect to https://git.postgresql.org due to > the Let's Encrypt expired CA. We're working on fixing things so that git.postgresql.org will advertise a cert chain that is compatible with older OpenSSL versions. I thought that was supposed to happen this weekend, but evidently it hasn't yet. You will need an up-to-date (less than several years old) /etc/ssl/certs.pem, but no software mods should be needed. I'd counsel just waiting a day or two more before trying to resurrect curculio. regards, tom lane
Re: Time to upgrade buildfarm coverage for some EOL'd OSes?
On 2021-10-10 20:00, Tom Lane wrote: =?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= writes: curculio: Is not able to connect to https://git.postgresql.org due to the Let's Encrypt expired CA. We're working on fixing things so that git.postgresql.org will advertise a cert chain that is compatible with older OpenSSL versions. I thought that was supposed to happen this weekend, but evidently it hasn't yet. You will need an up-to-date (less than several years old) /etc/ssl/certs.pem, but no software mods should be needed. I'd counsel just waiting a day or two more before trying to resurrect curculio. OK. Cool. Then I will just sit back and relax. Another thing I used the update_personality.pl and after that the name of my animals and compiler settings looks, hmm, how to say this, not entirely correct. Example: DragonFly BSD DragonFly BSD 6.0 gcc gcc 8.3 x86_64 also the status page seems to be broken. It doesn't show any Flags anymore. But maybe that is a known problem and someone is working on that? /Mikael
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
I wrote: > The closest I can get to what we want using cpanm is with this recipe: > cpanm install Test::Simple@0.87_01 > cpanm install IPC::Run@0.79 > cpanm install ExtUtils::MakeMaker@6.50 # downgrade Upon trying to actually use the perlbrew installation, I discovered another oversight in the recipe: at least with old perl versions, you end up with a non-shared libperl, so that --with-perl fails. That leads me to the attached revision... regards, tom lane diff --git a/src/test/perl/README b/src/test/perl/README index bfc7cdcfa3..67a050b9d4 100644 --- a/src/test/perl/README +++ b/src/test/perl/README @@ -70,20 +70,36 @@ perldoc for the test modules, e.g.: perldoc src/test/perl/PostgresNode.pm -Required Perl -- +Portability +--- + +Avoid using any bleeding-edge Perl features. We have buildfarm animals +running Perl versions as old as 5.8.3, so your tests will be expected +to pass on that. -Tests must run on perl 5.8.3 and newer. perlbrew is a good way to obtain such -a Perl; see http://perlbrew.pl . +Also, do not use any non-core Perl modules except IPC::Run. Or, if you +must do so for a particular test, arrange to skip the test when the needed +module isn't present. If unsure, you can consult Module::CoreList to find +out whether a given module is part of the Perl core, and which module +versions shipped with which Perl releases. -Just install and +One way to test for compatibility with old Perl versions is to use +perlbrew; see http://perlbrew.pl . After installing that, do +export PERLBREW_CONFIGURE_FLAGS='-de -Duseshrplib' perlbrew --force install 5.8.3 perlbrew use 5.8.3 perlbrew install-cpanm -cpanm install IPC::Run +cpanm install Test::Simple@0.87_01 +cpanm install IPC::Run@0.79 +cpanm install ExtUtils::MakeMaker@6.50 # downgrade -then re-run configure to ensure the correct Perl is used when running -tests. To verify that the right Perl was found: +Then re-run Postgres' configure to ensure the correct Perl is used when +running tests. To verify that the right Perl was found: grep ^PERL= config.log + +Due to limitations of cpanm, this recipe doesn't exactly duplicate the +module list of older buildfarm animals. The discrepancies should seldom +matter, but if you want to be sure, bypass cpanm and instead manually +install the desired versions of Test::Simple and IPC::Run.
Re: Adding CI to our tree
On 02.10.21 00:27, Andres Freund wrote: The attached patch adds CI using cirrus-ci. I like this in principle. But I don't understand what the docker stuff is about. I have used Cirrus CI before, and didn't have to do anything about Docker. This could use some explanation.
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
On Sun, Oct 10, 2021 at 01:17:10PM -0400, Tom Lane wrote: > However, back to the matter of the recipe. I'm feeling discouraged > again because experimentation shows that cpanm insists on updating > the ExtUtils suite to current while installing Test::Simple. You > can then downgrade that, but it's not a complete fix, because there > are some new ExtUtils modules that don't get uninstalled. There's > also assorted CPAN infrastructure left behind. > > The closest I can get to what we want using cpanm is with this recipe: > > cpanm install Test::Simple@0.87_01 > cpanm install IPC::Run@0.79 > cpanm install ExtUtils::MakeMaker@6.50 # downgrade > > (Note: the actual prerequisite of this release of Test::Simple seems > to be "> 6.30", but the first such version that actually passes its > own tests for me is 6.50. FWIW, prairiedog currently has 6.59.) While the MakeMaker litter is annoying, I'm not too worried about it. The only other thing I'd consider is doing the MakeMaker 6.50 install before Test::Simple, not after. Then you don't pull in additional dependencies of post-6.50 MakeMaker, if any. On Sun, Oct 10, 2021 at 02:42:11PM -0400, Tom Lane wrote: > Upon trying to actually use the perlbrew installation, I discovered > another oversight in the recipe: at least with old perl versions, > you end up with a non-shared libperl, so that --with-perl fails. > > That leads me to the attached revision... Looks good. Thanks.
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
Noah Misch writes: > On Sun, Oct 10, 2021 at 01:17:10PM -0400, Tom Lane wrote: >> The closest I can get to what we want using cpanm is with this recipe: >> cpanm install Test::Simple@0.87_01 >> cpanm install IPC::Run@0.79 >> cpanm install ExtUtils::MakeMaker@6.50 # downgrade > While the MakeMaker litter is annoying, I'm not too worried about it. The > only other thing I'd consider is doing the MakeMaker 6.50 install before > Test::Simple, not after. Tried that to begin with, doesn't work. There are at least two problems: 1. Before anything else, the first invocation of "cpanm install" wants to pull in "install". That seems to be a dummy module, but it's not without side-effects: it updates ExtUtils to current. If your first request is "cpanm install ExtUtils::MakeMaker@6.50", the version specification is effectively ignored. 2. I then tried doing a second "cpanm install ExtUtils::MakeMaker@6.50", and that did successfully downgrade to 6.50 ... but then the request to update Test::Simple upgraded it again. I'm not really sure why that happened. It looks more like a cpanm bug than anything Test::Simple asked for. I didn't do exhaustive experimentation to see if putting the downgrade before "install IPC::Run" would work. I think we're best off assuming that cpanm will cause that upgrade due to phase-of-the-moon conditions, so putting the downgrade last is the most robust recipe. regards, tom lane
Re: Adding CI to our tree
Hi, On 2021-10-10 21:48:09 +0200, Peter Eisentraut wrote: > On 02.10.21 00:27, Andres Freund wrote: > > The attached patch adds CI using cirrus-ci. > > I like this in principle. But I don't understand what the docker stuff is > about. I have used Cirrus CI before, and didn't have to do anything about > Docker. This could use some explanation. You don't *have* to do anything about docker - but especially for windows it takes longer to build without your own container, because we'd need to install our dependencies every time. And that turns out to take a while. Right now the docker containers are built as part of CI (cirrus rebuilds them when the container definition changes), but that doesn't have to be that way, we could do so independently of cirrus, so that they are usable on other platforms as well - although it's advantageous to use the cirrus containers as the base, as they're cached on the buildhosts. In principle we could also use docker for the linux tests, but I found that we can get better results using full blown virtual machines. Those I currently build from a separate repo, as mentioned upthread. There is a linux docker container, but that currently runs a separate task that compiles with -Werror for gcc, clang with / without asserts. That's a separate task so that compile warnings don't prevent one from seeing whether tests worked etc. One thing I was thinking of adding to the "compile warning" task was to cross-compile postgres from linux using mingw - that's a lot faster than running the windows builds, and it's not too hard to break that accidentally. Greetings, Andres Freund
Proposal: allow database-specific role memberships
Hi all, In building off of prior art regarding the 'pg_read_all_data' and 'pg_write_all_data' roles, I would like to propose an extension to roles that would allow for database-specific role memberships (for the purpose of granting database-specific privileges) as an additional layer of abstraction. = Problem = There is currently no mechanism to grant the privileges afforded by the default roles on a per-database basis. This makes it difficult to cleanly accomplish permissions such as 'db_datareader' and 'db_datawriter' (which are database-level roles in SQL Server that respectively grant read and write access within a specific database). The recently-added 'pg_read_all_data' and 'pg_write_all_data' work similarly to 'db_datareader' and 'db_datawriter', but work cluster-wide. = Proposal = I propose an extension to the GRANT / REVOKE syntax as well as an additional column within pg_auth_members in order to track role memberships that are only effective within the specified database. Role membership (and subsequent privileges) would be calculated using the following algorithm: - Check for regular (cluster-wide) role membership (the way it works today) - Check for database-specific role membership based on the currently-connected database Attached is a proof of concept patch that implements this. = Implementation Notes = - A new column (pg_auth_members.dbid) in the system catalog that is set to InvalidOid for regular role memberships, or the oid of the given database for database-specific role memberships. - GRANT / REVOKE syntax has been extended to include the ability to specify a database-specific role membership: - "IN DATABASE database_name" would cause the GRANT to be applicable only within the specified database. - "IN CURRENT DATABASE" would cause the GRANT to be applicable only within the currently-connected database. - Omission of the clause would create a regular (cluster-wide) role membership (the way it works today). The proposed syntax (applies to REVOKE as well): GRANT role_name [, ...] TO role_specification [, ...] [ IN DATABASE database_name | IN CURRENT DATABASE ] [ WITH ADMIN OPTION ] [ GRANTED BY role_specification ] - DROP DATABASE has been updated to clean up any database-specific role memberships that are associated with the database being dropped. - pg_dump_all will dump database-specific role memberships using the "IN CURRENT DATABASE" syntax. (pg_dump has not been modified) - is_admin_of_role()'s signature has been updated to include the oid of the database being checked as a third argument. This now returns true if the member has WITH ADMIN OPTION either globally or for the database given. - roles_is_member_of() will additionally include any database-specific role memberships for the database being checked in its result set. = Example = CREATE DATABASE accounting; CREATE DATABASE sales; CREATE ROLE alice; CREATE ROLE bob; -- Alice is granted read-all privileges cluster-wide (nothing new here) GRANT pg_read_all_data TO alice; -- Bob is granted read-all privileges to just the accounting database GRANT pg_read_all_data TO bob IN DATABASE accounting; = Final Thoughts = This is my first attempt at contributing code to the project, and I would not self-identify as a C programmer. I wanted to get a sense for how receptive the contributors and community would be to this proposal and whether there were any concerns or preferred alternatives before I further embark on a fool's errand. Thoughts? Thanks, -- Kenaniah poc-database-role-membership-v1.patch Description: Binary data
Re: [PATCH] Don't block HOT update by BRIN index
po 4. 10. 2021 v 16:17 odesílatel Tomas Vondra napsal: > > Hi, > > I took a look at this patch again to see if I can get it polished and > fixed. Per the discussion, I've removed the rd_indexattr list and > replaced it with a simple flag. While doing so, I noticed a couple of > places that should have consider (init or free) rd_hotblockingattr. Thanks for finishing this. I can confirm both patches do apply without problems. I did some simple testing locally and everything worked as intended. > Patch 0001 is the v2, 0002 removes the rd_indexattr etc. > > regards > > -- > Tomas Vondra > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company
Re: pgsql: Adjust configure to insist on Perl version >= 5.8.3.
On Sun, Oct 10, 2021 at 04:10:38PM -0400, Tom Lane wrote: > Noah Misch writes: > > On Sun, Oct 10, 2021 at 01:17:10PM -0400, Tom Lane wrote: > >> The closest I can get to what we want using cpanm is with this recipe: > >> cpanm install Test::Simple@0.87_01 > >> cpanm install IPC::Run@0.79 > >> cpanm install ExtUtils::MakeMaker@6.50 # downgrade > > > While the MakeMaker litter is annoying, I'm not too worried about it. The > > only other thing I'd consider is doing the MakeMaker 6.50 install before > > Test::Simple, not after. > > Tried that to begin with, doesn't work. There are at least two problems: > > 1. Before anything else, the first invocation of "cpanm install" wants > to pull in "install". That seems to be a dummy module, but it's not > without side-effects: it updates ExtUtils to current. If your first > request is "cpanm install ExtUtils::MakeMaker@6.50", the version > specification is effectively ignored. > > 2. I then tried doing a second "cpanm install ExtUtils::MakeMaker@6.50", > and that did successfully downgrade to 6.50 ... but then the request > to update Test::Simple upgraded it again. I'm not really sure why > that happened. It looks more like a cpanm bug than anything Test::Simple > asked for. > > I didn't do exhaustive experimentation to see if putting the downgrade > before "install IPC::Run" would work. I think we're best off assuming > that cpanm will cause that upgrade due to phase-of-the-moon conditions, > so putting the downgrade last is the most robust recipe. Got it. Good enough!
Re: Proposal: allow database-specific role memberships
On Sun, Oct 10, 2021 at 2:29 PM Kenaniah Cerny wrote: > In building off of prior art regarding the 'pg_read_all_data' and > 'pg_write_all_data' roles, I would like to propose an extension to roles > that would allow for database-specific role memberships (for the purpose of > granting database-specific privileges) as an additional layer of > abstraction. > > = Problem = > > There is currently no mechanism to grant the privileges afforded by the > default roles on a per-database basis. This makes it difficult to cleanly > accomplish permissions such as 'db_datareader' and 'db_datawriter' (which > are database-level roles in SQL Server that respectively grant read and > write access within a specific database). > > The recently-added 'pg_read_all_data' and 'pg_write_all_data' work > similarly to 'db_datareader' and 'db_datawriter', but work cluster-wide. > My first impression is that this is more complex than just restricting which databases users are allowed to connect to. The added flexibility this would provide has some benefit but doesn't seem worth the added complexity. David J.
RE: Question about client_connection_check_interval
Dear Horiguchi-san, Thank you for replying! I understood I was wrong. Sorry. > You're misunderstanding here. Maybe you saw that start_xact_command() > starts the timer but note that the function is called before every > command execution. Based on your advice I read codes again and I found that start_xact_command() is called from exec_XXX functions. They are called when backend processes read first char from front-end, hence I agreed enable_timeout_after() will call very quickly if timeout is disabled. > So this is wrong. I should see the check performed as expected. That > behavior would be clearly visualized if you inserted an elog() into > pq_check_connection(). Right. As mentioned above timeout is checked basically whenever reading commands. I embedded elog() to ClientCheckTimeoutHandler() and visualized easily. > And it seems that the documentation describes the behavior correctly. > > https://www.postgresql.org/docs/14/runtime-config-connection.html > > > client_connection_check_interval (integer) > > > > Sets the time interval between optional checks that the client is > > still connected, while running queries. Yeah I agreed that, I apologize for mistaking source and doc analysis. Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Parallel vacuum workers prevent the oldest xmin from advancing
On Sat, Oct 9, 2021 at 12:13 AM Alvaro Herrera wrote: > > On 2021-Oct-06, Masahiko Sawada wrote: > > > Hi all, > > > > A customer reported that during parallel index vacuum, the oldest xmin > > doesn't advance. Normally, the calculation of oldest xmin > > (ComputeXidHorizons()) ignores xmin/xid of processes having > > PROC_IN_VACUUM flag in MyProc->statusFlags. But since parallel vacuum > > workers don’t set their statusFlags, the xmin of the parallel vacuum > > worker is considered to calculate the oldest xmin. This issue happens > > from PG13 where the parallel vacuum was introduced. I think it's a > > bug. > > Augh, yeah, I agree this is a pretty serious problem. > > > But ISTM it’d be better to copy the leader’s status flags to workers > > in ParallelWorkerMain(). I've attached a patch for HEAD. > > Hmm, this affects not only PROC_IN_VACUUM and PROC_IN_SAFE_CIC (the bug > you're fixing), but also: > > * PROC_IS_AUTOVACUUM. That seems reasonable to me -- should a parallel > worker for autovacuum be considered autovacuum too? AFAICS it's only > used by the deadlock detector, so it should be okay. However, in the > normal path, that flag is set much earlier. > > * PROC_VACUUM_FOR_WRAPAROUND. Should be innocuous I think, since the > "parent" process already has this flag and thus shouldn't be cancelled. Currently, we don't support parallel vacuum for autovacuum. So parallel workers for vacuum don't have these two flags. > * PROC_IN_LOGICAL_DECODING. Surely not set for parallel vacuum workers, > so not a problem. Agreed. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
On Sun, Oct 10, 2021 at 04:07:43PM +0200, Peter Eisentraut wrote:
> On 03.10.21 09:03, Michael Paquier wrote:
>> On Sat, Oct 02, 2021 at 11:34:38PM -0400, Tom Lane wrote:
>>> Maybe we could leave test.sh in place for awhile? I'd rather
>>> not cause a flag day for buildfarm owners. (Also, how do we
>>> see this working in the back branches?)
>>
>> I would be fine with test.sh staying around for now.
>
> test.sh could be changed to invoke the TAP test.
That would remove the possibility to run the tests of pg_upgrade with
--enable-tap-tests, which is the point I think Tom was making, because
TestUpgrade.pm in the buildfarm code just uses "make check" as of the
following:
$cmd = "cd $self->{pgsql}/src/bin/pg_upgrade && $make $instflags check";
--
Michael
signature.asc
Description: PGP signature
Re: strange case of "if ((a & b))"
On Thu, Oct 07, 2021 at 04:49:10PM +0900, Michael Paquier wrote: > Looks right. I would be tempted to keep the one in readfuncs.c > though, mostly as a matter of style. I have left this one alone, and applied the rest as of 68f7c4b. -- Michael signature.asc Description: PGP signature
Re: Parallel vacuum workers prevent the oldest xmin from advancing
On Mon, Oct 11, 2021 at 09:23:32AM +0900, Masahiko Sawada wrote: > On Sat, Oct 9, 2021 at 12:13 AM Alvaro Herrera > wrote: >> * PROC_VACUUM_FOR_WRAPAROUND. Should be innocuous I think, since the >> "parent" process already has this flag and thus shouldn't be cancelled. > > Currently, we don't support parallel vacuum for autovacuum. So > parallel workers for vacuum don't have these two flags. That's something that should IMO be marked in the code as a comment as something to worry about once/if someone begins playing with parallel autovacuum. If the change involving autovacuum is simple, I see no reason to not add this part now, though. -- Michael signature.asc Description: PGP signature
Re: strange case of "if ((a & b))"
On Mon, Oct 11, 2021 at 9:45 AM Michael Paquier wrote: > > On Thu, Oct 07, 2021 at 04:49:10PM +0900, Michael Paquier wrote: > > Looks right. I would be tempted to keep the one in readfuncs.c > > though, mostly as a matter of style. > > I have left this one alone, and applied the rest as of 68f7c4b. Thank you! Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
RE: Added schema level support for publication.
> On Friday, October 8, 2021 7:05 PM Amit Kapila
> wrote:
>
> v37-0003-Tests-for-FOR-ALL-TABLES-IN-SCHEMA-publication
> 3.
> --- a/src/bin/pg_dump/t/002_pg_dump.pl
> +++ b/src/bin/pg_dump/t/002_pg_dump.pl
> ..
> + 'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test' => {
> + create_order => 51,
> + create_sql =>
> + 'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;',
> + regexp => qr/^
> + \QALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;\E
> + /xm,
> + like => { %full_runs, section_post_data => 1, },
> + unlike => { exclude_dump_test_schema => 1, },
>
> In this test, won't it exclude the schema dump_test because of unlike?
> If so, then we don't have coverage for "ALL Tables In Schema" except
> for public schema?
>
Yes, the unlike case will exclude the schema dump_test, but I think schema
dump_test could be
dumped in like case.
I checked the log file src/bin/pg_dump/tmp_check/log/regress_log_002_pg_dump and
saw some cases were described as "should dump ALTER PUBLICATION pub3 ADD ALL
TABLES IN SCHEMA dump_test". I think in these cases schema dump_test would be
dumped.
Besides, a small comment on tab-complete.c:
else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL"))
- COMPLETE_WITH("TABLES");
- else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES")
-|| Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
"TABLE", MatchAny))
+ COMPLETE_WITH("TABLES", "TABLE IN SCHEMA");
COMPLETE_WITH("TABLES", "TABLE IN SCHEMA");
->
COMPLETE_WITH("TABLES", "TABLES IN SCHEMA");
Regards
Tang
Re: enhance pg_log_backend_memory_contexts() to log memory contexts of auxiliary processes
Thanks for working on this! On 2021-10-09 22:23, Bharath Rupireddy wrote: Hi, Currently pg_log_backend_memory_contexts() doesn't log the memory contexts of auxiliary processes such as bgwriter, checkpointer, wal writer, archiver, startup process and wal receiver. It will be useful to look at the memory contexts of these processes too, for debugging purposes and better understanding of the memory usage pattern of these processes. As the discussion below, we thought logging memory contexts of other than client backends is possible but were not sure how useful it is. After all, we have ended up restricting the target process to client backends for now. https://www.postgresql.org/message-id/0b0657d5febd0e46565a6bc9c62ba3f6%40oss.nttdata.com If we can use debuggers, it's possible to know the memory contexts e.g. using MemoryContextStats(). So IMHO if it's necessary to know memory contexts without attaching gdb for other than client backends(probably this means using under production environment), this enhancement would be pay. -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION
Re: Skipping logical replication transactions on subscriber side
On Fri, Oct 8, 2021 at 4:09 PM osumi.takami...@fujitsu.com wrote: > > On Thursday, September 30, 2021 2:45 PM Masahiko Sawada > wrote: > > I've attached updated patches that incorporate all comments I got so far. > > Please > > review them. > Hi > > > Sorry, if I misunderstand something but > did someone check what happens when we > execute ALTER SUBSCRIPTION ... RESET (streaming) > in the middle of one txn which has several streaming of data to the sub, > especially after some part of txn has been already streamed. > My intention of this is something like *if* we can find an actual harm of > this, > I wanted to suggest the necessity of a safeguard or some measure into the > patch. > > An example) > > Set the logical_decoding_work_mem = 64kB on the pub. > and create a table and subscription with streaming = true. > In addition, log_min_messages = DEBUG1 on the sub > is helpful to check the LOG on the sub in stream_open_file(). > > connect to the publisher > > BEGIN; > INSERT INTO tab VALUES (generate_series(1, 1000)); -- this exceeds the memory > limit > SELECT * FROM pg_stat_replication_slots; -- check the actual streaming > bytes&counts just in case > > connect to the subscriber > -- after checking some logs of "open file for streamed changes" on the > sub > ALTER SUBSCRIPTION mysub RESET (streaming) > > > INSERT INTO tab VALUES (generate_series(1001, 2000)); -- again, exceeds the > limit > COMMIT; > > > I observed that the subscriber doesn't > accept STREAM_COMMIT in this case but gets BEGIN&COMMIT instead at the end. > I couldn't find any apparent and immediate issue from those steps > but is that no problem ? > Probably, this kind of situation applies to other reset target options ? I think that if a subscription parameter such as ‘streaming’ and ‘binary’ is changed, an apply worker exits and the launcher starts a new worker (see maybe_reread_subscription()). So I guess that in this case, the apply worker exited during receiving streamed changes, restarted, and received the same changes with ‘streaming = off’, therefore it got BEGIN and COMMIT instead. I think that this happens even by using ‘SET (‘streaming’ = off)’. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: enhance pg_log_backend_memory_contexts() to log memory contexts of auxiliary processes
On Mon, Oct 11, 2021 at 8:21 AM torikoshia wrote: > > Thanks for working on this! > > On 2021-10-09 22:23, Bharath Rupireddy wrote: > > Hi, > > > > Currently pg_log_backend_memory_contexts() doesn't log the memory > > contexts of auxiliary processes such as bgwriter, checkpointer, wal > > writer, archiver, startup process and wal receiver. It will be useful > > to look at the memory contexts of these processes too, for debugging > > purposes and better understanding of the memory usage pattern of these > > processes. > > As the discussion below, we thought logging memory contexts of other > than client backends is possible but were not sure how useful it is. > After all, we have ended up restricting the target process to client > backends for now. > > > https://www.postgresql.org/message-id/0b0657d5febd0e46565a6bc9c62ba3f6%40oss.nttdata.com > > If we can use debuggers, it's possible to know the memory contexts e.g. > using MemoryContextStats(). > So IMHO if it's necessary to know memory contexts without attaching gdb > for other than client backends(probably this means using under > production environment), this enhancement would be pay. Thanks for providing your thoughts. Knowing memory usage of auxiliary processes is as important as backends (user session processes) without attaching debugger in production environments. There are some open points as mentioned in my first mail in this thread, I will start working on this patch once we agree on them. Regards, Bharath Rupireddy.
is possible an remote access to some macos?
Hi I would like to fix an issue https://github.com/okbob/pspg/issues/188 where the write to clipboard from pspg doesn`t work on macos. But it is hard to fix it without any macos. I am not a user of macos and I would not buy it just for this purpose. Is it possible to get some remote ssh access? Regards Pavel
Re: Fix pg_log_backend_memory_contexts() 's delay
Thanks for the patch! It might be self-evident, but since there are comments on other process handlings in HandleAutoVacLauncherInterrupts like below, how about adding a comment for the consistency? /* Process barrier events */ if (ProcSignalBarrierPending) ProcessProcSignalBarrier(); /* Process sinval catchup interrupts that happened while sleeping */ ProcessCatchupInterrupt(); Regards, -- Atsushi Torikoshi NTT DATA CORPORATION
Re: Multi-Column List Partitioning
Thanks for the patch, it applied cleanly and fixed the reported issue. I
observed another case where
In case of multi-col list partition on the same column query is not picking
partition wise join. Is this expected?
CREATE TABLE plt1 (a int, b int, c varchar) PARTITION BY LIST(c,c);
CREATE TABLE plt1_p1 PARTITION OF plt1 FOR VALUES IN
(('0001','0001'),('0002','0002'),('0003','0003'));
CREATE TABLE plt1_p2 PARTITION OF plt1 FOR VALUES IN
(('0004','0004'),('0005','0005'),('0006','0006'));
CREATE TABLE plt1_p3 PARTITION OF plt1 DEFAULT;
INSERT INTO plt1 SELECT i, i % 47, to_char(i % 11, 'FM') FROM
generate_series(0, 500) i WHERE i % 11 NOT IN (0,10);
ANALYSE plt1;
CREATE TABLE plt2 (a int, b int, c varchar) PARTITION BY LIST(c,c);
CREATE TABLE plt2_p1 PARTITION OF plt2 FOR VALUES IN
(('0001','0001'),('0002','0002'),('0003','0003'));
CREATE TABLE plt2_p2 PARTITION OF plt2 FOR VALUES IN
(('0004','0004'),('0005','0005'),('0006','0006'));
CREATE TABLE plt2_p3 PARTITION OF plt2 DEFAULT;
INSERT INTO plt2 SELECT i, i % 47, to_char(i % 11, 'FM') FROM
generate_series(0, 500) i WHERE i % 11 NOT IN (0,10);
ANALYSE plt2;
SET enable_partitionwise_join TO true;
EXPLAIN (COSTS OFF) SELECT t1.a, t1.c, t2.a, t2.c FROM plt1 t1 INNER JOIN
plt2 t2 ON t1.c = t2.c;
postgres=# EXPLAIN (COSTS OFF) SELECT t1.a, t1.c, t2.a, t2.c FROM plt1 t1
INNER JOIN plt2 t2 ON t1.c = t2.c;
QUERY PLAN
Hash Join
Hash Cond: ((t1.c)::text = (t2.c)::text)
-> Append
-> Seq Scan on plt1_p1 t1_1
-> Seq Scan on plt1_p2 t1_2
-> Seq Scan on plt1_p3 t1_3
-> Hash
-> Append
-> Seq Scan on plt2_p1 t2_1
-> Seq Scan on plt2_p2 t2_2
-> Seq Scan on plt2_p3 t2_3
(11 rows)
Thanks & Regards,
Rajkumar Raghuwanshi
On Thu, Oct 7, 2021 at 6:03 PM Nitin Jadhav
wrote:
> Thanks Rajkumar for testing.
>
> > I think it should throw an error as the partition by list has only 1
> column but we are giving 2 values.
>
> I also agree that it should throw an error in the above case. Fixed the
> issue in the attached patch. Also added related test cases to the
> regression test suite.
>
>
> > also if you see \d+ showing plt1_p1 partition value as ‘(0001,0001)’
> instead of ('0001','0001').
>
> Now throwing errors in the initial stage, this case doesn't arise.
>
> Please share if you find any other issues.
>
> Thanks & Regards,
> Nitin Jadhav
>
>
>
>
>
> On Thu, Oct 7, 2021 at 4:05 PM Rajkumar Raghuwanshi <
> rajkumar.raghuwan...@enterprisedb.com> wrote:
>
>> Thanks Nitin,
>>
>> v4 patches applied cleanly and make check is passing now. While testing
>> further I observed that if multiple values are given for a single
>> column list partition it is not giving error instead it is changing
>> values itself. Please find the example below.
>>
>> postgres=# CREATE TABLE plt1 (a int, b varchar) PARTITION BY LIST(b);
>> CREATE TABLE
>> postgres=# CREATE TABLE plt1_p1 PARTITION OF plt1 FOR VALUES IN
>> (('0001','0001'),('0002','0002'));
>> CREATE TABLE
>> postgres=# \d+ plt1;
>> Partitioned table "public.plt1"
>> Column | Type| Collation | Nullable | Default | Storage |
>> Compression | Stats target | Description
>>
>> +---+---+--+-+--+-+--+-
>> a | integer | | | | plain|
>> | |
>> b | character varying | | | | extended |
>> | |
>> Partition key: LIST (b)
>> Partitions: plt1_p1 FOR VALUES IN ('(0001,0001)', '(0002,0002)')
>>
>> I think it should throw an error as the partition by list has only 1
>> column but we are giving 2 values.
>> also if you see \d+ showing plt1_p1 partition value as ‘(0001,0001)’
>> instead of ('0001','0001').
>>
>> Thanks & Regards,
>> Rajkumar Raghuwanshi
>>
>>
>>
>> On Sun, Oct 3, 2021 at 1:52 AM Nitin Jadhav <
>> nitinjadhavpostg...@gmail.com> wrote:
>>
>>> > > On PG head + Nitin's v3 patch + Amit's Delta patch. Make check is
>>> failing with below errors.
>>> >
>>> > Thanks Rajkumar for testing.
>>> >
>>> > Here's a v2 of the delta patch that should fix both of these test
>>> > failures. As I mentioned in my last reply, my delta patch fixed what
>>> > I think were problems in Nitin's v3 patch but were not complete by
>>> > themselves. Especially, I hadn't bothered to investigate various /*
>>> > TODO: handle multi-column list partitioning */ sites to deal with my
>>> > own changes.
>>>
>>> Thanks Rajkumar for testing and Thank you Amit for working on v2 of
>>> the delta patch. Actually I had done the code changes related to
>>> partition-wise join and I was in the middle of fixing the review
>>> comments, So I could not share the patch. Anyways thanks for your
>>> efforts.
>>>
>>> > I noticed that multi-column li
Re: pg_upgrade test for binary compatibility of core data types
On Fri, Oct 01, 2021 at 04:58:41PM +0900, Michael Paquier wrote:
> I was looking at this CF entry, and what you are doing in 0004 to move
> the tweaks from pg_upgrade's test.sh to a separate SQL script that
> uses psql's meta-commands like \if to check which version we are on is
> really interesting. The patch does not apply anymore, so this needs a
> rebase. The entry has been switched as waiting on author by Tom, but
> you did not update it after sending the new versions in [1]. I am
> wondering if we could have something cleaner than just a set booleans
> as you do here for each check, as that does not help with the
> readability of the tests.
And so, I am back at this thread, looking at the set of patches
proposed from 0001 to 0004. The patches are rather messy and mix many
things and concepts, but there are basically four things that stand
out:
- test.sh is completely broken when using PG >= 14 as new version
because of the removal of the test tablespace. Older versions of
pg_regress don't support --make-tablespacedir so I am fine to stick a
couple of extra mkdirs for testtablespace/, expected/ and sql/ to
allow the script to work properly for major upgrades as a workaround,
but only if we use an old version. We need to do something here for
HEAD and REL_14_STABLE.
- The script would fail when using PG <= 11 as old version because of
WITH OIDS relations. We need to do something down to REL_12_STABLE.
I did not like much the approach taken to stick 4 ALTER TABLE queries
though (the patch was actually failing here for me), so instead I have
borrowed what the buildfarm has been doing with a DO block. That
works fine, and that's more portable.
- Not using --extra-float-digits with PG <= 11 as older version causes
a bunch of diffs in the dumps, making the whole unreadable. The patch
was doing that unconditionally for *all version*, which is not good.
We should only do that on the versions that need it, and we know the
old version number before taking any dumps so that's easy to check.
- The addition of --wal-segsize and --allow-group-access breaks the
script when using PG < 10 at initdb time as these got added in 11.
With 10 getting EOL'd next year and per the lack of complaints, I am
not excited to do anything here and I'd rather leave this out so as we
keep coverage for those options across *all* major versions upgraded
from 11~. The buildfarm has tests down to 9.2, but for devs my take
is that this is enough for now.
This is for the basics in terms of fixing test.sh and what should be
backpatched. In this aspect patches 0001 and 0002 were a bit
incorrect. I am not sure that 0003 is that interesting as designed as
we would miss any new core types introduced.
0004 is something I'd like to get done on HEAD to ease the move of the
pg_upgrade tests to TAP, but it could be made a bit easier to read by
not having all those oldpgversion_XX_YY flags grouped together for
one. So I am going to rewrite portions of it once done with the
above.
For now, attached is a patch to address the issues with test.sh that I
am planning to backpatch. This fixes the facility on HEAD, while
minimizing the diffs between the dumps. We could do more, like a
s/PROCEDURE/FUNCTION/ but that does not make the diffs really
unreadable either. I have only tested that on HEAD as new version
down to 11 as the oldest version per the business with --wal-segsize.
This still needs tests with 12~ as new version though, which is boring
but not complicated at all :)
--
Michael
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 1ba326decd..8593488907 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -23,7 +23,8 @@ standard_initdb() {
# To increase coverage of non-standard segment size and group access
# without increasing test runtime, run these tests with a custom setting.
# Also, specify "-A trust" explicitly to suppress initdb's warning.
- "$1" -N --wal-segsize 1 -g -A trust
+ # --allow-group-access and --wal-segsize have been added in v11.
+ "$1" -N --wal-segsize 1 --allow-group-access -A trust
if [ -n "$TEMP_CONFIG" -a -r "$TEMP_CONFIG" ]
then
cat "$TEMP_CONFIG" >> "$PGDATA/postgresql.conf"
@@ -107,6 +108,14 @@ EXTRA_REGRESS_OPTS="$EXTRA_REGRESS_OPTS --outputdir=$outputdir"
export EXTRA_REGRESS_OPTS
mkdir "$outputdir"
+# pg_regress --make-tablespacedir would take care of that in 14~, but this is
+# still required for older versions where this option is not supported.
+if [ "$newsrc" != "$oldsrc" ]; then
+ mkdir "$outputdir"/testtablespace
+ mkdir "$outputdir"/sql
+ mkdir "$outputdir"/expected
+fi
+
logdir=`pwd`/log
rm -rf "$logdir"
mkdir "$logdir"
@@ -163,20 +172,32 @@ createdb "regression$dbname1" || createdb_status=$?
createdb "regression$dbname2" || createdb_status=$?
createdb "regression$dbname3" || createdb_status=$?
+# Extra options to apply to the dump. This may be changed later.
+extra_dump_options=""
+
if "$MAKE" -C "$oldsrc" installcheck-pa
Re: Fix pg_log_backend_memory_contexts() 's delay
On 2021/10/11 14:28, torikoshia wrote: Thanks for the patch! It might be self-evident, but since there are comments on other process handlings in HandleAutoVacLauncherInterrupts like below, how about adding a comment for the consistency? +1 I applied such cosmetic changes to the patch. Patch attached. Barring any objection, I will commit it and back-port to v14. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 3b3df8fa8c..96332320a7 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -836,6 +836,10 @@ HandleAutoVacLauncherInterrupts(void) if (ProcSignalBarrierPending) ProcessProcSignalBarrier(); + /* Perform logging of memory contexts of this process */ + if (LogMemoryContextPending) + ProcessLogMemoryContextInterrupt(); + /* Process sinval catchup interrupts that happened while sleeping */ ProcessCatchupInterrupt(); }
Re: Parallel vacuum workers prevent the oldest xmin from advancing
On Wed, Oct 6, 2021 at 6:11 PM Masahiko Sawada wrote: > > To fix it, I thought that we change the create index code and the > vacuum code so that the individual parallel worker sets its status > flags according to the leader’s one. But ISTM it’d be better to copy > the leader’s status flags to workers in ParallelWorkerMain(). I've > attached a patch for HEAD. > +1 The fix looks reasonable to me too. Is it possible for the patch to add test cases for the two identified problem scenarios? (PROC_IN_VACUUM, PROC_IN_SAFE_IC) Regards, Greg Nancarrow Fujitsu Australia
Re: VS2022: Support Visual Studio 2022 on Windows
On Mon, Oct 04, 2021 at 08:21:52AM -0400, Andrew Dunstan wrote: > I think we'll want to wait for the official release before we add > support for it. Agreed. I am pretty sure that the version strings this patch is using are going to change until the release happens. -- Michael signature.asc Description: PGP signature
Re: Reword docs of feature "Remove temporary files after backend crash"
On 2021/10/10 22:33, Bharath Rupireddy wrote: IIUC, the "server crash" includes any backend, auxiliary process, postmaster crash i.e. database cluster/instance crash. The commit cd91de0d1 especially added the temp file cleanup support if any backend or auxiliary process (except syslogger and stats collector) We should mention not only a backend and an auxiliary processe but also background worker? Because, per Glossary, background worker is neither a backend nor an auxiliary process. Instead, maybe it's better to use "child processes" or something rather than mentioning those three processes. IMO, we can add the new description as proposed in my v1 patch (after adding startup process to the exception list) to both the GUCs restart_after_crash and remove_temp_files_after_crash. And, in remove_temp_files_after_crash GUC description we can just add a note saying "this GUC is effective only when restart_after_crash is on". OK. Also, I see that the restart_after_crash and remove_temp_files_after_crash descriptions in guc.c say "Remove temporary files after backend crash.". I think we can also modify them to "Remove temporary files after the backend or auxiliary process (except startup, syslogger and stats collector) crash. I'm not sure if we really need this long log message. IMO it's enough to add that information in the docs. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Error "initial slot snapshot too large" in create replication slot
While creating an "export snapshot" I don't see any protection why the number of xids in the snapshot can not cross the "GetMaxSnapshotXidCount()"?. Basically, while converting the HISTORIC snapshot to the MVCC snapshot in "SnapBuildInitialSnapshot()", we add all the xids between snap->xmin to snap->xmax to the MVCC snap->xip array (xids for which commit were not recorded). The problem is that we add both topxids as well as the subxids into the same array and expect that the "xid" count does not cross the "GetMaxSnapshotXidCount()". So it seems like an issue but I am not sure what is the fix for this, some options are a) Don't limit the xid count in the exported snapshot and dynamically resize the array b) Increase the limit to GetMaxSnapshotXidCount() + GetMaxSnapshotSubxidCount(). But in option b) there would still be a problem that how do we handle the overflowed subtransaction? I have locally, reproduced the issue, 1. Configuration max_connections= 5 autovacuum = off max_worker_processes = 0 2.Then from pgbench I have run the attached script (test.sql) from 5 clients. ./pgbench -i postgres ./pgbench -c4 -j4 -T 3000 -f test1.sql -P1 postgres 3. Concurrently, create replication slot, [dilipkumar@localhost bin]$ ./psql "dbname=postgres replication=database" postgres[7367]=# postgres[6463]=# CREATE_REPLICATION_SLOT "slot" LOGICAL "test_decoding"; ERROR: 40001: initial slot snapshot too large LOCATION: SnapBuildInitialSnapshot, snapbuild.c:597 postgres[6463]=# CREATE_REPLICATION_SLOT "slot" LOGICAL "test_decoding"; ERROR: XX000: clearing exported snapshot in wrong transaction state LOCATION: SnapBuildClearExportedSnapshot, snapbuild.c:690 I could reproduce this issue, at least once in 8-10 attempts of creating the replication slot. Note: After that issue, I have noticed one more issue "clearing exported snapshot in wrong transaction state", that is because the "ExportInProgress" is not cleared on the transaction abort, for this, a simple fix is we can clear this state on the transaction abort, maybe I will raise this as a separate issue? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com test.sql Description: application/sql
Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()
On 2021/10/09 22:22, Bharath Rupireddy wrote: Hi, It looks like auxiliary processes will not have a valid MyBackendId as they don't call InitPostgres() and SharedInvalBackendInit() unlike backends. But the startup process (which is an auxiliary process) in hot standby mode seems to be different in the way that it does have a valid MyBackendId as SharedInvalBackendInit() gets called from InitRecoveryTransactionEnvironment(). The SharedInvalBackendInit() usually stores the MyBackendId in the caller's PGPROC structure i.e. MyProc->backendId. The auxiliary processes (including the startup process) usually register themselves in procsignal array with ProcSignalInit(MaxBackends + MyAuxProcType + 1) unlike the backends with ProcSignalInit(MyBackendId) after SharedInvalBackendInit() in InitPostgres(). The problem comes when a postgres process wants to send a multiplexed SIGUSR1 signal (probably using SendProcSignal()) to the startup process after receiving its ProcSignal->psh_slot[] with its backendId from the PGPROC (the postgres process can get the startup process PGPROC structure from AuxiliaryPidGetProc()). Remember the startup process has registered in the procsignal array with ProcSignalInit(MaxBackends + MyAuxProcType + 1), not with the ProcSignalInit(MyBackendId) like the backends did. So, the postgres process, wanting to send SIGUSR1 to the startup process, refers to the wrong ProcSignal->psh_slot[] and may not send the signal. Is this inconsistency of MyBackendId for a startup process a problem at all? Thoughts? These are the following ways I think we can fix it, if at all some other hacker agrees that it is actually an issue: 1) Fix the startup process code, probably by unregistering the procsignal array entry that was made with ProcSignalInit(MaxBackends + MyAuxProcType + 1) in AuxiliaryProcessMain() and register with ProcSignalInit(MyBackendId) immediately after SharedInvalBackendInit() calculates the MyBackendId in with SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). This seems risky to me as unregistering and registering ProcSignal array involves some barriers and during the registering and unregistering window, the startup process may miss the SIGUSR1. 2) Ensure that the process, that wants to send the startup process SIGUSR1 signal, doesn't use the backendId from the startup process PGPROC, in which case it has to loop over all the entries of ProcSignal->psh_slot[] array to find the entry with the startup process PID. It seems easier and less riskier but only caveat is that the sending process shouldn't look at the backendId from auxiliary process PGPROC, instead it should just traverse the entire proc signal array to find the right slot. 3) Add a comment around AuxiliaryPidGetProc() that says "auxiliary processes don't have valid backend ids, so don't use the backendId from the returned PGPROC". (2) and (3) seem reasonable to me. Thoughts? How about modifying SharedInvalBackendInit() so that it accepts BackendId as an argument and allocates the ProcState entry of the specified BackendId? That is, the startup process determines that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) + 1" in AuxiliaryProcessMain(), and then it passes that BackendId to SharedInvalBackendInit() in InitRecoveryTransactionEnvironment(). Maybe you need to enlarge ProcState array so that it also handles auxiliary processes if it does not for now. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
. On Fri, Oct 8, 2021 at 4:50 PM Kyotaro Horiguchi wrote: > > At Thu, 7 Oct 2021 13:20:14 +0900, Masahiko Sawada > wrote in > > Another idea to fix this problem would be that before calling > > SnapBuildCommitTxn() we create transaction entries in ReorderBuffer > > for (sub)transactions whose COMMIT record has XACT_XINFO_HAS_INVALS, > > and then mark all of them as catalog-changed by calling > > ReorderBufferXidSetCatalogChanges(). I've attached a PoC patch for > > this idea. What the patch does is essentially the same as what the > > proposed patch does. But the patch doesn't modify the > > SnapBuildCommitTxn(). And we remember the list of last running > > transactions in reorder buffer and the list is periodically purged > > during decoding RUNNING_XACTS records, eventually making it empty. > > I came up with the third way. SnapBuildCommitTxn already properly > handles the case where a ReorderBufferTXN with > RBTXN_HAS_CATALOG_CHANGES. So this issue can be resolved by create > such ReorderBufferTXNs in SnapBuildProcessRunningXacts. Thank you for the idea and patch! It's much simpler than mine. I think that creating an entry of a catalog-changed transaction in the reorder buffer before SunapBuildCommitTxn() is the right direction. After more thought, given DDLs are not likely to happen than DML in practice, probably we can always mark both the top transaction and its subtransactions as containing catalog changes if the commit record has XACT_XINFO_HAS_INVALS? I believe this is not likely to lead to overhead in practice. That way, the patch could be more simple and doesn't need the change of AssertTXNLsnOrder(). I've attached another PoC patch. Also, I've added the tests for this issue in test_decoding. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ poc_mark_all_txns_catalog_change.patch Description: Binary data
