Re: Support logical replication of DDLs
On Wed, 7 Dec 2022 at 17:50, Alvaro Herrera wrote: > > I think this patch is split badly. > > You have: > > 0001 an enormous patch including some required infrastructure, plus the > DDL deparsing bits themselves. > > 0002 another enormous (though not as much) patch, this time for > DDL replication using the above. > > 0003 a bugfix for 0001, which includes changes in both the > infrastructure and the deparsing bits. > > 0004 test stuff for 0002. > > 0005 Another bugfix for 0001 > > 0006 Another bugfix for 0001 > > As presented, I think it has very little chance of being reviewed > usefully. A better way to go about this, I think, would be: > > 0001 - infrastructure bits to support the DDL deparsing parts (all these > new functions in ruleutils.c, sequence.c, etc). That means, everything > (?) that's currently in your 0001 except ddl_deparse.c and friends. > Clearly there are several independent changes here; maybe it is possible > to break it down even further. This patch or these patches should also > include the parts of 0003, 0005, 0006 that require changes outside of > ddl_deparse.c. > I expect that this patch should be fairly small. Modified > 0002 - ddl_deparse.c and its very close friends. This should not have > any impact on places such as ruleutils.c, sequence.c, etc. The parts of > the bugfixes (0001, 0005, 0006) that touch this could should be merged > here as well; there's no reason to have them as separate patches. Some > test code should be here also, though it probably doesn't need to aim to > be complete. > This one is likely to be very large, but also self-contained. Modified, I have currently kept the testing of deparse as a separate patch, we are planning to add more tests to it. We can later merge it to 0002 if required or keep it as 0003 if it is too large. > 0003 - ddlmessage.c and friends. I understand that DDL-messaging is > supporting infrastructure for DDL replication; I think it should be its > own patch. Probably include its own simple-ish test bits. > Not a very large patch. Modified > 0004 - DDL replication proper, including 0004. > Probably not a very large patch either, not sure. Modified > Some review comments, just skimming: > - 0002 adds some functions to event_trigger.c, but that doesn't seem to > be their place. Maybe some new file in src/backend/replication/logical > would make more sense. Modified > - publication_deparse_ddl_command_end has a long strcmp() list; why? > Maybe change things so that it compares some object type enum instead. Modified wherever possible > - CreatePublication has a long list of command tags; is that good? > Maybe it'd be better to annotate the list in cmdtaglist.h somehow. > - The change in pg_dump's getPublications needs updated to 16. Modified > - Don't "git add" src/bin/pg_waldump/logicalddlmsgdesc.c, just update > its Makefile and meson.build Modified > > - I think psql's \dRp should not have the new column at the end. > Maybe one of: > + Name | Owner | DDL | All tables | Inserts | Updates | Deletes | Truncates | > Via root > + Name | Owner | All tables | DDL | Inserts | Updates | Deletes | Truncates | > Via root > + Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | DDL | > Via root > (I would not add the "s" at the end of that column title, also). Modified it to: Name | Owner | All tables | DDL | Inserts | Updates | Deletes | Truncates | Via root These issues were addressed as part of v55 at [1], v56 at [2] and v58 at [3] posted. [1] - https://www.postgresql.org/message-id/CALDaNm2V6YL6H4P9ZT95Ua_RDJaeDTUf6V0UDfrz4_vxhM5pMg%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CALDaNm0uXh%3DRgGD%3DoB1p83GONb5%3DL2n3nbpiLGVaMd57TimdZA%40mail.gmail.com [3] - https://www.postgresql.org/message-id/CAAD30ULrZB-RNmuD3NMr1jGNUt15ZpPgFdFRX53HbcAB76hefw%40mail.gmail.com Regards, Vignesh
Re: [DOCS] Stats views and functions not in order?
On 08.12.22 03:30, Peter Smith wrote: PSA patches for v9* v9-0001 - Now the table rows are ordered per PeterE's suggestions [1] committed v9-0002 - All the review comments from DavidJ [2] are addressed I'm not sure about this one. It removes the "see [link] for details" phrases and instead makes the view name a link. I think this loses the cue that there is more information elsewhere. Otherwise, one could think that, say, the entry about pg_stat_activity is the primary source and the link just links to itself. Also keep in mind that people use media where links are not that apparent (PDF), so the presence of a link by itself cannot be the only cue about the flow of the information.
Re: Allow placeholders in ALTER ROLE w/o superuser
Hi, Justin! On Wed, 28 Dec 2022 at 21:28, Justin Pryzby wrote: > > On Tue, Dec 27, 2022 at 11:29:40PM -0500, Tom Lane wrote: > > Justin Pryzby writes: > > > This fails when run more than once: > > > time meson test --setup running --print > > > test_pg_db_role_setting-running/regress > > > > Ah. > > > > > It didn't fail for you because it says: > > > ./src/test/modules/test_pg_db_role_setting/Makefile > > > +# disable installcheck for now > > > +NO_INSTALLCHECK = 1 > > > > So ... exactly why is the meson infrastructure failing to honor that? > > This test looks sufficiently careless about its side-effects that > > I completely agree with the decision to not run it in pre-existing > > installations. Failing to drop a created superuser is just one of > > its risk factors --- it also leaves around pg_db_role_setting entries. > > Meson doesn't try to parse the Makefiles (like the MSVC scripts) but > (since 3f0e786ccb) has its own implementation, which involves setting > 'runningcheck': false. > > 096dd80f3c seems to have copied the NO_INSTALLCHECK from oat's makefile, > but didn't copy "runningcheck" from oat's meson.build (but did copy its > regress_args). I completely agree with your analysis. Fixes by 3f0e786ccbf5 to oat and the other modules tests came just a couple of days before committing the main pg_db_role_setting commit 096dd80f3c so they were forgotten to be included into it. I support committing the same fix to pg_db_role_setting test and added this minor fix as a patch to this thread. Kind regards, and happy New year! Pavel Borisov, Supabase. 0001-meson-Add-running-test-setup-as-a-replacement-for-in.patch Description: Binary data
Re: heapgettup refactoring
On 30.11.22 23:34, Melanie Plageman wrote: I have attached a patchset with only the code changes contained in the previous patch 0003. I have broken the refactoring down into many smaller pieces for ease of review. To keep this moving along a bit, I have committed your 0002, which I think is a nice little improvement on its own.
Sampling-based timing for EXPLAIN ANALYZE
Hi, Since EXPLAIN ANALYZE with TIMING ON still carries noticeable overhead on modern hardware (despite time sources being faster), I'd like to propose a new setting EXPLAIN ANALYZE, called "TIMING SAMPLING", as compared to TIMING ON. This new timing mode uses a timer on a fixed recurring frequency (e.g. 100 or 1000 Hz) to gather a sampled timestamp on a predefined schedule, instead of getting the time on-demand when InstrStartNode/InstrStopNode is called. To implement the timer, we can use the existing timeout infrastructure, which is backed by a wall clock timer (ITIMER_REAL). Conceptually this is inspired by how sampling profilers work (e.g. "perf"), but it ties into the existing per-plan node instrumentation done by EXPLAIN ANALYZE, and simply provides a lower accuracy version of the total time for each plan node. In EXPLAIN output this is marked as "sampled time", and scaled to the total wall clock time (to adjust for the sampling undercounting): =# EXPLAIN (ANALYZE, BUFFERS, TIMING SAMPLING, SAMPLEFREQ 100) SELECT ...; QUERY PLAN - HashAggregate (cost=201747.90..201748.00 rows=10 width=12) (actual sampled time=5490.974 rows=9 loops=1) ... -> Hash Join (cost=0.23..199247.90 rows=49 width=4) (actual sampled time=3738.619 rows=900 loops=1) ... -> Seq Scan on large (cost=0.00..144247.79 rows=979 width=4) (actual sampled time=1004.671 rows=1001 loops=1) ... -> Hash (cost=0.10..0.10 rows=10 width=4) (actual sampled time=0.000 rows=10 loops=1) ... Execution Time: 5491.475 ms --- In simple query tests like this on my local machine, this shows a consistent benefit over TIMING ON (and behaves close to ANALYZE with TIMING OFF), whilst providing a "good enough" accuracy to identify which part of the query was problematic. Attached is a prototype patch for early feedback on the concept, with tests and documentation to come in a follow-up. Since the January commitfest is still marked as open I'll register it there, but note that my assumption is this is *not* Postgres 16 material. As an open item, note that in the patch the requested sampling frequency is not yet passed to parallel workers (it always defaults to 1000 Hz when sampling is enabled). Also, note the timing frequency is limited to a maximum of 1000 Hz (1ms) due to current limitations of the timeout infrastructure. With thanks to Andres Freund for help on refining the idea, collaborating on early code and finding the approach to hook into the timeout API. Thanks, Lukas -- Lukas Fittl v1-0001-Add-TIMING-SAMPLING-option-for-EXPLAIN-ANALYZE.patch Description: Binary data
Re: Add BufFileRead variants with short read and EOF detection
On Wed, Dec 28, 2022 at 4:17 PM Peter Eisentraut wrote: > > Most callers of BufFileRead() want to check whether they read the full > specified length. Checking this at every call site is very tedious. > This patch provides additional variants BufFileReadExact() and > BufFileReadMaybeEOF() that include the length checks. > > I considered changing BufFileRead() itself, but this function is also > used in extensions, and so changing the behavior like this would create > a lot of problems there. The new names are analogous to the existing > LogicalTapeReadExact(). > +1 for the new APIs. I have noticed that some of the existing places use %m and the file path in messages which are not used in the new common function. -- With Regards, Amit Kapila.
Bug in check for unreachable MERGE WHEN clauses
Re-reading my latest MERGE patch, I realised there is a trivial, pre-existing bug in the check for unreachable WHEN clauses, which means it won't spot an unreachable WHEN clause if it doesn't have an AND condition. So the checks need to be re-ordered, as in the attached. Regards, Dean diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c new file mode 100644 index 3844f2b..47d266b --- a/src/backend/parser/parse_merge.c +++ b/src/backend/parser/parse_merge.c @@ -155,12 +155,12 @@ transformMergeStmt(ParseState *pstate, M /* * Check for unreachable WHEN clauses */ - if (mergeWhenClause->condition == NULL) - is_terminal[when_type] = true; - else if (is_terminal[when_type]) + if (is_terminal[when_type]) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("unreachable WHEN clause specified after unconditional WHEN clause"))); + if (mergeWhenClause->condition == NULL) + is_terminal[when_type] = true; } /* diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out new file mode 100644 index 6c8a18f..bc53b21 --- a/src/test/regress/expected/merge.out +++ b/src/test/regress/expected/merge.out @@ -659,7 +659,7 @@ USING source AS s ON t.tid = s.sid WHEN MATCHED THEN /* Terminal WHEN clause for MATCHED */ DELETE -WHEN MATCHED AND s.delta > 0 THEN +WHEN MATCHED THEN UPDATE SET balance = t.balance - s.delta; ERROR: unreachable WHEN clause specified after unconditional WHEN clause ROLLBACK; diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql new file mode 100644 index 98fe104..fdbcd70 --- a/src/test/regress/sql/merge.sql +++ b/src/test/regress/sql/merge.sql @@ -438,7 +438,7 @@ USING source AS s ON t.tid = s.sid WHEN MATCHED THEN /* Terminal WHEN clause for MATCHED */ DELETE -WHEN MATCHED AND s.delta > 0 THEN +WHEN MATCHED THEN UPDATE SET balance = t.balance - s.delta; ROLLBACK;
Re: Data loss on logical replication, 12.12 to 14.5, ALTER SUBSCRIPTION
On Wed, Dec 28, 2022 at 4:52 PM Michail Nikolaev wrote: > > Hello. > > > None of these entries are from the point mentioned by you [1] > > yesterday where you didn't find the corresponding data in the > > subscriber. How did you identify that the entries corresponding to > > that timing were missing? > > Some of the before the interval, some after... But the source database > was generating a lot of WAL during logical replication > - some of these log entries from time AFTER completion of initial sync > of B but (probably) BEFORE finishing B table catch up (entering > streaming mode). > ... ... > > So, shortly the story looks like: > > * initial sync of A (and other tables) started and completed, they are > in streaming mode > * B and C initial sync started (by altering PUBLICATION and SUBSCRIPTION) > * B sync completed, but new changes are still applying to the tables > to catch up primary > The point which is not completely clear from your description is the timing of missing records. In one of your previous emails, you seem to have indicated that the data missed from Table B is from the time when the initial sync for Table B was in-progress, right? Also, from your description, it seems there is no error or restart that happened during the time of initial sync for Table B. Is that understanding correct? > * logical replication apply worker is restarted because IO error on WAL > receive > * Postgres killed > * Postgres restarted > * C initial sync restarted > * logical replication apply worker few times restarted because IO > error on WAL receive > * finally every table in streaming mode but with small gap in B > I am not able to see how these steps can lead to the problem. If the problem is reproducible at your end, you might want to increase LOG verbosity to DEBUG1 and see if there is additional information in the LOGs that can help or it would be really good if there is a self-sufficient test to reproduce it. -- With Regards, Amit Kapila.
Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
Hi, I re-based again on master and applied the following changes: I removed the fallback for obtaining the TSC frequency from /proc/cpu as suggested by Andres. Worst-case we fall back to clock_gettime(). I added code to obtain the TSC frequency via CPUID when under a hypervisor. I had to use __cpuid() directly instead of __get_cpuid(), because __get_cpuid() returns an error if the leaf is > 0x8000 (probably the implementation pre-dates the hypervisor timing leafs). Unfortunately, while testing my implementation under VMWare, I found that RDTSC runs awfully slow there (like 30x slower). [1] indicates that we cannot generally rely on RDTSC being actually fast on VMs. However, the same applies to clock_gettime(). It runs as slow as RDTSC on my VMWare setup. Hence, using RDTSC is not at disadvantage. I'm not entirely sure if there aren't cases where e.g. clock_gettime() is actually faster than RDTSC and it would be advantageous to use clock_gettime(). We could add a GUC so that the user can decide which clock source to use. Any thoughts? I also somewhat improved the accuracy of the cycles to milli- and microseconds conversion functions by having two more multipliers with higher precision. For microseconds we could also keep the computation integer-only. I'm wondering what to best do for seconds and milliseconds. I'm currently leaning towards just keeping it as is, because the durations measured and converted are usually long enough that precision shouldn't be a problem. In vacuum_lazy.c we do if ((INSTR_TIME_GET_MICROSEC(elapsed) / 1000). I changed that to use INSTR_TIME_GET_MILLISECS() instead. Additionally, I initialized a few variables of type instr_time which otherwise resulted in warnings due to use of potentially uninitialized variables. I also couldn't reproduce the reported timing discrepancy. For me the runtime reported by \timing is just slightly higher than the time reported by EXPLAIN ANALYZE, which is expected. Beyond that: What about renaming INSTR_TIME_GET_DOUBLE() to INSTR_TIME_GET_SECS() so that it's consistent with the _MILLISEC() and _MICROSEC() variants? The INSTR_TIME_GET_MICROSEC() returns a uint64 while the other variants return double. This seems error prone. What about renaming the function or also have the function return a double and cast where necessary at the call site? If no one objects I would also re-register this patch in the commit fest. [1] https://vmware.com/content/dam/digitalmarketing/vmware/en/pdf/techpaper/Timekeeping-In-VirtualMachines.pdf (page 11 "Virtual TSC") -- David Geier (ServiceNow) From 321d00ae5dd1bcffc8fbdb39879b7f5c78e3930f Mon Sep 17 00:00:00 2001 From: David Geier Date: Thu, 17 Nov 2022 10:22:01 +0100 Subject: [PATCH 1/3] Change instr_time to just store nanoseconds, that's cheaper. --- src/include/portability/instr_time.h | 62 1 file changed, 26 insertions(+), 36 deletions(-) diff --git a/src/include/portability/instr_time.h b/src/include/portability/instr_time.h index 22bcf3d288..4bd555113b 100644 --- a/src/include/portability/instr_time.h +++ b/src/include/portability/instr_time.h @@ -80,63 +80,53 @@ #define PG_INSTR_CLOCK CLOCK_REALTIME #endif -typedef struct timespec instr_time; +typedef int64 instr_time; +#define NS_PER_S INT64CONST(10) +#define US_PER_S INT64CONST(100) +#define MS_PER_S INT64CONST(1000) -#define INSTR_TIME_IS_ZERO(t) ((t).tv_nsec == 0 && (t).tv_sec == 0) +#define NS_PER_MS INT64CONST(100) +#define NS_PER_US INT64CONST(1000) -#define INSTR_TIME_SET_ZERO(t) ((t).tv_sec = 0, (t).tv_nsec = 0) +#define INSTR_TIME_IS_ZERO(t) ((t) == 0) -#define INSTR_TIME_SET_CURRENT(t) ((void) clock_gettime(PG_INSTR_CLOCK, &(t))) +#define INSTR_TIME_SET_ZERO(t) ((t) = 0) + +static inline instr_time pg_clock_gettime_ns(void) +{ + struct timespec tmp; + + clock_gettime(PG_INSTR_CLOCK, &tmp); + + return tmp.tv_sec * NS_PER_S + tmp.tv_nsec; +} + +#define INSTR_TIME_SET_CURRENT(t) \ + (t) = pg_clock_gettime_ns() #define INSTR_TIME_ADD(x,y) \ do { \ - (x).tv_sec += (y).tv_sec; \ - (x).tv_nsec += (y).tv_nsec; \ - /* Normalize */ \ - while ((x).tv_nsec >= 10) \ - { \ - (x).tv_nsec -= 10; \ - (x).tv_sec++; \ - } \ + (x) += (y); \ } while (0) #define INSTR_TIME_SUBTRACT(x,y) \ do { \ - (x).tv_sec -= (y).tv_sec; \ - (x).tv_nsec -= (y).tv_nsec; \ - /* Normalize */ \ - while ((x).tv_nsec < 0) \ - { \ - (x).tv_nsec += 10; \ - (x).tv_sec--; \ - } \ + (x) -= (y); \ } while (0) #define INSTR_TIME_ACCUM_DIFF(x,y,z) \ do { \ - (x).tv_sec += (y).tv_sec - (z).tv_sec; \ - (x).tv_nsec += (y).tv_nsec - (z).tv_nsec; \ - /* Normalize after each add to avoid overflow/underflow of tv_nsec */ \ - while ((x).tv_nsec < 0) \ - { \ - (x).tv_nsec += 10; \ - (x).tv_sec--; \ - } \ - while ((x).tv_nsec >= 10) \ - { \ - (x).tv_nsec -= 10; \ - (x).tv_sec++; \ - } \ + (x) +=
Re: Add a test to ldapbindpasswd
On 2023-01-01 Su 18:31, Andrew Dunstan wrote: > On 2023-01-01 Su 14:02, Thomas Munro wrote: >> On Mon, Jan 2, 2023 at 3:04 AM Andrew Dunstan wrote: >>> On 2022-12-19 Mo 11:16, Andrew Dunstan wrote: There is currently no test for the use of ldapbindpasswd in the pg_hba.conf file. This patch, mostly the work of John Naylor, remedies that. >>> This currently has failures on the cfbot for meson builds on FBSD13 and >>> Debian Bullseye, but it's not at all clear why. In both cases it fails >>> where the ldap server is started. >> I think it's failing when using meson. I guess it fails to fail on >> macOS only because you need to add a new path for Homebrew/ARM like >> commit 14d63dd2, so it's skipping (it'd be nice if we didn't need >> another copy of all that logic). Trying locally... it looks like >> slapd is failing silently, and with some tracing I can see it's >> sending an error message to my syslog daemon, which logged: >> >> 2023-01-02T07:50:20.853019+13:00 x1 slapd[153599]: main: TLS init def >> ctx failed: -1 >> >> Ah, it looks like this test is relying on "slapd-certs", which doesn't exist: >> >> tmunro@x1:~/projects/postgresql/build$ ls testrun/ldap/001_auth/data/ >> ldap.conf ldappassword openldap-data portlock slapd-certs slapd.conf >> tmunro@x1:~/projects/postgresql/build$ ls testrun/ldap/002_bindpasswd/data/ >> portlock slapd.conf >> >> I didn't look closely, but apparently there is something wrong in the >> part that copies certs from the ssl test? Not sure why it works for >> autoconf... > > > Let's see how we fare with this patch. > > Not so well :-(. This version tries to make the tests totally independent, as they should be. That's an attempt to get the cfbot to go green, but I am intending to refactor this code substantially so the common bits are in a module each test file will load. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com From c2bedfd8a5b326ffb563da49b7b4b4006ddac361 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Mon, 2 Jan 2023 09:41:06 -0500 Subject: [PATCH] Add a test for ldapbindpasswd The existing LDAP tests don't cover the use of ldapbindpasswd in pg_hba.conf, so remedy that. Authors: John Naylor and Andrew Dunstan --- src/test/ldap/meson.build | 1 + src/test/ldap/t/002_bindpasswd.pl | 223 ++ 2 files changed, 224 insertions(+) create mode 100644 src/test/ldap/t/002_bindpasswd.pl diff --git a/src/test/ldap/meson.build b/src/test/ldap/meson.build index 90d88138e7..7628a9c7c6 100644 --- a/src/test/ldap/meson.build +++ b/src/test/ldap/meson.build @@ -7,6 +7,7 @@ tests += { 'tap': { 'tests': [ 't/001_auth.pl', + 't/002_bindpasswd.pl', ], 'env': { 'with_ldap': ldap.found() ? 'yes' : 'no', diff --git a/src/test/ldap/t/002_bindpasswd.pl b/src/test/ldap/t/002_bindpasswd.pl new file mode 100644 index 00..330a2b7dad --- /dev/null +++ b/src/test/ldap/t/002_bindpasswd.pl @@ -0,0 +1,223 @@ + +# Copyright (c) 2022, PostgreSQL Global Development Group + +use strict; +use warnings; +use File::Copy; +use File::Basename; +use PostgreSQL::Test::Utils; +use PostgreSQL::Test::Cluster; +use Test::More; + + +my ($slapd, $ldap_bin_dir, $ldap_schema_dir); + +$ldap_bin_dir = undef;# usually in PATH + +if ($ENV{with_ldap} ne 'yes') +{ + plan skip_all => 'LDAP not supported by this build'; +} +elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/) +{ + plan skip_all => 'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA'; +} +elsif ($^O eq 'darwin' && -d '/opt/homebrew/opt/openldap') +{ + # typical paths for Homebrew on ARM + $slapd = '/opt/homebrew/opt/openldap/libexec/slapd'; + $ldap_schema_dir = '/opt/homebrew/etc/openldap/schema'; +} +elsif ($^O eq 'darwin' && -d '/usr/local/opt/openldap') +{ + # typical paths for Homebrew on Intel + $slapd = '/usr/local/opt/openldap/libexec/slapd'; + $ldap_schema_dir = '/usr/local/etc/openldap/schema'; +} +elsif ($^O eq 'darwin' && -d '/opt/local/etc/openldap') +{ + # typical paths for MacPorts + $slapd = '/opt/local/libexec/slapd'; + $ldap_schema_dir = '/opt/local/etc/openldap/schema'; +} +elsif ($^O eq 'linux') +{ + $slapd = '/usr/sbin/slapd'; + $ldap_schema_dir = '/etc/ldap/schema' if -d '/etc/ldap/schema'; + $ldap_schema_dir = '/etc/openldap/schema' if -d '/etc/openldap/schema'; +} +elsif ($^O eq 'freebsd') +{ + $slapd = '/usr/local/libexec/slapd'; + $ldap_schema_dir = '/usr/local/etc/openldap/schema'; +} +elsif ($^O eq 'openbsd') +{ + $slapd = '/usr/local/libexec/slapd'; + $ldap_schema_dir = '/usr/local/share/examples/openldap/schema'; +} +else +{ + plan skip_all => + "ldap tests not supported on $^O or dependencies not installed"; +} + +# make your own edits here +#$slapd = ''; +#$ldap_bin_dir = ''; +#$ldap_schema_dir = ''; + +$ENV{PATH} = "$ldap_bin_dir:$ENV{PATH}" if $ldap_bin_dir; + +my $tst_name = basename(__FILE__,'.pl'); +my $test_temp = Po
Re: Allow placeholders in ALTER ROLE w/o superuser
Justin, Tom, Pavel, thank you for catching this. On Mon, Jan 2, 2023 at 11:54 AM Pavel Borisov wrote: > I completely agree with your analysis. Fixes by 3f0e786ccbf5 to oat > and the other modules tests came just a couple of days before > committing the main pg_db_role_setting commit 096dd80f3c so they were > forgotten to be included into it. > > I support committing the same fix to pg_db_role_setting test and added > this minor fix as a patch to this thread. I'm going to push this if no objections. > Kind regards, and happy New year! Thanks, and happy New Year to you! -- Regards, Alexander Korotkov
verbose mode for pg_input_error_message?
I've been wondering if it might be a good idea to have a third parameter for pg_input_error_message() which would default to false, but which if true would cause it to emit the detail and hint fields, if any, as well as the message field from the error_data. Thoughts? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Allow placeholders in ALTER ROLE w/o superuser
On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > I'm going to push this if no objections. I also suggest that meson.build should not copy regress_args. -- Justin
Re: verbose mode for pg_input_error_message?
Andrew Dunstan writes: > I've been wondering if it might be a good idea to have a third parameter > for pg_input_error_message() which would default to false, but which if > true would cause it to emit the detail and hint fields, if any, as well > as the message field from the error_data. I don't think that just concatenating those strings would make for a pleasant API. More sensible, perhaps, to have a separate function that returns a record. Or we could redefine the existing function that way, but I suspect that "just the primary error" will be a principal use-case. Being able to get the SQLSTATE is likely to be interesting too. regards, tom lane
Re: CAST(... ON DEFAULT) - WIP build on top of Error-Safe User Functions
Corey Huinker writes: > The proposed changes are as follows: > CAST(expr AS typename) > continues to behave as before. > CAST(expr AS typename ERROR ON ERROR) > has the identical behavior as the unadorned CAST() above. > CAST(expr AS typename NULL ON ERROR) > will use error-safe functions to do the cast of expr, and will return > NULL if the cast fails. > CAST(expr AS typename DEFAULT expr2 ON ERROR) > will use error-safe functions to do the cast of expr, and will return > expr2 if the cast fails. While I approve of trying to get some functionality in this area, I'm not sure that extending CAST is a great idea, because I'm afraid that the SQL committee will do something that conflicts with it. If we know that they are about to standardize exactly this syntax, where is that information available? If we don't know that, I'd prefer to invent some kind of function or other instead of extending the grammar. regards, tom lane
Re: [RFC] Add jit deform_counter
> On Sun, Dec 25, 2022 at 06:55:02PM +0100, Pavel Stehule wrote: > there are some problems with stability of regress tests > > http://cfbot.cputube.org/dmitry-dolgov.html Looks like this small change predates moving to meson, the attached version should help. >From 1b19af29b4a71e008d9d4ac7ca39d06dc89d6237 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sat, 11 Jun 2022 11:54:40 +0200 Subject: [PATCH v2 1/2] Add deform_counter At the moment generation_counter includes time spent on both JITing expressions and tuple deforming. Those are configured independently via corresponding options (jit_expressions and jit_tuple_deforming), which means there is no way to find out what fraction of time tuple deforming alone is taking. Add deform_counter dedicated to tuple deforming, which allows seeing more directly the influence jit_tuple_deforming is having on the query. --- src/backend/commands/explain.c | 7 ++- src/backend/jit/jit.c | 1 + src/backend/jit/llvm/llvmjit_expr.c | 6 ++ src/include/jit/jit.h | 3 +++ 4 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index f86983c660..2f23602a87 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -882,6 +882,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji) /* calculate total time */ INSTR_TIME_SET_ZERO(total_time); + /* don't add deform_counter, it's included into generation_counter */ INSTR_TIME_ADD(total_time, ji->generation_counter); INSTR_TIME_ADD(total_time, ji->inlining_counter); INSTR_TIME_ADD(total_time, ji->optimization_counter); @@ -909,8 +910,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji) { ExplainIndentText(es); appendStringInfo(es->str, -"Timing: %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n", +"Timing: %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n", "Generation", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->generation_counter), +"Deforming", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->deform_counter), "Inlining", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->inlining_counter), "Optimization", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->optimization_counter), "Emission", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->emission_counter), @@ -937,6 +939,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji) ExplainPropertyFloat("Generation", "ms", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->generation_counter), 3, es); + ExplainPropertyFloat("Deforming", "ms", +1000.0 * INSTR_TIME_GET_DOUBLE(ji->deform_counter), +3, es); ExplainPropertyFloat("Inlining", "ms", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->inlining_counter), 3, es); diff --git a/src/backend/jit/jit.c b/src/backend/jit/jit.c index 91a6b2b63a..a6bdf03718 100644 --- a/src/backend/jit/jit.c +++ b/src/backend/jit/jit.c @@ -185,6 +185,7 @@ InstrJitAgg(JitInstrumentation *dst, JitInstrumentation *add) { dst->created_functions += add->created_functions; INSTR_TIME_ADD(dst->generation_counter, add->generation_counter); + INSTR_TIME_ADD(dst->deform_counter, add->deform_counter); INSTR_TIME_ADD(dst->inlining_counter, add->inlining_counter); INSTR_TIME_ADD(dst->optimization_counter, add->optimization_counter); INSTR_TIME_ADD(dst->emission_counter, add->emission_counter); diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c index f114337f8e..1ad384f15e 100644 --- a/src/backend/jit/llvm/llvmjit_expr.c +++ b/src/backend/jit/llvm/llvmjit_expr.c @@ -121,7 +121,9 @@ llvm_compile_expr(ExprState *state) LLVMValueRef v_aggnulls; instr_time starttime; + instr_time deform_starttime; instr_time endtime; + instr_time deform_endtime; llvm_enter_fatal_on_oom(); @@ -315,10 +317,14 @@ llvm_compile_expr(ExprState *state) */
Is OpenSSL AES-NI not available in pgcrypto?
Hi all, A question, may I wrong. I've a Rocky Linux 8 with OpenSSL 1.1.1 FIPS and Intel cpu with aes support (cat /proc/cpuinfo | grep aes) Test made with openssl gives me a huge performance with aes enabled vs not: "openssl speed -elapsed -evp aes-128-cbc" is about 5 time faster than "openssl speed -elapsed aes-128-cbc" or another "software calculated test", eg. "openssl speed -elapsed bf-cbc" So OpenSSL is ok. Postgresql 15 is compiled with openssl: select name, setting from pg_settings where name = 'ssl_library'; name | setting -+- ssl_library | OpenSSL (1 row) So, a test with pgcrypto: select pgp_sym_encrypt(data::text, 'pwd') --default to aes128 from generate_series('2022-01-01'::timestamp, '2022-12-31'::timestamp, '1 hour'::interval) data vs select pgp_sym_encrypt(data::text, 'pwd','cipher-algo=bf') -- blowfish from generate_series('2022-01-01'::timestamp, '2022-12-31'::timestamp, '1 hour'::interval) data In my test both queries execution is similaraes-128 was expected about 5 time faster. So, why? Pgcrypto use OpenSSL as backend, so, does it explicit force software aes calculation instead of AES-NI cpu ones? Thanksfor support. Best regards, Agharta
Re: Allow pageinspect's bt_page_stats function to return a set of rows instead of a single row
Hamid Akhtar writes: > I wasn't aware of the meson.build file. Attached is the latest version of > the patch that contains the updated meson.build. Pushed with minor corrections, plus one major one: you missed the point of aeaaf520f, that pageinspect functions that touch relations need to be parallel-restricted not parallel-safe. regards, tom lane
Re: Infinite Interval
On Sat, Dec 31, 2022 at 12:09 AM jian he wrote: > In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch > select interval 'infinity' / float8 'infinity'; returns infinity. > I am not sure it's right. I found this related post > (https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity). Good point, I agree this should return an error. We also need to properly handle multiplication and division of infinite intervals by float8 'nan'. My patch is returning an infinite interval, but it should be returning an error. I'll upload a new patch shortly. - Joe
Re: MERGE ... WHEN NOT MATCHED BY SOURCE
On Fri, 30 Dec 2022 at 16:56, Dean Rasheed wrote: > > Attached is a WIP patch. > Updated patch attached, now with updated docs and some other minor tidying up. Regards, Dean diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml new file mode 100644 index b87ad5c..1482ede --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -396,8 +396,8 @@ originally matched appears later in the list of actions. On the other hand, if the row is concurrently updated or deleted so that the join condition fails, then MERGE will -evaluate the condition's NOT MATCHED actions next, -and execute the first one that succeeds. +evaluate the condition's NOT MATCHED [BY TARGET] +actions next, and execute the first one that succeeds. If MERGE attempts an INSERT and a unique index is present and a duplicate row is concurrently inserted, then a uniqueness violation error is raised; diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml new file mode 100644 index 0995fe0..8ef121a --- a/doc/src/sgml/ref/merge.sgml +++ b/doc/src/sgml/ref/merge.sgml @@ -33,7 +33,8 @@ USING dat and when_clause is: { WHEN MATCHED [ AND condition ] THEN { merge_update | merge_delete | DO NOTHING } | - WHEN NOT MATCHED [ AND condition ] THEN { merge_insert | DO NOTHING } } + WHEN NOT MATCHED BY SOURCE [ AND condition ] THEN { merge_update | merge_delete | DO NOTHING } | + WHEN NOT MATCHED [BY TARGET] [ AND condition ] THEN { merge_insert | DO NOTHING } } and merge_insert is: @@ -70,7 +71,9 @@ DELETE from data_source to target_table_name producing zero or more candidate change rows. For each candidate change - row, the status of MATCHED or NOT MATCHED + row, the status of MATCHED, + NOT MATCHED BY SOURCE, + or NOT MATCHED [BY TARGET] is set just once, after which WHEN clauses are evaluated in the order specified. For each candidate change row, the first clause to evaluate as true is executed. No more than one WHEN @@ -226,16 +229,37 @@ DELETE At least one WHEN clause is required. + The WHEN clause may specify WHEN MATCHED, + WHEN NOT MATCHED BY SOURCE, or + WHEN NOT MATCHED [BY TARGET]. + Note that the SQL standard only defines + WHEN MATCHED and WHEN NOT MATCHED + (which is defined to mean no matching target row). + WHEN NOT MATCHED BY SOURCE is an extension to the + SQL standard, as is the option to append + BY TARGET to WHEN NOT MATCHED, to + make its meaning more explicit. + + If the WHEN clause specifies WHEN MATCHED - and the candidate change row matches a row in the + and the candidate change row matches a row in the source to a row in the target_table_name, the WHEN clause is executed if the condition is absent or it evaluates to true. - Conversely, if the WHEN clause specifies - WHEN NOT MATCHED + If the WHEN clause specifies + WHEN NOT MATCHED BY SOURCE and the candidate change + row represents a row in the + target_table_name that does + not match a source row, the WHEN clause is executed + if the condition is + absent or it evaluates to true. + + + If the WHEN clause specifies + WHEN NOT MATCHED [BY TARGET] and the candidate change row does not match a row in the target_table_name, the WHEN clause is executed if the @@ -257,7 +281,10 @@ DELETE A condition on a WHEN MATCHED clause can refer to columns in both the source and the target relations. A condition on a - WHEN NOT MATCHED clause can only refer to columns from + WHEN NOT MATCHED BY SOURCE clause can only refer to + columns from the target relation, since by definition there is no matching + source row. A condition on a WHEN NOT MATCHED [BY TARGET] + clause can only refer to columns from the source relation, since by definition there is no matching target row. Only the system attributes from the target table are accessible. @@ -382,8 +409,10 @@ DELETE WHEN MATCHED clause, the expression can use values from the original row in the target table, and values from the data_source row. - If used in a WHEN NOT MATCHED clause, the - expression can use values from the data_source. + If used in a WHEN NOT MATCHED BY SOURCE clause, the + expression can only use values from the original row in the target table. + If used in a WHEN NOT MATCHED [BY TARGET] clause, the + expression can only use values from the data_source. @@ -452,8 +481,9 @@ MERGE tot - Evaluate whether each row is MATCHED or - NOT MATCHED. + Evaluate whether each row is MATCHED, + NOT MATCHED BY SOURCE, or + NOT MATCHED [BY TARGET]. @@ -528,7 +558,8 @@ MERGE
Re: Fixing a couple of buglets in how VACUUM sets visibility map bits
On Sat, Dec 31, 2022 at 4:53 PM Peter Geoghegan wrote: > The first patch makes sure that the snapshotConflictHorizon cutoff > (XID cutoff for recovery conflicts) is never a special XID, unless > that XID is InvalidTransactionId, which is interpreted as a > snapshotConflictHorizon value that will never need a recovery conflict > (per the general convention for snapshotConflictHorizon values > explained above ResolveRecoveryConflictWithSnapshot). Pushed this just now. Attached is another very simple refactoring patch for vacuumlazy.c. It makes vacuumlazy.c save the result of get_database_name() in vacrel, which matches what we already do with things like get_namespace_name(). Would be helpful if I could get a +1 on v1-0002-Never-just-set-the-all-frozen-bit-in-VM.patch, which is somewhat more substantial than the others. -- Peter Geoghegan 0001-Save-get_database_name-in-vacrel.patch Description: Binary data
Re: Infinite Interval
On Mon, Jan 2, 2023 at 1:21 PM Joseph Koshakow wrote: > > On Sat, Dec 31, 2022 at 12:09 AM jian he wrote: > > In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch > > select interval 'infinity' / float8 'infinity'; returns infinity. > > I am not sure it's right. I found this related post > > (https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity). > > Good point, I agree this should return an error. We also need to > properly handle multiplication and division of infinite intervals by > float8 'nan'. My patch is returning an infinite interval, but it should > be returning an error. I'll upload a new patch shortly. > > - Joe Attached is the patch to handle these scenarios. Apparently dividing by NaN is currently broken: postgres=# SELECT INTERVAL '1 day' / float8 'nan'; ?column? --- -178956970 years -8 mons -2562047788:00:54.775808 (1 row) This patch will fix the issue, but we may want a separate patch that handles this specific, existing issue. Any thoughts? - Joe From 2110bbe8be4b1c5c66eb48c35b958d84352a6287 Mon Sep 17 00:00:00 2001 From: Joseph Koshakow Date: Sat, 17 Dec 2022 14:21:26 -0500 Subject: [PATCH] This is WIP. Following things are supported 1. Accepts '+/-infinity' as a valid string input for interval type. 2. Support interval_pl, interval_div 3. Tests in interval.sql for comparison operators working fine. TODOs 1. Various TODOs in code 2. interval_pl: how to handle infinite values with opposite signs 3. timestamp, timestamptz, date and time arithmetic 4. Fix horology test. Ashutosh Bapat --- src/backend/utils/adt/date.c | 20 + src/backend/utils/adt/datetime.c | 14 +- src/backend/utils/adt/timestamp.c | 347 - src/include/datatype/timestamp.h | 22 + src/test/regress/expected/horology.out | 6 +- src/test/regress/expected/interval.out | 993 - src/test/regress/sql/horology.sql | 6 +- src/test/regress/sql/interval.sql | 194 - 8 files changed, 1527 insertions(+), 75 deletions(-) diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c index 1cf7c7652d..c6259cd9c1 100644 --- a/src/backend/utils/adt/date.c +++ b/src/backend/utils/adt/date.c @@ -2073,6 +2073,11 @@ time_pl_interval(PG_FUNCTION_ARGS) Interval *span = PG_GETARG_INTERVAL_P(1); TimeADT result; + if (INTERVAL_NOT_FINITE(span)) + ereport(ERROR, +(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("cannot add infinite interval to time"))); + result = time + span->time; result -= result / USECS_PER_DAY * USECS_PER_DAY; if (result < INT64CONST(0)) @@ -2091,6 +2096,11 @@ time_mi_interval(PG_FUNCTION_ARGS) Interval *span = PG_GETARG_INTERVAL_P(1); TimeADT result; + if (INTERVAL_NOT_FINITE(span)) + ereport(ERROR, +(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("cannot subtract infinite interval from time"))); + result = time - span->time; result -= result / USECS_PER_DAY * USECS_PER_DAY; if (result < INT64CONST(0)) @@ -2605,6 +2615,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS) Interval *span = PG_GETARG_INTERVAL_P(1); TimeTzADT *result; + if (INTERVAL_NOT_FINITE(span)) + ereport(ERROR, +(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("cannot add infinite interval to time"))); + result = (TimeTzADT *) palloc(sizeof(TimeTzADT)); result->time = time->time + span->time; @@ -2627,6 +2642,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS) Interval *span = PG_GETARG_INTERVAL_P(1); TimeTzADT *result; + if (INTERVAL_NOT_FINITE(span)) + ereport(ERROR, +(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("cannot subtract infinite interval from time"))); + result = (TimeTzADT *) palloc(sizeof(TimeTzADT)); result->time = time->time - span->time; diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c index b5b117a8ca..b60d91dfb8 100644 --- a/src/backend/utils/adt/datetime.c +++ b/src/backend/utils/adt/datetime.c @@ -70,7 +70,7 @@ static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t, const char *abbr, pg_tz *tzp, int *offset, int *isdst); static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp, - DateTimeErrorExtra *extra); + DateTimeErrorExtra * extra); const int day_tab[2][13] = @@ -977,7 +977,7 @@ ParseDateTime(const char *timestr, char *workbuf, size_t buflen, int DecodeDateTime(char **field, int *ftype, int nf, int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp, - DateTimeErrorExtra *extra) + DateTimeErrorExtra * extra) { int fmask = 0, tmask, @@ -1927,7 +1927,7 @@ DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t, const char *abbr, pg_tz *tzp, int DecodeTimeOnly(char **field, int *ftype, int nf, int *dtype, struct pg_tm *tm, fsec_t *fsec, int
Why is char an internal-use category
I see in v15 there is a note that there is a new category for "char" however it is categorized as "internal use" I would think that char and char(n) would be used by external programs as a user type. Dave Cramer
Re: Why is char an internal-use category
Dave Cramer writes: > I see in v15 there is a note that there is a new category for "char" > however it is categorized as "internal use" > I would think that char and char(n) would be used by external programs as a > user type. "char" (with quotes) is not at all the same type as char without quotes. regards, tom lane
Re: New strategies for freezing, advancing relfrozenxid early
On Sat, Dec 31, 2022 at 12:45 PM Peter Geoghegan wrote: > On Sat, Dec 31, 2022 at 11:46 AM Jeff Davis wrote: > > "We have no freeze plans to execute, so there's no cost to following > > the freeze path. This is important in the case where the page is > > entirely frozen already, so that the page will be marked as such in the > > VM." > > I'm happy to use your wording instead -- I'll come up with a patch for that. What do you think of the wording adjustments in the attached patch? It's based on your suggested wording. -- Peter Geoghegan 0001-Tweak-page-level-freezing-comments.patch Description: Binary data
Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
Hi David, Thanks for continuing to work on this patch, and my apologies for silence on the patch. Its been hard to make time, and especially so because I typically develop on an ARM-based macOS system where I can't test this directly - hence my tests with virtualized EC2 instances, where I ran into the timing oddities. On Mon, Jan 2, 2023 at 5:28 AM David Geier wrote: > The INSTR_TIME_GET_MICROSEC() returns a uint64 while the other variants > return double. This seems error prone. What about renaming the function > or also have the function return a double and cast where necessary at > the call site? > Minor note, but in my understanding using a uint64 (where we can) is faster for any simple arithmetic we do with the values. > If no one objects I would also re-register this patch in the commit fest. > +1, and feel free to carry this patch forward - I'll try to make an effort to review my earlier testing issues again, as well as your later improvements to the patch. Also, FYI, I just posted an alternate idea for speeding up EXPLAIN ANALYZE with timing over in [0], using a sampling-based approach to reduce the timing overhead. [0]: https://www.postgresql.org/message-id/CAP53PkxXMk0j-%2B0%3DYwRti2pFR5UB2Gu4v2Oyk8hhZS0DRART6g%40mail.gmail.com Thanks, Lukas -- Lukas Fittl
Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
On Fri, Jul 15, 2022 at 11:21 AM Maciek Sakrejda wrote: > 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? For what it's worth, I can no longer reproduce this. In fact, I went back to master-as-of-around-then and applied Lukas' v2 patches again, and I still can't reproduce that. I do remember it happening consistently across several executions, but now \timing consistently shows 0.5-1ms slower, as expected. This does not explain the different timing issue Lukas was seeing in his tests, but I think we can assume what I reported originally here is not an issue.
Re: An oversight in ExecInitAgg for grouping sets
Richard Guo writes: > On Thu, Dec 22, 2022 at 2:02 PM Richard Guo wrote: >> If it is an empty grouping set, its length will be zero, and accessing >> phasedata->eqfunctions[length - 1] is not right. > Attached is a trivial patch for the fix. Agreed, that's a latent bug. It's only latent because the word just before a palloc chunk will never be zero, either in our historical palloc code or in v16's shiny new implementation. Nonetheless it is a horrible idea for ExecInitAgg to depend on that fact, so I pushed your fix. The thing that I find really distressing here is that it's been like this for years and none of our automated testing caught it. You'd have expected valgrind testing to do so ... but it does not, because we've never marked that word NOACCESS. Maybe we should rethink that? It'd require making mcxt.c do some valgrind flag manipulations so it could access the hdrmask when appropriate. regards, tom lane
TAP tests for psql \g piped into program
Hi, This is a follow-up to commit d2a44904 from the 2022-11 CF [1] The TAP tests were left out with the suggestion to use Perl instead of cat (Unix) / findstr (Windows) as the program to pipe into. PFA a patch implementing that suggestion. [1] https://commitfest.postgresql.org/40/4000/ Best regards, -- Daniel Vérité https://postgresql.verite.pro/ Twitter: @DanielVerite diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl index f447845717..9b908171e0 100644 --- a/src/bin/psql/t/001_basic.pl +++ b/src/bin/psql/t/001_basic.pl @@ -325,4 +325,31 @@ is($row_count, '10', 'client-side error commits transaction, no ON_ERROR_STOP and multiple -c switches' ); +# Test \g output piped into a program. +# The program is perl -pe '' to simply copy the input to the output. +my $g_file = "$tempdir/g_file_1.out"; +my $perlbin = PostgreSQL::Test::Utils::perl_binary(); +my $pipe_cmd = "$perlbin -pe '' >$g_file"; + +psql_like($node, "SELECT 'one' \\g | $pipe_cmd", qr//, "one command \\g"); +my $c1 = slurp_file($g_file); +like($c1, qr/one/); + +psql_like($node, "SELECT 'two' \\; SELECT 'three' \\g | $pipe_cmd", qr//, "two commands \\g"); +my $c2 = slurp_file($g_file); +like($c2, qr/two.*three/s); + + +psql_like($node, "\\set SHOW_ALL_RESULTS 0\nSELECT 'four' \\; SELECT 'five' \\g | $pipe_cmd", qr//, + "two commands \\g with only last result"); +my $c3 = slurp_file($g_file); +like($c3, qr/five/); +unlike($c3, qr/four/); + +psql_like($node, "copy (values ('foo'),('bar')) to stdout \\g | $pipe_cmd", + qr//, + "copy output passed to \\g pipe"); +my $c4 = slurp_file($g_file); +like($c4, qr/foo.*bar/s); + done_testing(); diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm index b139190cc8..4e97c49dca 100644 --- a/src/test/perl/PostgreSQL/Test/Utils.pm +++ b/src/test/perl/PostgreSQL/Test/Utils.pm @@ -74,6 +74,7 @@ our @EXPORT = qw( run_log run_command pump_until + perl_binary command_ok command_fails @@ -448,6 +449,23 @@ sub pump_until =pod +=item perl_binary() + +Return the location of the currently running Perl interpreter. + +=cut + +sub perl_binary +{ + # Note: use of forward slashes here avoids any escaping problems + # that arise from use of backslashes. + my $perlbin = $^X; + $perlbin =~ s!\\!/!g if $windows_os; + return $perlbin; +} + +=pod + =item generate_ascii_string(from_char, to_char) Generate a string made of the given range of ASCII characters. diff --git a/src/test/recovery/t/025_stuck_on_old_timeline.pl b/src/test/recovery/t/025_stuck_on_old_timeline.pl index fd821242e8..9b04175ef2 100644 --- a/src/test/recovery/t/025_stuck_on_old_timeline.pl +++ b/src/test/recovery/t/025_stuck_on_old_timeline.pl @@ -25,12 +25,11 @@ my $node_primary = PostgreSQL::Test::Cluster->new('primary'); # get there. $node_primary->init(allows_streaming => 1, has_archiving => 1); +my $perlbin = PostgreSQL::Test::Utils::perl_binary(); +my $archivedir_primary = $node_primary->archive_dir; # Note: consistent use of forward slashes here avoids any escaping problems # that arise from use of backslashes. That means we need to double-quote all # the paths in the archive_command -my $perlbin = $^X; -$perlbin =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os; -my $archivedir_primary = $node_primary->archive_dir; $archivedir_primary =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os; $node_primary->append_conf( 'postgresql.conf', qq(
Re: daitch_mokotoff module
Alvaro Herrera writes: > Hello > > On 2022-Dec-23, Dag Lem wrote: > [...] > So, yes, I'm proposing that we returns those as array elements and that > @> is used to match them. Looking into the array operators I guess that to match such arrays directly one would actually use && (overlaps) rather than @> (contains), but I digress. The function is changed to return an array of soundex codes - I hope it is now to your liking :-) I also improved on the documentation example (using Full Text Search). AFAIK you can't make general queries like that using arrays, however in any case I must admit that text arrays seem like more natural building blocks than space delimited text here. Search to perform is the best match for Daitch-Mokotoff, however , but in any case I've changed it into return arrays now. I hope it is to your liking. > >> Daitch-Mokotoff Soundex indexes alternative sounds for the same name, >> however if I understand correctly, you want to index names by single >> sounds, linking all alike sounding names to the same soundex code. I >> fail to see how that is useful - if you want to find matches for a name, >> you simply match against all indexed names. If you only consider one >> sound, you won't find all names that match. > > Hmm, I think we're saying the same thing, but from opposite points of > view. No, I want each name to return multiple codes, but that those > multiple codes can be treated as a multiple-value array of codes, rather > than as a single string of space-separated codes. > >> In any case, as explained in the documentation, the implementation is >> intended to be a companion to Full Text Search, thus text is the natural >> representation for the soundex codes. > > Hmm, I don't agree with this point. The numbers are representations of > the strings, but they don't necessarily have to be strings themselves. > > >> BTW Vera 79 does not match Veras 794000, because they don't sound >> the same (up to the maximum soundex code length). > > No, and maybe that's okay because they have different codes. But they > are both similar, in Daitch-Mokotoff, to Borja, which has two codes, > 79 and 794000. (Any Spanish speaker will readily tell you that > neither Vera nor Veras are similar in any way to Borja, but D-M has > chosen to say that each of them matches one of Borjas' codes. So they > *are* related, even though indirectly, and as a genealogist you *may* be > interested in getting a match for a person called Vera when looking for > relatives to a person called Veras. And, as a Spanish speaker, that > would make a lot of sense to me.) > > > Now, it's true that I've chosen to use Spanish names for my silly little > experiment. Maybe this isn't terribly useful as a practical example, > because this algorithm seems to have been designed for Jew surnames and > perhaps not many (or not any) Jews had Spanish surnames. I don't know; > I'm not a Jew myself (though Noah Gordon tells the tale of a Spanish Jew > called Josep Álvarez in his book "The Winemaker", so I guess it's not > impossible). Anyway, I suspect if you repeat the experiment with names > of other origins, you'll find pretty much the same results apply there, > and that is the whole reason D-M returns multiple codes and not just > one. > > > Merry Christmas :-) -- Dag
Re: daitch_mokotoff module
Sorry about the latest unfinished email - don't know what key combination I managed to hit there. Alvaro Herrera writes: > Hello > > On 2022-Dec-23, Dag Lem wrote: > [...] > > So, yes, I'm proposing that we returns those as array elements and that > @> is used to match them. > Looking into the array operators I guess that to match such arrays directly one would actually use && (overlaps) rather than @> (contains), but I digress. The function is changed to return an array of soundex codes - I hope it is now to your liking :-) I also improved on the documentation example (using Full Text Search). AFAIK you can't make general queries like that using arrays, however in any case I must admit that text arrays seem like more natural building blocks than space delimited text here. [...] >> BTW Vera 79 does not match Veras 794000, because they don't sound >> the same (up to the maximum soundex code length). > > No, and maybe that's okay because they have different codes. But they > are both similar, in Daitch-Mokotoff, to Borja, which has two codes, > 79 and 794000. (Any Spanish speaker will readily tell you that > neither Vera nor Veras are similar in any way to Borja, but D-M has > chosen to say that each of them matches one of Borjas' codes. So they > *are* related, even though indirectly, and as a genealogist you *may* be > interested in getting a match for a person called Vera when looking for > relatives to a person called Veras. And, as a Spanish speaker, that > would make a lot of sense to me.) It is what it is - we can't call it Daitch-Mokotoff Soundex while implementing something else. Having said that, one can always pre- or postprocess to tweak the results. Daitch-Mokotoff Soundex is known to produce false positives, but that is in many cases not a problem. Even though it's clearly tuned for Jewish names, the soundex algorithm seems to work just fine for European names (we use it to match mostly Norwegian names). Best regards Dag Lem diff --git a/contrib/fuzzystrmatch/Makefile b/contrib/fuzzystrmatch/Makefile index 0704894f88..12baf2d884 100644 --- a/contrib/fuzzystrmatch/Makefile +++ b/contrib/fuzzystrmatch/Makefile @@ -3,14 +3,15 @@ MODULE_big = fuzzystrmatch OBJS = \ $(WIN32RES) \ + daitch_mokotoff.o \ dmetaphone.o \ fuzzystrmatch.o EXTENSION = fuzzystrmatch -DATA = fuzzystrmatch--1.1.sql fuzzystrmatch--1.0--1.1.sql +DATA = fuzzystrmatch--1.1.sql fuzzystrmatch--1.1--1.2.sql fuzzystrmatch--1.0--1.1.sql PGFILEDESC = "fuzzystrmatch - similarities and distance between strings" -REGRESS = fuzzystrmatch +REGRESS = fuzzystrmatch fuzzystrmatch_utf8 ifdef USE_PGXS PG_CONFIG = pg_config @@ -22,3 +23,14 @@ top_builddir = ../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk endif + +# Force this dependency to be known even without dependency info built: +daitch_mokotoff.o: daitch_mokotoff.h + +daitch_mokotoff.h: daitch_mokotoff_header.pl + perl $< $@ + +distprep: daitch_mokotoff.h + +maintainer-clean: + rm -f daitch_mokotoff.h diff --git a/contrib/fuzzystrmatch/daitch_mokotoff.c b/contrib/fuzzystrmatch/daitch_mokotoff.c new file mode 100644 index 00..2548903770 --- /dev/null +++ b/contrib/fuzzystrmatch/daitch_mokotoff.c @@ -0,0 +1,582 @@ +/* + * Daitch-Mokotoff Soundex + * + * Copyright (c) 2021 Finance Norway + * Author: Dag Lem + * + * This implementation of the Daitch-Mokotoff Soundex System aims at high + * performance. + * + * - The processing of each phoneme is initiated by an O(1) table lookup. + * - For phonemes containing more than one character, a coding tree is traversed + * to process the complete phoneme. + * - The (alternate) soundex codes are produced digit by digit in-place in + * another tree structure. + * + * References: + * + * https://www.avotaynu.com/soundex.htm + * https://www.jewishgen.org/InfoFiles/Soundex.html + * https://familypedia.fandom.com/wiki/Daitch-Mokotoff_Soundex + * https://stevemorse.org/census/soundex.html (dmlat.php, dmsoundex.php) + * https://github.com/apache/commons-codec/ (dmrules.txt, DaitchMokotoffSoundex.java) + * https://metacpan.org/pod/Text::Phonetic (DaitchMokotoff.pm) + * + * A few notes on other implementations: + * + * - All other known implementations have the same unofficial rules for "UE", + * these are also adapted by this implementation (0, 1, NC). + * - The only other known implementation which is capable of generating all + * correct soundex codes in all cases is the JOS Soundex Calculator at + * https://www.jewishgen.org/jos/jossound.htm + * - "J" is considered (only) a vowel in dmlat.php + * - The official rules for "RS" are commented out in dmlat.php + * - Identical code digits for adjacent letters are not collapsed correctly in + * dmsoundex.php when double digit codes are involved. E.g. "BESST" yields + * 744300 instead of 743000 as for "BEST". + * - "J" is considered (only) a consonant in DaitchMokotoffSoundex.java + * - "Y" is n
Re: perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local
Hi, On 2022-12-29 13:40:13 -0800, Andres Freund wrote: > > > Should we backpatch this? Given the volume of warnings it's probably a > > > good > > > idea. But I'd let it step in HEAD for a few days of buildfarm coverage > > > first. > > > > +1 to both points. > > Pushed to HEAD. I haven't seen any problems in HEAD, so I'm working on backpatching. Greetings, Andres Freund
Doc: Rework contrib appendix -- informative titles, tweaked sentences
Hi, Attached is a patch: contrib_v1.patch It modifies Appendix F, the contrib directory. It adds brief text into the titles shown in the table of contents so it's easier to tell what each module does. It also suffixes [trusted] or [obsolete] on the relevant titles. I added the word "extension" into the appendix title because I always have problems scanning through the appendix and finding the one to do with extensions. The sentences describing what the modules are and how to build them have been reworked. Some split in 2, some words removed or replaced, etc. I introduced the word "component" because the appendix has build instructions for command line programs as well as extensions and libraries loaded with shared_preload_libraries(). This involved removing most occurrences of the word "module", although it is left in the section title. Regards, Karl Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein diff --git a/doc/src/sgml/adminpack.sgml b/doc/src/sgml/adminpack.sgml index 184e96d7a0..04f3b52379 100644 --- a/doc/src/sgml/adminpack.sgml +++ b/doc/src/sgml/adminpack.sgml @@ -1,7 +1,7 @@ - adminpack + adminpack — pgAdmin support toolpack adminpack diff --git a/doc/src/sgml/amcheck.sgml b/doc/src/sgml/amcheck.sgml index 5d61a33936..48d72a24a3 100644 --- a/doc/src/sgml/amcheck.sgml +++ b/doc/src/sgml/amcheck.sgml @@ -1,7 +1,7 @@ - amcheck + amcheck — tools to verify index consistency amcheck diff --git a/doc/src/sgml/auth-delay.sgml b/doc/src/sgml/auth-delay.sgml index 3bc9cfb207..690774f86f 100644 --- a/doc/src/sgml/auth-delay.sgml +++ b/doc/src/sgml/auth-delay.sgml @@ -1,7 +1,7 @@ - auth_delay + auth_delay — pause on authentication failure auth_delay diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index 394fec94e8..a80910dab5 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -1,7 +1,7 @@ - auto_explain + auto_explain — log execution plans of slow queries auto_explain diff --git a/doc/src/sgml/basebackup-to-shell.sgml b/doc/src/sgml/basebackup-to-shell.sgml index b2ecc373eb..fd082ceb0b 100644 --- a/doc/src/sgml/basebackup-to-shell.sgml +++ b/doc/src/sgml/basebackup-to-shell.sgml @@ -1,7 +1,7 @@ - basebackup_to_shell + basebackup_to_shell — example "shell" pg_basebackup module basebackup_to_shell diff --git a/doc/src/sgml/basic-archive.sgml b/doc/src/sgml/basic-archive.sgml index 0b650f17a8..c412590dd6 100644 --- a/doc/src/sgml/basic-archive.sgml +++ b/doc/src/sgml/basic-archive.sgml @@ -1,7 +1,7 @@ - basic_archive + basic_archive — an example WAL archive module basic_archive diff --git a/doc/src/sgml/bloom.sgml b/doc/src/sgml/bloom.sgml index a3f51cfdc4..4a188ad5f1 100644 --- a/doc/src/sgml/bloom.sgml +++ b/doc/src/sgml/bloom.sgml @@ -1,7 +1,7 @@ - bloom + bloom — bloom filter index access bloom diff --git a/doc/src/sgml/btree-gin.sgml b/doc/src/sgml/btree-gin.sgml index 5bc5a054e8..5aafe856b5 100644 --- a/doc/src/sgml/btree-gin.sgml +++ b/doc/src/sgml/btree-gin.sgml @@ -1,7 +1,8 @@ - btree_gin + btree_gin — + sample GIN B-tree equalivent operator classes [trusted] btree_gin diff --git a/doc/src/sgml/btree-gist.sgml b/doc/src/sgml/btree-gist.sgml index b67f20a00f..67ff13c77f 100644 --- a/doc/src/sgml/btree-gist.sgml +++ b/doc/src/sgml/btree-gist.sgml @@ -1,7 +1,8 @@ - btree_gist + btree_gist — + B-tree equalivent GiST index operators [trusted] btree_gist diff --git a/doc/src/sgml/citext.sgml b/doc/src/sgml/citext.sgml index 5986601327..bf08e9e6a2 100644 --- a/doc/src/sgml/citext.sgml +++ b/doc/src/sgml/citext.sgml @@ -1,7 +1,8 @@ - citext + citext — + a case-insensitive character string type [trusted] citext diff --git a/doc/src/sgml/contrib-spi.sgml b/doc/src/sgml/contrib-spi.sgml index fed6f24932..edeb5b6346 100644 --- a/doc/src/sgml/contrib-spi.sgml +++ b/doc/src/sgml/contrib-spi.sgml @@ -1,7 +1,7 @@ - spi + spi — Server Programming Interface features/examples SPI diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml index 4e7b87a42f..fd2e40980d 100644 --- a/doc/src/sgml/contrib.sgml +++ b/doc/src/sgml/contrib.sgml @@ -1,27 +1,31 @@ - Additional Supplied Modules + Additional Supplied Modules and Extensions - This appendix and the next one contain information regarding the modules that - can be found in the contrib directory of the + This appendix and the next one contain information on the + optional components + found in the contrib directory of the PostgreSQL distribution. These include porting tools, analysis utilities, - and plug-in features that are not part of the core PostgreSQL system, - mainly because they address a limited audience or are too experimental + and plug-in features that are not part of the core PostgreSQL system. + They are separate mainly + because they address a limite
Re: Infinite Interval
I have another patch, this one adds validations to operations that return intervals and updated error messages. I tried to give all of the error messages meaningful text, but I'm starting to think that almost all of them should just say "interval out of range". The current approach may reveal some implementation details and lead to confusion. For example, some subtractions are converted to additions which would lead to an error message about addition. SELECT date 'infinity' - interval 'infinity'; ERROR: cannot add infinite values with opposite signs I've also updated the commit message to include the remaining TODOs, which I've copied below 1. Various TODOs in code. 2. Correctly implement interval_part for infinite intervals. 3. Test consolidation. 4. Should we just use the months field to test for infinity? From 65aceb25bc090375b60d140b1630cabcc90f1c9c Mon Sep 17 00:00:00 2001 From: Joseph Koshakow Date: Sat, 17 Dec 2022 14:21:26 -0500 Subject: [PATCH] This is WIP. TODOs 1. Various TODOs in code. 2. Correctly implement interval_part for infinite intervals. 3. Test consolidation. 4. Should we just use the months field to test for infinity? Ashutosh Bapat and Joe Koshakow --- src/backend/utils/adt/date.c | 20 + src/backend/utils/adt/datetime.c | 14 +- src/backend/utils/adt/timestamp.c | 372 - src/include/datatype/timestamp.h | 22 + src/test/regress/expected/horology.out |6 +- src/test/regress/expected/interval.out | 1006 +++- src/test/regress/sql/horology.sql |6 +- src/test/regress/sql/interval.sql | 200 - 8 files changed, 1571 insertions(+), 75 deletions(-) diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c index 1cf7c7652d..c6259cd9c1 100644 --- a/src/backend/utils/adt/date.c +++ b/src/backend/utils/adt/date.c @@ -2073,6 +2073,11 @@ time_pl_interval(PG_FUNCTION_ARGS) Interval *span = PG_GETARG_INTERVAL_P(1); TimeADT result; + if (INTERVAL_NOT_FINITE(span)) + ereport(ERROR, +(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("cannot add infinite interval to time"))); + result = time + span->time; result -= result / USECS_PER_DAY * USECS_PER_DAY; if (result < INT64CONST(0)) @@ -2091,6 +2096,11 @@ time_mi_interval(PG_FUNCTION_ARGS) Interval *span = PG_GETARG_INTERVAL_P(1); TimeADT result; + if (INTERVAL_NOT_FINITE(span)) + ereport(ERROR, +(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("cannot subtract infinite interval from time"))); + result = time - span->time; result -= result / USECS_PER_DAY * USECS_PER_DAY; if (result < INT64CONST(0)) @@ -2605,6 +2615,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS) Interval *span = PG_GETARG_INTERVAL_P(1); TimeTzADT *result; + if (INTERVAL_NOT_FINITE(span)) + ereport(ERROR, +(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("cannot add infinite interval to time"))); + result = (TimeTzADT *) palloc(sizeof(TimeTzADT)); result->time = time->time + span->time; @@ -2627,6 +2642,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS) Interval *span = PG_GETARG_INTERVAL_P(1); TimeTzADT *result; + if (INTERVAL_NOT_FINITE(span)) + ereport(ERROR, +(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("cannot subtract infinite interval from time"))); + result = (TimeTzADT *) palloc(sizeof(TimeTzADT)); result->time = time->time - span->time; diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c index b5b117a8ca..b60d91dfb8 100644 --- a/src/backend/utils/adt/datetime.c +++ b/src/backend/utils/adt/datetime.c @@ -70,7 +70,7 @@ static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t, const char *abbr, pg_tz *tzp, int *offset, int *isdst); static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp, - DateTimeErrorExtra *extra); + DateTimeErrorExtra * extra); const int day_tab[2][13] = @@ -977,7 +977,7 @@ ParseDateTime(const char *timestr, char *workbuf, size_t buflen, int DecodeDateTime(char **field, int *ftype, int nf, int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp, - DateTimeErrorExtra *extra) + DateTimeErrorExtra * extra) { int fmask = 0, tmask, @@ -1927,7 +1927,7 @@ DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t, const char *abbr, pg_tz *tzp, int DecodeTimeOnly(char **field, int *ftype, int nf, int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp, - DateTimeErrorExtra *extra) + DateTimeErrorExtra * extra) { int fmask = 0, tmask, @@ -3232,7 +3232,7 @@ DecodeTimezone(const char *str, int *tzp) int DecodeTimezoneAbbrev(int field, const char *lowtoken, int *ftype, int *offset, pg_tz **tz, - DateTimeErrorExtra *extra) + DateTimeErrorExtra * extra) { const datetkn *tp; @@ -3634,6 +3634,8 @@ DecodeInterval(char **field, int *ftype, int nf, int range, cas
Re: perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local
On 2023-01-02 15:46:36 -0800, Andres Freund wrote: > On 2022-12-29 13:40:13 -0800, Andres Freund wrote: > > > > Should we backpatch this? Given the volume of warnings it's probably a > > > > good > > > > idea. But I'd let it step in HEAD for a few days of buildfarm coverage > > > > first. > > > > > > +1 to both points. > > > > Pushed to HEAD. > > I haven't seen any problems in HEAD, so I'm working on backpatching. And done.
Re: Simplify standby state machine a bit in WaitForWALToBecomeAvailable()
On Fri, Dec 30, 2022 at 10:32:57AM -0800, Nathan Bossart wrote: > This looks correct to me. The only thing that stood out to me was the loop > through 'tles' in XLogFileReadyAnyTLI. With this change, we'd loop through > the timelines for both XLOG_FROM_PG_ARCHIVE and XLOG_FROM_PG_WAL, whereas > now we only loop through the timelines once. However, I doubt this makes > much difference in practice. You'd only do the extra loop whenever > restoring from the archives failed. case XLOG_FROM_ARCHIVE: + + /* +* After failing to read from archive, we try to read from +* pg_wal. +*/ + currentSource = XLOG_FROM_PG_WAL; + break; In standby mode, the priority lookup order is pg_wal -> archive -> stream. With this change, we would do pg_wal -> archive -> pg_wal -> stream, meaning that it could influence some recovery scenarios while involving more lookups than necessary to the local pg_wal/ directory? See, on failure where the current source is XLOG_FROM_ARCHIVE, we would not switch anymore directly to XLOG_FROM_STREAM. -- Michael signature.asc Description: PGP signature
Re: CFM for 2023-01
On Wed, Dec 28, 2022 at 10:52:38AM +0530, vignesh C wrote: > If no one has volunteered for the upcoming (January 2023) commitfest. > I would like to volunteer for it. If you want to be up to the task, that would be great, of course. For now, I have switched the CF as in progress. -- Michael signature.asc Description: PGP signature
Re: [PATCH] random_normal function
On Fri, Dec 30, 2022 at 09:58:04PM -0600, Justin Pryzby wrote: > This is still failing tests - did you enable cirrusci on your own github > account to run available checks on the patch ? FYI, here is the failure: [21:23:10.814] In file included from pg_prng.c:27: [21:23:10.814] ../../src/include/utils/float.h:46:16: error: ‘struct Node’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror] [21:23:10.814]46 | struct Node *escontext); And a link to it, from the CF bot: https://cirrus-ci.com/task/5969961391226880?logs=gcc_warning#L452 -- Michael signature.asc Description: PGP signature
Re: New strategies for freezing, advancing relfrozenxid early
On Mon, 2023-01-02 at 11:45 -0800, Peter Geoghegan wrote: > What do you think of the wording adjustments in the attached patch? > It's based on your suggested wording. Great, thank you. -- Jeff Davis PostgreSQL Contributor Team - AWS
Re: [PATCH] random_normal function
Michael Paquier writes: > FYI, here is the failure: > [21:23:10.814] In file included from pg_prng.c:27: > [21:23:10.814] ../../src/include/utils/float.h:46:16: error: ‘struct > Node’ declared inside parameter list will not be visible outside of > this definition or declaration [-Werror] > [21:23:10.814]46 | struct Node *escontext); Hmm ... this looks an awful lot like it is the fault of ccff2d20e not of the random_normal patch; that is, we probably need a "struct Node" stub declaration in float.h. However, why are we not seeing any reports of this from elsewhere? I'm concerned now that there are more places also needing stub declarations, but my test process didn't expose it. regards, tom lane
Re: CFM for 2023-01
On Tue, 3 Jan 2023 at 07:52, Michael Paquier wrote: > > On Wed, Dec 28, 2022 at 10:52:38AM +0530, vignesh C wrote: > > If no one has volunteered for the upcoming (January 2023) commitfest. > > I would like to volunteer for it. > > If you want to be up to the task, that would be great, of course. For > now, I have switched the CF as in progress. Thanks, I will start working on this. Regards, Vignesh
Re: Todo: Teach planner to evaluate multiple windows in the optimal order
On Mon, 26 Dec 2022 at 02:04, Ankit Kumar Pandey wrote: > Point #1 > > In the above query Oracle 10g performs 2 sorts, DB2 and Sybase perform 3 > sorts. We also perform 3. This shouldn't be too hard to do. See the code in select_active_windows(). You'll likely want to pay attention to the DISTINCT pathkeys if they exist and just use the ORDER BY pathkeys if the query has no DISTINCT clause. DISTINCT is evaluated after Window and before ORDER BY. One idea to implement this would be to adjust the loop in select_active_windows() so that we record any WindowClauses which have the pathkeys contained in the ORDER BY / DISTINCT pathkeys then record those separately and append those onto the end of the actives array after the sort. I do think you'll likely want to put any WindowClauses which have pathkeys which are a true subset or true superset of the ORDER BY / DISTINCT pathkeys last. If they're a superset then we won't need to perform any additional ordering for the DISTINCT / ORDER BY clause. If they're a subset then we might be able to perform an Incremental Sort, which is likely much cheaper than a full sort. The existing code should handle that part. You just need to make select_active_windows() more intelligent. You might also think that we could perform additional optimisations and also adjust the ORDER BY clause of a WindowClause if it contains the pathkeys of the DISTINCT / ORDER BY clause. For example: SELECT *,row_number() over (order by a,b) from tab order by a,b,c; However, if you were to adjust the WindowClauses ORDER BY to become a,b,c then you could produce incorrect results for window functions that change their result based on peer rows. Note the difference in results from: create table ab(a int, b int); insert into ab select x,y from generate_series(1,5) x, generate_Series(1,5)y; select a,b,count(*) over (order by a) from ab order by a,b; select a,b,count(*) over (order by a,b) from ab order by a,b; > and Point #2 > > Teach planner to decide which window to evaluate first based on costs. > Currently the first window in the query is evaluated first, there may be no > index to help sort the first window, but perhaps there are for other windows > in the query. This may allow an index scan instead of a seqscan -> sort. What Tom wrote about that in the first paragraph of [1] still applies. The problem is that if the query contains many joins that to properly find the cheapest way of executing the query we'd have to perform the join search once for each unique sort order of each WindowClause. That's just not practical to do from a performance standpoint. The join search can be very expensive. There may be something that could be done to better determine the most likely candidate for the first WindowClause using some heuristics, but I've no idea what those would be. You should look into point #1 first. Point #2 is significantly more difficult to solve in a way that would be acceptable to the project. David [1] https://www.postgresql.org/message-id/11535.1230501658%40sss.pgh.pa.us
Re: [PATCH] Improve ability to display optimizer analysis using OPTIMIZER_DEBUG
On Mon, 26 Dec 2022 at 08:05, Ankit Kumar Pandey wrote: > Also, inputs from other hackers are welcomed here. I'm with Tom on this. I've never once used this feature to try to figure out why a certain plan was chosen or not chosen. Do you actually have a need for this or are you just trying to tick off some TODO items? I'd really rather not see us compiling all that debug code in by default unless it's actually going to be useful to a meaningful number of people. David
RADIUS tests and improvements
Hi, Here's a draft patch to tackle a couple of TODOs in the RADIUS code in auth.c. The first change is to replace select() with a standard latch loop that responds to interrupts, postmaster death etc promptly. It's not really too much of a big deal because the timeout was only 3 seconds (hardcoded), but it's not good to have places that ignore ProcSignal, and it's good to move code to our modern pattern for I/O multiplexing. We know from experience that we have to crank timeouts up to be able to run tests reliably on slow/valgrind/etc systems, so the second change is to change the timeout to a GUC, as also requested by a comment. One good side-effect is that it becomes easy and fast to test the timed-out code path too, with a small value. While adding the GUC I couldn't help wondering why RADIUS even needs a timeout separate from authentication_timeout; another way to go here would be to remove it completely, but that'd be a policy change (removing the 3 second timeout we always had). Thoughts? The patch looks bigger than it really is because it changes the indentation level. But first, some basic tests to show that it works. We can test before and after the change and have a non-zero level of confidence about whacking the code around. Like existing similar tests, you need to install an extra package (FreeRADIUS) and opt in with PG_EXTRA_TESTS=radius. I figured out how to do that for our 3 CI Unixen, so cfbot should run the tests and pass once I add this to the March commitfest. FreeRADIUS claims to work on Windows too, but I don't know how to set that up; maybe someday someone will fix that for all the PG_EXTRA_TESTS tests. I've also seen this work on a Mac with MacPorts. There's only one pathname in there that's a wild guess: non-Debianoid Linux systems; if you know the answer there please LMK. From 72a63c87b595a31f3d5e68722ca6bc7460190cc0 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Sat, 31 Dec 2022 14:41:57 +1300 Subject: [PATCH 1/3] Add simple test for RADIUS authentication. This is similar to the existing tests for ldap and kerberos. It requires FreeRADIUS to be installed, and opens ports that may be considered insecure, so users have to opt in with PG_EXTRA_TESTS=radius. --- src/test/Makefile | 2 +- src/test/meson.build | 1 + src/test/radius/Makefile | 23 + src/test/radius/meson.build | 12 +++ src/test/radius/t/001_auth.pl | 187 ++ 5 files changed, 224 insertions(+), 1 deletion(-) create mode 100644 src/test/radius/Makefile create mode 100644 src/test/radius/meson.build create mode 100644 src/test/radius/t/001_auth.pl diff --git a/src/test/Makefile b/src/test/Makefile index dbd3192874..687164412c 100644 --- a/src/test/Makefile +++ b/src/test/Makefile @@ -12,7 +12,7 @@ subdir = src/test top_builddir = ../.. include $(top_builddir)/src/Makefile.global -SUBDIRS = perl regress isolation modules authentication recovery subscription +SUBDIRS = perl regress isolation modules authentication recovery radius subscription ifeq ($(with_icu),yes) SUBDIRS += icu diff --git a/src/test/meson.build b/src/test/meson.build index 5f3c9c2ba2..b5da17b531 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -5,6 +5,7 @@ subdir('isolation') subdir('authentication') subdir('recovery') +subdir('radius') subdir('subscription') subdir('modules') diff --git a/src/test/radius/Makefile b/src/test/radius/Makefile new file mode 100644 index 00..56768a3ca9 --- /dev/null +++ b/src/test/radius/Makefile @@ -0,0 +1,23 @@ +#- +# +# Makefile for src/test/radius +# +# Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group +# Portions Copyright (c) 1994, Regents of the University of California +# +# src/test/radius/Makefile +# +#- + +subdir = src/test/radius +top_builddir = ../../.. +include $(top_builddir)/src/Makefile.global + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) + +clean distclean maintainer-clean: + rm -rf tmp_check diff --git a/src/test/radius/meson.build b/src/test/radius/meson.build new file mode 100644 index 00..ea7afc4555 --- /dev/null +++ b/src/test/radius/meson.build @@ -0,0 +1,12 @@ +# Copyright (c) 2022, PostgreSQL Global Development Group + +tests += { + 'name': 'radius', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'tap': { +'tests': [ + 't/001_auth.pl', +], + }, +} diff --git a/src/test/radius/t/001_auth.pl b/src/test/radius/t/001_auth.pl new file mode 100644 index 00..44db62a3d7 --- /dev/null +++ b/src/test/radius/t/001_auth.pl @@ -0,0 +1,187 @@ + +# Copyright (c) 2021-2022, PostgreSQL Global Development Group + +# Debian: apt-get install freeradius +# Homebrew: brew install freeradius-server +# FreeBSD: pkg install freeradius3 +# MacPorts: po
Re: wake up logical workers after ALTER SUBSCRIPTION
On Thu, Dec 15, 2022 at 4:47 AM Nathan Bossart wrote: > > On Wed, Dec 14, 2022 at 02:02:58PM -0500, Tom Lane wrote: > > Maybe we could have workers that are exiting for that reason set a > > flag saying "please restart me without delay"? > > That helps a bit, but there are still delays when starting workers for new > subscriptions. I think we'd need to create a new array in shared memory > for subscription OIDs that need their workers started immediately. > That would be tricky because the list of subscription OIDs can be longer than the workers. Can't we set a boolean variable (check_immediate or something like that) in LogicalRepCtxStruct and use that to traverse the subscriptions? So, when any worker will restart because of a parameter change, we can set the variable and send a signal to the launcher. The launcher can then check this variable to decide whether to start the missing workers for enabled subscriptions. -- With Regards, Amit Kapila.
Re: Add a test to ldapbindpasswd
Hi, On Mon, Jan 02, 2023 at 09:45:27AM -0500, Andrew Dunstan wrote: > > On 2023-01-01 Su 18:31, Andrew Dunstan wrote: > > Let's see how we fare with this patch. > > > > > > Not so well :-(. This version tries to make the tests totally > independent, as they should be. That's an attempt to get the cfbot to go > green, but I am intending to refactor this code substantially so the > common bits are in a module each test file will load. FTR you can run the same set of CI tests using your own GH account rather than sedning patches, see src/tools/ci/README/
Re: wake up logical workers after ALTER SUBSCRIPTION
On Wed, Dec 7, 2022 at 11:42 PM Nathan Bossart wrote: > > On Wed, Dec 07, 2022 at 02:07:11PM +0300, Melih Mutlu wrote: > > Do we also need to wake up all sync workers too? Even if not, I'm not > > actually sure whether doing that would harm anything though. > > Just asking since currently the patch wakes up all workers including sync > > workers if any still exists. > > After sleeping on this, I think we can do better. IIUC we can simply check > for AllTablesyncsReady() at the end of process_syncing_tables_for_apply() > and wake up the logical replication workers (which should just consiѕt of > setting the current process's latch) if we are ready for two_phase mode. > How just waking up will help with two_phase mode? For that, we need to restart the apply worker as we are doing at the beginning of process_syncing_tables_for_apply(). -- With Regards, Amit Kapila.
Re: Allow placeholders in ALTER ROLE w/o superuser
On Mon, Jan 2, 2023 at 6:42 PM Justin Pryzby wrote: > On Mon, Jan 02, 2023 at 06:14:48PM +0300, Alexander Korotkov wrote: > > I'm going to push this if no objections. > > I also suggest that meson.build should not copy regress_args. Good point, thanks. -- Regards, Alexander Korotkov 0001-meson-Add-running-test-setup-as-a-replacement-for-v2.patch Description: Binary data
Re: [PATCH] Improve ability to display optimizer analysis using OPTIMIZER_DEBUG
On 03/01/23 08:38, David Rowley wrote: I'm with Tom on this. I've never once used this feature to try to figure out why a certain plan was chosen or not chosen. I'd really rather not see us compiling all that debug code in by default unless it's actually going to be useful to a meaningful number of people. Okay this makes sense. Do you actually have a need for this or are you just trying to tick off some TODO items? I would say Iatter but reason I picked it up was more on side of learning optimizer better. Currently, I am left with explain analyze which does its job but for understanding internal working of optimizer, there are not much alternatives. Again, if I know where to put breakpoint, I could see required path/states but point of this todo item is ability to do this without need of developer tools. Also from the thread, https://www.postgresql.org/message-id/20120821.121611.501104647612634419.t-is...@sraoss.co.jp +1. It would also be popular with our academic users. There could be potential for this as well. -- Regards, Ankit Kumar Pandey
Re: Time delayed LR (WAS Re: logical replication restrictions)
On Tue, 27 Dec 2022 at 14:59, Hayato Kuroda (Fujitsu) wrote: > Note that more than half of the modifications are done by Osumi-san. 1) This global variable can be removed as it is used only in send_feedback which is called from maybe_delay_apply so we could pass it as a function argument: + * delay, avoid having positions of the flushed and apply LSN overwritten by + * the latest LSN. + */ +static bool in_delaying_apply = false; +static XLogRecPtr last_received = InvalidXLogRecPtr; + 2) -1 gets converted to -1000 +int64 +interval2ms(const Interval *interval) +{ + int64 days; + int64 ms; + int64 result; + + days = interval->month * INT64CONST(30); + days += interval->day; + + /* Detect whether the value of interval can cause an overflow. */ + if (pg_mul_s64_overflow(days, MSECS_PER_DAY, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), +errmsg("bigint out of range"))); + + /* Adds portion time (in ms) to the previous result. */ + ms = interval->time / INT64CONST(1000); + if (pg_add_s64_overflow(result, ms, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), +errmsg("bigint out of range"))); create subscription sub7 connection 'dbname=regression host=localhost port=5432' publication pub1 with (min_apply_delay = '-1'); ERROR: -1000 ms is outside the valid range for parameter "min_apply_delay" 3) This can be slightly reworded: + + The length of time (ms) to delay the application of changes. + to: Delay applying the changes by a specified amount of time(ms). 4) maybe_delay_apply can be moved from apply_handle_stream_prepare to apply_spooled_messages so that it is consistent with maybe_start_skipping_changes: @@ -1120,6 +1240,19 @@ apply_handle_stream_prepare(StringInfo s) elog(DEBUG1, "received prepare for streamed transaction %u", prepare_data.xid); + /* +* Should we delay the current prepared transaction? +* +* Although the delay is applied in BEGIN PREPARE messages, streamed +* prepared transactions apply the delay in a STREAM PREPARE message. +* That's ok because no changes have been applied yet +* (apply_spooled_messages() will do it). The STREAM START message does +* not contain a prepare time (it will be available when the in-progress +* prepared transaction finishes), hence, it was not possible to apply a +* delay at that time. +*/ + maybe_delay_apply(prepare_data.prepare_time); That way the call from apply_handle_stream_commit can also be removed. 5) typo transfering should be transferring + publisher and the current time on the subscriber. Time spent in logical + decoding and in transfering the transaction may reduce the actual wait + time. If the system clocks on publisher and subscriber are not 6) feedbacks can be changed to feedback messages + * it's necessary to keep sending feedbacks during the delay from the worker + * process. Meanwhile, the feature delays the apply before starting the 7) + /* + * Suppress overwrites of flushed and writtten positions by the lastest + * LSN in send_feedback(). + */ 7a) typo writtten should be written 7b) lastest should latest Regards, Vignesh
Re: Todo: Teach planner to evaluate multiple windows in the optimal order
On 03/01/23 08:21, David Rowley wrote: On Mon, 26 Dec 2022 at 02:04, Ankit Kumar Pandey wrote: Point #1 In the above query Oracle 10g performs 2 sorts, DB2 and Sybase perform 3 sorts. We also perform 3. This shouldn't be too hard to do. See the code in select_active_windows(). You'll likely want to pay attention to the DISTINCT pathkeys if they exist and just use the ORDER BY pathkeys if the query has no DISTINCT clause. DISTINCT is evaluated after Window and before ORDER BY. One idea to implement this would be to adjust the loop in select_active_windows() so that we record any WindowClauses which have the pathkeys contained in the ORDER BY / DISTINCT pathkeys then record those separately and append those onto the end of the actives array after the sort. I do think you'll likely want to put any WindowClauses which have pathkeys which are a true subset or true superset of the ORDER BY / DISTINCT pathkeys last. If they're a superset then we won't need to perform any additional ordering for the DISTINCT / ORDER BY clause. If they're a subset then we might be able to perform an Incremental Sort, which is likely much cheaper than a full sort. The existing code should handle that part. You just need to make select_active_windows() more intelligent. You might also think that we could perform additional optimisations and also adjust the ORDER BY clause of a WindowClause if it contains the pathkeys of the DISTINCT / ORDER BY clause. For example: SELECT *,row_number() over (order by a,b) from tab order by a,b,c; However, if you were to adjust the WindowClauses ORDER BY to become a,b,c then you could produce incorrect results for window functions that change their result based on peer rows. Note the difference in results from: create table ab(a int, b int); insert into ab select x,y from generate_series(1,5) x, generate_Series(1,5)y; select a,b,count(*) over (order by a) from ab order by a,b; select a,b,count(*) over (order by a,b) from ab order by a,b; Thanks, let me try this. and Point #2 Teach planner to decide which window to evaluate first based on costs. Currently the first window in the query is evaluated first, there may be no index to help sort the first window, but perhaps there are for other windows in the query. This may allow an index scan instead of a seqscan -> sort. What Tom wrote about that in the first paragraph of [1] still applies. The problem is that if the query contains many joins that to properly find the cheapest way of executing the query we'd have to perform the join search once for each unique sort order of each WindowClause. That's just not practical to do from a performance standpoint. The join search can be very expensive. There may be something that could be done to better determine the most likely candidate for the first WindowClause using some heuristics, but I've no idea what those would be. You should look into point #1 first. Point #2 is significantly more difficult to solve in a way that would be acceptable to the project. Okay, leaving this out for now. -- Regards, Ankit Kumar Pandey
Re: typos
On Fri, Dec 30, 2022 at 05:12:57PM -0600, Justin Pryzby wrote: # Use larger ccache cache, as this task compiles with multiple compilers / # flag combinations -CCACHE_MAXSIZE: "1GB" +CCACHE_MAXSIZE: "1G" In 0006, I am not sure how much this matters. Perhaps somebody more fluent with Cirrus, though, has a different opinion.. * pointer to this structure. The information here must be sufficient to * properly initialize each new TableScanDesc as workers join the scan, and it - * must act as a information what to scan for those workers. + * must provide information what to scan for those workers. This comment in 0009 is obviously incorrect, but I am not sure whether your new suggestion is an improvement. Do workers provide such information or has this structure some information that the workers rely on? Not sure that the whitespace issue in 0021 for the header of inval.c is worth caring about. 0014 and 0013 do not reduce the translation workload, as the messages include some stuff specific to the GUC names accessed to, or some specific details about the code paths triggered. The rest has been applied where they matter. -- Michael signature.asc Description: PGP signature
Re: typos
On Tue, Jan 3, 2023 at 12:58 PM Michael Paquier wrote: > > On Fri, Dec 30, 2022 at 05:12:57PM -0600, Justin Pryzby wrote: > > # Use larger ccache cache, as this task compiles with multiple compilers > / > # flag combinations > -CCACHE_MAXSIZE: "1GB" > +CCACHE_MAXSIZE: "1G" > > In 0006, I am not sure how much this matters. > The other places in that file use M, so maybe, this is more consistent. One minor comment: -spoken in Belgium (BE), with a UTF-8 character set +spoken in Belgium (BE), with a UTF-8 character set Shouldn't this be UTF8 as we are using in func.sgml? -- With Regards, Amit Kapila.
[Commitfest 2023-01] has started
Hi All, Just a reminder that Commitfest 2023-01 has started. There are many patches based on the latest run from [1] which require a) Rebased on top of head b) Fix compilation failures c) Fix test failure, please have a look and rebase it so that it is easy for the reviewers and committers: 1. TAP output format for pg_regress 2. Add BufFileRead variants with short read and EOF detection 3. Add SHELL_EXIT_CODE variable to psql 4. Add foreign-server health checks infrastructure 5. Add last_vacuum_index_scans in pg_stat_all_tables 6. Add index scan progress to pg_stat_progress_vacuum 7. Add the ability to limit the amount of memory that can be allocated to backends. 8. Add tracking of backend memory allocated to pg_stat_activity 9. CAST( ... ON DEFAULT) 10. CF App: add "Returned: Needs more interest" close status 11. CI and test improvements 12. Cygwin cleanup 13. Expand character set for ltree labels 14. Fix tab completion MERGE 15. Force streaming every change in logical decoding 16. More scalable multixacts buffers and locking 17. Move SLRU data into the regular buffer pool 18. Move extraUpdatedCols out of RangeTblEntry 19.New [relation] options engine 20. Optimizing Node Files Support 21. PGDOCS - Stats views and functions not in order? 22. POC: Lock updated tuples in tuple_update() and tuple_delete() 23. Parallelize correlated subqueries that execute within each worker 24. Pluggable toaster 25. Prefetch the next tuple's memory during seqscans 26. Pulling up direct-correlated ANY_SUBLINK 27. Push aggregation down to base relations and joins 28. Reduce timing overhead of EXPLAIN ANALYZE using rdtsc 29. Refactor relation extension, faster COPY 30. Remove NEW placeholder entry from stored view query range table 31. TDE key management patches 32. Use AF_UNIX for tests on Windows (ie drop fallback TCP code) 33. Windows filesystem support improvements 34. making relfilenodes 56 bit 35. postgres_fdw: commit remote (sub)transactions in parallel during pre-commit 36.recovery modules Commitfest status as of now: Needs review:177 Waiting on Author: 47 Ready for Committer: 20 Committed: 31 Withdrawn:4 Rejected: 0 Returned with Feedback: 0 Total: 279 We will be needing more members to actively review the patches to get more patches to the committed state. I would like to remind you that each patch submitter is expected to review at least one patch from another submitter during the CommitFest, those members who have not picked up patch for review please pick someone else's patch to review as soon as you can. I'll send out reminders this week to get your patches rebased and update the status of the patch accordingly. [1] - http://cfbot.cputube.org/ Regards, Vignesh