Re: Commitfest manager for July
On Tue, Jul 4, 2023 at 5:03 AM Michael Paquier wrote: > On Mon, Jul 03, 2023 at 12:26:44PM +0200, Daniel Gustafsson wrote: > > Since this didn't get any takers, and we are in July AoE since a few > days ago, > > I guess I'll assume the role this time in the interest of moving things > along. > > I've switched the 2023-07 CF to in-progress and 2023-09 to open, let's > try to > > close patches! > > Thanks, Daniel! > -- > Michael > If nobody taking that, I can take the responsibility.-- Ibrar Ahmed
Re: explain analyze rows=%.0f
On Mon, Mar 20, 2023 at 7:56 PM Gregory Stark (as CFM) wrote: > On Wed, 4 Jan 2023 at 10:05, Ibrar Ahmed wrote: > > > > Thanks, I have modified everything as suggested, except one point > > > > > Don't use variable format strings. They're hard to read and they > > > probably defeat compile-time checks that the arguments match the > > > format string. You could use a "*" field width instead, ie > ... > > > * Another thought is that the non-text formats tend to prize output > > > consistency over readability, so maybe we should just always use 2 > > > fractional digits there, rather than trying to minimize visible > changes. > > > > In that, we need to adjust a lot of test case outputs. > > > > * We need some doc adjustments, surely, to explain what the heck this > > > means. > > That sounds like three points :) But they seem like pretty good > arguments to me and straightforward if a little tedious to adjust. > > This patch was marked Returned with Feedback and then later Waiting on > Author. And it hasn't had any updates since January. What is the state > on this feedback? If it's already done we can set the patch to Ready > for Commit and if not do you disagree with the proposed changes? > > If there is a consensus to modify the test cases' output, I am willing to make the necessary changes and adjust the patch accordingly. However, if there is a preference to keep the output of certain test cases unchanged, I can rebase and modify the patch accordingly to accommodate those preferences. > It's actually the kind of code cleanup changes I'm reluctant to bump a > patch for. It's not like a committer can't make these kinds of changes > when committing. But there are so many patches they're likely to just > focus on a different patch when there are adjustments like this > pending. > > -- > Gregory Stark > As Commitfest Manager > -- Ibrar Ahmed
Re: explain analyze rows=%.0f
On Sun, Nov 6, 2022 at 10:12 AM Tom Lane wrote: > Robert Haas writes: > > On Fri, Jul 22, 2022 at 6:47 AM Amit Kapila > wr= > ote: > >> I feel the discussion has slightly deviated which makes it unclear > >> whether this patch is required or not? > > > My opinion is that showing some fractional digits at least when > > loops>1 would be better than what we have now. It might not be the > > best thing we could do, but it would be better than doing nothing. > > Yeah, I think that is a reasonable compromise. > > Thanks, I have modified everything as suggested, except one point > I took a brief look through the patch, and I have some review > comments: > > * Code like this is pretty awful: > > appendStringInfo(es->str, > (nloops =3D=3D 1 || !HAS_DECIMAL(rows)) ? > " rows=3D%.0f loops=3D%.0f)" : " rows=3D%= > .2f loops=3D%.0f)", > rows, nloops); > > Don't use variable format strings. They're hard to read and they > probably defeat compile-time checks that the arguments match the > format string. You could use a "*" field width instead, ie > > appendStringInfo(es->str, > " rows=3D%.*f loops=3D%.0f)", > (nloops =3D=3D 1 || !HAS_DECIMAL(rows)) ?= > 2 : 0, > rows, nloops); > > That'd also allow you to reduce the code churn you've added by > splitting some appendStringInfo calls. > > * I'm fairly concerned about how stable this'll be in the buildfarm, > in particular I fear HAS_DECIMAL() is not likely to give consistent > results across platforms. I gather that an earlier version of the patch > tried to check whether the fractional part would be zero to two decimal > places, rather than whether it's exactly zero. Probably want to put > back something like that. > > * Another thought is that the non-text formats tend to prize output > consistency over readability, so maybe we should just always use 2 > fractional digits there, rather than trying to minimize visible changes. > > In that, we need to adjust a lot of test case outputs. * We need some doc adjustments, surely, to explain what the heck this > means. > > regards, tom lane > -- Ibrar Ahmed
Re: [Commitfest 2022-09] Date is Over.
On Wed, Oct 5, 2022 at 3:01 PM Julien Rouhaud wrote: > Hi, > > On Wed, Oct 05, 2022 at 02:50:58PM +0500, Ibrar Ahmed wrote: > > On Wed, 5 Oct 2022 at 1:43 PM, Alvaro Herrera > > wrote: > > > > > On 2022-Oct-03, Ibrar Ahmed wrote: > > > > > > > The date of the current commitfest is over, here is the current > status of > > > > the "September 2022 commitfest." > > > > There were 296 patches in the commitfest and 58 were get committed. > > > > > > Are you moving the open patches to the next commitfest, closing some as > > > RwF, etc? I'm not clear what the status is, for the November > commitfest. > > > > > > I am also not clear about that should I move that or wait till November. > > Anybody guide me > > The CF should be marked as closed, and its entries fully processed within > a few > days after its final day. > > The general rule is that patches that have been waiting on authors for 2 > weeks > or more without any answer from the author(s) should get returned with > feedback with some message to the author, and the rest is moved to the next > commitfest. If you have the time to look at some patches and see if they > need > something else, that's always better. > > Thanks for your response; I will do that in two days. -- Ibrar Ahmed. Senior Software Engineer, PostgreSQL Consultant.
Re: [Commitfest 2022-09] Date is Over.
On Wed, 5 Oct 2022 at 1:43 PM, Alvaro Herrera wrote: > On 2022-Oct-03, Ibrar Ahmed wrote: > > > The date of the current commitfest is over, here is the current status of > > the "September 2022 commitfest." > > There were 296 patches in the commitfest and 58 were get committed. > > Are you moving the open patches to the next commitfest, closing some as > RwF, etc? I'm not clear what the status is, for the November commitfest. I am also not clear about that should I move that or wait till November. Anybody guide me > > > -- > Álvaro Herrera PostgreSQL Developer — > https://www.EnterpriseDB.com/ > "Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans) > -- Ibrar Ahmed. Senior Software Engineer, PostgreSQL Consultant.
[Commitfest 2022-09] Date is Over.
The date of the current commitfest is over, here is the current status of the "September 2022 commitfest." There were 296 patches in the commitfest and 58 were get committed. Total: 296. Needs review: 155. Waiting on Author: 41. Ready for Committer: 19. Committed: 58. Moved to next CF: 8. Returned with Feedback: 5. Rejected: 2. Withdrawn: 8. -- Ibrar Ahmed.
[Commitfest 2022-09] Last days
Just a reminder that only some days left of "September 2022 commitfest" As of now, there are "295" patches in total. Out of these 295 patches, "18" patches required committer attention, and 167 patches needed reviews. Total: 295. Needs review: 167. Waiting on Author: 44. Ready for Committer: 18. Committed: 50. Moved to next CF: 3. Returned with Feedback: 3. Rejected: 2. Withdrawn: 8. On the last days of Commitfest, I will perform these activities - For patches marked "Waiting for Author" and having at least one review, set to "Returned with Feedback" and send the appropriate email - For patches marked "Needs review * If it received at least one good review, move it to the next CF (removing the current reviewer reservation) * Otherwise, leave them pending -- Ibrar Ahmed
Re: CFM Manager
On Tue, Sep 20, 2022 at 10:45 PM Jacob Champion wrote: > On Thu, Sep 8, 2022 at 2:34 PM Jacob Champion > wrote: > > I still have yet to update the section "5 to 7 days before end of CF" > > and onward. > > Well, I've saved the hardest part for last... > > Ibrar, Hamid, have the checklist rewrites been helpful so far? Are you > planning on doing an (optional!) triage, and if so, are there any > pieces in particular you'd like me to document? > > --Jacob > I think we are good now, thanks Jacob, for the effort. -- Ibrar Ahmed
Re: Cleaning up historical portability baggage
On Mon, Aug 29, 2022 at 3:13 AM Thomas Munro wrote: > On Mon, Aug 29, 2022 at 9:40 AM Tom Lane wrote: > > Here's another bit of baggage handling: fixing up the places that > > were afraid to use fflush(NULL). We could doubtless have done > > this years ago (indeed, I found several places already using it) > > but as long as we're making a push to get rid of obsolete code, > > doing it now seems appropriate. > > +1, must be OK (pg_dump and pg_upgrade). > > Archeology: > > commit 79fcde48b229534fd4a5e07834e5e0e84dd38bee > Author: Tom Lane > Date: Sun Nov 29 01:51:56 1998 + > > Portability fix for old SunOS releases: fflush(NULL) > > > https://www.postgresql.org/message-id/199811241847.NAA04690%40tuna.uimage.com > > SunOS 4.x was still in some kind of support phase for a few more > years, but I guess they weren't working too hard on conformance and > features, given that SunOS 5 (the big BSD -> System V rewrite) had > come out in '92... > > > One thing that's not clear to me is what the appropriate rules > > should be for popen(). POSIX makes clear that you shouldn't > > expect popen() to include an fflush() itself, but we seem quite > > haphazard about whether to do one or not before popen(). In > > the case of popen(..., "r") we can expect that the child can't > > write on our stdout, but stderr could be a problem anyway. > > > > Likewise, there are some places that fflush before system(), > > but they are a minority. Again it seems like the main risk > > is duplicated or mis-ordered stderr output. > > > > I'm inclined to add fflush(NULL) before any popen() or system() > > that hasn't got one already, but did not do that in the attached. > > Couldn't hurt. (Looking around at our setvbuf() setup to check the > expected stream state in various places... and huh, I hadn't > previously noticed the thing about Windows interpreting line buffering > to mean full buffering. Pfnghghl...) > > > The patch does not apply successfully; please rebase the patch. patching file src/backend/postmaster/fork_process.c Hunk #1 FAILED at 37. 1 out of 1 hunk FAILED -- saving rejects to file src/backend/postmaster/fork_process.c.rej patching file src/backend/storage/file/fd.c Hunk #1 FAILED at 2503. 1 out of 1 hunk FAILED -- saving rejects to file src/backend/storage/file/fd.c.rej patching file src/backend/utils/error/elog.c Hunk #1 FAILED at 643. Hunk #2 FAILED at 670. -- Ibrar Ahmed
Re: Add the ability to limit the amount of memory that can be allocated to backends.
On Mon, Sep 12, 2022 at 8:30 PM Reid Thompson
wrote:
> On Fri, 2022-09-09 at 12:14 -0500, Justin Pryzby wrote:
> > On Sat, Sep 03, 2022 at 11:40:03PM -0400, Reid Thompson wrote:
> > > > > + 0, 0, INT_MAX,
> > > > > + NULL, NULL, NULL
> > > > I think this needs a maximum like INT_MAX/1024/1024
> > >
> > > Is this noting that we'd set a ceiling of 2048MB?
> >
> > The reason is that you're later multiplying it by 1024*1024, so you
> > need
> > to limit it to avoid overflowing. Compare with
> > min_dynamic_shared_memory, Log_RotationSize, maintenance_work_mem,
> > autovacuum_work_mem.
>
> What I originally attempted to implement is:
> GUC "max_total_backend_memory" max value as INT_MAX = 2147483647 MB
> (2251799812636672 bytes). And the other variables and comparisons as
> bytes represented as uint64 to avoid overflow.
>
> Is this invalid?
>
> > typo: Explicitely
>
> corrected
>
> > +errmsg("request will exceed postgresql.conf
> > defined max_total_backend_memory limit (%lu > %lu)",
> >
> > I wouldn't mention postgresql.conf - it could be in
> > postgresql.auto.conf, or an include file, or a -c parameter.
> > Suggest: allocation would exceed max_total_backend_memory limit...
> >
>
> updated
>
> >
> > + ereport(LOG, errmsg("decrease reduces reported
> > backend memory allocated below zero; setting reported to 0"));
> >
> > Suggest: deallocation would decrease backend memory below zero;
>
> updated
>
> > + {"max_total_backend_memory", PGC_SIGHUP,
> > RESOURCES_MEM,
> >
> >
> >
> > Should this be PGC_SU_BACKEND to allow a superuser to set a higher
> > limit (or no limit)?
>
> Sounds good to me. I'll update to that.
> Would PGC_SUSET be too open?
>
> > There's compilation warning under mingw cross compile due to
> > sizeof(long). See d914eb347 and other recent commits which I guess
> > is
> > the current way to handle this.
> > http://cfbot.cputube.org/reid-thompson.html
>
> updated %lu to %llu and changed cast from uint64 to
> unsigned long long in the ereport call
>
> > For performance test, you'd want to check what happens with a large
> > number of max_connections (and maybe a large number of clients). TPS
> > isn't the only thing that matters. For example, a utility command
> > might
> > sometimes do a lot of allocations (or deallocations), or a
> > "parameterized nested loop" may loop over over many outer tuples and
> > reset for each. There's also a lot of places that reset to a
> > "per-tuple" context. I started looking at its performance, but
> > nothing
> > to show yet.
>
> Thanks
>
> > Would you keep people copied on your replies ("reply all") ?
> > Otherwise
> > I (at least) may miss them. I think that's what's typical on these
> > lists (and the list tool is smart enough not to send duplicates to
> > people who are direct recipients).
>
> Ok - will do, thanks.
>
> --
> Reid Thompson
> Senior Software Engineer
> Crunchy Data, Inc.
>
> reid.thomp...@crunchydata.com
> www.crunchydata.com
>
>
> The patch does not apply; please rebase the patch.
patching file src/backend/utils/misc/guc.c
Hunk #1 FAILED at 3664.
1 out of 1 hunk FAILED -- saving rejects to file
src/backend/utils/misc/guc.c.rej
patching file src/backend/utils/misc/postgresql.conf.sample
--
Ibrar Ahmed
Re: Add parameter jit_warn_above_fraction
On Sat, Apr 9, 2022 at 8:43 PM Julien Rouhaud wrote: > On Sat, Apr 09, 2022 at 12:31:23PM -0400, Tom Lane wrote: > > Julien Rouhaud writes: > > > On Sat, Apr 09, 2022 at 10:42:12AM -0400, Tom Lane wrote: > > >> Also, good luck with "looking in the logs", because by default > > >> WARNING-level messages don't go to the postmaster log. > > > > > I must be missing something because by default log_min_messages is > WARNING, and > > > AFAICS I do get WARNING-level messages in some vanilla cluster logs, > using > > > vanilla .psqlrc. > > > > Ah, sorry, I was looking at the wrong thing namely > > log_min_error_statement. The point still holds though: if we emit this > > at level WARNING, what will get logged is just the bare message and not > > the statement that triggered it. How useful do you think that will be? > > Ah right, I did miss that "small detail". And of course I agree, as-is > that > would be entirely useless and it should be LOG, like the rest of similar > features. > > > The patch does not apply successfully; please rebase the patch. patching file src/backend/utils/misc/guc.c Hunk #1 FAILED at 642. Hunk #2 FAILED at 3943. 2 out of 2 hunks FAILED -- saving rejects to file src/backend/utils/misc/guc.c.rej patching file src/backend/utils/misc/postgresql.conf.sample patching file src/include/jit/jit.h -- Ibrar Ahmed
[Commitfest 2022-09] First week is over
Just a reminder that the first week of "September 2022 commitfest" is over, As of now, there are "295" patches in total. Out of these 295 patches, "29" patches required committer attention, and 188 patches need reviews. I think we need more reviewers to low down the number. I will keep sending the emails for the reminder and change the status of the patches entries accordingly. Thanks to everyone who is participating in commitfest. -- Ibrar Ahmed. Senior Software Engineer, PostgreSQL Consultant.
Re: BufferAlloc: don't take two simultaneous locks
On Tue, Jun 28, 2022 at 4:50 PM Yura Sokolov wrote: > В Вт, 28/06/2022 в 14:26 +0300, Yura Sokolov пишет: > > В Вт, 28/06/2022 в 14:13 +0300, Yura Sokolov пишет: > > > > > Tests: > > > - tests done on 2 socket Xeon 5220 2.20GHz with turbo bust disabled > > > (ie max frequency is 2.20GHz) > > > > Forgot to mention: > > - this time it was Centos7.9.2009 (Core) with Linux mn10 > 3.10.0-1160.el7.x86_64 > > > > Perhaps older kernel describes poor master's performance on 2 sockets > > compared to my previous results (when this server had Linux 5.10.103-1 > Debian). > > > > Or there is degradation in PostgreSQL's master branch between. > > I'll try to check today. > > No, old master commit ( 7e12256b47 Sat Mar 12 14:21:40 2022) behaves same. > So it is clearly old-kernel issue. Perhaps, futex was much slower than this > days. > > > > The patch requires a rebase; please do that. Hunk #1 FAILED at 231. Hunk #2 succeeded at 409 (offset 82 lines). 1 out of 2 hunks FAILED -- saving rejects to file src/include/storage/buf_internals.h.rej -- Ibrar Ahmed
Re: Proposal: allow database-specific role memberships
On Mon, Jul 25, 2022 at 4:03 AM Kenaniah Cerny wrote: > Hi Antonin, > > Thank you again for the detailed review and questions. It was encouraging > to see the increasing level of nuance in this latest round. > > It's not clear from the explanation of the GRANT ... IN DATABASE ... / >> GRANT >> ... IN CURRENT DATABASE ... that, even if "membership in ... will be >> effective only when the recipient is connected to the database ...", the >> ADMIN option might not be "fully effective". > > > While I'm not entirely sure what you mean by fully effective, it sounds > like you may have expected a database-specific WITH ADMIN OPTION grant to > be able to take effect when connected to a different database (such as > being able to use db_4's database-specific grants when connected to db_3). > The documentation updated in this patch specifies that membership (for > database-specific grants) would be effective only when the grantee is > connected to the same database that the grant was issued for. > > In the case of attempting to make a role grant to db_4 from within db_3, > the user would need to have a cluster-wide admin option for the role being > granted, as the test case you referenced in your example aims to verify. > > I have added a couple of lines to the documentation included with this > patch in order to clarify. > > >> Specifically on the regression tests: >> >> * The function check_memberships() has no parameters - is there a >> reason not to use a view? >> > > I believe a view would work just as well -- this was an implementation > detail that was fashioned to match the pre-existing rolenames.sql file's > test format. > > >> * I'm not sure if the pg_auth_members catalog can contain InvalidOid >> in >> other columns than dbid. Thus I think that the query in >> check_memberships() only needs an outer JOIN for the pg_database >> table, >> while the other joins can be inner. >> > > This is probably true. The tests run just as well using inner joins for > pg_roles, as this latest version of the patch reflects. > > >> * In this part >> >> SET SESSION AUTHORIZATION role_read_12_noinherit; >> SELECT * FROM data; -- error >> SET ROLE role_read_12; -- error >> SELECT * FROM data; -- error >> >> I think you don't need to query the table again if the SET ROLE >> statement >> failed and the same query had been executed before the SET ROLE. > > > I left that last query in place as a sanity check to ensure that > role_read_12's privileges were indeed not in effect after the call to SET > ROLE. > > As we appear to now be working through the minutiae, it is my hope that > this will soon be ready for merge. > > - Kenaniah > The patch requires a rebase, please do that. Hunk #5 succeeded at 454 (offset 28 lines). 1 out of 5 hunks FAILED -- saving rejects to file doc/src/sgml/ref/grant.sgml.rej ... ... -- Ibrar Ahmed
Re: Add last_vacuum_index_scans in pg_stat_all_tables
On Fri, Jul 15, 2022 at 1:49 PM Ken Kato wrote: > On 2022-07-09 03:18, Peter Geoghegan wrote: > > On Fri, Jul 8, 2022 at 10:47 AM Alvaro Herrera > > wrote: > >> Saving some sort of history would be much more useful, but of course a > >> lot more work. > > Thank you for the comments! > Yes, having some sort of history would be ideal in this case. > However, I am not sure how to implement those features at this moment, > so I will take some time to consider. > > At the same time, I think having this metrics exposed in the > pg_stat_all_tables comes in handy when tuning the > maintenance_work_mem/autovacuume_work_mem even though it shows the value > of only last vacuum/autovacuum. > > Regards, > > -- > Ken Kato > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION > > > Regression is failing on all platforms; please correct that and resubmit the patch. [06:17:08.194] Failed test: 2 [06:17:08.194] Non-zero exit status: 1 [06:17:08.194] Files=33, Tests=411, 167 wallclock secs ( 0.20 usr 0.05 sys + 37.96 cusr 21.61 csys = 59.82 CPU) [06:17:08.194] Result: FAIL [06:17:08.194] make[2]: *** [Makefile:23: check] Error 1 [06:17:08.194] make[1]: *** [Makefile:52: check-recovery-recurse] Error 2 [06:17:08.194] make: *** [GNUmakefile:71: check-world-src/test-recurse] Error 2 -- Ibrar Ahmed
Re: Summary Sort workers Stats in EXPLAIN ANALYZE
On Mon, Mar 28, 2022 at 2:55 PM Jian Guo wrote:
>
> I have updated the patch addressing the review comments, but I didn't
> moved this code block into VERBOSE mode, to keep consistency with `
> show_incremental_sort_info`:
>
>
> https://github.com/postgres/postgres/blob/d4ba8b51c76300f06cc23f4d8a41d9f7210c4866/src/backend/commands/explain.c#L2890
>
> Please review, thanks.
>
> --
> *From:* Julien Rouhaud
> *Sent:* Friday, March 25, 2022 17:04
> *To:* Jian Guo
> *Cc:* pgsql-hackers@lists.postgresql.org <
> pgsql-hackers@lists.postgresql.org>; Zhenghua Lyu
> *Subject:* Re: Summary Sort workers Stats in EXPLAIN ANALYZE
>
> ⚠ External Email
>
> Hi,
>
> On Thu, Mar 24, 2022 at 07:50:11AM +, Jian Guo wrote:
> > For a simple demo, with this explain statement：
> >
> > -- Test sort stats summary
> > set force_parallel_mode=on;
> > select explain_filter('explain (analyze, summary off, timing off, costs
> off, format json) select * from tenk1 order by unique1');
> >
> > Before this patch, we got plan like this:
> >
> >
> >"Node Type": "Sort",+
> >"Parent Relationship": "Outer", +
> >"Parallel Aware": false,+
> >"Async Capable": false, +
> >"Actual Rows": 1, +
> >"Actual Loops": 1, +
> >"Sort Key": ["unique1"],+
> >"Workers": [+
> > { +
> >"Worker Number": 0, +
> >"Sort Method": "external merge",+
> >"Sort Space Used": 2496,+
> >"Sort Space Type": "Disk" +
> > } +
> >], +
>
> > After this patch, the effected plan is this:
> >
> >"Node Type": "Sort", +
> >"Parent Relationship": "Outer",+
> >"Parallel Aware": false, +
> >"Async Capable": false,+
> >"Actual Rows": N, +
> >"Actual Loops": N, +
> >"Sort Key": ["unique1"], +
> >"Workers planned": N, +
> >"Sort Method": "external merge", +
> >"Average Sort Space Used": N, +
> >"Peak Sort Space Used": N, +
> >"Sort Space Type": "Disk", +
>
> I think the idea is interesting, however there are a few problems in the
> patch.
>
> First, I think that it should only be done in the VERBOSE OFF mode. If
> you ask
> for a VERBOSE output you don't need both the details and the summarized
> version.
>
> Other minor problems:
>
> - why (only) emitting the number of workers planned and not the number of
> workers launched?
> - the textual format is missing details about what the numbers are, which
> is
> particularly obvious since avgSpaceUsed and peakSpaceUsed don't have any
> unit
> or even space between them:
>
> +"Sort Method: %s %s: " INT64_FORMAT INT64_FORMAT
> "kB\n",
> +sortMethod, spaceType, avgSpaceUsed,
> peakSpaceUsed);
>
>
>
>
> ⚠ External Email: This email originated from outside of the organization.
> Do not click links or open attachments unless you recognize the sender.
>
The patch failed different regression tests on all platforms. Please
correct that and send an updated patch.
[06:40:02.370] Test Summary Report
[06:40:02.370] ---
[06:40:02.370] t/002_pg_upgrade.pl (Wstat: 256 Tests: 13 Failed: 1)
[06:40:02.370] Failed test: 4
[06:40:02.370] Non-zero exit status: 1
[06:40:02.370] Files=2, Tests=21, 45 wallclock secs ( 0.02 usr 0.00 sys +
3.52 cusr 2.06 csys = 5.60 CPU)
--
Ibrar Ahmed
Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
On Fri, Jul 15, 2022 at 11:22 PM Maciek Sakrejda wrote: > I ran that original test case with and without the patch. Here are the > numbers I'm seeing: > > master (best of three): > > postgres=# SELECT count(*) FROM lotsarows; > Time: 582.423 ms > > postgres=# EXPLAIN (ANALYZE, TIMING OFF) SELECT count(*) FROM lotsarows; > Time: 616.102 ms > > postgres=# EXPLAIN (ANALYZE, TIMING ON) SELECT count(*) FROM lotsarows; > Time: 1068.700 ms (00:01.069) > > patched (best of three): > > postgres=# SELECT count(*) FROM lotsarows; > Time: 550.822 ms > > postgres=# EXPLAIN (ANALYZE, TIMING OFF) SELECT count(*) FROM lotsarows; > Time: 612.572 ms > > postgres=# EXPLAIN (ANALYZE, TIMING ON) SELECT count(*) FROM lotsarows; > Time: 690.875 ms > > On Fri, Jul 1, 2022 at 10:26 AM Andres Freund wrote: > > On 2022-07-01 01:23:01 -0700, Lukas Fittl wrote: > >... > > > Known WIP problems with this patch version: > > > > > > * There appears to be a timing discrepancy I haven't yet worked out, > where > > > the \timing data reported by psql doesn't match what EXPLAIN ANALYZE > is > > > reporting. With Andres' earlier test case, I'm seeing a consistent > ~700ms > > > higher for \timing than for the EXPLAIN ANALYZE time reported on the > > > server > > > side, only when rdtsc measurement is used -- its likely there is a > problem > > > somewhere with how we perform the cycles to time conversion > > > > Could you explain a bit more what you're seeing? I just tested your > patches > > and didn't see that here. > > I did not see this either, but I did see that the execution time > reported by \timing is (for this test case) consistently 0.5-1ms > *lower* than the Execution Time reported by EXPLAIN. I did not see > that on master. Is that expected? > > Thanks, > Maciek > > > The patch requires a rebase; please rebase the patch with the latest code. Hunk #5 succeeded at 147 with fuzz 2 (offset -3 lines). Hunk #6 FAILED at 170. Hunk #7 succeeded at 165 (offset -69 lines). 2 out of 7 hunks FAILED -- saving rejects to file src/include/portability/instr_time.h.rej patching file src/tools/msvc/Mkvcbuild.pm -- Ibrar Ahmed
Re: Proposal to provide the facility to set binary format output for specific OID's per session
On Fri, Aug 12, 2022 at 5:48 PM Dave Cramer wrote: > > > On Fri, 5 Aug 2022 at 17:51, Justin Pryzby wrote: > >> On Tue, Jul 26, 2022 at 08:11:04AM -0400, Dave Cramer wrote: >> > Attached patch to correct these deficiencies. >> >> You sent a patch to be applied on top of the first patch, but cfbot >> doesn't >> know that, so it says the patch doesn't apply. >> http://cfbot.cputube.org/dave-cramer.html >> >> BTW, a previous discussion about this idea is here: >> >> https://www.postgresql.org/message-id/flat/40cbb35d-774f-23ed-3079-03f938aac...@2ndquadrant.com > > > squashed patch attached > > Dave > The patch does not apply successfully; a rebase is required. === applying patch ./0001-add-format_binary.patch patching file src/backend/tcop/postgres.c Hunk #1 succeeded at 97 (offset -8 lines). patching file src/backend/tcop/pquery.c patching file src/backend/utils/init/globals.c patching file src/backend/utils/misc/guc.c Hunk #1 succeeded at 144 (offset 1 line). Hunk #2 succeeded at 244 with fuzz 2 (offset 1 line). Hunk #3 succeeded at 4298 (offset -1 lines). Hunk #4 FAILED at 12906. 1 out of 4 hunks FAILED -- saving rejects to file src/backend/utils/misc/guc.c.rej patching file src/include/miscadmin.h -- Ibrar Ahmed
Re: Allow pageinspect's bt_page_stats function to return a set of rows instead of a single row
On Mon, Aug 1, 2022 at 11:29 PM Naeem Akhter wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: not tested > Documentation:not tested > > Looks good to me. > > The new status of this patch is: Ready for Committer > The patch has a compilation error on the latest code base, please rebase your patch. [03:08:46.087] /tmp/cirrus-ci-build/contrib/cube/cubeparse.h:77:27: error: ‘result’ was not declared in this scope [03:08:46.087] 77 | int cube_yyparse (NDBOX **result, Size scanbuflen); [03:08:46.087] | ^~ [03:08:46.087] /tmp/cirrus-ci-build/contrib/cube/cubeparse.h:77:40: error: expected primary-expression before ‘scanbuflen’ [03:08:46.087] 77 | int cube_yyparse (NDBOX **result, Size scanbuflen); ... -- Ibrar Ahmed
Re: Compilation issue on Solaris.
On Tue, Sep 6, 2022 at 9:24 AM John Naylor wrote: > On Sun, Jul 10, 2022 at 9:27 PM Tom Lane wrote: > > > > Thomas Munro writes: > > > Something bothers me about adding yet more clutter to every compile > > > line for the rest of time to solve a problem that exists only for > > > unpatched systems, and also that it's not even really a Solaris thing, > > > it's a C11 thing. > > > > I tend to agree with this standpoint: if it's only a warning, and > > it only appears in a small range of not-up-to-date Solaris builds, > > then a reasonable approach is "update your system if you don't want > > to see the warning". > > > > A positive argument for doing nothing is that there's room to worry > > whether -D__STDC_WANT_LIB_EXT1__ might have any side-effects we > > *don't* want. > > This is still listed in the CF as needing review, so I went and marked > it rejected. > > +1, Thanks > -- > John Naylor > EDB: http://www.enterprisedb.com > -- Ibrar Ahmed
Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error
On Sat, Sep 3, 2022 at 12:09 PM Julien Rouhaud wrote: > Hi, > > On Sat, Sep 03, 2022 at 10:36:37AM +0500, Ibrar Ahmed wrote: > > > > Hi Kyotaro Horiguchi, Fabien Coelho, Daniel Gustafsson, > > > > Since you haven't had time to write a review the last many days, the > author > > replied > > with a rebased patch for a long time and never heard. We've taken your > name > > off the reviewer list for this patch. Of course, you are still welcome to > > review it if you can > > find the time. We're removing your name so that other reviewers know the > > patch still needs > > attention. We understand that day jobs and other things get in the way of > > doing patch > > reviews when you want to, so please come back and review a patch or two > > later when you > > have more time. > > I thought that we decided not to remove assigned reviewers from a CF entry, > even if they didn't reply recently? See the discussion around > > https://www.postgresql.org/message-id/CA%2BTgmoZSBNhX0zCkG5T5KiQize9Aq4%2Bec%2BuqLcfBhm_%2B12MbQA%40mail.gmail.com > Ah, ok, thanks for the clarification. I will add them back. @Jacob Champion, we need to update the CommitFest Checklist [1] document accordingly. *"Reviewer Clear [reviewer name]:* * Since you haven't had time to write a review of [patch] in the last 5 days, we've taken your name off the reviewer list for this patch."* [1] https://wiki.postgresql.org/wiki/CommitFest_Checklist -- Ibrar Ahmed
Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
On Sat, Sep 3, 2022 at 3:09 AM Nathan Bossart wrote: > On Fri, Sep 02, 2022 at 05:09:14PM +0900, Michael Paquier wrote: > > I had a few hours and I've spent them looking at what you had here in > > details, and there were a few things I have tweaked before applying > > the patch. > > Thanks! > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com > > > Hi, Your patch require a rebase. Please provide the latest rebase patch. === applying patch ./v17-0001-Make-more-use-of-get_dirent_type-in-place-of-sta.patch patching file src/backend/access/heap/rewriteheap.c Hunk #1 FAILED at 113. Hunk #2 FAILED at 1213. Hunk #3 FAILED at 1221. 3 out of 3 hunks FAILED -- saving rejects to file src/backend/access/heap/rewriteheap.c.rej -- Ibrar Ahmed
Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error
On Mon, Mar 28, 2022 at 8:35 AM Yugo NAGATA wrote: > On Fri, 25 Mar 2022 16:19:54 -0400 > Tom Lane wrote: > > > Fabien COELHO writes: > > >> [...] One way to avoid these errors is to send Parse messages before > > >> pipeline mode starts. I attached a patch to fix to prepare commands > at > > >> starting of a script instead of at the first execution of the command. > > > > > ISTM that moving prepare out of command execution is a good idea, so > I'm > > > in favor of this approach: the code is simpler and cleaner. > > > ISTM that a minor impact is that the preparation is not counted in the > > > command performance statistics. I do not think that it is a problem, > even > > > if it would change detailed results under -C -r -M prepared. > > > > I am not convinced this is a great idea. The current behavior is that > > a statement is not prepared until it's about to be executed, and I think > > we chose that deliberately to avoid semantic differences between prepared > > and not-prepared mode. For example, if a script looks like > > > > CREATE FUNCTION foo(...) ...; > > SELECT foo(...); > > DROP FUNCTION foo; > > > > trying to prepare the SELECT in advance would lead to failure. > > > > We could perhaps get away with preparing the commands within a pipeline > > just before we start to execute the pipeline, but it looks to me like > > this patch tries to prepare the entire script in advance. > > > Well, the semantic differences is already in the current behavior. > Currently, pgbench fails to execute the above script in prepared mode > because it prepares the entire script in advance just before the first > command execution. This patch does not change the semantic. > > > BTW, the cfbot says the patch is failing to apply anyway ... > > I think it was sideswiped by 4a39f87ac. > > I attached the rebased patch. > > Regards, > Yugo Nagata > > -- > Yugo NAGATA > Hi Kyotaro Horiguchi, Fabien Coelho, Daniel Gustafsson, Since you haven't had time to write a review the last many days, the author replied with a rebased patch for a long time and never heard. We've taken your name off the reviewer list for this patch. Of course, you are still welcome to review it if you can find the time. We're removing your name so that other reviewers know the patch still needs attention. We understand that day jobs and other things get in the way of doing patch reviews when you want to, so please come back and review a patch or two later when you have more time. -- Ibrar Ahmed
[Commitfest 2022-09] Begins This Thursday
Hi all, Just a reminder that September 2022 commitfest will begin this coming Thursday, September 1. As of now, there have been “267” patches in total. Out of these 267 patches, “22” patches required committer attention. Unfortunately, only three patches have a committer. I think the author needs to find a committer, or the committer needs to look at the patches. 1. Fix assertion failure with barriers in parallel hash join 2. pg_dump - read data for some options from external file 3. Add non-blocking version of PQcancel 4. use has_privs_of_role() for pg_hba.conf (jconway) 5. explain analyze rows=%.0f 6. Allow pageinspect's bt_page_stats function to return a set of rows instead of a single row 7. pg_stat_statements: Track statement entry timestamp 8. Add connection active, idle time to pg_stat_activity 9. Add Amcheck option for checking unique constraints in btree indexes 10. jit_warn_above_fraction parameter 11. fix spinlock contention in LogwrtResult 12. Faster pglz compression (fuzzycz) 13. Parallel Hash Full Join (macdice) 14. KnownAssignedXidsGetAndSetXmin performance 15. psql - refactor echo code 16. Use "WAL segment" instead of "log segment" consistently in user-facing messages 17. Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work 18. pg_receivewal fail to streams when the partial file to write is not fully initialized present in the wal receiver directory 19. On client login event trigger 20. Update relfrozenxmin when truncating temp tables 21. XID formatting and SLRU refactorings (Independent part of: Add 64-bit XIDs into PostgreSQL 15) 22. Unit tests for SLRU Currently, we have 31 patches which require the author's attention. If you already fixed and replied, change the status. I'll send out reminders this week to get your patches registered/rebased, I'll update stale statuses in the CF app. Thanks, -- Ibrar Ahmed.
Re: postgres_fdw hint messages
On Thu, Aug 25, 2022 at 6:42 PM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > The postgres_fdw tests contain this (as amended by patch 0001): > > ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw'); > ERROR: invalid option "password" > HINT: Valid options in this context are: service, passfile, > channel_binding, connect_timeout, dbname, host, hostaddr, port, options, > application_name, keepalives, keepalives_idle, keepalives_interval, > keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, > sslkey, sslrootcert, sslcrl, sslcrldir, sslsni, requirepeer, > ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, > krbsrvname, gsslib, target_session_attrs, use_remote_estimate, > fdw_startup_cost, fdw_tuple_cost, extensions, updatable, truncatable, > fetch_size, batch_size, async_capable, parallel_commit, keep_connections > > This annoys developers who are working on libpq connection options, > because any option added, removed, or changed causes this test to need > to be updated. > > It's also questionable how useful this hint is in its current form, > considering how long it is and that the options are in an > implementation-dependent order. > > Thanks Peter, for looking at that; this HINT message is growing over time. In my opinion, we should hide the complete message in case of an invalid option. But try to show dependent options; for example, if someone specify "sslcrl" and that option require some more options, then show the HINT of that options. Possible changes: > > - Hide the hint from this particular test (done in the attached patches). > > > - Keep the hint, but maybe make it sorted? > > - Remove all the hints like this from foreign data commands. > > - Don't show the hint when there are more than N valid options. > > - Do some kind of "did you mean" like we have for column names. > > Thoughts? -- Ibrar Ahmed
Re: CFM Manager
On Tue, Aug 23, 2022 at 1:46 AM Hamid Akhtar wrote: > > > On Tue, 23 Aug 2022 at 1:26 AM, Ibrar Ahmed > wrote: > >> >> >> On Mon, Aug 22, 2022 at 9:47 PM Jacob Champion >> wrote: >> >>> On Mon, Aug 22, 2022 at 9:40 AM Tom Lane wrote: >>> > You attribute more organization to this than actually exists ;-) >>> >>> Ha, fair enough! >>> >>> > If Ibrar wants the job I think it's his. >>> >>> Excellent. Ibrar, I'll be updating the CFM checklist [1] over the next >>> couple of weeks. I'll try to have sections of it touched up by the >>> time you're due to use them. Let me know if there's anything in >>> particular that is confusing or needs more explanation. >>> >>> Thanks, >>> --Jacob >>> >>> [1] https://wiki.postgresql.org/wiki/CommitFest_Checklist >>> >>> >>> Thanks, I will start working. >> > > I’d like to assist. > > Thanks, Hamid This will help to complete the tasks. I start looking at that; I will let you know how we both manage to share the load -- Ibrar Ahmed
Re: CFM Manager
On Mon, Aug 22, 2022 at 9:47 PM Jacob Champion wrote: > On Mon, Aug 22, 2022 at 9:40 AM Tom Lane wrote: > > You attribute more organization to this than actually exists ;-) > > Ha, fair enough! > > > If Ibrar wants the job I think it's his. > > Excellent. Ibrar, I'll be updating the CFM checklist [1] over the next > couple of weeks. I'll try to have sections of it touched up by the > time you're due to use them. Let me know if there's anything in > particular that is confusing or needs more explanation. > > Thanks, > --Jacob > > [1] https://wiki.postgresql.org/wiki/CommitFest_Checklist > > > Thanks, I will start working. -- Ibrar Ahmed. Senior Software Engineer, PostgreSQL Consultant.
Re: CFM Manager
On Tue, Jul 5, 2022 at 4:31 PM Ibrar Ahmed wrote: > > > On Tue, Jul 5, 2022 at 8:50 AM Michael Paquier > wrote: > >> On Tue, Jul 05, 2022 at 08:17:26AM +0500, Ibrar Ahmed wrote: >> > If nobody has already volunteered for the next upcoming commitfest. >> > I'd like to volunteer. I think early to say is better as always. >> >> Jacob and Daniel have already volunteered. Based on the number of >> patches at hand (305 in total), getting more help is always welcome, I >> guess. >> -- >> Michael >> > I am happy to help, but I am talking about the next one. > > > -- > Ibrar Ahmed > Is anybody else volunteer for that, if not I am ready to take that resposibility. -- Ibrar Ahmed
Re: Get the statistics based on the application name and IP address
On Mon, Aug 8, 2022 at 10:11 PM Julien Rouhaud wrote: > Hi, > > On Mon, Aug 08, 2022 at 08:21:06PM +0500, Ibrar Ahmed wrote: > > While working on pg_stat_stements, I got some questions from customers to > > have statistics by application and IP address. > > [...] > > name. I did some POC and had a patch. But before sharing the patch. > > > > I need to know if there has been any previous discussion about this > topic; > > by the way, > > Thanks for the input. > I don't think there was any discussion on this exactly, but there have been > some related discussions. > > This would likely bring 2 problems. > First, for now each entry contains its own > query text in the query file. There can already be some duplication, which > isn't great, but adding the application_name and/or IP address will make > things > way worse, so you would probably need to fix that first. I doubt that makes it worst because these (IP and Application) will be part of the key, not the query text. But yes, I agree that it will increase the footprint of rows, excluding query text. I am not 100% sure about the query text duplication but will look at that in detail, if you have more insight, then it will help to solve that. > There has been some > discussion about it recently (1) but more work and benchmarking are needed. > > The other problem is the multiplication of entries. It's a well known > limitation that pg_stat_statements eviction are so costly that it makes it > unusable. The last numbers I saw about it was ~55% overhead (2). Adding > application_name or ip address to the key would probably make > pg_stat_statements unusable for anyone who would actually need those > metrics. > I am sure adding a new item in the key does not affect the performance of evictions of the row, as it will not be part of that area. I am doing some benchmarking and hacking to reduce that and will send results with the patch. > [1]: > https://www.postgresql.org/message-id/flat/604E3199-2DD2-47DD-AC47-774A6F97DCA9%40amazon.com > [2]: https://twitter.com/AndresFreundTec/status/1105585237772263424 > -- Ibrar Ahmed. Senior Software Engineer, PostgreSQL Consultant.
Get the statistics based on the application name and IP address
While working on pg_stat_stements, I got some questions from customers to have statistics by application and IP address. I know that we are collecting the statistics by query id, user id, database id and top-level query. There is no way to collect the statistics based on IP address and application name. That's possible that multiple applications issue the same queries with the same user on the same database. We cannot segregate those queries from which application this query comes. I know we can this in the log file with log_line_prefix, but I want to see that aggregates like call count based on IP and application name. I did some POC and had a patch. But before sharing the patch. I need to know if there has been any previous discussion about this topic; by the way, I did some Googling to find that but failed. Thoughts? -- Ibrar Ahmed. Senior Software Engineer, PostgreSQL Consultant.
Re: Compilation issue on Solaris.
On Sat, Jul 9, 2022 at 10:28 AM Thomas Munro wrote: > On Sat, Jul 9, 2022 at 2:02 PM Ibrar Ahmed wrote: > > Thanks for looking at that, yes you are right, the attached patch do > that now > > > > if test "$PORTNAME" = "solaris"; then > > > >CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS" > > > > + CPPFLAGS="$CPPFLAGS -D__STDC_WANT_LIB_EXT1__" > > > > fi > > Hmm. K.3.3.1 of [1] says you can show or hide all that _s stuff by > defining that macro to 0 or 1 before you include , but it's > implementation-defined whether they are exposed by default, and the > template file is one way to deal with that > implementation-definedness... it's not quite in the autoconf spirit > though, it's kinda manual. Another approach would be to define it > unconditionally at the top of explicit_bzero.c before including "c.h", > on all platforms. The man page on my system tells me I should do that > anyway, even though you don't need to on my system. > > Why is your Solaris system trying to compile that file in the first > place? A quick check of the Solaris and Illumos build farm animals > and some online man pages tells me they have explicit_bzero(). > Ahhh... looks like it came a few years ago in some Solaris 11.4 > update[2], and Illumos (which forked around 10) probably added it > independently (why do Solaris man pages not have a history section to > tell us these things?!). I guess you must be running an old version. > OK then. > > [1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf > [2] https://blogs.oracle.com/solaris/post/expanding-the-library > I am using "SunOS solaris-vagrant 5.11 11.4.0.15.0 i86pc i386 i86pc", I gave another thought and Tom is right src/template/solaris is a better place to add that. -- Ibrar Ahmed
Re: Compilation issue on Solaris.
On Sat, Jul 9, 2022 at 6:46 AM Tom Lane wrote: > Ibrar Ahmed writes: > > While compiling the PostgreSQL I have found that *memset_s function > > requires a define "*__STDC_WANT_LIB_EXT1__*" * > > *explicit_bzero.c:* In function ‘*explicit_bzero*’: > > *explicit_bzero.c:23:9:* *warning: *implicit declaration of function ‘ > > *memset_s*’; did you mean ‘*memset*’? [*-Wimplicit-function-declaration*] > > Hmm. > > > Attached is the patch to define that in the case of Solaris. > > If you don't have any test you want to make before adding the > #define, I don't think this is idiomatic use of autoconf. > Personally I'd have just added "-D__STDC_WANT_LIB_EXT1__" into > the CPPFLAGS for Solaris, perhaps in src/template/solaris, > or maybe just adjust the stanza immediately above this one: > > if test "$PORTNAME" = "solaris"; then > CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS" > fi > > regards, tom lane > Thanks for looking at that, yes you are right, the attached patch do that now if test "$PORTNAME" = "solaris"; then CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS" + CPPFLAGS="$CPPFLAGS -D__STDC_WANT_LIB_EXT1__" fi -- Ibrar Ahmed solaris_memset_s_v2.patch Description: Binary data
Compilation issue on Solaris.
Hi, While compiling the PostgreSQL I have found that *memset_s function requires a define "*__STDC_WANT_LIB_EXT1__*" * *explicit_bzero.c:* In function ‘*explicit_bzero*’: *explicit_bzero.c:23:9:* *warning: *implicit declaration of function ‘ *memset_s*’; did you mean ‘*memset*’? [*-Wimplicit-function-declaration*] (void) *memset_s*(buf, len, 0, len); *^~~~* Attached is the patch to define that in the case of Solaris. -- Ibrar Ahmed solaris_memset_s_v1.patch Description: Binary data
Re: Aggregate leads to superfluous projection from the scan
On Fri, Jul 8, 2022 at 10:32 PM Zhihong Yu wrote: > > > On Fri, Jul 8, 2022 at 9:40 AM Zhihong Yu wrote: > >> Hi, >> Here is the query which involves aggregate on a single column: >> >> >> https://dbfiddle.uk/?rdbms=postgres_13=44bfd8f6b6b5aad34d00d449c04c5a96 >> >> As you can see from `Output:`, there are many columns added which are not >> needed by the query executor. >> >> I wonder if someone has noticed this in the past. >> If so, what was the discussion around this topic ? >> >> Thanks >> > Hi, > With the patch, I was able to get the following output: > > explain (analyze, verbose) /*+ IndexScan(t) */select count(fire_year) > from fires t where objectid <= 200; > QUERY PLAN > > -- > Aggregate (cost=119.00..119.01 rows=1 width=8) (actual time=9.453..9.453 > rows=1 loops=1) >Output: count(fire_year) >-> Index Scan using fires_pkey on public.fires t (cost=0.00..116.50 > rows=1000 width=4) (actual time=9.432..9.432 rows=0 loops=1) > Output: fire_year > Index Cond: (t.objectid <= 200) > Planning Time: 52.598 ms > Execution Time: 13.082 ms > > Please pay attention to the column list after `Output:` > > Tom: > Can you take a look and let me know what I may have missed ? > > Thanks > I give a quick look and I think in case whenever data is extracted from the heap it shows all the columns. Therefore when columns are extracted from the index only it shows the indexed column only. postgres=# explain (analyze, verbose) /*+ IndexScan(idx) */select count(fire_year) from fires t where objectid = 20; QUERY PLAN -- Aggregate (cost=8.31..8.32 rows=1 width=8) (actual time=0.029..0.030 rows=1 loops=1) Output: count(fire_year) -> Index Scan using fires_pkey on public.fires t (cost=0.29..8.31 rows=1 width=4) (actual time=0.022..0.023 rows=1 loops=1) Output: objectid, fire_name, fire_year, discovery_date, discovery_time, stat_cause_descr, fire_size, fire_size_class, latitude, longitude, state, county, discovery_date_j, discovery_date_d Index Cond: (t.objectid = 20) Planning Time: 0.076 ms Execution Time: 0.059 ms (7 rows) Index-only. postgres=# explain (analyze, verbose) /*+ IndexScan(idx) */select count(fire_year) from fires t where fire_year = 20; QUERY PLAN --- Aggregate (cost=8.31..8.32 rows=1 width=8) (actual time=0.026..0.027 rows=1 loops=1) Output: count(fire_year) -> Index Only Scan using idx on public.fires t (cost=0.29..8.31 rows=1 width=4) (actual time=0.023..0.024 rows=0 loops=1) Output: fire_year Index Cond: (t.fire_year = 20) Heap Fetches: 0 Planning Time: 0.140 ms Execution Time: 0.052 ms (8 rows) Index Scans postgres=# explain (analyze, verbose) select count(fire_year) from fires t where objectid = 20; Aggregate (cost=8.31..8.32 rows=1 width=8) (actual time=0.030..0.031 rows=1 loops=1) Output: count(fire_year) -> Index Scan using fires_pkey on public.fires t (cost=0.29..8.31 rows=1 width=4) (actual time=0.021..0.023 rows=1 loops=1) Output: objectid, fire_name, fire_year, discovery_date, discovery_time, stat_cause_descr, fire_size, fire_size_class, latitude, longitude, state, county, discovery_date_j, discovery_date_d Index Cond: (t.objectid = 20) Planning Time: 0.204 ms Execution Time: 0.072 ms (7 rows) Seq scans. -- postgres=# explain (analyze, verbose) select count(fire_year) from fires t; Aggregate (cost=1791.00..1791.01 rows=1 width=8) (actual time=13.172..13.174 rows=1 loops=1) Output: count(fire_year) -> Seq Scan on public.fires t (cost=0.00..1541.00 rows=10 width=4) (actual time=0.007..6.500 rows=10 loops=1) Output: objectid, fire_name, fire_year, discovery_date, discovery_time, stat_cause_descr, fire_size, fire_size_class, latitude, longitude, state, county, discovery_date_j, discovery_date_d Planning Time: 0.094 ms Execution Time: 13.201 ms (6 rows) -- Ibrar Ahmed
Re: explain analyze rows=%.0f
On Thu, Jul 7, 2022 at 10:53 PM Greg Stark wrote: > > - -> Parallel Seq Scan on tenk1 (actual rows=1960 > loops=50) > > + -> Parallel Seq Scan on tenk1 (actual rows=1960.00 > > At the not inconsiderable risk of bike-shedding > > I'm wondering if printing something like 0.00 will be somewhat > deceptive when the real value is non-zero but less than 1 row per 200 > loops. I wonder if the number of decimal places should be calculated > to produce a minimum of one non-zero digit for non-zero values. > > -- > greg > + -> Parallel Seq Scan on tenk1 (actual rows=1960.00 I have added a new check to remove any ".00" from the output because in the case of parallel queries we are getting that. Secondly, it is disturbing many test case outputs. -- Ibrar Ahmed
Re: explain analyze rows=%.0f
On Thu, Jul 7, 2022 at 3:14 PM vignesh C wrote:
> On Thu, Jun 23, 2022 at 2:25 AM Ibrar Ahmed wrote:
> >
> >
> >
> > On Thu, Jun 23, 2022 at 1:04 AM David G. Johnston <
> david.g.johns...@gmail.com> wrote:
> >>
> >> On Wed, Jun 22, 2022 at 12:11 PM Ibrar Ahmed
> wrote:
> >>>
> >>> On Thu, Jun 23, 2022 at 12:01 AM Tom Lane wrote:
> >>>>
> >>>> Robert Haas writes:
> >>>> > On Jun 2, 2009, at 9:41 AM, Simon Riggs
> wrote:
> >>>> >> You're right that the number of significant digits already exceeds
> the
> >>>> >> true accuracy of the computation. I think what Robert wants to see
> is
> >>>> >> the exact value used in the calc, so the estimates can be checked
> more
> >>>> >> thoroughly than is currently possible.
> >>>>
> >>>> > Bingo.
> >>>>
> >>>> Uh, the planner's estimate *is* an integer. What was under discussion
> >>>> (I thought) was showing some fractional digits in the case where
> EXPLAIN
> >>>> ANALYZE is outputting a measured row count that is an average over
> >>>> multiple loops, and therefore isn't necessarily an integer. In that
> >>>> case the measured value can be considered arbitrarily precise ---
> though
> >>>> I think in practice one or two fractional digits would be plenty.
> >>>>
> >>>> regards, tom lane
> >>>>
> >>>>
> >>> Hi,
> >>> I was looking at the TODO list and found that the issue requires
> >>> a quick fix. Attached is a patch which shows output like this.
> >>
> >>
> >> Quick code review:
> >>
> >> + "actual rows=%.0f loops=%.0f": " rows=%.2f loops=%.0f",
> >>
> >> The leading space before the else block "rows" does not belong.
> >>
> >> There should be a space after the colon.
> >>
> > Thanks, David for your quick response. I have updated the patch.
> >
> >>
> >> The word "actual" that you are dropping in the else block seems like it
> should belong - it is a header for the entire section not just a modifier
> for the word "rows". This is evidenced by the timing block verbiage where
> rows is standalone and the word actual comes before time. In short, only
> the format specifier should change under the current scheme. Both sections.
> >>
> >> - WRITE_FLOAT_FIELD(rows, "%.0f");
> >> + WRITE_FLOAT_FIELD(rows, "%.2f");
> >>
> >> This one looks suspicious, though I haven't dug into the code to see
> exactly what all is being touched. That it doesn't have an nloops
> condition like everything else stands out.
> >>
> > I was also thinking about that, but I don't see any harm when we
> ultimately truncating that decimal
> > at a latter stage of code in case of loop = 1.
>
> Thanks for the patch.
>
Thanks for the review.
>
> 1) There are some existing regression tests that are failing, you
> should update the expect files accordingly for the same:
> --- /home/vignesh/postgres/src/test/regress/expected/select_parallel.out
>2022-05-18 20:51:46.874818044 +0530
> +++ /home/vignesh/postgres/src/test/regress/results/select_parallel.out
> 2022-07-07 15:27:34.450440922 +0530
> @@ -545,17 +545,17 @@
> explain (analyze, timing off, summary off, costs off)
> select count(*) from tenk1, tenk2 where tenk1.hundred > 1
> and tenk2.thousand=0;
> -QUERY PLAN
> ---
> + QUERY PLAN
>
> +-
> Aggregate (actual rows=1 loops=1)
> -> Nested Loop (actual rows=98000 loops=1)
> -> Seq Scan on tenk2 (actual rows=10 loops=1)
> Filter: (thousand = 0)
> Rows Removed by Filter: 9990
> - -> Gather (actual rows=9800 loops=10)
> + -> Gather (actual rows=9800.00 loops=10)
> Workers Planned: 4
> Workers Launched: 4
> - -> Parallel Seq Scan on tenk1 (actual rows=1960 loops=50)
> + -> Parallel Seq Scan on tenk1 (actual rows=1960.00
> loops=50)
> Filter: (hundred > 1)
>
> test select_parallel ... FAILED 744 ms
> partition_prune ... FAILED 861 ms
> explain ... FAILED 134 ms
> memoize ... FAILED 250 ms
>
> 2) This change is not required as part of this patch:
> --- a/src/backend/access/transam/xact.c
> +++ b/src/backend/access/transam/xact.c
> @@ -122,7 +122,7 @@ boolbsysscan = false;
> * lookups as fast as possible.
> */
> static FullTransactionId XactTopFullTransactionId =
> {InvalidTransactionId};
> -static int nParallelCurrentXids = 0;
> +static int nParallelCurrentXids = 0;
> static TransactionId *ParallelCurrentXids;
>
>
I have fixed the regression and removed non-related code.
> Regards,
> Vignesh
>
--
Ibrar Ahmed
Re: explain analyze rows=%.0f
On Thu, Jul 7, 2022 at 2:41 PM Amit Kapila wrote:
> On Thu, Jun 23, 2022 at 2:25 AM Ibrar Ahmed wrote:
> >
> > On Thu, Jun 23, 2022 at 1:04 AM David G. Johnston <
> david.g.johns...@gmail.com> wrote:
> >>
> >> - WRITE_FLOAT_FIELD(rows, "%.0f");
> >> + WRITE_FLOAT_FIELD(rows, "%.2f");
> >>
> >> This one looks suspicious, though I haven't dug into the code to see
> exactly what all is being touched. That it doesn't have an nloops
> condition like everything else stands out.
> >>
> > I was also thinking about that, but I don't see any harm when we
> ultimately truncating that decimal
> > at a latter stage of code in case of loop = 1.
> >
>
> That change is in the path node which we anyway not going to target as
> part of this change. We only want to change the display for actual
> rows in Explain Analyze. So, I can't see how the quoted change can
> help in any way.
>
> Agreed removed.
> Few miscellaneous comments:
>
> *
> static FullTransactionId XactTopFullTransactionId =
> {InvalidTransactionId};
> -static int nParallelCurrentXids = 0;
> +static int nParallelCurrentXids = 0;
>
> Removed.
> I don't see why this change is required.
>
> * Can you please add a comment explaining why we are making this
> change for actual rows?
>
Done
>
> * Can you please write a test case unless there is some existing test
> that covers the change by displaying actual rows values in decimal but
> in that case patch should have that changed output test? If you don't
> think we can reliably write such a test then please let me know the
> reason?
>
> I think there are tests, and I have updated the results accordingly.
> --
> With Regards,
> Amit Kapila.
>
--
Ibrar Ahmed
explain_float_row_v3.patch
Description: Binary data
Re: CFM Manager
On Tue, Jul 5, 2022 at 8:50 AM Michael Paquier wrote: > On Tue, Jul 05, 2022 at 08:17:26AM +0500, Ibrar Ahmed wrote: > > If nobody has already volunteered for the next upcoming commitfest. > > I'd like to volunteer. I think early to say is better as always. > > Jacob and Daniel have already volunteered. Based on the number of > patches at hand (305 in total), getting more help is always welcome, I > guess. > -- > Michael > I am happy to help, but I am talking about the next one. -- Ibrar Ahmed
CFM Manager
If nobody has already volunteered for the next upcoming commitfest. I'd like to volunteer. I think early to say is better as always. -- Ibrar Ahmed
Re: Minimal logical decoding on standbys
On Mon, Jul 4, 2022 at 6:12 PM Drouvot, Bertrand wrote: > Hi, > On 7/1/22 10:03 PM, Ibrar Ahmed wrote: > > > On Thu, Jun 30, 2022 at 1:49 PM Drouvot, Bertrand > wrote: > >> I'm going to re-create a CF entry for it, as: >> >> - It seems there is a clear interest for the feature (given the time >> already spend on it and the number of people that worked on) >> >> - I've in mind to resume working on it >> >> I have already done some research on that, I can definitely look at it. > > Thanks! > > This feature proposal is currently made of 5 sub-patches: > > 0001: Add info in WAL records in preparation for logical slot conflict > handling > 0002: Handle logical slot conflicts on standby > 0003: Allow logical decoding on standby. > 0004: New TAP test for logical decoding on standby > 0005: Doc changes describing details about logical decoding > > I suggest that we focus on one sub-patch at a time. > > I'll start with 0001 and come back with a rebase addressing Andres and > Robert's previous comments. > > Sounds good to you? > > Thanks > > -- > Bertrand Drouvot > Amazon Web Services: https://aws.amazon.com > > That's great I am looking at "0002: Handle logical slot conflicts on standby". -- Ibrar Ahmed
Re: Minimal logical decoding on standbys
On Thu, Jun 30, 2022 at 1:49 PM Drouvot, Bertrand wrote: > Hi, > > On 2/25/22 10:34 AM, Drouvot, Bertrand wrote: > > Hi, > > > > On 10/28/21 11:07 PM, Andres Freund wrote: > >> Hi, > >> > >> On 2021-10-28 16:24:22 -0400, Robert Haas wrote: > >>> On Wed, Oct 27, 2021 at 2:56 AM Drouvot, Bertrand > >>> wrote: > >>>> So you have in mind to check for XLogLogicalInfoActive() first, and > >>>> if true, then open the relation and call > >>>> RelationIsAccessibleInLogicalDecoding()? > >>> I think 0001 is utterly unacceptable. We cannot add calls to > >>> table_open() in low-level functions like this. Suppose for example > >>> that _bt_getbuf() calls _bt_log_reuse_page() which with 0001 applied > >>> would call get_rel_logical_catalog(). _bt_getbuf() will have acquired > >>> a buffer lock on the page. The idea that it's safe to call > >>> table_open() while holding a buffer lock cannot be taken seriously. > >> Yes - that's pretty clearly a deadlock hazard. It shouldn't too hard > >> to fix, I > >> think. Possibly a bit more verbose than nice, but... > >> > >> Alternatively we could propagate the information whether a relcache > >> entry is > >> for a catalog from the table to the index. Then we'd not need to > >> change the > >> btree code to pass the table down. > > > > +1 for the idea of propagating to the index. If that sounds good to > > you too, I can try to have a look at it. > > > > Thanks Robert and Andres for the feedbacks you have done on the > > various sub-patches. > > > > I've now in mind to work sub patch by sub patch (starting with 0001 > > then) and move to the next one once we agree that the current one is > > "ready". > > > > I think that could help us to get this new feature moving forward more > > "easily", what do you think? > > > > Thanks > > > > Bertrand > > > I'm going to re-create a CF entry for it, as: > > - It seems there is a clear interest for the feature (given the time > already spend on it and the number of people that worked on) > > - I've in mind to resume working on it > > I have already done some research on that, I can definitely look at it. > - It would give more visibility in case others want to jump in > > Hope that makes sense, > > Thanks, > > Bertrand > > -- Ibrar Ahmed
Re: explain analyze rows=%.0f
On Thu, Jun 23, 2022 at 1:04 AM David G. Johnston < david.g.johns...@gmail.com> wrote: > On Wed, Jun 22, 2022 at 12:11 PM Ibrar Ahmed > wrote: > >> On Thu, Jun 23, 2022 at 12:01 AM Tom Lane wrote: >> >>> Robert Haas writes: >>> > On Jun 2, 2009, at 9:41 AM, Simon Riggs wrote: >>> >> You're right that the number of significant digits already exceeds the >>> >> true accuracy of the computation. I think what Robert wants to see is >>> >> the exact value used in the calc, so the estimates can be checked more >>> >> thoroughly than is currently possible. >>> >>> > Bingo. >>> >>> Uh, the planner's estimate *is* an integer. What was under discussion >>> (I thought) was showing some fractional digits in the case where EXPLAIN >>> ANALYZE is outputting a measured row count that is an average over >>> multiple loops, and therefore isn't necessarily an integer. In that >>> case the measured value can be considered arbitrarily precise --- though >>> I think in practice one or two fractional digits would be plenty. >>> >>> regards, tom lane >>> >>> >>> Hi, >> I was looking at the TODO list and found that the issue requires >> a quick fix. Attached is a patch which shows output like this. >> > > Quick code review: > > + "actual rows=%.0f loops=%.0f": " rows=%.2f loops=%.0f", > > The leading space before the else block "rows" does not belong. > > There should be a space after the colon. > > Thanks, David for your quick response. I have updated the patch. > The word "actual" that you are dropping in the else block seems like it > should belong - it is a header for the entire section not just a modifier > for the word "rows". This is evidenced by the timing block verbiage where > rows is standalone and the word actual comes before time. In short, only > the format specifier should change under the current scheme. Both sections. > > - WRITE_FLOAT_FIELD(rows, "%.0f"); > + WRITE_FLOAT_FIELD(rows, "%.2f"); > > This one looks suspicious, though I haven't dug into the code to see > exactly what all is being touched. That it doesn't have an nloops > condition like everything else stands out. > > I was also thinking about that, but I don't see any harm when we ultimately truncating that decimal at a latter stage of code in case of loop = 1. > Tooling that expects an integer is the only downside I see here, but I > concur that the usability of always showing two decimal places when nloops > > 1 overcomes any objection I have on those grounds. > > David J. > > -- Ibrar Ahmed explain_float_row_v2.patch Description: Binary data
Re: explain analyze rows=%.0f
On Thu, Jun 23, 2022 at 12:01 AM Tom Lane wrote: > Robert Haas writes: > > On Jun 2, 2009, at 9:41 AM, Simon Riggs wrote: > >> You're right that the number of significant digits already exceeds the > >> true accuracy of the computation. I think what Robert wants to see is > >> the exact value used in the calc, so the estimates can be checked more > >> thoroughly than is currently possible. > > > Bingo. > > Uh, the planner's estimate *is* an integer. What was under discussion > (I thought) was showing some fractional digits in the case where EXPLAIN > ANALYZE is outputting a measured row count that is an average over > multiple loops, and therefore isn't necessarily an integer. In that > case the measured value can be considered arbitrarily precise --- though > I think in practice one or two fractional digits would be plenty. > > regards, tom lane > > > Hi, I was looking at the TODO list and found that the issue requires a quick fix. Attached is a patch which shows output like this. It shows the fraction digits in case of loops > 1 postgres=# explain analyze select * from foo; QUERY PLAN -- Seq Scan on foo (cost=0.00..64414.79 rows=2326379 width=8) (actual time=0.025..277.096 rows=2344671 loops=1 Planning Time: 0.516 ms Execution Time: 356.993 ms (3 rows) postgres=# explain analyze select * from foo where b = (select c from bar where c = 1); QUERY PLAN Seq Scan on foo (cost=8094.37..78325.11 rows=2326379 width=8) (actual time=72.352..519.159 rows=2344671 loops=1 Filter: (b = $1) InitPlan 1 (returns $1) -> Gather (cost=1000.00..8094.37 rows=1 width=4) (actual time=0.872..72.434 rows=1 loops=1 Workers Planned: 2 Workers Launched: 2 -> Parallel Seq Scan on bar (cost=0.00..7094.27 rows=1 width=4) (actual time=41.931..65.382 rows=0.33 loops=3) Filter: (c = 1) Rows Removed by Filter: 245457 Planning Time: 0.277 ms Execution Time: 597.795 ms (11 rows) -- Ibrar Ahmed explain_float_row.patch Description: Binary data
Re: Column Redaction
On Wed, Jun 22, 2022 at 11:53 PM Simon Riggs wrote: > Postgres currently supports column level SELECT privileges. > > 1. If we want to confirm a credit card number, we can issue SELECT 1 > FROM customer WHERE stored_card_number = '1234 5678 5344 7733' > > 2. If we want to look for card fraud, we need to be able to use the > full card number to join to transaction data and look up blocked card > lists etc.. > > 3. We want to block the direct retrieval of card numbers for > additional security. > In some cases, we might want to return an answer like ' * > 7733' > > We can't do all of the above with current facilities inside the database. > > The ability to mask output for data in certain cases, for the purpose > of security, is known lately as data redaction, or column-level data > redaction. > > The best way to support this requirement would be to allow columns to > have an additional "output formatting function". This would be > executed only when data is about to be returned by a query. All other > uses of that would not restrict the data. > > This would have other uses as well, such as default report formats, so > we can store financial amounts as NUMERIC, but format them on > retrieval as $12,345.78 etc.. > > Suggested user interface would be... >FORMAT functionname(parameters, if any) > > e.g. > CREATE TABLE customer > ( id ... > ... > , stored_card_number NUMERIC FORMAT pci_card_number_redaction() > ... > ); > > We'd need to implement something to allow pg_dump to ignore format > functions. I suggest the best way to do that is by providing a BACKUP > role that can be delegated to other users. We would then allow a > parameter for SET output_formatting = on | off, which can only be set > by superuser and BACKUP role, then have pg_dump issue SET > output_formatting = off explicitly when it runs. > > Do we want redaction in PostgreSQL? > Do we want it generalised into output format functions? > > -- > Simon Riggs http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services > > > -- > Sent via pgsql-hackers mailing list (pgsql-hack...@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > > Hi, Do we still have some interest in this? People generally like that the idea, if yes I am happy to work on that and can send the complete design first. -- Ibrar Ahmed
Re: Commitfest overflow
On Tue, Aug 3, 2021 at 11:31 PM Simon Riggs wrote: > On Tue, 3 Aug 2021 at 17:13, Tom Lane wrote: > > > > Bruce Momjian writes: > > > On Tue, Aug 3, 2021 at 04:53:40PM +0100, Simon Riggs wrote: > > >> There are 273 patches in the queue for the Sept Commitfest already, so > > >> it seems clear the queue is not being cleared down each CF as it was > > >> before. We've been trying hard, but it's overflowing. > > > > > I wonder if our lack of in-person developer meetings is causing some of > > > our issues to not get closed. > > > > I think there are a couple of things happening here: > > > > 1. There wasn't that much getting done during this CF because it's > > summer and many people are on vacation (in the northern hemisphere > > anyway). > > > > 2. As a community, we don't really have the strength of will to > > flat-out reject patches. I think the dynamic is that individual > > committers look at something, think "I don't like that, I'll go > > work on some better-designed patch", and it just keeps slipping > > to the next CF. In the past we've had some CFMs who were assertive > > enough and senior enough to kill off patches that didn't look like > > they were going to go anywhere. But that hasn't happened for > > awhile, and I'm not sure it should be the CFM's job anyway. > > > > (I hasten to add that I'm not trying to imply that all the > > long-lingering patches are hopeless. But I think some of them are.) > > > > I don't think there's much to be done about the vacation effect; > > we just have to accept that the summer CF is likely to be less > > productive than others. But I'd like to see some better-formalized > > way of rejecting patches that aren't going anywhere. Maybe there > > should be a time limit on how many CFs a patch is allowed to just > > automatically slide through? > > I guess the big number is 233 patches moved to next CF, out of 342, or > 68% deferred. > > Perhaps we need a budget of how many patches can be moved to next CF, > i.e. CF mgr is responsible for ensuring that no more than ?50% of > patches can be moved forwards. Any in excess of that need to join the > kill list. > > I would still ask for someone to spend a little time triaging things, > so as to direct people who volunteer to be so directed. Many will not > want to be directed, but I'm sure there must be 5-10 people who would > do that? (Volunteers?) > Count me as a Volunteer. > -- > Simon Riggshttp://www.EnterpriseDB.com/ > > > -- Ibrar Ahmed
Re: Commitfest overflow
On Tue, Aug 3, 2021 at 9:13 PM Tom Lane wrote: > Bruce Momjian writes: > > On Tue, Aug 3, 2021 at 04:53:40PM +0100, Simon Riggs wrote: > >> There are 273 patches in the queue for the Sept Commitfest already, so > >> it seems clear the queue is not being cleared down each CF as it was > >> before. We've been trying hard, but it's overflowing. > > > I wonder if our lack of in-person developer meetings is causing some of > > our issues to not get closed. > > I think there are a couple of things happening here: > > 1. There wasn't that much getting done during this CF because it's > summer and many people are on vacation (in the northern hemisphere > anyway). > > 2. As a community, we don't really have the strength of will to > flat-out reject patches. I think the dynamic is that individual > committers look at something, think "I don't like that, I'll go > work on some better-designed patch", and it just keeps slipping > to the next CF. In the past we've had some CFMs who were assertive > enough and senior enough to kill off patches that didn't look like > they were going to go anywhere. But that hasn't happened for > awhile, and I'm not sure it should be the CFM's job anyway. > > (I hasten to add that I'm not trying to imply that all the > long-lingering patches are hopeless. But I think some of them are.) > > I don't think there's much to be done about the vacation effect; > we just have to accept that the summer CF is likely to be less > productive than others. But I'd like to see some better-formalized > way of rejecting patches that aren't going anywhere. Maybe there > should be a time limit on how many CFs a patch is allowed to just > automatically slide through? > +1 for the idea of allowed CFs. Secondly we can think about the patches which have not had a response from the author since long. > > regards, tom lane > > > -- Ibrar Ahmed
Re: 2021-07 CF now in progress
On Mon, Jul 26, 2021 at 5:52 PM Ibrar Ahmed wrote: > > > On Mon, Jul 19, 2021 at 4:37 PM Ibrar Ahmed wrote: > >> >> >> On Mon, Jul 12, 2021 at 4:59 PM Ibrar Ahmed >> wrote: >> >>> >>> Hackers, >>> >>> The Commitfest 2021-07 is now in progress. It is one of the biggest one. >>> Total number of patches of this commitfest is 342. >>> >>> Needs review: 204. >>> Waiting on Author: 40. >>> Ready for Committer: 18. >>> Committed: 57. >>> Moved to next CF: 3. >>> Withdrawn: 15. >>> Rejected: 3. >>> Returned with Feedback: 2. >>> Total: 342. >>> >>> If you are a patch author, please check http://commitfest.cputube.org to >>> be sure your patch still applies, compiles, and passes tests. >>> >>> We need your involvement and participation in reviewing the patches. >>> Let's try and make this happen. >>> >>> -- >>> Regards. >>> Ibrar Ahmed >>> >> >> >> Over the past one week, statuses of 47 patches have been changed from >> "Needs review". This still leaves us with 157 patches >> requiring reviews. As always, your continuous support is appreciated to >> get us over the line. >> >> Please look at the patches requiring review in the current commitfest. >> Test, provide feedback where needed, and update the patch status. >> >> Total: 342. >> >> Needs review: 157. >> Waiting on Author: 74. >> Ready for Committer: 15. >> Committed: 68. >> Moved to next CF: 3. >> Withdrawn: 19. >> Rejected: 4. >> Returned with Feedback: 2. >> >> >> -- >> Ibrar Ahmed >> > > Over the past one week, some progress was made, however, there are still > 155 patches in total that require reviews. Time to continue pushing for > maximising patch reviews and getting stuff committed in PostgreSQL. > > Total: 342. > Needs review: 155. > Waiting on Author: 67. > Ready for Committer: 20. > Committed: 70. > Moved to next CF: 3. > Withdrawn: 20. > Rejected: 5. > Returned with Feedback: 2. > > Thank you for your continued effort andI support. > > -- > Ibrar Ahmed > Here is the current state of the commitfest. It looks like it should be closed now. I don't have permission to do that. Needs review: 148. Waiting on Author: 61. Ready for Committer: 22. Committed: 79. Moved to next CF: 4. Returned with Feedback: 2. Rejected: 6. Withdrawn: 20. Thanks to everyone who worked on the commitfest. -- Ibrar Ahmed
Re: 2021-07 CF now in progress
On Mon, Jul 19, 2021 at 4:37 PM Ibrar Ahmed wrote: > > > On Mon, Jul 12, 2021 at 4:59 PM Ibrar Ahmed wrote: > >> >> Hackers, >> >> The Commitfest 2021-07 is now in progress. It is one of the biggest one. >> Total number of patches of this commitfest is 342. >> >> Needs review: 204. >> Waiting on Author: 40. >> Ready for Committer: 18. >> Committed: 57. >> Moved to next CF: 3. >> Withdrawn: 15. >> Rejected: 3. >> Returned with Feedback: 2. >> Total: 342. >> >> If you are a patch author, please check http://commitfest.cputube.org to >> be sure your patch still applies, compiles, and passes tests. >> >> We need your involvement and participation in reviewing the patches. >> Let's try and make this happen. >> >> -- >> Regards. >> Ibrar Ahmed >> > > > Over the past one week, statuses of 47 patches have been changed from > "Needs review". This still leaves us with 157 patches > requiring reviews. As always, your continuous support is appreciated to > get us over the line. > > Please look at the patches requiring review in the current commitfest. > Test, provide feedback where needed, and update the patch status. > > Total: 342. > > Needs review: 157. > Waiting on Author: 74. > Ready for Committer: 15. > Committed: 68. > Moved to next CF: 3. > Withdrawn: 19. > Rejected: 4. > Returned with Feedback: 2. > > > -- > Ibrar Ahmed > Over the past one week, some progress was made, however, there are still 155 patches in total that require reviews. Time to continue pushing for maximising patch reviews and getting stuff committed in PostgreSQL. Total: 342. Needs review: 155. Waiting on Author: 67. Ready for Committer: 20. Committed: 70. Moved to next CF: 3. Withdrawn: 20. Rejected: 5. Returned with Feedback: 2. Thank you for your continued effort and support. -- Ibrar Ahmed
Re: 2021-07 CF now in progress
On Mon, Jul 12, 2021 at 4:59 PM Ibrar Ahmed wrote: > > Hackers, > > The Commitfest 2021-07 is now in progress. It is one of the biggest one. > Total number of patches of this commitfest is 342. > > Needs review: 204. > Waiting on Author: 40. > Ready for Committer: 18. > Committed: 57. > Moved to next CF: 3. > Withdrawn: 15. > Rejected: 3. > Returned with Feedback: 2. > Total: 342. > > If you are a patch author, please check http://commitfest.cputube.org to > be sure your patch still applies, compiles, and passes tests. > > We need your involvement and participation in reviewing the patches. Let's > try and make this happen. > > -- > Regards. > Ibrar Ahmed > Over the past one week, statuses of 47 patches have been changed from "Needs review". This still leaves us with 157 patches requiring reviews. As always, your continuous support is appreciated to get us over the line. Please look at the patches requiring review in the current commitfest. Test, provide feedback where needed, and update the patch status. Total: 342. Needs review: 157. Waiting on Author: 74. Ready for Committer: 15. Committed: 68. Moved to next CF: 3. Withdrawn: 19. Rejected: 4. Returned with Feedback: 2. -- Ibrar Ahmed
Re: Improve join selectivity estimation using extended statistics
der only case of restrictlist consists of > > * one clause. > > */ > > > > If I understand the comment and the code after it, it essentially tries > > to apply extended statistics from both the join clauses and filters at > > the relation level. That is, with a query like > > > > SELECT * FROM t1 JOIN t2 ON (t1.a = t2.a) WHERE t1.b = 10 > > > > we would be looking at statistics on t1(a,b), because we're interested > > in estimating conditional probability distribution > > > > P(t1.a = a? | t1.b = 10) > > > > I think that's extremely interesting and powerful, because it allows us > > to "restrict" the multi-column MCV lists, we could probably estimate > > number of distinct "a" values in rows with "b=10" like: > > > > ndistinct(a,b) / ndistinct(b) > > > > and do various interesting stuff like this. > > > > That will require some improvements to the extended statistics code (to > > allow passing a list of conditions), but that's quite doable. I think > > the code actually did something like that originally ;-) > > > > > > Obviously, none of this is achievable for PG14, as we're in the middle > > of the last CF. But if you're interested in working on this for PG15, > > I'd love to cooperate on that. > > > > > > regards > > > Hi Tomas, > Thank you for review of my patch. > My primary attention was to implement some kid of adaptive query > optimization based online_analyze extension and building extended > statistic on demand. > I have change clausesel.c because right now extended statistic is not > used for join selectivity estimation and manual or automatic adding such > statistic can help to > choose more efficient plan for queries with joins. > I agree wit you that it can be done in better way, handling more use cases. > I will be glad to cooperate with you in improving join selectivity > estimation using extended statistic. > > > > The patch does not compile, and needs your attention. https://cirrus-ci.com/task/6397726985289728 clausesel.c:74:28: error: too few arguments to function ‘choose_best_statistics’ StatisticExtInfo *stat = choose_best_statistics(rel->statlist, STATS_EXT_DEPENDENCIES, ^~ In file included from clausesel.c:24: ../../../../src/include/statistics/statistics.h:123:26: note: declared here exter I am changing the status to "Waiting on Author". -- Ibrar Ahmed
Re: shared-memory based stats collector
On Wed, Apr 7, 2021 at 8:05 AM Kyotaro Horiguchi wrote: > At Tue, 6 Apr 2021 09:32:16 -0700, Andres Freund > wrote in > > Hi, > > > > On 2021-04-05 02:29:14 -0700, Andres Freund wrote: > .. > > I'm inclined to push patches > > [PATCH v60 05/17] pgstat: split bgwriter and checkpointer messages. > > [PATCH v60 06/17] pgstat: Split out relation stats handling from > AtEO[Sub]Xact_PgStat() etc. > > [PATCH v60 09/17] pgstat: xact level cleanups / consolidation. > > [PATCH v60 10/17] pgstat: Split different types of stats into separate > files. > > [PATCH v60 12/17] pgstat: reorder file pgstat.c / pgstat.h contents. > > FWIW.. > > 05 is a straight forward code-rearrange and reasonable to apply. > > 06 is same as above and it seems to make things cleaner. > > 09 mainly adds ensure_tabtat_xact_level() to remove repeated code > blocks a straight-forward way. I wonder if > pgstat_xact_stack_level_get() might better be > pgstat_get_xact_stack_level(), but I'm fine with the name in the > patch. > > 10 I found that the kind in "pgstat_kind" meant the placeholder for > specific types. It looks good to separate them into smaller pieces. > It is also a simple rearrangement of code. > > > pgstat.c is very long, and it's hard to find an order that makes sense > > and is likely to be maintained over time. Splitting the different > > I deeply agree to "hard to find an order that makes sense". > > 12 I'm not sure how it looks after this patch (I failed to apply 09 at > my hand.), but it is also a simple rearrangement of code blocks. > > > to v14. They're just moving things around, so are fairly low risk. But > > they're going to be a pain to maintain. And I think 10 and 12 make > > pgstat.c a lot easier to understand. > > I think that pgstat.c doesn't get frequent back-patching. It seems to > me that at least 10 looks good. > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > > > The patch does not apply, and require rebase, 1 out of 8 hunks FAILED -- saving rejects to file src/include/pgstat.h.rej patching file src/backend/access/transam/xlog.c Hunk #1 succeeded at 8758 (offset 34 lines). patching file src/backend/postmaster/checkpointer.c Hunk #3 succeeded at 496 with fuzz 1. Hunk #4 FAILED at 576. 1 out of 6 hunks FAILED -- saving rejects to file src/backend/postmaster/checkpointer.c.rej patching file src/backend/postmaster/pgstat.c I am changing the status to "Waiting on Author". -- Ibrar Ahmed
Re: Column Filtering in Logical Replication
On Tue, Jul 13, 2021 at 7:44 PM Rahila Syed wrote: > > Hi Tomas, > > Thank you for your comments. > > >> >> > >> > Currently, this capability is not included in the patch. If the table on >> > the subscriber >> > server has lesser attributes than that on the publisher server, it >> > throws an error at the >> > time of CREATE SUBSCRIPTION. >> > >> >> That's a bit surprising, to be honest. I do understand the patch simply >> treats the filtered columns as "unchanged" because that's the simplest >> way to filter the *data* of the columns. But if someone told me we can >> "filter columns" I'd expect this to work without the columns on the >> subscriber. >> >> OK, I will look into adding this. > > >> >> > However, need to carefully consider situations in which a server >> > subscribes to multiple >> > publications, each publishing a different subset of columns of a >> table. > > Isn't that pretty much the same situation as for multiple subscriptions >> each with a different set of I/U/D operations? IIRC we simply merge >> those, so why not to do the same thing here and merge the attributes? >> >> > Yeah, I agree with the solution to merge the attributes, similar to how > operations are merged. My concern was also from an implementation point > of view, will it be a very drastic change. I now had a look at how remote > relation > attributes are acquired for comparison with local attributes at the > subscriber. > It seems that the publisher will need to send the information about the > filtered columns > for each publication specified during CREATE SUBSCRIPTION. > This will be read at the subscriber side which in turn updates its cache > accordingly. > Currently, the subscriber expects all attributes of a published relation > to be present. > I will add code for this in the next version of the patch. > > To nitpick, I find "Bitmapset *att_list" a bit annoying, because it's > > not really a list ;-) > > > I will make this change with the next version > > > >> FWIW "make check" fails for me with this version, due to segfault in >> OpenTableLists. Apparenly there's some confusion - the code expects the >> list to contain PublicationTable nodes, and tries to extract the >> RangeVar from the elements. But the list actually contains RangeVar, so >> this crashes and burns. See the attached backtrace. >> >> > Thank you for the report, This is fixed in the attached version, now all > publication > function calls accept the PublicationTableInfo list. > > Thank you, > Rahila Syed > > > The patch does not apply, and an rebase is required Hunk #8 succeeded at 1259 (offset 99 lines). Hunk #9 succeeded at 1360 (offset 99 lines). 1 out of 9 hunks FAILED -- saving rejects to file src/backend/replication/pgoutput/pgoutput.c.rej patching file src/include/catalog/pg_publication.h Changing the status to "Waiting on Author" -- Ibrar Ahmed
Re: Re[3]: On login trigger: take three
On Wed, Jul 7, 2021 at 5:55 AM Greg Nancarrow wrote: > On Sun, Jul 4, 2021 at 1:21 PM vignesh C wrote: > > > > CFBot shows the following failure: > > # poll_query_until timed out executing this query: > > # SELECT '0/3046250' <= replay_lsn AND state = 'streaming' FROM > > pg_catalog.pg_stat_replication WHERE application_name = 'standby_1'; > > # expecting this output: > > # t > > # last actual query output: > > # t > > # with stderr: > > # NOTICE: You are welcome! > > # Looks like your test exited with 29 before it could output anything. > > t/001_stream_rep.pl .. > > Dubious, test returned 29 (wstat 7424, 0x1d00) > > > > Thanks. > I found that the patch was broken by commit f452aaf7d (the part > "adjust poll_query_until to insist on empty stderr as well as a stdout > match"). > So I had to remove a "RAISE NOTICE" (which was just an informational > message) from the login trigger function, to satisfy the new > poll_query_until expectations. > Also, I updated a PG14 version check (now must check PG15 version). > > Regards, > Greg Nancarrow > Fujitsu Australia > The patch does not apply, and rebase is required. Hunk #1 FAILED at 93. 1 out of 1 hunk FAILED -- saving rejects to file src/test/regress/expected/sysviews.out.rej I am not changing the status because it is a minor change and the patch is already "Ready for Committer". -- Ibrar Ahmed
Re: Minimal logical decoding on standbys
t; I am not sure of the added value and they are currently also not > >> mentioned during standby recovery snapshot conflict). > >> > >>> > >>> Unfortunately I think the things I have found are too many for me to > >>> address within the given time. I'll send a version with a somewhat > >>> polished set of the changes I made in the next few days... > >> > >> Thanks for the review and feedback. > >> > >> Please find enclosed v18 with the changes I worked on. > >> > >> I still need to have a look on the tests. > > > > Please find enclosed v19 that also contains the changes related to > > your TAP tests remarks, mainly: > > > > - get rid of 024 and add more tests in 026 (025 has been used in the > > meantime) > > > > - test that logical decoding actually produces useful and correct results > > > > - test standby promotion and logical decoding behavior once done > > > > - useless "use" removal > > > > - check_confl_logicalslot() function removal > > > > - rewrote make_slot_active() to make use of poll_query_until() and > > timeout > > > > - remove the useless eval() > > > > - remove the "Catalog xmins should advance after standby logical slot > > fetches the changes" test > > > > One thing that's not clear to me is your remark "There's also no test > > for a recovery conflict due to row removal": Don't you think that the > > "vacuum full" conflict test is enough? if not, what kind of additional > > tests would you like to see? > > > >> > >> There is also the 10s delay to work on, do you already have an idea > >> on how we should handle it? > >> > >> Thanks > >> > >> Bertrand > >> > > Thanks > > > > Bertrand > > > Please find enclosed v20 a needed rebase (nothing serious worth > mentioning) of v19. > > FWIW, just to sum up that v19 (and so v20): > > - contained the changes (see details above) related to your TAP tests > remarks > > - contained the changes (see details above) related to your code remarks > > There is still the 10s delay thing that need work: do you already have > an idea on how we should handle it? > > And still one thing that's not clear to me is your remark "There's also > no test for a recovery conflict due to row removal": Don't you think > that the "vacuum full" conflict test is enough? if not, what kind of > additional tests would you like to see? > > Thanks > > Bertrand > > > > > The patch does not apply and an updated patch is required. patching file src/include/replication/slot.h Hunk #1 FAILED at 214. 1 out of 2 hunks FAILED -- saving rejects to file src/include/replication/slot.h.rej -- Ibrar Ahmed
Re: SQL:2011 PERIODS vs Postgres Ranges?
On Fri, Apr 9, 2021 at 4:54 PM David Steele wrote: > On 4/8/21 7:40 PM, Paul A Jungwirth wrote: > > On Thu, Apr 8, 2021 at 7:22 AM David Steele wrote: > >> > >> Paul, you can submit to the next CF when you are ready with a new patch. > > > > Thanks David! I've made a lot of progress but still need to finish > > support for CASCADE on temporal foreign keys. I've been swamped with > > other things, but hopefully I can get something during this current > > CF. > > The next CF starts on July 1 so you have some time. > > Regards, > -- > -David > da...@pgmasters.net Based on last comments of Paul and David S I am changing the status to "Waiting on Author". -- Ibrar Ahmed
Re: Asymmetric partition-wise JOIN
On Thu, Jul 15, 2021 at 11:32 AM Andrey Lepikhov wrote: > On 5/7/21 23:15, Zhihong Yu wrote: > > On Mon, Jul 5, 2021 at 2:57 AM Andrey Lepikhov > > mailto:a.lepik...@postgrespro.ru>> wrote: > > +* Can't imagine situation when join relation already > > exists. But in > > +* the 'partition_join' regression test it happens. > > +* It may be an indicator of possible problems. > > > > Should a log be added in the above case ? > I worked more on this case and found more serious mistake. During > population of additional paths on the existed RelOptInfo we can remove > some previously generated paths that pointed from a higher-level list of > subplans and it could cause to lost of subplan links. I prohibit such > situation (you can read comments in the new version of the patch). > Also, choosing of a cheapest path after appendrel creation was added. > Unstable tests were fixed. > > -- > regards, > Andrey Lepikhov > Postgres Professional > Patch is failing the regression, can you please take a look at that. partition_join ... FAILED 6328 ms -- Ibrar Ahmed
Re: BUG #16583: merge join on tables with different DB collation behind postgres_fdw fails
On Thu, Jul 15, 2021 at 2:35 PM Etsuro Fujita wrote: > On Thu, Jul 15, 2021 at 4:17 AM Ibrar Ahmed wrote: > > On Wed, Jul 14, 2021 at 1:41 AM Tom Lane wrote: > >> Seems to just need an update of the expected-file to account for test > >> cases added recently. (I take no position on whether the new results > >> are desirable; some of these might be breaking the intent of the case. > >> But this should quiet the cfbot anyway.) > > > The test case was added by commit "Add support for asynchronous > execution" > > "27e1f14563cf982f1f4d71e21ef24782a052" by Etsuro Fujita. He can > comment > > whether the new results are desirable or not. > > The new results aren't what I intended. I'll update the patch to > avoid that by modifying the original test cases properly, if there are > no objections. > > Best regards, > Etsuro Fujita > Thanks Etsuro, I have changed the status to "Waiting On Author", because patch need changes. Etsuro, can you make yourself a reviewer/co-author to keep track of that? -- Ibrar Ahmed
Re: Remove page-read callback from XLogReaderState.
On Wed, Jun 30, 2021 at 12:54 PM Kyotaro Horiguchi wrote: > At Fri, 09 Apr 2021 09:36:59 +0900 (JST), Kyotaro Horiguchi < > horikyota@gmail.com> wrote in > > I'm surprised to see this pushed this soon. Thanks for pushing this! > > Then this has been reverted. I'm not sure how to check for the > possible defect happens on that platform, but, anyways I reverted the > CF item to "Needs Review" then moved to the next CF. > > Maybe I will rebase it soon. > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > > > Yes, rebase is required, therefore I am changing the status to "Waiting On Author" http://cfbot.cputube.org/patch_33_2113.log -- Ibrar Ahmed
Re: [PATCH] Partial foreign key updates in referential integrity triggers
On Wed, Jul 14, 2021 at 6:51 PM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > > On 05.01.21 22:40, Paul Martinez wrote: > > I've created a patch to better support referential integrity constraints > when > > using composite primary and foreign keys. This patch allows creating a > foreign > > key using the syntax: > > > >FOREIGN KEY (tenant_id, fk_id) REFERENCES fktable ON DELETE SET NULL > (fk_id) > > > > which means that only the fk_id column will be set to NULL when the > referenced > > row is deleted, rather than both the tenant_id and fk_id columns. > > I think this is an interesting feature with a legitimate use case. > > I'm wondering a bit about what the ON UPDATE side of this is supposed to > mean. Consider your example: > > > CREATE TABLE tenants (id serial PRIMARY KEY); > > CREATE TABLE users ( > >tenant_id int REFERENCES tenants ON DELETE CASCADE, > >id serial, > >PRIMARY KEY (tenant_id, id), > > ); > > CREATE TABLE posts ( > > tenant_id int REFERENCES tenants ON DELETE CASCADE, > > id serial, > > author_id int, > > PRIMARY KEY (tenant_id, id), > > FOREIGN KEY (tenant_id, author_id) REFERENCES users ON DELETE SET > NULL > > ); > > > > INSERT INTO tenants VALUES (1); > > INSERT INTO users VALUES (1, 101); > > INSERT INTO posts VALUES (1, 201, 101); > > DELETE FROM users WHERE id = 101; > > ERROR: null value in column "tenant_id" violates not-null constraint > > DETAIL: Failing row contains (null, 201, null). > > Consider what should happen when you update users.id. Per SQL standard, > for MATCH SIMPLE an ON UPDATE SET NULL should only set to null the > referencing column that corresponds to the referenced column actually > updated, not all of them. PostgreSQL doesn't do this, but if it did, > then this would work just fine. > > Your feature requires specifying a fixed column or columns to update, so > it cannot react differently to what column actually updated. In fact, > you might want different referential actions depending on what columns > are updated, like what you can do with general triggers. > > So, unless I'm missing an angle here, I would suggest leaving out the ON > UPDATE variant of this feature. > > > Patch does not apply on head, I am marking the status "Waiting on author" http://cfbot.cputube.org/patch_33_2932.log -- Ibrar Ahmed
Re: BUG #16583: merge join on tables with different DB collation behind postgres_fdw fails
On Wed, Jul 14, 2021 at 1:41 AM Tom Lane wrote: > Ibrar Ahmed writes: > > The patch is failing the regression, @Tom Lane can > you > > please take a look at that. > > Seems to just need an update of the expected-file to account for test > cases added recently. (I take no position on whether the new results > are desirable; some of these might be breaking the intent of the case. > But this should quiet the cfbot anyway.) > > regards, tom lane > > Thanks for the update. The test case was added by commit "Add support for asynchronous execution" "27e1f14563cf982f1f4d71e21ef24782a052" by Etsuro Fujita. He can comment whether the new results are desirable or not. -- Ibrar Ahmed
Re: POC: GROUP BY optimization
the "optimal" pathkeys (by lowest cost). > > * the places calling group_keys_reorder_by_pathkeys should loop on the > result, and generate separate path for each option. > > I'd guess in the future we'll "peek forward" in the plan and see if > there are other interesting pathkeys (that's the expectation for > get_useful_pathkeys_for_relation). > > > regards > > -- > Tomas Vondra > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > The patch does not apply successfully, can you please take a look at that http://cfbot.cputube.org/patch_33_1651.log === applying patch ./0001-v12-20210309.patch atching file src/include/utils/selfuncs.h Hunk #1 FAILED at 198. 1 out of 1 hunk FAILED -- saving rejects to file src/include/utils/selfuncs.h.rej Based on @Tomas Vondra comments, and patch does not apply, I am changing the status to "Waiting On Author". -- Ibrar Ahmed
Re: a misbehavior of partition row movement (?)
On Fri, Apr 2, 2021 at 6:09 PM Amit Langote wrote: > On Thu, Apr 1, 2021 at 10:56 AM Masahiko Sawada > wrote: > > On Tue, Mar 23, 2021 at 6:27 PM Amit Langote > wrote: > > > Actually, I found a big hole in my assumptions around deferrable > > > foreign constraints, invalidating the approach I took in 0002 to use a > > > query-lifetime tuplestore to record root parent tuples. I'm trying to > > > find a way to make the tuplestore transaction-lifetime so that the > > > patch still works. > > > > > > In the meantime, I'm attaching an updated set with 0001 changed per > > > your comments. > > > > 0001 patch conflicts with 71f4c8c6f74. Could you please rebase the > patchset? > > Thanks for the heads up. > > I still don't have a working patch to address the above mentioned > shortcoming of the previous approach, but here is a rebased version in > the meantime. > > > -- > Amit Langote > EDB: http://www.enterprisedb.com > @Amit patch is not successfully applying, can you please rebase that. Masahiko Sawada, it's been a bit long since you reviewed the patch, are you still interested to review that? -- Ibrar Ahmed
Re: BUG #16583: merge join on tables with different DB collation behind postgres_fdw fails
On Wed, Mar 3, 2021 at 1:42 PM Neil Chen
wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: not tested
> Documentation:tested, passed
>
> Greetings,
> I learned about the patch and read your discussions. I'm not sure why this
> patch has not been discussed now. In short, I think it's beneficial to
> submit it as a temporary solution.
> Another thing I want to know is whether these codes can be simplified:
> - if (state > outer_cxt->state)
> + if (collation == outer_cxt->collation &&
> + ((state == FDW_COLLATE_UNSAFE &&
> + outer_cxt->state == FDW_COLLATE_SAFE) ||
> +(state == FDW_COLLATE_SAFE &&
> + outer_cxt->state == FDW_COLLATE_UNSAFE)))
> + {
> + outer_cxt->state = FDW_COLLATE_SAFE;
> + }
> + else if (state > outer_cxt->state)
>
> If the state is determined by the collation, when the collations are
> equal, do we just need to judge the state not equal to FDW_COLLATE_NONE?
The patch is failing the regression, @Tom Lane can you
please take a look at that.
https://cirrus-ci.com/task/4593497492684800
== running regression test queries ==
test postgres_fdw ... FAILED 2782 ms
== shutting down postmaster ==
==
1 of 1 tests failed.
==
The differences that caused some tests to fail can be viewed in the
file "/tmp/cirrus-ci-build/contrib/postgres_fdw/regression.diffs". A copy
of the test summary that you see
above is saved in the file
"/tmp/cirrus-ci-build/contrib/postgres_fdw/regression.out".
--
Ibrar Ahmed
Re: DROP relation IF EXISTS Docs and Tests - Bug Fix
On Tue, Mar 9, 2021 at 9:01 PM David Steele wrote: > On 3/9/21 10:08 AM, David G. Johnston wrote: > > > > On Tuesday, March 9, 2021, David Steele > <mailto:da...@pgmasters.net>> wrote: > > > > Further, I think we should close this entry at the end of the CF if > > it does not attract committer interest. Tom is not in favor of the > > patch and it appears Alexander decided not to commit it. > > > > Pavel re-reviewed it and was fine with ready-to-commit so that status > > seems fine. > > Ah yes, that was my mistake. > > Regards, > -- > -David > da...@pgmasters.net > > > The status of the patch is "Need Review" which was previously "Ready for Committer ''. After @David G and @David Steele comments, it's not clear whether it should be "Read for commit" or "Need Review". -- Ibrar Ahmed
2021-07 CF now in progress
Hackers, The Commitfest 2021-07 is now in progress. It is one of the biggest one. Total number of patches of this commitfest is 342. Needs review: 204. Waiting on Author: 40. Ready for Committer: 18. Committed: 57. Moved to next CF: 3. Withdrawn: 15. Rejected: 3. Returned with Feedback: 2. Total: 342. If you are a patch author, please check http://commitfest.cputube.org to be sure your patch still applies, compiles, and passes tests. We need your involvement and participation in reviewing the patches. Let's try and make this happen. -- Regards. Ibrar Ahmed
Re: Online verification of checksums
On Tue, Mar 9, 2021 at 10:43 PM David Steele wrote: > On 11/30/20 6:38 PM, David Steele wrote: > > On 11/30/20 9:27 AM, Stephen Frost wrote: > >> * Michael Paquier (mich...@paquier.xyz) wrote: > >>> On Fri, Nov 27, 2020 at 11:15:27AM -0500, Stephen Frost wrote: > >>>> * Magnus Hagander (mag...@hagander.net) wrote: > >>>>> On Thu, Nov 26, 2020 at 8:42 AM Michael Paquier > >>>>> wrote: > >>>>>> But here the checksum is broken, so while the offset is something we > >>>>>> can rely on how do you make sure that the LSN is fine? A broken > >>>>>> checksum could perfectly mean that the LSN is actually *not* fine if > >>>>>> the page header got corrupted. > >>>> > >>>> Of course that could be the case, but it gets to be a smaller and > >>>> smaller chance by checking that the LSN read falls within reasonable > >>>> bounds. > >>> > >>> FWIW, I find that scary. > >> > >> There's ultimately different levels of 'scary' and the risk here that > >> something is actually wrong following these checks strikes me as being > >> on the same order as random bits being flipped in the page and still > >> getting a valid checksum (which is entirely possible with our current > >> checksum...), or maybe even less. > > > > I would say a lot less. First you'd need to corrupt one of the eight > > bytes that make up the LSN (pretty likely since corruption will probably > > affect the entire block) and then it would need to be updated to a value > > that falls within the current backup range, a 1 in 16 million chance if > > a terabyte of WAL is generated during the backup. Plus, the corruption > > needs to happen during the backup since we are going to check for that, > > and the corrupted LSN needs to be ascending, and the LSN originally read > > needs to be within the backup range (another 1 in 16 million chance) > > since pages written before the start backup checkpoint should not be > torn. > > > > So as far as I can see there are more likely to be false negatives from > > the checksum itself. > > > > It would also be easy to add a few rounds of checks, i.e. test if the > > LSN ascends but stays in the backup LSN range N times. > > > > Honestly, I'm much more worried about corruption zeroing the entire > > page. I don't know how likely that is, but I know none of our proposed > > solutions would catch it. > > > > Andres, since you brought this issue up originally perhaps you'd like to > > weigh in? > > I had another look at this patch and though I think my suggestions above > would improve the patch, I have no objections to going forward as is (if > that is the consensus) since this seems an improvement over what we have > now. > > It comes down to whether you prefer false negatives or false positives. > With the LSN checking Stephen and I advocate it is theoretically > possible to have a false negative but the chances of the LSN ascending N > times but staying within the backup LSN range due to corruption seems to > be approaching zero. > > I think Michael's method is unlikely to throw false positives, but it > seems at least possible that a block would be hot enough to be appear > torn N times in a row. Torn pages themselves are really easy to reproduce. > > If we do go forward with this method I would likely propose the > LSN-based approach as a future improvement. > > Regards, > -- > -David > da...@pgmasters.net > > > I am changing the status to "Waiting on Author" based on the latest comments of @David Steele and secondly the patch does not apply cleanly. http://cfbot.cputube.org/patch_33_2719.log -- Ibrar Ahmed
Re: standby recovery fails (tablespace related) (tentative patch and discussion)
On Tue, Mar 30, 2021 at 12:12 PM Paul Guo wrote: > On 2021/3/27, 10:23 PM, "Alvaro Herrera" wrote: > > >Hmm, can you post a rebased set, where the points under discussion > > are marked in XXX comments explaining what the issue is? This thread > is > >long and old ago that it's pretty hard to navigate the whole thing in > >order to find out exactly what is being questioned. > > OK. Attached are the rebased version that includes the change I discussed > in my previous reply. Also added POD documentation change for > RecursiveCopy, > and modified the patch to use the backup_options introduced in > 081876d75ea15c3bd2ee5ba64a794fd8ea46d794 for tablespace mapping. > > >I think 0004 can be pushed without further ado, since it's a clear and > >simple fix. 0001 needs a comment about the new parameter in > >RecursiveCopy's POD documentation. > > Yeah, 0004 is no any risky. One concern seemed to be the compatibility of > some > WAL dump/analysis tools(?). I have no idea about this. But if we do not > backport > 0004 we do not seem to need to worry about this. > > >As I understand, this is a backpatchable bug-fix. > > Yes. > > Thanks. > > Patch does not apply successfully, http://cfbot.cputube.org/patch_33_2161.log Can you please rebase the patch. -- Ibrar Ahmed
Re: Commit fest manager
On Tue, Jul 6, 2021 at 3:58 PM Michael Paquier wrote: > On Tue, Jul 06, 2021 at 03:38:23PM +0500, Ibrar Ahmed wrote: > > Any update and decision on this? so I can start working on this. > > Working on the CF does not strongly require the admin permissions. I > have already switched the current CF as in progress, so most of the > admin job is done for this month :) > > Another piece of interest are the reviewer/author stat reports, but > poking at patching does not need this information in most cases. > -- > Michael > Great, thanks Michael, I will start doing my work :) -- Ibrar Ahmed
Re: Commit fest manager
On Fri, Jul 2, 2021 at 7:15 PM Ibrar Ahmed wrote: > > > On Fri, 2 Jul 2021 at 7:06 PM, vignesh C wrote: > >> On Fri, Jul 2, 2021 at 6:05 PM Ibrar Ahmed wrote: >> > >> > >> > >> > On Fri, 2 Jul 2021 at 1:47 PM, David Rowley >> wrote: >> >> >> >> On Fri, 2 Jul 2021 at 15:04, vignesh C wrote: >> >> > I'm interested in assisting Ibrar Ahmed. >> >> >> >> It might be worth talking to Ibrar to see where you can lend a hand. I >> >> think in terms of the number of patches, this might be our biggest >> >> commitfest yet. >> >> >> >> 2020-07 246 >> >> 2020-09 235 >> >> 2020-11 244 >> >> 2021-01 260 >> >> 2020-03 295 >> >> 2020-07 342 >> >> >> >> It's possible Ibrar would welcome you helping to take care of some of >> >> the duties. I've never been brave enough to take on the CF manager >> >> role yet, but from what I can see, to do a good job takes a huge >> >> amount of effort. >> >> >> >> David >> > >> > >> > I am willing to take the responsibility, help from vegnsh is welcome >> >> Thanks, Can someone provide me permissions as this will be my first >> time. My username is vignesh.postgres. >> >> Regards, >> Vignesh > > i need permission my id is ibrar.ah...@gmail.com > >> >> -- > Ibrar Ahmed > Any update and decision on this? so I can start working on this. -- Ibrar Ahmed
Re: Commit fest manager
On Fri, 2 Jul 2021 at 7:06 PM, vignesh C wrote: > On Fri, Jul 2, 2021 at 6:05 PM Ibrar Ahmed wrote: > > > > > > > > On Fri, 2 Jul 2021 at 1:47 PM, David Rowley > wrote: > >> > >> On Fri, 2 Jul 2021 at 15:04, vignesh C wrote: > >> > I'm interested in assisting Ibrar Ahmed. > >> > >> It might be worth talking to Ibrar to see where you can lend a hand. I > >> think in terms of the number of patches, this might be our biggest > >> commitfest yet. > >> > >> 2020-07 246 > >> 2020-09 235 > >> 2020-11 244 > >> 2021-01 260 > >> 2020-03 295 > >> 2020-07 342 > >> > >> It's possible Ibrar would welcome you helping to take care of some of > >> the duties. I've never been brave enough to take on the CF manager > >> role yet, but from what I can see, to do a good job takes a huge > >> amount of effort. > >> > >> David > > > > > > I am willing to take the responsibility, help from vegnsh is welcome > > Thanks, Can someone provide me permissions as this will be my first > time. My username is vignesh.postgres. > > Regards, > Vignesh i need permission my id is ibrar.ah...@gmail.com > > -- Ibrar Ahmed
Re: Commit fest manager
On Fri, 2 Jul 2021 at 1:47 PM, David Rowley wrote: > On Fri, 2 Jul 2021 at 15:04, vignesh C wrote: > > I'm interested in assisting Ibrar Ahmed. > > It might be worth talking to Ibrar to see where you can lend a hand. I > think in terms of the number of patches, this might be our biggest > commitfest yet. > > 2020-07 246 > 2020-09 235 > 2020-11 244 > 2021-01 260 > 2020-03 295 > 2020-07 342 > > It's possible Ibrar would welcome you helping to take care of some of > the duties. I've never been brave enough to take on the CF manager > role yet, but from what I can see, to do a good job takes a huge > amount of effort. > > David I am willing to take the responsibility, help from vegnsh is welcome > > -- Ibrar Ahmed
Re: Commit fest manager
Hi, Yes, I want to do that for sure. On Wed, Jun 30, 2021 at 6:33 PM Daniel Gustafsson wrote: > > On 30 Jun 2021, at 15:31, vignesh C wrote: > > > The next commit fest is going to begin soon. > > I would like to volunteer as commit fest manager for 2021-07 if the > > role is not filled and there are no objections. > > Ibrar Ahmed has already volunteered since a while back, so let's see if he > is > still keen to take it on: > > > https://postgr.es/m/caltqxtcgbfeuwg-8mgmsaojp8te61_qaedjbnexjbybeeyz...@mail.gmail.com > > -- > Daniel Gustafsson https://vmware.com/ > > -- Ibrar Ahmed
Re: Next Commitfest Manager.
On Thu, Feb 4, 2021 at 1:13 AM Stephen Frost wrote: > Greetings, > > * Ibrar Ahmed (ibrar.ah...@gmail.com) wrote: > > Anyone else already volunteers that? It is my first time so need some > > access, if all agree. > > Thanks for volunteering! > > That said, our last commitfest tends to be the most difficult as it's > the last opportunity for features to land in time for the next major > release and, for my part at least, I think it'd be best to have > someone who has experience running a CF previously manage it. > > To that end, I've talked to David Steele, who has run this last CF for > the past few years and we're in agreement that he's willing to run this > CF again this year, assuming there's no objections. What we've thought > to suggest is that you follow along with David as he runs this CF and > then offer to run the July CF. Of course, we would encourage you and > David to communicate and for you to ask David any questions you have > about how he handles things as part of the CF. This is in line with how > other CF managers have started out also. > As we know July commitfest is coming, I already volunteered to manage that. I did small work with David in the last commitfest and now ready to work on that > > Open to your thoughts, as well as those of anyone else who wishes to > comment. > > Thanks! > > Stephen > -- Ibrar Ahmed
Re: [HACKERS] make async slave to wait for lsn to be replayed
On Thu, Jan 21, 2021 at 1:30 PM Kyotaro Horiguchi wrote: > Hello. > > At Wed, 18 Nov 2020 15:05:00 +0300, a.pervush...@postgrespro.ru wrote in > > I've changed the BEGIN WAIT FOR LSN statement to core functions > > pg_waitlsn, pg_waitlsn_infinite and pg_waitlsn_no_wait. > > Currently the functions work inside repeatable read transactions, but > > waitlsn creates a snapshot if called first in a transaction block, > > which can possibly lead the transaction to working incorrectly, so the > > function gives a warning. > > According to the discuttion here, implementing as functions is not > optimal. As a Poc, I made it as a procedure. However I'm not sure it > is the correct implement as a native procedure but it seems working as > expected. > > > Usage examples > > == > > select pg_waitlsn(‘LSN’, timeout); > > select pg_waitlsn_infinite(‘LSN’); > > select pg_waitlsn_no_wait(‘LSN’); > > The first and second usage is coverd by a single procedure. The last > function is equivalent to pg_last_wal_replay_lsn(). As the result, the > following procedure is provided in the attached. > > pg_waitlsn(wait_lsn pg_lsn, timeout integer DEFAULT -1) > > Any opinions mainly compared to implementation as a command? > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > The patch (pg_waitlsn_v10_2_kh.patch) does not compile successfully and has compilation errors. Can you please take a look? https://cirrus-ci.com/task/6241565996744704 xlog.c:45:10: fatal error: commands/wait.h: No such file or directory #include "commands/wait.h" ^ compilation terminated. make[4]: *** [: xlog.o] Error 1 make[4]: *** Waiting for unfinished jobs make[3]: *** [../../../src/backend/common.mk:39: transam-recursive] Error 2 make[2]: *** [common.mk:39: access-recursive] Error 2 make[1]: *** [Makefile:42: all-backend-recurse] Error 2 make: *** [GNUmakefile:11: all-src-recurse] Error 2 I am changing the status to "Waiting on Author" -- Ibrar Ahmed
Re: should INSERT SELECT use a BulkInsertState?
On Mon, Mar 8, 2021 at 2:18 PM houzj.f...@fujitsu.com < houzj.f...@fujitsu.com> wrote: > > > > I am very interested in this patch, and I plan to do some > > > > experiments with the > > > patch. > > > > Can you please rebase the patch because it seems can not applied to > > > > the > > > master now. > > > > > > Thanks for your interest. > > > > > > I was sitting on a rebased version since the bulk FDW patch will cause > > > conflicts, and since this should maybe be built on top of the table-am > patch > > (2871). > > > Have fun :) > > Hi, > > > > When I testing with the patch, I found I can not use "\d tablename". > > It reports the following error, it this related to the patch? > > Sorry, solved by re-initdb. > > Best regards, > houzj > > > One of the patch (v10-0001-INSERT-SELECT-to-use-BulkInsertState-and-multi_i.patch) from the patchset does not apply. http://cfbot.cputube.org/patch_32_2553.log 1 out of 13 hunks FAILED -- saving rejects to file src/backend/commands/copyfrom.c.rej It is a minor change, therefore I fixed that to make cfbot happy. Please take a look if that works for you. -- Ibrar Ahmed v11-0001-INSERT-SELECT-to-use-BulkInsertState-and-multi_i.patch Description: Binary data
Re: Transactions involving multiple postgres foreign servers, take 2
r OID > but I think this is because they consulted to old postgres_fdw code. I > suspect that there is no use case where FDW needs to identify > connections in that way. If the core GTM identifies them by user > mapping OID, we could enforce those FDWs to change their way but I > think that change would be the right improvement. > > Regards, > > -- > Masahiko Sawada > EDB: https://www.enterprisedb.com/ > > > Regression is failing, can you please take a look. https://cirrus-ci.com/task/5522445932167168 t/080_pg_isready.pl ... ok # Failed test 'parallel reindexdb for system with --concurrently skips catalogs status (got 1 vs expected 0)' # at t/090_reindexdb.pl line 191. Bailout called. Further testing stopped: system pg_ctl failed FAILED--Further testing stopped: system pg_ctl failed make[2]: *** [Makefile:57: check] Error 255 make[1]: *** [Makefile:43: check-scripts-recurse] Error 2 make: *** [GNUmakefile:71: check-world-src/bin-recurse] Error 2 === ./contrib/hstore_plperl/log/initdb.log === Running in no-clean mode. Mistakes will not be cleaned up. The files belonging to this database system will be owned by user "postgres". This user must also own the server process. -- -- Ibrar Ahmed
Re: SQL/JSON: functions
On Sat, Jan 23, 2021 at 3:37 PM Erik Rijkers wrote: > On 2021-01-20 03:49, Nikita Glukhov wrote: > > > [0001-Add-common-SQL-JSON-clauses-v52.patch.gz] > > [0002-SQL-JSON-constructors-v52.patch.gz] > > [0003-IS-JSON-predicate-v52.patch.gz] > > [0004-SQL-JSON-query-functions-v52.patch.gz] > > [0005-SQL-JSON-functions-for-json-type-v52.patch.gz] > > [0006-GUC-sql_json-v52.patch.gz] > > Hi, > > I read through the file func.sgml (only that file) and put the > errors/peculiarities in the attached diff. (Small stuff; typos really) > > > Your patch includes a CREATE TABLE my_films + INSERT, to run the > examples against. I think this is a great idea and we should do it more > often. > > But, the table has a text-column to contain the subsequently inserted > json values. The insert runs fine but it turns out that some later > examples queries only run against a jsonb column. So I propose to > change: >CREATE TABLE my_films (js text); > to: >CREATE TABLE my_films (js jsonb); > > This change is not yet included in the attached file. An alternative > would be to cast the text-column in the example queries as js::jsonb > > > I also noticed that some errors were different in the sgml file than 'in > the event': > > > SELECT JSON_QUERY(js, '$.favorites[*].kind' ERROR ON ERROR) FROM > my_films_jsonb; > (table 'my_films_jsonb' is the same as your 'my_films', but with js > as a jsonb column) > > manual says: "ERROR: more than one SQL/JSON item" > in reality: "ERROR: JSON path expression in JSON_QUERY should return > singleton item without wrapper" > and: "HINT: use WITH WRAPPER clause to wrap SQL/JSON item > sequence into array" > > > Thanks, > > Erik Rijkers > > > > > -- > > Nikita Glukhov > > Postgres Professional: http://www.postgrespro.com > > The Russian Postgres Company > The patch (func.sgml.20210123.diff) does not apply successfully. http://cfbot.cputube.org/patch_32_2901.log === Applying patches on top of PostgreSQL commit ID 0ce4cd04da558178b0186057b721c50a00b7a945 === === applying patch ./func.sgml.20210123.diff patching file doc/src/sgml/func.sgml Hunk #1 FAILED at 16968. Hunk #2 FAILED at 17034. ... Hunk #19 FAILED at 18743. 19 out of 19 hunks FAILED -- saving rejects to file doc/src/sgml/func.sgml.rej Can we get a rebase? I am marking the patch "Waiting on Author". -- Ibrar Ahmed
Re: partial heap only tuples
On Wed, Feb 24, 2021 at 3:22 AM Bossart, Nathan wrote: > On 2/10/21, 2:43 PM, "Bruce Momjian" wrote: > > I wonder if you should create a Postgres wiki page to document all of > > this. I agree PG 15 makes sense. I would like to help with this if I > > can. I will need to study this email more later. > > I've started the wiki page for this: > > https://wiki.postgresql.org/wiki/Partial_Heap_Only_Tuples > > Nathan > > The regression test case (partial-index) is failing https://cirrus-ci.com/task/5310522716323840 === ./src/test/isolation/output_iso/regression.diffs === diff -U3 /tmp/cirrus-ci-build/src/test/isolation/expected/partial-index.out /tmp/cirrus-ci-build/src/test/isolation/output_iso/results/partial-index.out --- /tmp/cirrus-ci-build/src/test/isolation/expected/partial-index.out 2021-03-06 23:11:08.018868833 + +++ /tmp/cirrus-ci-build/src/test/isolation/output_iso/results/partial-index.out 2021-03-06 23:26:15.857027075 + @@ -30,6 +30,8 @@ 6 a 1 7 a 1 8 a 1 +9 a 2 +10 a 2 step c2: COMMIT; starting permutation: rxy1 wx1 wy2 c1 rxy2 c2 @@ -83,6 +85,7 @@ 6 a 1 7 a 1 8 a 1 +9 a 2 10 a 1 step c1: COMMIT; ---- Can you please take a look at that? -- Ibrar Ahmed
Re: TRUNCATE on foreign table
On Thu, Feb 11, 2021 at 6:23 PM Ashutosh Bapat wrote: > On Wed, Feb 10, 2021 at 10:58 PM Kazutaka Onishi > wrote: > > > > That's because using the foreign server is difficult for the user. > > > > For example, the user doesn't always have the permission to login to the > forein server. > > In some cases, the foreign table has been created by the administrator > that has permission to access the two servers and the user only uses the > local server. > > Then the user has to ask the administrator to run TRUNCATE every time. > > That might actually be seen as a loophole but ... > > > > > Furthermore,there are some fdw extensions which don't support SQL. > mongo_fdw, redis_fdw, etc... > > These extensions have been used to provide SQL interfaces to the users. > > It's hard for the user to run TRUNCATE after learning each database. > > this has some appeal. > > Thanks for sharing the usecases. > -- > Best Wishes, > Ashutosh Bapat > > > The patch (pgsql14-truncate-on-foreign-table.v2.patch) does not apply successfully. http://cfbot.cputube.org/patch_32_2972.log patching file contrib/postgres_fdw/expected/postgres_fdw.out Hunk #2 FAILED at 9179. 1 out of 2 hunks FAILED -- saving rejects to file contrib/postgres_fdw/expected/postgres_fdw.out.rej As this is a minor change therefore I have updated the patch. Please take a look. -- Ibrar Ahmed pgsql14-truncate-on-foreign-table.v3.patch Description: Binary data
Re: shared-memory based stats collector
On Fri, Mar 5, 2021 at 8:32 PM Fujii Masao
wrote:
>
>
> On 2021/03/05 17:18, Kyotaro Horiguchi wrote:
> > At Thu, 21 Jan 2021 12:03:48 +0900 (JST), Kyotaro Horiguchi <
> horikyota@gmail.com> wrote in
> >> Commit 960869da08 (database statistics) conflicted with this. Rebased.
> >>
> >> I'm concerned about the behavior that pgstat_update_connstats calls
> >> GetCurrentTimestamp() every time stats update happens (with intervals
> >> of 10s-60s in this patch). But I didn't change that design since that
> >> happens with about 0.5s intervals in master and the rate is largely
> >> reduced in this patch, to make this patch simpler.
> >
> > I stepped on my foot, and another commit coflicted. Just rebased.
>
> Thanks for rebasing the patches!
>
> I think that 0003 patch is self-contained and useful, for example which
> enables us to monitor archiver process in pg_stat_activity. So IMO
> it's worth pusing 0003 patch firstly.
>
> Here are the review comments for 0003 patch.
>
> + /* Archiver process's latch */
> + Latch *archiverLatch;
> + /* Current shared estimate of appropriate spins_per_delay value */
>
> The last line in the above seems not necessary.
>
> In proc.h, NUM_AUXILIARY_PROCS needs to be incremented.
>
> /* --
> * Functions called from postmaster
> * --
> */
> extern int pgarch_start(void);
>
> In pgarch.h, the above is not necessary.
>
> +extern void XLogArchiveWakeup(void);
>
> This seems no longer necessary.
>
> +extern void XLogArchiveWakeupStart(void);
> +extern void XLogArchiveWakeupEnd(void);
> +extern void XLogArchiveWakeup(void);
>
> These seem also no longer necessary.
>
> PgArchPID = 0;
> if (!EXIT_STATUS_0(exitstatus))
> - LogChildExit(LOG, _("archiver process"),
> -pid, exitstatus);
> - if (PgArchStartupAllowed())
> - PgArchPID = pgarch_start();
> + HandleChildCrash(pid, exitstatus,
> +
> _("archiver process"));
>
> I don't think that we should treat non-zero exit condition as a crash,
> as before. Otherwise when archive_command fails on a signal,
> archiver emits FATAL error and which leads the server restart.
>
> - * walwriter, autovacuum, or background worker.
> + * walwriter, autovacuum, archiver or background worker.
>*
>* The objectives here are to clean up our local state about the child
>* process, and to signal all other remaining children to quickdie.
> @@ -3609,6 +3606,18 @@ HandleChildCrash(int pid, int exitstatus, const
> char *procname)
> signal_child(AutoVacPID, (SendStop ? SIGSTOP : SIGQUIT));
> }
>
> + /* Take care of the archiver too */
> + if (pid == PgArchPID)
> + PgArchPID = 0;
> + else if (PgArchPID != 0 && take_action)
> + {
> + ereport(DEBUG2,
> + (errmsg_internal("sending %s to process
> %d",
> +(SendStop
> ? "SIGSTOP" : "SIGQUIT"),
> +(int)
> PgArchPID)));
> + signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT));
> + }
> +
>
> Same as above.
>
>
> In xlogarchive.c, "#include "storage/pmsignal.h"" is no longer necessary.
>
>
> pgarch_forkexec() should be removed from pgarch.c because it's no longer
> used.
>
>
> /*
> * Public functions called from postmaster follow
> *
> */
>
> The definition of PgArchiverMain() should be placed just
> after the above comment.
>
>
> exit(0) in PgArchiverMain() should be proc_exit(0)?
>
> Regards,
>
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION
>
>
>
The code does not compile and has compilation warnings and errors.
--
pgstat.c:446:25: note: ‘cached_slrustats’ declared here
static PgStat_SLRUStats cached_slrustats;
^~~~
guc.c:4372:4: error: use of undeclared identifier 'pgstat_temp_directory';
did you mean 'pgstat_stat_directory'?
_temp_directory,
^
pgstat_stat_directory
../../../../src/include/pgstat.h:922:14: note: 'pgstat_stat_directory'
declared here
extern char *pgstat_stat_directory;
^
guc.c:4373:3: error: use of undeclared identifier 'PG_STAT_TMP_DIR'
PG_STAT_TMP_DIR,
^
guc.c:4374:25: error: use of undeclared identifier
'assign_pgstat_temp_directory'
check_canonical_path, assign_pgstat_temp_directory, NULL
---
Can we get an updated patch?
I am marking the patch "Waiting on Author"
--
Ibrar Ahmed
Re: ResourceOwner refactoring
On Mon, Jan 25, 2021 at 10:15 PM Robert Haas wrote: > On Thu, Jan 21, 2021 at 5:14 AM Heikki Linnakangas > wrote: > > Here you can see that as numsnaps increases, the test becomes slower, > > but then it becomes faster again at 64-66, when it switches to the hash > > table. So 64 seems too much. 32 seems to be the sweet spot here, that's > > where scanning the hash and scanning the array are about the same speed. > > That sounds good. I mean, it could be that not all hardware behaves > the same here. But trying to get it into the right ballpark makes > sense. > > I also like the fact that this now has some cases where it wins by a > significant margin. That's pretty cool; thanks for working on it! > > -- > Robert Haas > EDB: http://www.enterprisedb.com > > > The patchset does not apply successfully, there are some hunk failures. http://cfbot.cputube.org/patch_32_2834.log v6-0002-Make-resowners-more-easily-extensible.patch 1 out of 6 hunks FAILED -- saving rejects to file src/backend/utils/cache/plancache.c.rej 2 out of 15 hunks FAILED -- saving rejects to file src/backend/utils/resowner/resowner.c.rej Can we get a rebase? I am marking the patch "Waiting on Author" -- Ibrar Ahmed
Re: Let people set host(no)ssl settings from initdb
On Thu, Mar 4, 2021 at 7:25 AM Michael Paquier wrote: > On Wed, Mar 03, 2021 at 03:07:30PM +0100, Peter Eisentraut wrote: > > I think there is enough sustained opposition to this patch that we can > mark > > this as rejected in the commitfest. > > +1. > -- > Michael > The patch (v5-0001-Enable-setting-pg_hba.conf-permissions-from-initd.patch) does not apply successfully. There are two reasons first is it was not generated with proper "-p" which confused cfbot. Second, after fixing that issue you still need to rebase that. http://cfbot.cputube.org/patch_32_2916.log |+++ doc/src/sgml/ref/initdb.sgml -- No file to patch. Skipping patch. 1 out of 1 hunk ignored can't find file to patch at input line 77 Perhaps you used the wrong -p or --strip option? The text leading up to this was: Can we get a rebase? I am marking the patch "Waiting on Author" -- Ibrar Ahmed
Re: Yet another fast GiST build
On Mon, Mar 8, 2021 at 8:59 PM Peter Geoghegan wrote: > On Mon, Mar 8, 2021 at 6:41 AM Ibrar Ahmed wrote: > > The patch > (0001-Add-bool-column-for-LP_DEAF-flag-to-GiST-pageinspect.patch) > > does not apply successfully and has multiple hanks failed. > > That's because it was committed. > > Thanks for the clarification, its status was not changed which confused me :) > -- > Peter Geoghegan > -- Ibrar Ahmed
Re: Evaluate expression at planning time for two more cases
On Tue, Nov 24, 2020 at 12:47 PM Surafel Temesgen wrote: > Hi Pavel Borisov, > It's always good to select the optimal way even if it didn't have > performance gain > but in case of this patch i see 4x speed up on my laptop and it will work > on any > table that have NULL constraint > > regards > Surafel > The patch (null_check_on_pkey_optimization_v2.patch) does not apply successfully. http://cfbot.cputube.org/patch_32_2699.log 1 out of 10 hunks FAILED -- saving rejects to file src/backend/optimizer/util/clauses.c.rej It was a minor change therefore I rebased the patch, please take a look. -- Ibrar Ahmed null_check_on_pkey_optimization_v3.patch Description: Binary data
Re: [PATCH] New default role allowing to change per-role/database settings
On Thu, Dec 31, 2020 at 6:16 PM Michael Banck wrote: > Hi, > > in today's world, some DBAs have no superuser rights, but we can > delegate them additional powers like CREATEDB or the pg_monitor default > role etc. Usually, the DBA can also view the database logs, either via > shell access or some web interface. > > One thing that I personally find lacking is that it is not possible to > change role-specific log settings (like log_statement = 'mod' for a > security sensitive role) without being SUPERUSER as their GUC context is > "superuser". This makes setup auditing much harder if there is no > SUPERUSER access, also pgaudit then only allows to configure object- > based auditing. Amazon RDS e.g. has patched Postgres to allow the > cluster owner/pseudo-superuser `rds_superuser' to change those log > settings that define what/when we log something, while keeping the > "where to log" entries locked down. > > The attached patch introduces a new guc context "administrator" (better > names/bikeshedding for this welcome) that is meant as a middle ground > between "superuser" and "user". It also adds a new default role > "pg_change_role_settings" (better names also welcome) that can be > granted to DBAs so that they can change the "administrator"-context GUCs > on a per-role (or per-database) basis. Whether the latter should be > included is maybe debatable, but I added both on the basis that they are > the same "source". > > The initial set of "administrator" GUCs are all current GUCs with > "superuser" context from these categories: > > * Reporting and Logging / What to Log > * Reporting and Logging / When to > Log > * Statistics / Query and Index Statistics Collector > * Statistics / > Monitoring > > Of course, further categories (or particular GUCs) could be added now or > in the future, e.g. RDS also patches the following GUCs in their v12 > offering: > > * temp_file_limit > * session_replication_role > > RDS does *not* patch log_transaction_sample_rate from "Reporting and > Logging / When to Log", but that might be more of an oversight than a > security consideration, or does anybody see a big problem with that > (compared to the others in that set)? > > I initially pondered not introducing a new context but just filtering on > category, but as categories seem to be only descriptive in guc.c and not > used for any policy decisions so far, I have abandoned this pretty > quickly. > > > Thoughts? > > Michael > > -- > Michael Banck > Projektleiter / Senior Berater > Tel.: +49 2166 9901-171 > Fax: +49 2166 9901-100 > Email: michael.ba...@credativ.de > > credativ GmbH, HRB Mönchengladbach 12080 > USt-ID-Nummer: DE204566209 > Trompeterallee 108, 41189 Mönchengladbach > Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer > > Unser Umgang mit personenbezogenen Daten unterliegt > folgenden Bestimmungen: https://www.credativ.de/datenschutz > The patch (0001-Add-new-PGC_ADMINSET-guc-context-and-pg_change_role_.patch) does not apply successfully and has some hunks failed. http://cfbot.cputube.org/patch_32_2918.log 1 out of 23 hunks FAILED -- saving rejects to file src/backend/utils/misc/guc.c.rej patching file src/include/catalog/catversion.h Hunk #1 FAILED at 53. 1 out of 1 hunk FAILED -- saving rejects to file src/include/catalog/catversion.h.rej patching file src/include/catalog/pg_authid.dat Can we get a rebase? I am marking the patch "Waiting on Author". -- Ibrar Ahmed
Re: popcount
On Tue, Jan 19, 2021 at 9:42 PM Isaac Morland wrote: > On Tue, 19 Jan 2021 at 11:38, David Fetter wrote: > >> You bring up an excellent point, which is that our builtin functions >> could use a lot more documentation directly to hand than they now >> have. For example, there's a lot of needless ambiguity created by >> function comments which leave it up in the air as to which positional >> argument does what in functions like string_to_array, which take >> multiple arguments. I'll try to get a patch in for the next CF with a >> fix for that, and a separate one that doesn't put it on people to use >> \df+ to find the comments we do provide. There have been proposals for >> including an optional space for COMMENT ON in DDL, but I suspect that >> those won't fly unless and until they make their way into the >> standard. >> > > Since you mention \df+, I wonder if this is the time to consider removing > the source code from \df+ (since we have \sf) and adding in the proacl > instead? > > The cf bot failed to apply the patch (v4-0001-popcount.patch) because of the wrong "-p" I have regenerated the patch, can you please take a look. http://cfbot.cputube.org/patch_32_2917.log === applying patch ./v4-0001-popcount.patch can't find file to patch at input line 21 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -- -- Ibrar Ahmed v5-0001-popcount.patch Description: Binary data
Re: Yet another fast GiST build
On Mon, Jan 18, 2021 at 3:52 AM Heikki Linnakangas wrote: > On 18/01/2021 00:35, Peter Geoghegan wrote: > > On Sun, Jan 17, 2021 at 12:50 PM Tom Lane wrote: > >> I noticed that gist_page_items() thinks it can hold inter_call_data->rel > >> open across a series of calls. That's completely unsafe: the executor > >> might not run the call series to completion (see LIMIT), resulting in > >> relcache leak complaints. > > Fixed, thanks! I changed it to return a tuplestore. > > > It also has the potential to run into big problems should the user > > input a raw page image with an regclass-argument-incompatible tuple > > descriptor. Maybe that's okay (this is a tool for experts), but it > > certainly is a consideration. > > I'm not sure I understand. It's true that the raw page image can contain > data from a different index, or any garbage really. And the function > will behave badly if you do that. That's an accepted risk with > pageinspect functions, that's why they're superuser-only, although some > of them are more tolerant of corrupt pages than others. The > gist_page_items_bytea() variant doesn't try to parse the key data and is > less likely to crash on bad input. > > - Heikki > > > The patch (0001-Add-bool-column-for-LP_DEAF-flag-to-GiST-pageinspect.patch ) does not apply successfully and has multiple hanks failed. http://cfbot.cputube.org/patch_32_2824.log patching file contrib/pageinspect/gistfuncs.c Hunk #1 FAILED at 151. Hunk #2 FAILED at 175. Hunk #3 FAILED at 245. Hunk #4 FAILED at 271. ... Can we get a rebase? I am marking the patch "Waiting on Author" -- Ibrar Ahmed
Re: pg_rewind race condition just after promotion
On Wed, Dec 9, 2020 at 6:35 PM Heikki Linnakangas wrote: > On 08/12/2020 06:45, Kyotaro Horiguchi wrote: > > At Mon, 7 Dec 2020 20:13:25 +0200, Heikki Linnakangas > wrote in > >> I think we should fix this properly. I'm not sure if it can lead to a > >> broken cluster, but at least it can cause pg_rewind to fail > >> unnecessarily and in a user-unfriendly way. But this is actually > >> pretty simple to fix. pg_rewind looks at the control file to find out > >> the timeline the server is on. When promotion happens, the startup > >> process updates minRecoveryPoint and minRecoveryPointTLI fields in the > >> control file. We just need to read it from there. Patch attached. > > > > Looks fine to me. A bit concerned about making sourceHistory > > needlessly file-local but on the other hand unifying sourceHistory and > > targetHistory looks better. > > Looking closer, findCommonAncestorTimeline() was freeing sourceHistory, > which was pretty horrible when it's a file-local variable. I changed it > so that both the source and target histories are passed to > findCommonAncestorTimeline() as arguments. That seems more clear. > > > For the test part, that change doesn't necessariry catch the failure > > of the current version, but I *believe* the prevous code is the result > > of an actual failure in the past so the test probablistically (or > > dependently on platforms?) hits the failure if it happned. > > Right. I think the current test coverage is good enough. We've been > bitten by this a few times by now, when we've forgotten to add the > manual checkpoint commands to new tests, and the buildfarm has caught it > pretty quickly. > > >> I think we should also backpatch this. Back in 2015, we decided that > >> we can live with this, but it's always been a bit bogus, and seems > >> simple enough to fix. > > > > I don't think this changes any successful behavior and it just saves > > the failure case so +1 for back-patching. > > Thanks for the review! New patch version attached. > > - Heikki > The patch does not apply successfully http://cfbot.cputube.org/patch_32_2864.log 1 out of 10 hunks FAILED -- saving rejects to file src/bin/pg_rewind/pg_rewind.c.rej There is a minor issue therefore I rebase the patch. Please take a look at that. -- Ibrar Ahmed v3-0001-pg_rewind-Fix-determining-TLI-when-server-was-jus.patch Description: Binary data
Re: POC: GROUP BY optimization
On Thu, Oct 29, 2020 at 8:15 PM Pavel Borisov wrote: > In case if I'm missing something and Pavel's proposal is significantly >> different from the original patch (if I understand correctly, at the >> moment the latest patch posted here is a rebase and adjusting the old >> patch to work with the latest changes in master, right?), then indeed >> they could be merged, but please in the newer thread [1]. >> > > Sure, my patch has the only difference from the previous Theodor's code > for compatibility with v.13, though it is not small, and I appreciate the > changes in paths processing. The only thing that caused my notice, is that > some useful changes which I've mentioned before, are discarded now. But as > long as they are planned to be put in later it is completely fine. I agree > to discuss the thing in any thread, though I don't quite understand the > reason for a switch. > > Still I don't see a problem. > > -- > Best regards, > Pavel Borisov > > Postgres Professional: http://postgrespro.com <http://www.postgrespro.com> > Regression (stats_ext) is failing because you forgot to drop the table created in a test case (aggregates), It's a bit minor change so the attached patch fixes that issue. https://cirrus-ci.com/task/6704792446697472 -- Ibrar Ahmed v12-0001-GROUP-BY-optimization-made-compatible-with-chang.patch Description: Binary data
Re: Corruption during WAL replay
On Wed, Jan 6, 2021 at 1:33 PM Kyotaro Horiguchi wrote: > At Mon, 17 Aug 2020 11:22:15 -0700, Andres Freund > wrote in > > Hi, > > > > On 2020-08-17 14:05:37 +0300, Heikki Linnakangas wrote: > > > On 14/04/2020 22:04, Teja Mupparti wrote: > > > > Thanks Kyotaro and Masahiko for the feedback. I think there is a > > > > consensus on the critical-section around truncate, > > > > > > +1 > > > > I'm inclined to think that we should do that independent of the far more > > complicated fix for other related issues. > ... > > > Perhaps a better approach would be to prevent the checkpoint from > > > completing, until all in-progress truncations have completed. We have a > > > mechanism to wait out in-progress commits at the beginning of a > checkpoint, > > > right after the redo point has been established. See comments around > the > > > GetVirtualXIDsDelayingChkpt() function call in CreateCheckPoint(). We > could > > > have a similar mechanism to wait out the truncations before > *completing* a > > > checkpoint. > > > > What I outlined earlier *is* essentially a way to do so, by preventing > > checkpointing from finishing the buffer scan while a dangerous state > > exists. > > Seems reasonable. The attached does that. It actually works for the > initial case. > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > The regression is failing for this patch, do you mind look at that and send the updated patch? https://api.cirrus-ci.com/v1/task/6313174510075904/logs/test.log ... t/006_logical_decoding.pl ok t/007_sync_rep.pl ok Bailout called. Further testing stopped: system pg_ctl failed FAILED--Further testing stopped: system pg_ctl failed make[2]: *** [Makefile:19: check] Error 255 make[1]: *** [Makefile:49: check-recovery-recurse] Error 2 make: *** [GNUmakefile:71: check-world-src/test-recurse] Error 2 ... -- Ibrar Ahmed
Re: [patch] bit XOR aggregate functions
On Wed, Mar 3, 2021 at 7:30 PM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > On 10.02.21 06:42, Kyotaro Horiguchi wrote: > > We already had CREATE AGGREATE at the time, so BIT_XOR can be thought > > as it falls into the same category with BIT_AND and BIT_OR, that is, > > we may have BIT_XOR as an intrinsic aggregation? > > I think the use of BIT_XOR is quite separate from BIT_AND and BIT_OR. > The latter give you an "all" or "any" result of the bits set. BIT_XOR > will return 1 or true if an odd number of inputs are 1 or true, which > isn't useful by itself. But it can be used as a checksum, so it seems > pretty reasonable to me to add it. Perhaps the use case could be > pointed out in the documentation. > > > > Hi Alex, The patch is failing just because of a comment, which is already changed by another patch -/* Define to build with OpenSSL support. (--with-ssl=openssl) */ +/* Define to 1 if you have OpenSSL support. */ Do you mind sending an updated patch? http://cfbot.cputube.org/patch_32_2980.log. I am changing the status to "Waiting for Author" In my opinion that change no more requires so I removed that and attached the patch. -- Ibrar Ahmed bit-xor-agg-v002.diff Description: Binary data
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
On Thu, Mar 4, 2021 at 8:01 PM Alvaro Herrera wrote: > Apparently, the archives system or the commitfest system is not picking > up new messages to the thread, so the CF app is trying to apply a > very old patch version. I'm not sure what's up with that. Thomas, any > clues on where to look? > > -- > Álvaro Herrera Valdivia, Chile > "Oh, great altar of passive entertainment, bestow upon me thy discordant > images > at such speed as to render linear thought impossible" (Calvin a la TV) > Hi Alvaro, The thread splits and CF app still has the previous thread. The old thread has the v18 as the latest patch which is failing. The new thread which ( https://www.postgresql.org/message-id/CAJkzx4T5E-2cQe3dtv2R78dYFvz%2Bin8PY7A8MArvLhs_pg75gg%40mail.gmail.com ) has the new patch. -- Ibrar Ahmed
Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT
On Mon, Aug 3, 2020 at 9:31 PM Wolfgang Walther
wrote:
> Tom Lane:
> > We don't generally act that way in other ALTER commands and I don't see
> > a strong argument to start doing so here. [...]
> >
> > In short, I'm inclined to argue that this variant of ALTER TABLE
> > should replace *all* the fields of the constraint with the same
> > properties it'd have if you'd created it fresh using the same syntax.
> > This is by analogy to CREATE OR REPLACE commands, which don't
> > preserve any of the old properties of the replaced object. Given
> > the interactions between these fields, I think you're going to end up
> > with a surprising mess of ad-hoc choices if you do differently.
> > Indeed, you already have, but I think it'll get worse if anyone
> > tries to extend the feature set further.
>
> I don't think the analogy to CREATE OR REPLACE holds. Semantically
> REPLACE and ALTER are very different. Using ALTER the expectation is to
> change something, keeping everything else unchanged. Looking at all the
> other ALTER TABLE actions, especially ALTER COLUMN, it looks like every
> command does exactly one thing and not more. I don't think deferrability
> and ON UPDATE / ON CASCADE should be changed together at all, neither
> implicitly nor explicitly.
>
> There seems to be a fundamental difference between deferrability and the
> ON UPDATE/ON DELETE clauses as well - the latter only apply to FOREIGN
> KEYs, while the former apply to multiple types of constraints.
>
> Matheus de Oliveira:
> > I'm still not sure if the chosen path is the best way. But I'd be glad
> > to follow any directions we all see fit.
> >
> > For now, this patch applies two methods:
> > 1. Changes full constraint definition (which keeps compatibility with
> > current ALTER CONSTRAINT):
> > ALTER CONSTRAINT [] [] []
> > 2. Changes only the subset explicit seem in the command (a new way, I've
> > chosen to just add SET in the middle, similar to `ALTER COLUMN ... SET
> > {DEFAULT | NOT NULL}` ):
> > ALTER CONSTRAINT SET [] [] []
> >
> > I'm OK with changing the approach, we just need to chose the color :D
>
> The `ALTER CONSTRAINT SET [] [] []`
> has the same problem about implied changes: What happens if you only do
> e.g. ALTER CONSTRAINT SET ON UPDATE xy - will the ON DELETE part be kept
> as-is or set to the default?
>
> Also, since the ON UPDATE/ON DELETE just applies to FOREIGN KEYs and no
> other constraints, there's one level of "nesting" missing in your SET
> variant, I think.
>
> I suggest to:
>
> - keep `ALTER CONSTRAINT constraint_name [ DEFERRABLE | NOT DEFERRABLE ]
> [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]` exactly as-is
>
> - add both:
> + `ALTER CONSTRAINT constraint_name [ALTER] FOREIGN KEY ON UPDATE
> referential_action`
> + `ALTER CONSTRAINT constraint_name [ALTER] FOREIGN KEY ON DELETE
> referential_action`
>
> This does not imply any changes, that are not in the command - very much
> analog to the ALTER COLUMN variants.
>
> This could also be extended in the future with stuff like `ALTER
> CONSTRAINT constraint_name [ALTER] FOREIGN KEY MATCH [ FULL | PARTIAL |
> SIMPLE ]`.
>
>
>
This patch set no longer applies
http://cfbot.cputube.org/patch_32_1533.log
Can we get a rebase?
I am marking the patch "Waiting on Author"
--
Ibrar Ahmed
Re: About to add WAL write/fsync statistics to pg_stat_wal view
gt;> pg_stat_wal_receiver is only related to the walreceiver process
> >>>> running
> >>>> at that moment. IOW, it seems strange that some values show dynamic
> >>>> stats and the others show collected stats, even though they are in
> >>>> the same view pg_stat_wal_receiver. Thought?
> >>>
> >>> OK, I fixed it.
> >>> The stats collected in the WAL receiver is exposed in pg_stat_wal
> >>> view in v11 patch.
> >>
> >> Thanks for updating the patches! I'm now reading 001 patch.
> >>
> >> +/* Check whether the WAL file was synced to disk right now */
> >> +if (enableFsync &&
> >> +(sync_method == SYNC_METHOD_FSYNC ||
> >> + sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH ||
> >> + sync_method == SYNC_METHOD_FDATASYNC))
> >> +{
> >>
> >> Isn't it better to make issue_xlog_fsync() return immediately
> >> if enableFsync is off, sync_method is open_sync or open_data_sync,
> >> to simplify the code more?
> >
> > Thanks for the comments.
> > I added the above code in v12 patch.
> >
> >>
> >> +/*
> >> + * Send WAL statistics only if WalWriterDelay has elapsed
> to
> >> minimize
> >> + * the overhead in WAL-writing.
> >> + */
> >> +if (rc & WL_TIMEOUT)
> >> +pgstat_send_wal();
> >>
> >> On second thought, this change means that it always takes
> >> wal_writer_delay
> >> before walwriter's WAL stats is sent after XLogBackgroundFlush() is
> >> called.
> >> For example, if wal_writer_delay is set to several seconds, some
> >> values in
> >> pg_stat_wal would be not up-to-date meaninglessly for those seconds.
> >> So I'm thinking to withdraw my previous comment and it's ok to send
> >> the stats every after XLogBackgroundFlush() is called. Thought?
> >
> > Thanks, I didn't notice that.
> >
> > Although PGSTAT_STAT_INTERVAL is 500msec, wal_writer_delay's
> > default value is 200msec and it may be set shorter time.
> >
> > Why don't to make another way to check the timestamp?
> >
> > + /*
> > +* Don't send a message unless it's been at least
> > PGSTAT_STAT_INTERVAL
> > +* msec since we last sent one
> > +*/
> > + now = GetCurrentTimestamp();
> > + if (TimestampDifferenceExceeds(last_report, now,
> > PGSTAT_STAT_INTERVAL))
> > + {
> > + pgstat_send_wal();
> > + last_report = now;
> > + }
> > +
> >
> > Although I worried that it's better to add the check code in
> > pgstat_send_wal(),
> > I didn't do so because to avoid to double check PGSTAT_STAT_INTERVAL.
> > pgstat_send_wal() is invoked pg_report_stat() and it already checks the
> > PGSTAT_STAT_INTERVAL.
>
> I forgot to remove an unused variable.
> The attached v13 patch is fixed.
>
> Regards
> --
> Masahiro Ikeda
> NTT DATA CORPORATION
This patch set no longer applies
http://cfbot.cputube.org/patch_32_2859.log
Can we get a rebase?
I am marking the patch "Waiting on Author"
--
Ibrar Ahmed
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
much. It's fine to send more than 64k
> with pqSendSome(), they'll just be send with separate pgsecure_write()
> invocations. And only on windows.
>
> It clearly makes sense to start sending out data at a certain
> granularity to avoid needing unnecessary amounts of memory, and to make
> more efficient use of latency / serer side compute.
>
> It's not implausible that 64k is the right amount for that, I just don't
> think the explanation above is good.
>
> > diff --git a/src/test/modules/test_libpq/testlibpqbatch.c
> b/src/test/modules/test_libpq/testlibpqbatch.c
> > new file mode 100644
> > index 00..4d6ba266e5
> > --- /dev/null
> > +++ b/src/test/modules/test_libpq/testlibpqbatch.c
> > @@ -0,0 +1,1456 @@
> > +/*
> > + * src/test/modules/test_libpq/testlibpqbatch.c
> > + *
> > + *
> > + * testlibpqbatch.c
> > + * Test of batch execution functionality
> > + */
> > +
> > +#ifdef WIN32
> > +#include
> > +#endif
>
> ISTM that this shouldn't be needed in a test program like this?
> Shouldn't libpq abstract all of this away?
>
>
> > +static void
> > +simple_batch(PGconn *conn)
> > +{
>
> ISTM that all or at least several of these should include tests of
> transactional behaviour with pipelining (e.g. using both implicit and
> explicit transactions inside a single batch, using transactions across
> batches, multiple explicit transactions inside a batch).
>
>
>
> > + PGresult *res = NULL;
> > + const char *dummy_params[1] = {"1"};
> > + Oid dummy_param_oids[1] = {INT4OID};
> > +
> > + fprintf(stderr, "simple batch... ");
> > + fflush(stderr);
>
> Why do we need fflush()? IMO that shouldn't be needed in a use like
> this? Especially not on stderr, which ought to be unbuffered?
>
>
> > + /*
> > + * Enter batch mode and dispatch a set of operations, which we'll
> then
> > + * process the results of as they come in.
> > + *
> > + * For a simple case we should be able to do this without interim
> > + * processing of results since our out buffer will give us enough
> slush to
> > + * work with and we won't block on sending. So blocking mode is
> fine.
> > + */
> > + if (PQisnonblocking(conn))
> > + {
> > + fprintf(stderr, "Expected blocking connection mode\n");
> > + goto fail;
> > + }
>
> Perhaps worth adding a helper for this?
>
> #define EXPECT(condition, explanation) \
> ...
> or such?
>
> Greetings,
>
> Andres Freund
>
>
>
The build is failing for this patch, can you please take a look at this?
https://cirrus-ci.com/task/4568547922804736
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.129221
I am marking the patch "Waiting on Author"
--
Ibrar Ahmed
Re: Hybrid Hash/Nested Loop joins and caching results from subplans
On Tue, Feb 23, 2021 at 10:44 AM Tom Lane wrote: > Andres Freund writes: > > We could add a wrapper node around "planner expressions" that stores > > metadata about them during planning, without those properties leaking > > over expressions used at other times. E.g. having > > preprocess_expression() return a PlannerExpr that that points to the > > expression as preprocess_expression returns it today. That'd make it > > easy to cache information like volatility. But it also seems > > prohibitively invasive :(. > > I doubt it's that bad. We could cache such info in RestrictInfo > for quals, or PathTarget for tlists, without much new notational > overhead. That doesn't cover everything the planner deals with > of course, but it would cover enough that you'd be chasing pretty > small returns to worry about more. > > regards, tom lane > > > This patch set no longer applies http://cfbot.cputube.org/patch_32_2569.log Can we get a rebase? I am marking the patch "Waiting on Author" -- Ibrar Ahmed
Re: Extending range type operators to cope with elements
On Sun, Feb 28, 2021 at 1:36 AM Justin Pryzby wrote: > On Fri, Oct 30, 2020 at 11:08:19PM +0100, Tomas Vondra wrote: > > Hi, > > > > + > > + > > +anyelement > anyrange > > +boolean > > + > > + > > +Is the element strictly right of the element? > > + > > should say "of the range" ? > > > +++ b/src/backend/utils/adt/rangetypes.c > > > + /* An empty range is neither left nor right any other range */ > > + /* An empty range is neither left nor right any element */ > > + /* An empty range is neither left nor right any other range */ > > + /* An empty range is neither left nor right any element */ > > + /* An empty range is neither left nor right any element */ > > + /* An empty range is neither left nor right any element */ > > I these comments should all say ".. left nor right OF any ..." > > -- > Justin > > > This patch set no longer applies. http://cfbot.cputube.org/patch_32_2747.log Can we get a rebase? I am marking the patch "Waiting on Author" -- Ibrar Ahmed
Re: [POC] Fast COPY FROM command for the table with foreign partitions
On Thu, Mar 4, 2021 at 12:40 PM tsunakawa.ta...@fujitsu.com < tsunakawa.ta...@fujitsu.com> wrote: > From: Zhihong Yu > > This feature enables bulk COPY into foreign table in the case of > > multi inserts is possible > > > > 'is possible' -> 'if possible' > > > > FDWAPI was extended by next routines: > > > > next routines -> the following routines > > Thank you, fixed slightly differently. (I feel the need for pgEnglish > again.) > > > > + if ((!OK && PQresultStatus(res) != PGRES_FATAL_ERROR) || > > > > Is PGRES_FATAL_ERROR handled somewhere else ? I don't seem to find that > in the patch. > > Good catch. ok doesn't need to be consulted here, because failure during > row transmission causes PQputCopyEnd() to receive non-NULL for its second > argument, which in turn makes PQgetResult() return non-COMMAND_OK. > > > Regards > Takayuki Tsunakawa > > This patch set no longer applies http://cfbot.cputube.org/patch_32_2601.log Can we get a rebase? I am marking the patch "Waiting on Author" -- Ibrar Ahmed
Re: Allow matching whole DN from a client certificate
On Wed, Mar 3, 2021 at 3:03 AM Jacob Champion wrote: > On Fri, 2021-02-26 at 15:40 -0500, Andrew Dunstan wrote: > > I think the thing that's principally outstanding w.r.t. this patch is > > what format we should use to extract the DN. > > That and the warning label for sharp edges. > > > Should we use RFC2253, > > which reverses the field order, as has been suggested upthread and is in > > the latest patch? I'm slightly worried that it might be a POLA > > violation. > > All I can provide is the hindsight from httpd. [1] is the thread that > gave rise to its LegacyDNStringFormat. > > Since RFC 2253 isn't a canonical encoding scheme, and we've already > established that different TLS implementations do things slightly > differently even when providing RFC-compliant output, maybe it doesn't > matter in the end: to get true compatibility, we need to implement a DN > matching scheme rather than checking string equality. But using RFC2253 > for version 1 of the feature at least means that the *simplest* cases > are the same across backends, since I doubt the NSS implementation is > going to try to recreate OpenSSL's custom format. > > --Jacob > > [1] > https://lists.apache.org/thread.html/2055b56985c69e7a6977151bf9817a0f982a4ad3b78a6a1984977fd0%401289507617%40%3Cusers.httpd.apache.org%3E > This patch set no longer applies http://cfbot.cputube.org/patch_32_2835.log Can we get a rebase? I marked the patch "Waiting on Author". -- Ibrar Ahmed
Re: Next Commitfest Manager.
On Thu, Feb 4, 2021 at 2:00 AM David Steele wrote: > On 2/3/21 3:13 PM, Stephen Frost wrote: > > Greetings, > > > > * Ibrar Ahmed (ibrar.ah...@gmail.com) wrote: > >> Anyone else already volunteers that? It is my first time so need some > >> access, if all agree. > > > > Thanks for volunteering! > > > > That said, our last commitfest tends to be the most difficult as it's > > the last opportunity for features to land in time for the next major > > release and, for my part at least, I think it'd be best to have > > someone who has experience running a CF previously manage it. > > > > To that end, I've talked to David Steele, who has run this last CF for > > the past few years and we're in agreement that he's willing to run this > > CF again this year, assuming there's no objections. What we've thought > > to suggest is that you follow along with David as he runs this CF and > > then offer to run the July CF. Of course, we would encourage you and > > David to communicate and for you to ask David any questions you have > > about how he handles things as part of the CF. This is in line with how > > other CF managers have started out also. > > > > Open to your thoughts, as well as those of anyone else who wishes to > > comment. > > +1. This all sounds good to me! > > -- > -David > da...@pgmasters.net Sounds good, I am happy to work with David. -- Ibrar Ahmed
