Re: some grammar refactoring
On 2020-05-19 08:43, Peter Eisentraut wrote: Here is a series of patches to do some refactoring in the grammar around the commands COMMENT, DROP, SECURITY LABEL, and ALTER EXTENSION ... ADD/DROP. These patches have been committed. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Read access for pg_monitor to pg_replication_origin_status view
On Wed, Jun 10, 2020 at 12:35:49PM +0900, Michael Paquier wrote: > On Tue, Jun 09, 2020 at 05:07:39PM +0900, Masahiko Sawada wrote: >> I agreed with the change you proposed. > > OK, thanks. Then let's wait a couple of days to see if anybody has > any objections with the removal of the hardcoded superuser check > for those functions. Committed the part removing the superuser checks as of cc07264. -- Michael signature.asc Description: PGP signature
Re: Add tap test for --extra-float-digits option
On Sat, Jun 13, 2020 at 06:34:46PM +0900, Dong Wook Lee wrote: > First of all, thank you for merging my patch. > And I'm sorry, I should have been more careful about it. Next time I > will follow format. And there is something I will tell you We are all here to learn. It is good to begin with small contributions to get a sense of how the project works, so I think that you are doing well. > Because many opensource hackers who interested in > PostgreSQL project can want to keep a record of author info > on commits they wrote. Otherwise, contribution records can not be found > by 'git shortlog -sn' and GitHub and OpenHub cannot track their > opensource contribution records... > > So what about using --author for PostgreSQL contributors > when merging their patches? like the Linux Kernel project That may be something to discuss with the project policy per-se. When it comes to credit people, committers list authors, reviewers, reporters, etc. directly in the commit log. And your name is mentioned in 64725728, I made sure of it. The latest discussions we had about the commit log format involved encouraging as much as possible the use of a "Discussion" tag in commit logs, the rest depends on each committer, and nobody uses --author. -- Michael signature.asc Description: PGP signature
Re: Transactions involving multiple postgres foreign servers, take 2
On Sat, 13 Jun 2020 at 14:02, Amit Kapila wrote: > > On Fri, Jun 12, 2020 at 6:24 PM Masahiko Sawada > wrote: > > > > On Fri, 12 Jun 2020 at 19:24, Amit Kapila wrote: > > > > > > > > Which are these other online transactions? I had assumed that foreign > > > > > transaction resolver process is to resolve in-doubt transactions but > > > > > it seems it is also used for some other purpose which anyway was the > > > > > next question I had while reviewing other sections of docs but let's > > > > > clarify as it came up now. > > > > > > > > When a distributed transaction is committed by COMMIT command, the > > > > postgres backend process prepare all foreign transaction and commit > > > > the local transaction. > > > > > > > > Thank you for your review comments! Let me answer your question first. > > I'll see the review comments. > > > > > > > > Does this mean that we will mark the xid as committed in CLOG of the > > > local server? > > > > Well what I meant is that when the client executes COMMIT command, the > > backend executes PREPARE TRANSACTION command on all involved foreign > > servers and then marks the xid as committed in clog in the local > > server. > > > > Won't it create an inconsistency in viewing the data from the > different servers? Say, such a transaction inserts one row into a > local server and another into the foreign server. Now, if we follow > the above protocol, the user will be able to see the row from the > local server but not from the foreign server. Yes, you're right. This atomic commit feature doesn't guarantee such consistent visibility so-called atomic visibility. Even the local server is not modified, since a resolver process commits prepared foreign transactions one by one another user could see an inconsistent result. Providing globally consistent snapshots to transactions involving foreign servers is one of the solutions. Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: exp() versus the POSIX standard
Emre Hasegeli writes:
>> No, no kind of GUC switch is needed. Just drop underflow/overflow checks.
>> You'll get 0 or Infinity in expected places, and infinities are okay since
>> 2007.
> This is out of scope of this thread.
Yeah, that. At the moment I'm just interested in making the float and
numeric functions give equivalent results for infinite inputs. If you
want to make a more general proposal about removing error checks, that
seems like a separate topic.
> I am not sure opening it to
> discussion on another thread would yield any result. Experienced
> developers like Tom appear to be in agreement of us needing to protect
> users from oddities of floating point numbers. (I am not.)
I think there's a pretty fundamental distinction between this behavior:
regression=# select exp('-inf'::float8);
exp
-
0
(1 row)
and this one:
regression=# select exp('-1000'::float8);
ERROR: value out of range: underflow
In the first case, zero is the correct answer to any precision you care
to name. In the second case, zero is *not* the correct answer; we simply
cannot represent the correct answer (somewhere around 1e-434) as a float8.
Returning zero would represent 100% loss of accuracy. Now, there may well
be applications where you'd rather take the zero result and press on, but
I'd argue that they're subtle ones that you're not likely gonna be writing
in SQL.
Anyway, for now I propose the attached patch. The test cases inquire into
the edge-case behavior of pow() much more closely than we have done in the
past, and I wouldn't be a bit surprised if some of the older buildfarm
critters fail some of them. So my inclination is to try this only in
HEAD for starters. Even if we want to back-patch, I'd be hesitant to
put it in versions older than v12, where we started to require C99.
regards, tom lane
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 6a717f19bb..84d37de930 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1565,7 +1565,7 @@ dpow(PG_FUNCTION_ARGS)
if (unlikely(isinf(result)) && !isinf(arg1) && !isinf(arg2))
float_overflow_error();
- if (unlikely(result == 0.0) && arg1 != 0.0)
+ if (unlikely(result == 0.0) && arg1 != 0.0 && !isinf(arg1) && !isinf(arg2))
float_underflow_error();
PG_RETURN_FLOAT8(result);
@@ -1581,15 +1581,38 @@ dexp(PG_FUNCTION_ARGS)
float8 arg1 = PG_GETARG_FLOAT8(0);
float8 result;
- errno = 0;
- result = exp(arg1);
- if (errno == ERANGE && result != 0 && !isinf(result))
- result = get_float8_infinity();
-
- if (unlikely(isinf(result)) && !isinf(arg1))
- float_overflow_error();
- if (unlikely(result == 0.0))
- float_underflow_error();
+ /*
+ * Handle NaN and Inf cases explicitly. This avoids needing to assume
+ * that the platform's exp() conforms to POSIX for these cases, and it
+ * removes some edge cases for the overflow checks below.
+ */
+ if (isnan(arg1))
+ result = arg1;
+ else if (isinf(arg1))
+ {
+ /* Per POSIX, exp(-Inf) is 0 */
+ result = (arg1 > 0.0) ? arg1 : 0;
+ }
+ else
+ {
+ /*
+ * On some platforms, exp() will not set errno but just return Inf or
+ * zero to report overflow/underflow; therefore, test both cases.
+ */
+ errno = 0;
+ result = exp(arg1);
+ if (unlikely(errno == ERANGE))
+ {
+ if (result != 0.0)
+float_overflow_error();
+ else
+float_underflow_error();
+ }
+ else if (unlikely(isinf(result)))
+ float_overflow_error();
+ else if (unlikely(result == 0.0))
+ float_underflow_error();
+ }
PG_RETURN_FLOAT8(result);
}
diff --git a/src/test/regress/expected/float8.out b/src/test/regress/expected/float8.out
index aaef20bcfd..3957fb58d8 100644
--- a/src/test/regress/expected/float8.out
+++ b/src/test/regress/expected/float8.out
@@ -385,6 +385,158 @@ SELECT power(float8 'NaN', float8 '0');
1
(1 row)
+SELECT power(float8 'inf', float8 '0');
+ power
+---
+ 1
+(1 row)
+
+SELECT power(float8 '-inf', float8 '0');
+ power
+---
+ 1
+(1 row)
+
+SELECT power(float8 '0', float8 'inf');
+ power
+---
+ 0
+(1 row)
+
+SELECT power(float8 '0', float8 '-inf');
+ERROR: zero raised to a negative power is undefined
+SELECT power(float8 '1', float8 'inf');
+ power
+---
+ 1
+(1 row)
+
+SELECT power(float8 '1', float8 '-inf');
+ power
+---
+ 1
+(1 row)
+
+SELECT power(float8 '-1', float8 'inf');
+ power
+---
+ 1
+(1 row)
+
+SELECT power(float8 '-1', float8 '-inf');
+ power
+---
+ 1
+(1 row)
+
+SELECT power(float8 '0.1', float8 'inf');
+ power
+---
+ 0
+(1 row)
+
+SELECT power(float8 '-0.1', float8 'inf');
+ power
+---
+ 0
+(1 row)
+
+SELECT power(float8 '1.1', float8 'inf');
+ power
+--
+ Infinity
+(1 row)
+
+SELECT power(float8 '-1.1', float8 'inf');
+ power
+--
+ Infinity
+(1 row)
+
+SELECT power(float8 '0.1', float8 '-inf');
+ power
+--
+ Infinity
+(1 row)
+
+SELECT power(float8
Re: Warn when parallel restoring a custom dump without data offsets
On Mon, May 25, 2020 at 01:54:29PM -0500, David Gilman wrote:
> > Is it possible to dump to stdout (or pipe to cat or dd) to avoid a new
> > option ?
>
> The underlying IPC::Run code seems to support piping in a cross-platform
> way. I am not a Perl master though and after spending an evening trying
> to get it to work I went with this approach. If you can put me in touch
> with anyone to help me out here I'd appreciate it.
I think you can do what's needed like so:
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -152,10 +152,13 @@ my %pgdump_runs = (
},
defaults_custom_format_no_seek_parallel_restore => {
test_key => 'defaults',
- dump_cmd => [
- 'pg_dump', '-Fc', '-Z6', '--no-sync',
'--disable-seeking',
+ dump_cmd => (
+ [
+ 'pg_dump', '-Fc', '-Z6', '--no-sync',
"--file=$tempdir/defaults_custom_format_no_seek_parallel_restore.dump",
'postgres',
- ],
+ ],
+ "|", [ "cat" ], # disable seeking
+ ),
Also, these are failing intermittently:
t/002_pg_dump.pl .. 1649/6758
# Failed test 'defaults_custom_format_no_seek_parallel_restore: should dump
GRANT SELECT (proname ...) ON TABLE pg_proc TO public'
# at t/002_pg_dump.pl line 3635.
# Review defaults_custom_format_no_seek_parallel_restore results in
/var/lib/pgsql/postgresql.src/src/bin/pg_dump/tmp_check/tmp_test_NqRC
t/002_pg_dump.pl .. 2060/6758
# Failed test 'defaults_dir_format_parallel: should dump GRANT SELECT
(proname ...) ON TABLE pg_proc TO public'
# at t/002_pg_dump.pl line 3635.
# Review defaults_dir_format_parallel results in
/var/lib/pgsql/postgresql.src/src/bin/pg_dump/tmp_check/tmp_test_NqRC
If you can address those, I think this will be "ready for committer".
--
Justin
Re: new heapcheck contrib module (typos)
On 2020-06-12 23:06, Mark Dilger wrote: [v7-0001-Adding-verify_heapam-and-pg_amcheck.patch] [v7-0002-Adding-checks-o...ations-of-hint-bit.patch] I came across these typos in the sgml: --exclude-scheam should be --exclude-schema table should be --table I found this connection problem (or perhaps it is as designed): $ env | grep ^PG PGPORT=6965 PGPASSFILE=/home/aardvark/.pg_aardvark PGDATABASE=testdb PGDATA=/home/aardvark/pg_stuff/pg_installations/pgsql.amcheck/data -- just to show that psql is connecting (via $PGPASSFILE and $PGPORT and $PGDATABASE): -- and showing a table t that I made earlier $ psql SET Timing is on. psql (14devel_amcheck_0612_2f48) Type "help" for help. testdb=# \dt+ t List of relations Schema | Name | Type | Owner | Persistence | Size | Description +--+---+--+-++- public | t| table | aardvark | permanent | 346 MB | (1 row) testdb=# \q I think this should work: $ pg_amcheck -i -t t pg_amcheck: error: no matching tables were found It seems a bug that I have to add '-d testdb': This works OK: pg_amcheck -i -t t -d testdb Is that error as expected? thanks, Erik Rijkers
Re: Infinities in type numeric
Here's a v2 patch:
* Rebased over today's nearby commits
* Documentation changes added
* Sort abbrev support improved per Andrew's suggestions
* Infinities now considered to fail any typmod precision limit,
per discussion with Robert.
regards, tom lane
diff --git a/contrib/jsonb_plperl/jsonb_plperl.c b/contrib/jsonb_plperl/jsonb_plperl.c
index ed361efbe2..b81ba54b80 100644
--- a/contrib/jsonb_plperl/jsonb_plperl.c
+++ b/contrib/jsonb_plperl/jsonb_plperl.c
@@ -227,10 +227,8 @@ SV_to_JsonbValue(SV *in, JsonbParseState **jsonb_state, bool is_elem)
/*
* jsonb doesn't allow infinity or NaN (per JSON
* specification), but the numeric type that is used for the
- * storage accepts NaN, so we have to prevent it here
- * explicitly. We don't really have to check for isinf()
- * here, as numeric doesn't allow it and it would be caught
- * later, but it makes for a nicer error message.
+ * storage accepts those, so we have to reject them here
+ * explicitly.
*/
if (isinf(nval))
ereport(ERROR,
diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c
index e09308daf0..836c178770 100644
--- a/contrib/jsonb_plpython/jsonb_plpython.c
+++ b/contrib/jsonb_plpython/jsonb_plpython.c
@@ -387,14 +387,17 @@ PLyNumber_ToJsonbValue(PyObject *obj, JsonbValue *jbvNum)
pfree(str);
/*
- * jsonb doesn't allow NaN (per JSON specification), so we have to prevent
- * it here explicitly. (Infinity is also not allowed in jsonb, but
- * numeric_in above already catches that.)
+ * jsonb doesn't allow NaN or infinity (per JSON specification), so we
+ * have to reject those here explicitly.
*/
if (numeric_is_nan(num))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("cannot convert NaN to jsonb")));
+ if (numeric_is_inf(num))
+ ereport(ERROR,
+(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("cannot convert infinity to jsonb")));
jbvNum->type = jbvNumeric;
jbvNum->val.numeric = num;
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 3df189ad85..a9ed269e15 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -554,9 +554,9 @@ NUMERIC(precision)
NUMERIC
- without any precision or scale creates a column in which numeric
- values of any precision and scale can be stored, up to the
- implementation limit on precision. A column of this kind will
+ without any precision or scale creates an unconstrained
+ numeric column in which numeric values of any length can be
+ stored, up to the implementation limits. A column of this kind will
not coerce input values to any particular scale, whereas
numeric columns with a declared scale will coerce
input values to that scale. (The SQL standard
@@ -568,10 +568,10 @@ NUMERIC
- The maximum allowed precision when explicitly specified in the
- type declaration is 1000; NUMERIC without a specified
- precision is subject to the limits described in .
+ The maximum precision that can be explicitly specified in
+ a NUMERIC type declaration is 1000. An
+ unconstrained NUMERIC column is subject to the limits
+ described in .
@@ -593,6 +593,11 @@ NUMERIC
plus three to eight bytes overhead.
+
+ infinity
+ numeric (data type)
+
+
NaN
not a number
@@ -604,13 +609,39 @@ NUMERIC
- In addition to ordinary numeric values, the numeric
- type allows the special value NaN, meaning
- not-a-number. Any operation on NaN
- yields another NaN. When writing this value
- as a constant in an SQL command, you must put quotes around it,
- for example UPDATE table SET x = 'NaN'. On input,
- the string NaN is recognized in a case-insensitive manner.
+ In addition to ordinary numeric values, the numeric type
+ has several special values:
+
+Infinity
+-Infinity
+NaN
+
+ These are adapted from the IEEE 754 standard, and represent
+ infinity, negative infinity, and
+ not-a-number, respectively. When writing these values
+ as constants in an SQL command, you must put quotes around them,
+ for example UPDATE table SET x = '-Infinity'.
+ On input, these strings are recognized in a case-insensitive manner.
+ The infinity values can alternatively be spelled inf
+ and -inf.
+
+
+
+ The infinity values behave as per mathematical expectations. For
+ example, Infinity plus any finite value equals
+ Infinity, as does Infinity
+ plus Infinity; but Infinity
+ minus Infinity yields NaN (not a
+ number), because it has no well-defined interpretation. Note that an
+ infinity can only be stored in an unconstrained numeric
+ column, because it notionally exceeds any finite precision limit.
+
+
+
+ The NaN (not a number) value is
Re: hashagg slowdown due to spill changes
On Sat, Jun 13, 2020 at 11:48:09AM -0700, Jeff Davis wrote: On Fri, 2020-06-12 at 17:12 -0700, Andres Freund wrote: Do you think we should tackle this for 13? To me 4cad2534da seems like a somewhat independent improvement to spillable hashaggs. We've gone back and forth on this issue a few times, so let's try to get some agreement before we revert 4cad2534da. I added Robert because he also seemed to think it was a reasonable idea. I can't speak for Robert, but I haven't expected the extra projection would be this high. And I agree with Andres it's not very nice we have to do this even for aggregates with just a handful of groups that don't need to spill. In any case, I think we need to address this somehow for v13 - either we keep the 4cad2534da patch in, or we tweak the cost model to reflect the extra I/O costs, or we project only when spilling. I'm not in a position to whip up a patch soon, though :-( regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: jacana vs -Wimplicit-fallthrough
On 6/13/20 12:25 PM, Tom Lane wrote: > I happened to notice today that, while the rest of the buildfarm is free > of implicit-fallthrough warnings, jacana is emitting a whole boatload of > them. It looks like it must have a different idea of which spellings of > the "fall through" comment are allowed. Could you check its documentation > to see what it claims to allow? > > There doesn't seem to be any docco with it. What's odd is that fairywren is supposedly using the identical compiler, but it's on msys2 while jacana is msys1. I can't see why that should make a difference, though. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: hashagg slowdown due to spill changes
On Fri, 2020-06-12 at 17:12 -0700, Andres Freund wrote: > Do you think we should tackle this for 13? To me 4cad2534da seems > like a > somewhat independent improvement to spillable hashaggs. We've gone back and forth on this issue a few times, so let's try to get some agreement before we revert 4cad2534da. I added Robert because he also seemed to think it was a reasonable idea. Regards, Jeff Davis
Re: what can go in root.crt ?
On Fri, Jun 12, 2020 at 04:17:56PM -0400, Chapman Flack wrote: > On 06/12/20 15:13, Bruce Momjian wrote: > > Without that ability, every client would need be changed as soon as the > > server certificate was changed. Allowing intermediate certificates to > > function as root certificates would fix that problem. When the > > non-trusted CA changes your certificate, you are going to have the same > > problem updating everything at once. > > There seems to be a use of language here that works to make the picture > muddier rather than clearer. > > I mean the use of "trusted"/"non-trusted" as if they somehow mapped onto > "self-signed"/"not self-signed" (unless you had some other mapping in mind > there). I meant you trust your local/intermediate CA, but not the root one. > That's downright ironic, as a certificate that is self-signed is one that > carries with it the absolute minimum grounds for trusting it: precisely > zero. There can't be any certificate you have less reason to trust than > a self-signed one. Self-signed certs can certainly be trusted by the creator. Organizations often create self-signed certs that are trusted inside the organization. > As far as the TLS client is concerned, the endorsement that counts is > still the local one, that it has been placed in the local file by the > admin responsible for deciding what this client should trust. The fact > that somebody else vouched for it too is no reason for this client > to trust it, but is also no reason for this client not to trust it. > It is certainly in no way less to be trusted than a cert signed only > by itself. Yes, I see your point in that the intermediate has more validity than a self-signed certificate, though that extra validity is useless in the use-case we are describing. > But once you have followed those steps and arrived at a cert that > was placed in your trust store by the admin, it's unnecessary and > limiting to insist arbitrarily on other properties of the cert you > found there. Well, I can see the use-case for what you are saying, but I also think it could lead to misconfiguration. Right now, Postgres uses the client root.cert, which can contain intermediates certs, and the server-provided cert, which can also contain intermediates shipped to the client, to try to check for a common root: https://www.postgresql.org/docs/13/ssl-tcp.html What you are suggesting is that we take the server chain and client chain and claim success when _any_ cert matches between the two, not just the root one. I can see that working but I can also imagine people putting only intermediate certs in their root.cert and not realizing that they are misconfigured since they might want to expire the intermediate someday or might want to trust a different intermediate from the same root. Frankly, we really didn't even documention how to handle intermediate certificates until 2018, which shows how obscure this security stuff can be: https://git.postgresql.org/gitweb/?p=postgresql.git=commitdiff=815f84aa16 Do we want to allow such cases, or is the risk of misconfiguration too high? I am thinking it is the later. I think we could have a libpq parameter that allowed it, but is there enough demand to add it since it would be a user-visible API? -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Uninitialized-variable warnings in nbtinsert.c
Peter Geoghegan writes: > On Sat, Jun 13, 2020 at 9:17 AM Tom Lane wrote: >> I scraped the buildfarm's compiler warnings today, as I do from >> time to time, and I noticed that half a dozen animals that normally >> don't report any uninitialized-variable warnings are complaining >> about "curitup" in _bt_doinsert. > (Clearly you meant _bt_check_unique(), not _bt_doinsert().) Ah, right. I was looking at calliphoridae's complaint when I wrote that: In file included from /home/andres/build/buildfarm-calliphoridae/HEAD/pgsql.build/../pgsql/src/backend/access/nbtree/nbtinsert.c:18: /home/andres/build/buildfarm-calliphoridae/HEAD/pgsql.build/../pgsql/src/backend/access/nbtree/nbtinsert.c: In function \xe2\x80\x98_bt_doinsert\xe2\x80\x99: but it must have inlined some stuff first. (A lot of the other complainers are fingering inline functions in nbtree.h, which is even less helpful.) > Thanks for bringing this to my attention. I'll push a commit that > initializes curitup shortly, targeting both the v13 branch and the > master branch. Thanks! regards, tom lane
Re: Uninitialized-variable warnings in nbtinsert.c
On Sat, Jun 13, 2020 at 9:17 AM Tom Lane wrote: > I scraped the buildfarm's compiler warnings today, as I do from > time to time, and I noticed that half a dozen animals that normally > don't report any uninitialized-variable warnings are complaining > about "curitup" in _bt_doinsert. (Clearly you meant _bt_check_unique(), not _bt_doinsert().) > The simplest fix would be to just initialize curitup to NULL in its > declaration. But perhaps there's a better way. Thanks for bringing this to my attention. I'll push a commit that initializes curitup shortly, targeting both the v13 branch and the master branch. -- Peter Geoghegan
jacana vs -Wimplicit-fallthrough
I happened to notice today that, while the rest of the buildfarm is free of implicit-fallthrough warnings, jacana is emitting a whole boatload of them. It looks like it must have a different idea of which spellings of the "fall through" comment are allowed. Could you check its documentation to see what it claims to allow? regards, tom lane
Uninitialized-variable warnings in nbtinsert.c
I scraped the buildfarm's compiler warnings today, as I do from time to time, and I noticed that half a dozen animals that normally don't report any uninitialized-variable warnings are complaining about "curitup" in _bt_doinsert. We traditionally ignore such warnings from compilers that have demonstrated track records of being stupid about it, but when a reasonably modern compiler shows such a warning I think we ought to suppress it. Right now the counts of uninitialized-variable warnings in HEAD builds are 1 calliphoridae 1 chipmunk 1 coypu 1 culicidae 2 curculio 1 frogfish 25 locust 24 prairiedog (curculio is additionally whining about "curitemid" in the same function.) So you can see that this one issue has greatly expanded the set of compilers that are unhappy. I can see their point too -- it requires some study to be sure we are assigning curitup before dereferencing it. The simplest fix would be to just initialize curitup to NULL in its declaration. But perhaps there's a better way. regards, tom lane
Re: Definitional issue: stddev_pop (and related) for 1 input
Dean Rasheed writes: > The patch looks reasonable, except I wonder if all compilers are smart > enough to realise that totCount is always initialised. I think they should be, since that if-block ends with a return; the only way to get to the use of totCount is for both parts of the first if-condition to be executed. In any case, we do have an effective policy of ignoring uninitialized-variable warnings from very old/stupid compilers. locust and prairiedog, which I think use the same ancient gcc version, emit a couple dozen such warnings. If they are the only ones that complain about this new code, I'll not worry. Thanks for looking at the patch! regards, tom lane
Re: pg_upgrade fails if vacuum_defer_cleanup_age > 0
On Wed, Jun 10, 2020 at 04:07:05PM +0200, Laurenz Albe wrote: > A customer's upgrade failed, and it took me a while to > figure out that the problem was that they had set > "vacuum_defer_cleanup_age=1" on the new cluster. > > The consequence was that the "vacuumdb --freeze" that > takes place before copying commit log files failed to > freeze "pg_database". > That caused later updates to the table to fail with > "Could not open file "pg_xact/": No such file or directory." > > I think it would increase the robustness of pg_upgrade to > force "vacuum_defer_cleanup_age" to 0 on the new cluster. Wow, I can see setting "vacuum_defer_cleanup_age" to a non-zero value as causing big problems for pg_upgrade, and being hard to diagnose. I will apply this patch to all branches. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Resetting spilled txn statistics in pg_stat_replication
On 2020/06/13 14:23, Amit Kapila wrote: On Fri, Jun 12, 2020 at 6:11 PM Magnus Hagander wrote: On Fri, Jun 12, 2020 at 10:23 AM Amit Kapila wrote: The problem with "lifetime of a process" is that it's not predictable. A replication process might "bounce" for any reason, and it is normally not a problem. But if you suddenly lose your stats when you do that, it starts to matter a lot more. Especially when you don't know if it bounced. (Sure you can look at the backend_start time, but that adds a whole different sets of complexitites). It is not clear to me what is a good way to display the stats for a process that has exited or bounced due to whatever reason. OTOH, if we just display per-slot stats, it is difficult to imagine how the user can make any sense out of it or in other words how such stats can be useful to users. If we allow users to set logical_decoding_work_mem per slot, maybe the users can tune it directly from the stats? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Definitional issue: stddev_pop (and related) for 1 input
On Fri, 12 Jun 2020 at 20:53, Tom Lane wrote: > > I wrote: > > Before v12, stddev_pop() had the following behavior with just a > > single input value: > > ... > > As of v12, though, all three cases produce 0. I am not sure what > > to think about that with respect to an infinity input, but I'm > > quite sure I don't like it for NaN input. > > While I'm still not sure whether there's an academic argument that > zero is a reasonable stddev value for a single input that is Inf, > it seems to me that backwards compatibility is a sufficient reason > for going back to producing NaN for that. Yeah, it was an oversight, not considering that case. I think the academic argument could equally well be made that the result should be NaN if any input is Inf or NaN, even if there's only one input (it's effectively "Inf - Inf" or "NaN - NaN"), so I agree that backwards compatibility clinches it. > Hence, attached are some proposed patches. 0001 just adds test > cases demonstrating the current behavior; then 0002 makes the > proposed code change. It's easy to check that the test case results > after 0002 match what v11 produces. Those both look reasonable to me. > 0003 deals with a different problem which I noted in [1]: the numeric > variants of var_samp and stddev_samp also do the wrong thing for a > single special input. Their disease is that they produce NaN for a > single NaN input, where it seems more sensible to produce NULL. > At least, NULL is what we get for the same case with the float > aggregates, so we have to change one or the other set of functions > if we want consistency. Hmm, yeah it's a bit annoying that they're different. NULL seems like the more logical result -- sample standard deviation isn't defined for a sample of 1, so why should it be different if that one value is NaN. The patch looks reasonable, except I wonder if all compilers are smart enough to realise that totCount is always initialised. > I propose back-patching 0001/0002 as far as v12, since the failure > to match the old outputs seems like a pretty clear bug/regression. > However, I'd be content to apply 0003 only to HEAD. That misbehavior > is very ancient, and the lack of complaints suggests that few people > really care about this fine point. Makes sense. Regards, Dean
Re: Add tap test for --extra-float-digits option
> That's more of an habit to look around, find similar patterns and the > check if these are covered. > > I have applied your patch, and you may want to be careful about a > couple of things: > - Please avoid top-posting on the mailing lists: > https://en.wikipedia.org/wiki/Posting_style#Top-posting > Top-posting breaks the logic of a thread. > - Your patch format is good. When sending a new version of the patch, > it may be better to send things as a complete diff on the master > branch (or the branch you are working on), instead of just sending one > patch that applies on top of something you sent previously. Here for > example your patch 0002 applied on top of 0001 that was sent at the > top of the thread. We have also guidelines about patch submission: > https://wiki.postgresql.org/wiki/Submitting_a_Patch > > Thanks! > -- > Michael Hi Michael First of all, thank you for merging my patch. And I'm sorry, I should have been more careful about it. Next time I will follow format. And there is something I will tell you Would you mind if I ask you specify my author info with --author on the git commit? The new contributor can get involved in the PostgreSQL project. When they sent a patch and it was merged to the main repository, it'd be better to keep the author info on the git commit, IMHO. Because many opensource hackers who interested in PostgreSQL project can want to keep a record of author info on commits they wrote. Otherwise, contribution records can not be found by 'git shortlog -sn' and GitHub and OpenHub cannot track their opensource contribution records... So what about using --author for PostgreSQL contributors when merging their patches? like the Linux Kernel project https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8a16c09edc58982d56c49ab577fdcdf830fbc3a5 If so, many contributors would be highly encouraged. Thanks, Dong Wook
Re: POC and rebased patch for CSN based snapshots
On Fri, Jun 12, 2020 at 3:11 PM movead...@highgo.ca wrote: > > Hello hackers, > > Currently, I do some changes based on the last version: > 1. Catch up to the current commit (c2bd1fec32ab54). > 2. Add regression and document. > 3. Add support to switch from xid-base snapshot to csn-base snapshot, > and the same with standby side. > AFAIU, this patch is to improve scalability and also will be helpful for Global Snapshots stuff, is that right? If so, how much performance/scalability benefit this patch will have after Andres's recent work on scalability [1]? [1] - https://www.postgresql.org/message-id/20200301083601.ews6hz5dduc3w2se%40alap3.anarazel.de -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Parallel Seq Scan vs kernel read ahead
On Fri, Jun 12, 2020 at 11:28 PM Robert Haas wrote: > > On Thu, Jun 11, 2020 at 4:54 PM David Rowley wrote: > > I think someone at some point is not going to like the automatic > > choice. So perhaps a reloption to allow users to overwrite it is a > > good idea. -1 should most likely mean use the automatic choice based > > on relation size. I think for parallel seq scans that filter a large > > portion of the records most likely need some sort of index, but there > > are perhaps some genuine cases for not having one. e.g perhaps the > > query is just not run often enough for an index to be worthwhile. In > > that case, the performance is likely less critical, but at least the > > reloption would allow users to get the old behaviour. > > Let me play the devil's advocate here. I feel like if the step size is > limited by the relation size and there is ramp-up and ramp-down, or > maybe even if you don't have all 3 of those but perhaps say 2 of them, > the chances of there being a significant downside from using this seem > quite small. At that point I wonder whether you really need an option. > It's true that someone might not like it, but there are all sorts of > things that at least one person doesn't like and one can't cater to > all of them. > > To put that another way, in what scenario do we suppose that a > reasonable person would wish to use this reloption? > The performance can vary based on qualification where some workers discard more rows as compared to others, with the current system with step-size as one, the probability of unequal work among workers is quite low as compared to larger step-sizes. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: TAP tests and symlinks on Windows
On Fri, Jun 12, 2020 at 02:02:52PM +0200, Juan José SantamarÃa Flecha wrote: > The first thing that comes to mind is adding an option to vcregress to > choose whether symlinks will be tested or skipped, would that be an > acceptable solution? My take would be to actually enforce that as a requirement for 14~ if that works reliably, and of course not backpatch that change as that's clearly an improvement and not a bug fix. It would be good to check the status of each buildfarm member first though. And I would need to also check my own stuff to begin with.. -- Michael signature.asc Description: PGP signature
