Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance
> "Lætitia" == Lætitia Avrot writes: [snip patch] The spec doesn't require the inverse functions (asinh, acosh, atanh), but surely there is no principled reason to omit them? -- Andrew (irc:RhodiumToad)
Re: Removing \cset from pgbench
Hola Alvaro, In reply to https://postgr.es/m/alpine.DEB.2.21.1901102211350.27692@lancre wherein Fabien wrote: I'm not very happy with the resulting syntax, but IMO the feature is useful. My initial design was to copy PL/pgSQL "into" with some "\into" orthogonal to \; and ;, but the implementation was not especially nice and I was told to use psql's \gset approach, which I did. If we do not provide \cset, then combined queries and getting results are not orthogonal, although from a performance testing point of view an application could do both, and the point is to allow pgbench to test the performance impact of doing that. We very briefly discussed this topic at FOSDEM pgday. My feeling on the general opinion is that there's appreciation for \gset in general, but that people feel that \cset is too much cruft to take for not enough additional added value (compared to great value delivered by \gset). What I'm going to do now is to write a patch to remove the \cset part of the commit and post it, intending to push at some point next week. If somebody has grown really fond of \cset, they can work on a patch to implement it properly, which it isn't now. My usless 0.02€: I'm willing to implement it properly. Do you have any advice? For me the issue comes from the fact that postgres silently ignores empty queries, i.e. on: SELECT 1 \; \; SELECT 2; Three queries are sent, the middle one empty, but two results returned (PGRES_TUPLES_OK, PGRES_TUPLES_OK) instead of (PGRES_TUPLES_OK, PGRES_EMPTY_QUERY, PGRES_TUPLES_OK). However, on: ; You do have an PGRES_EMPTY_QUERY result. How to deal cleanly and simply with that? Removing the "skip empty query" optimizations would remove the "it does not work with empty queries" documentation warning that I understood Tom complains about. It may have a little impact on "psql" implementation to keep the "show the last non-empty result" behavior on combined queries. Providing non-orthogonal features (eg combined queries cannot use some options, such as -M in pgbench) is as much substandard as awkward optimizations like the above. An alternative is to detect whether a query is empty, but that complicates the lexing phase to detect "\; \;" and adjust the query count to match variable setting to their queries: one version of the patch kept the position of embedded semi-colons so as to be able to check that only spaces appear (comments are removed by the lexer). Because this was seen as awkward code working around awkward protocol behavior for an unlikely corner case, it was removed in the reviewing process and the limitation was documented instead. -- Fabien.
Re: Synchronize with imath upstream
> "Noah" == Noah Misch writes: Noah> - # -Wdeclaration-after-statement isn't applicable for C++ Noah> + # -Wdeclaration-after-statement isn't applicable for C++. Specific C files Noah> + # disable it, so AC_SUBST the negative form. Noah> + PERMIT_DECLARATION_AFTER_STATEMENT= Noah> + if test x"$save_CFLAGS" != "$CFLAGS"; then Missing "x" here? + if test x"$save_CFLAGS" != x"$CFLAGS"; then -- Andrew (irc:RhodiumToad)
Re: pg_dump multi VALUES INSERT
On Sat, 2 Feb 2019 at 21:26, Fabien COELHO wrote: > I do not understand why dump_inserts declaration has left the "flags for > options" section. I moved that because it's no longer just a flag. It now stores an int value. > I'd suggest not to rely on "atoi" because it does not check the argument > syntax, so basically anything is accepted, eg "1O" is 1; Seems like it's good enough for --jobs and --compress. Do you think those should be changed too? or what's the reason to hold --rows-per-insert to a different standard? > There is a test, that is good! Charater "." should be backslashed in the > regexpr. Yeah, you're right. I wonder if we should fix the test of them in another patch. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Synchronize with imath upstream
> "Noah" == Noah Misch writes:
>> I found it much simpler to strip out -Wdeclaration-after-statement
>> instead:
>>
>> $(RYU_OBJS): override CFLAGS := $(filter-out
>> -Wdeclaration-after-statement,$(CFLAGS))
Noah> The -Wno-declaration-after-statement approach takes eight lines
Noah> of code, and the filter-out approach takes one. On the other
Noah> hand, using $(filter-out) changes any runs of whitespace to
Noah> single spaces ("$(filter-out foo,a b c)" yields "a b c"). We do
Noah> risk that with CPPFLAGS and LDFLAGS in a few places. I don't want
Noah> to proliferate that practice, because it changes semantics of
Noah> CFLAGS containing -DFOO="arbitrary text".
In that case I propose that we avoid the whole issue by removing
-Wdeclaration-after-statement entirely.
So far, expressed opinions have run about 4:2 in favour of allowing
mixed declarations and code.
--
Andrew (irc:RhodiumToad)
Re: Able to do ALTER DEFAULT PRIVILEGES from a user who is not the owner
Andrew, Another question, If the user student is not the owner of the Schema(additional) and has only USAGE / no privileges, How come it is able to modify permissions at schema level? - -- Thanks, Rajan. -- Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: Able to do ALTER DEFAULT PRIVILEGES from a user who is not the owner
> "rajan" == rajan writes: rajan> Andrew, rajan> Another question, If the user student is not the owner of the rajan> Schema(additional) and has only USAGE / no privileges, How come rajan> it is able to modify permissions at schema level? Because it's not modifying anything that affects any other user. If "student" can't create objects in schema "additional", then the default has no effect (since it applies only to objects created by "student"); if those permissions are later granted, then the previously set default still applies. -- Andrew (irc:RhodiumToad)
Re: Synchronize with imath upstream
On Sun, Feb 03, 2019 at 06:01:51AM +, Andrew Gierth wrote:
> > "Noah" == Noah Misch writes:
>
> Noah> If the compiler supports -Wdeclaration-after-statement, I add
> Noah> -Wno-declaration-after-statement for imath.c.
>
> I found it much simpler to strip out -Wdeclaration-after-statement
> instead:
>
> $(RYU_OBJS): override CFLAGS := $(filter-out
> -Wdeclaration-after-statement,$(CFLAGS))
The -Wno-declaration-after-statement approach takes eight lines of code, and
the filter-out approach takes one. On the other hand, using $(filter-out)
changes any runs of whitespace to single spaces ("$(filter-out foo,ab c)"
yields "a b c"). We do risk that with CPPFLAGS and LDFLAGS in a few places.
I don't want to proliferate that practice, because it changes semantics of
CFLAGS containing -DFOO="arbitrarytext".
UNIQUE_PATH_NOOP is dead code?
I happened to notice while looking at code coverage reports that (1) the portions of create_unique_path() that report that the input relation is already known unique are no longer reached in our regression tests; (2) the loop in relation_has_unique_index_for() that deals with an exprlist and oprlist supplied by the caller is never iterated. This is related to (1) because create_unique_path() is the only caller that supplies such lists. While I've not looked at it really closely, I suspect that this is not just an indication of inadequate testing, but that those sections are actually dead code now. create_unique_path() is only used on a relation that is the RHS of a semijoin, and light testing suggests that in the cases that create_unique_path() knows about, analyzejoins.c will have strength-reduced the semijoin to a plain join. All this code predates the introduction of that phase, so it was useful when written, but maybe it's not anymore. Not planning to do anything about this observation right at this moment, but perhaps we could remove some code here. regards, tom lane
Synchronize with imath upstream
pgcrypto bundles a copy of the imath library for arbitrary-precision integer
arithmetic in non-SSL builds. Upstream fixed buffer overflows through the
years, and commit 8b59672 brought those fixes into PostgreSQL. In master, I
would like to fully resynchronize with fresh imath 1.29. We're better off
naively tracking upstream than reactively maintaining a twelve-year-old
snapshot of upstream.
imath1.29-raw-sync-v1.patch is the result of copying new imath.c and imath.h
into place, removing "#ifdef __cplusplus" blocks that upset pgindent, running
pgindent, and filtering through "unexpand -t4 --first-only".
imath1.29-pgedits-v1.patch then restores PostgreSQL-specific changes. I would
squash these together for eventual commit, since the intermediate state is
broken, but it should ease review to see them separately. I did not keep the
INVERT_COMPARE_RESULT() change from c87cb5f; the domain of the comparisons in
question is {-1,0,1}, controlled entirely by code in imath.c.
Upstream has fixed bugs over the years, but I am not specifically aware of any
represented fix here that affects pgcrypto. Most suspicious to me are the
division fixes, which could affect our pgp_elgamal_{en,de}crypt(). You can
examine https://github.com/creachadair/imath/blob/master/ChangeLog for all
changes between imath-1.3 and imath-1.29.
Like PostgreSQL, imath now assumes C99. Unlike PostgreSQL, it has adopted
mixed declarations and code; our -Wdeclaration-after-statement would add
sixty-two warnings. If the compiler supports -Wdeclaration-after-statement, I
add -Wno-declaration-after-statement for imath.c.
Thanks,
nm
diff --git a/contrib/pgcrypto/imath.c b/contrib/pgcrypto/imath.c
index b94a51b..96be668 100644
--- a/contrib/pgcrypto/imath.c
+++ b/contrib/pgcrypto/imath.c
@@ -1,90 +1,69 @@
-/* imath version 1.3 */
/*
- Name:imath.c
- Purpose: Arbitrary precision integer arithmetic routines.
- Author: M. J. Fromberger
Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance
Hi Alvaro,
Thank you so much for taking the time to review the patch and for taking
the time again to sort things
out with me this evening.
> I see that in dtanh() you set errno to 0 before calling tanh(), but 1)
> you don't check for it afterwards (seems like you should be checking for
> ERANGE, as well as checking the return value for isinf()), and 2) you
> don't do that in dsinh() and dcosh() and I'm not quite sure I see why.
> What's up with that?
>
>
At the time I wrote that patch, I tried to include errno testing.Then, I
think again and
came back with the wrong idea everything would be fine.
By re-reading math.h documentation, it is now clear that the three
functions can raise a
ERANGE error.
There are two cases :
- range error due to overflow occurs.
- range error occurs due to underflow. In that case, the correct result
(after rounding) is returned. So I assume we can ignore that case.
For sinh and cosh, we can have both cases and we added support for overflow.
For tanh, the only possible case is underflow and then, the result is
correct.
We included comments to explain errno handling in those functions.
Cheers,
Lætitia
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4930ec17f6..30ecf496ca 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -896,6 +896,19 @@
2
+
+
+
+ log10
+
+log10(dp or numeric)
+
+ (same as input)
+ base 10 logarithm
+ log10(100.0)
+ 2
+
+
log(b numeric,
x numeric)
@@ -1147,7 +1160,7 @@
- Finally, shows the
+shows the
available trigonometric functions. All trigonometric functions
take arguments and return values of type double
precision. Each of the trigonometric functions comes in
@@ -1311,8 +1324,66 @@
-
+
+ Finally, shows the
+ available hyperbolic functions. All hyperbolic functions apply to
+ hyperbolic angles expressed in hyperbolic radians.
+
+
+Hyperbolic Functions
+
+
+
+
+ Function
+ Return Type
+ Description
+ Example
+ Result
+
+
+
+
+
+
+ sinh
+
+sinh(dp)
+
+ dp
+ hyperbolic sine
+ sinh(0)
+ 0
+
+
+
+
+ cosh
+
+cosh(dp)
+
+ dp
+ hyperbolic cosine
+ cosh(0)
+ 1
+
+
+
+
+ tanh
+
+tanh(dp)
+
+ dp
+ hyperbolic tangent
+ tanh(0)
+ 0
+
+
+
+
+
String Functions and Operators
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 117ded8d1d..29a6ba0859 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2375,6 +2375,77 @@ radians(PG_FUNCTION_ARGS)
PG_RETURN_FLOAT8(float8_mul(arg1, RADIANS_PER_DEGREE));
}
+/* == HYPERBOLIC FUNCTIONS == */
+
+/*
+ * dsinh - returns the hyperbolic sine of arg1
+ */
+Datum
+dsinh(PG_FUNCTION_ARGS)
+{
+ float8 arg1 = PG_GETARG_FLOAT8(0);
+ float8 result;
+
+ errno = 0;
+ result = sinh(arg1);
+
+ /*
+ * if an ERANGE error occurs, it means there is an overflow. For sinh, it
+ * can be either -infinite or infinite, depending on the sign of arg1.
+ */
+ if (errno == ERANGE)
+ {
+ if (arg1 < 0)
+ result = -get_float8_infinity();
+ else
+ result = get_float8_infinity();
+ }
+
+ check_float8_val(result, isinf(arg1), true);
+ PG_RETURN_FLOAT8(result);
+}
+
+
+/*
+ * dcosh - returns the hyperbolic cosine of arg1
+ */
+Datum
+dcosh(PG_FUNCTION_ARGS)
+{
+ float8 arg1 = PG_GETARG_FLOAT8(0);
+ float8 result;
+
+ errno = 0;
+ result = cosh(arg1);
+
+ /*
+ * if an ERANGE error occurs, it means there is an overflow. As cosh is
+ * always positive, it always means the result is positive infinite.
+ */
+ if (errno == ERANGE)
+ result = get_float8_infinity();
+
+ check_float8_val(result, isinf(arg1), false);
+ PG_RETURN_FLOAT8(result);
+}
+
+/*
+ * dtanh - returns the hyperbolic tangent of arg1
+ */
+Datum
+dtanh(PG_FUNCTION_ARGS)
+{
+ float8 arg1 = PG_GETARG_FLOAT8(0);
+ float8 result;
+
+ /*
+ * For tanh, we don't have errno check because it nevers overflows.
+ */
+ result = tanh(arg1);
+
+ check_float8_val(result, false, true);
+ PG_RETURN_FLOAT8(result);
+}
/*
* drandom - returns a random number
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index b8de13f03b..ecd05949a2 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -2617,6 +2617,9 @@
{ oid => '1340', descr => 'base 10 logarithm',
proname => 'log', prorettype => 'float8', proargtypes => 'float8',
prosrc => 'dlog10' },
+{ oid => '1364', descr => 'base 10 logarithm',
+ proname => 'log10', prorettype => 'float8', proargtypes => 'float8',
+ prosrc => 'dlog10' },
{ oid
Re: ATTACH/DETACH PARTITION CONCURRENTLY
On Sat, 2 Feb 2019 at 09:31, Robert Haas wrote: > After having written this code, I'm still torn about whether to go > further with this design. On the one hand, this is such boilerplate > code that it's kinda hard to imagine it having too many more bugs; on > the other hand, as you can see, it's a non-trivial amount of code to > add without a real clear reason, and I'm not sure we have one, even > though in the abstract it seems like a better way to go. I think we do need to ensure that the PartitionDesc matches between worker and leader. Have a look at choose_next_subplan_for_worker() in nodeAppend.c. Notice that a call is made to ExecFindMatchingSubPlans(). -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: initdb --allow-group-access behaviour in windows
On Sat, Feb 02, 2019 at 08:50:14AM +0200, David Steele wrote: > How about: > > +files created by initdb. This option is ignored > +on Windows, which does not support > +POSIX-style group permissions. Fine for me. Anybody else has an opinion to offer? -- Michael signature.asc Description: PGP signature
Re: Refactoring IndexPath representation of index conditions
Andres Freund writes: > On 2019-02-02 11:29:10 -0500, Tom Lane wrote: >> I think that the original idea here was that we should do as little >> work as possible "up front", since most index paths will get discarded >> before we reach createplan.c. But to the extent that that was valid >> at all, it's gotten overtaken by circumstances. In particular, >> postponing work to expand_indexqual_conditions (which is called by >> create_index_path) is just stupid, because these days we typically >> call that several times with the same index conditions. It's really >> dubious that postponing commutation to createplan.c is a net win either, > It seems your approach isn't particularly in contradiction to the > stated historical goal. We could create the new struct, but just not > populate it eagerly, right? Not really. A large part of the point here is to record what match_clause_to_indexcol has found out rather than have to rediscover it later (perhaps repeatedly). Concretely, the main extra cost that I've added in this patch (that wouldn't have been paid anyway by the time we've finished create_index_path) is creation of a commuted OpExpr and a RestrictInfo for it, in cases where we have an operator that matches the index but the index column is on the right. I was able to reduce that cost quite a bit by adding a bespoke "commute_restrictinfo" function, but it's still a few palloc's more than we did before. However, I think the benefits are substantial: subsequent code can uniformly assume that indexquals have indexcol-on-left, rather than having to figure that out repeatedly, and we can avoid repeated syscache lookups to find out the OID of the commutator operator. The patch as posted is still doing one more commutator lookup than it needs to, but that'll be fixed by folding expand_indexqual_conditions and match_clause_to_indexcol into one step. Also I'm not sure if I've found all the places that are expending now-useless effort for indexcol-on-right or not; I've not looked exhaustively. So at the worst this choice seems to be a wash in terms of cycles, but I have hopes that it'll be a win before all the dust settles. In any case I think it makes things simpler and clearer, which is worth a good deal. Another idea that I looked into is to not create RestrictInfos for derived indexqual clauses, with the hope that that would further reduce the added overhead for the commuted-clause case. However that crashed and burned when I found out that the extended-stats machinery punts when given a bare clause rather than a RestrictInfo. It could possibly be fixed to not do that, but it looks like the consequences would be extra lookups that'd probably cost more than we saved by omitting the RestrictInfo. Also, having RestrictInfos means that we can cache selectivity estimates across multiple calls. I'm not entirely sure how much that matters in this context, but it's probably not negligible. regards, tom lane
Re: Changing SQL Inlining Behaviour (or...?)
Further question about this ... I'm thinking about exactly when indxpath.c should interrogate extensions' planner support functions. Since it'll cost some cycles to look up and call such functions, we don't want to do it if there's little or no chance of getting an index match. Currently, what match_clause_to_indexcol() does first for an operator clause is to see if match_index_to_operand() succeeds for either operator input; if so it investigates more closely, if not it ignores the clause. I'm thinking of maintaining that rule for operators and adding the same rule for function clauses appearing in WHERE. Therefore, an extension would only have the opportunity to add a lossy index clause when some argument of its function or operator is the same Var or expression that is indexed. Is that going to be sufficient, or are there weird cases where an index condition could be built but none of the top-level function or operator inputs exactly match the index column? The cases you've shown us, where you transform a function with argument x into an indexable operator with the same argument x as the potentially-indexed side, don't look like they'd have any trouble with that restriction. But I'm worried that maybe I'm missing something. regards, tom lane
Re: Refactoring IndexPath representation of index conditions
Hi, On 2019-02-02 11:29:10 -0500, Tom Lane wrote: > I think that the original idea here was that we should do as little > work as possible "up front", since most index paths will get discarded > before we reach createplan.c. But to the extent that that was valid > at all, it's gotten overtaken by circumstances. In particular, > postponing work to expand_indexqual_conditions (which is called by > create_index_path) is just stupid, because these days we typically > call that several times with the same index conditions. It's really > dubious that postponing commutation to createplan.c is a net win either, It seems your approach isn't particularly in contradiction to the stated historical goal. We could create the new struct, but just not populate it eagerly, right? > Thoughts? If there's not objections I'd like to push this soon. Seems reasonable from a very very quick skim. Andres
Re: Early WIP/PoC for inlining CTEs
I wrote:
> I propose that we implement and document this as
> WITH ctename AS [ MATERIALIZE { ON | OFF } ] ( query )
> which is maybe a bit clunky but not awful, and it would leave room
> to generalize it to "AS [ optionname optionvalue [ , ... ] ]" if we
> ever need to. Looking at the precedent of e.g. EXPLAIN, we could
> probably allow just "MATERIALIZE" as well, with the boolean value
> defaulting to true.
In hopes of moving things along, here's a version of the patch that
does it like that. This demonstrates that, in fact, we can accept
"keyword [value] [, ...]" style options without any parens and
there's no syntax conflict. We'd have to work a bit harder on the
actual code in gram.y if we wanted to handle multiple options,
but the Bison productions will work.
There's nothing particularly stopping us from accepting
"materialized" with a D in this syntax, instead of or in addition
to "materialize"; though I hesitate to mention it for fear of
another round of bikeshedding.
After further reflection I really don't like Andrew's suggestion
that we not document the rule that multiply-referenced CTEs won't
be inlined by default. That would be giving up the principle
that WITH calculations are not done multiple times by default,
and I draw the line at that. It's an often-useful behavior as
well as one that's been documented from day one, so I do not accept
the argument that we might someday override it on the basis of
nothing but planner cost estimates.
regards, tom lane
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index ea2e4bc..9905593 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2927,6 +2927,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
/* we store the string name because RTE_CTE RTEs need it */
APP_JUMB_STRING(cte->ctename);
+APP_JUMB(cte->ctematerialized);
JumbleQuery(jstate, castNode(Query, cte->ctequery));
}
break;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index b3894d0..226ba56 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1888,7 +1888,7 @@ SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
-- join in CTE
EXPLAIN (VERBOSE, COSTS OFF)
-WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
+WITH t (c1_1, c1_3, c2_1) AS MATERIALIZE ON (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
QUERY PLAN
-
Limit
@@ -1905,7 +1905,7 @@ WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2
Output: t.c1_1, t.c2_1, t.c1_3
(12 rows)
-WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
+WITH t (c1_1, c1_3, c2_1) AS MATERIALIZE ON (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
c1_1 | c2_1
--+--
101 | 101
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index f438165..6399b8b 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -493,8 +493,8 @@ SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR SHARE;
-- join in CTE
EXPLAIN (VERBOSE, COSTS OFF)
-WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
-WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
+WITH t (c1_1, c1_3, c2_1) AS MATERIALIZE ON (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
+WITH t (c1_1, c1_3, c2_1) AS MATERIALIZE ON (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
-- ctid with whole-row reference
EXPLAIN (VERBOSE, COSTS OFF)
SELECT t1.ctid, t1, t2, t1.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3,
Re: Able to do ALTER DEFAULT PRIVILEGES from a user who is not the owner
THanks for the response, Andrew. - -- Thanks, Rajan. -- Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: New vacuum option to do only freezing
On 2019-Feb-01, Bossart, Nathan wrote:
> IMHO we could document this feature at a slightly higher level without
> leaving out any really important user-facing behavior. Here's a quick
> attempt to show what I am thinking:
>
> With this option, VACUUM skips all index cleanup behavior and
> only marks tuples as "dead" without reclaiming the storage.
> While this can help reclaim transaction IDs faster to avoid
> transaction ID wraparound (see Section 24.1.5), it will not
> reduce bloat.
Hmm ... don't we compact out the storage for dead tuples? If we do (and
I think we should) then this wording is not entirely correct.
> Note that this option is ignored for tables
> that have no indexes. Also, this option cannot be used in
> conjunction with the FULL option, since FULL requires
> rewriting the table.
I would remove the "Also," in there, since it seems to me to give the
wrong impression about those two things being similar, but they're not:
one is convenient behavior, the other is a limitation.
> + /* Notify user that DISABLE_INDEX_CLEANUP option is ignored */
> + if (!vacrelstats->hasindex && (options & VACOPT_DISABLE_INDEX_CLEANUP))
> + ereport(NOTICE,
> + (errmsg("DISABLE_INDEX_CLEANUP is ignored
> because table \"%s\" does not have index",
> +
> RelationGetRelationName(onerel;
>
> It might make more sense to emit this in lazy_scan_heap() where we
> determine the value of skip_index_vacuum.
Why do we need a NOTICE here? I think this case should be silent.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Removing \cset from pgbench
Hello In reply to https://postgr.es/m/alpine.DEB.2.21.1901102211350.27692@lancre wherein Fabien wrote: > I'm not very happy with the resulting syntax, but IMO the feature is useful. > My initial design was to copy PL/pgSQL "into" with some "\into" orthogonal > to \; and ;, but the implementation was not especially nice and I was told > to use psql's \gset approach, which I did. > > If we do not provide \cset, then combined queries and getting results are > not orthogonal, although from a performance testing point of view an > application could do both, and the point is to allow pgbench to test the > performance impact of doing that. We very briefly discussed this topic at FOSDEM pgday. My feeling on the general opinion is that there's appreciation for \gset in general, but that people feel that \cset is too much cruft to take for not enough additional added value (compared to great value delivered by \gset). What I'm going to do now is to write a patch to remove the \cset part of the commit and post it, intending to push at some point next week. If somebody has grown really fond of \cset, they can work on a patch to implement it properly, which it isn't now. Thanks -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Refactoring IndexPath representation of index conditions
I've been poking at the problem discussed in a couple of recent threads of letting extensions in on the ability to create "lossy index conditions" for complex operators/functions. The current design for that in indxpath.c is frankly just a pile of kluges of varying ages. In the initial pass, the code merely decides that a given clause is or is not capable of being used with an index (match_clause_to_indexcol), and then later it generates an actual indexqual for some lossy cases (expand_indexqual_conditions), and then still later it generates an actual indexqual for some other cases (in particular, commutation of reversed clauses doesn't happen until fix_indexqual_references in createplan.c). Both the second and third passes have to expensively rediscover some things the first pass knew already, like which side of the operator the index column is on. In between, still other places like costsize.c and selfuncs.c also expensively rediscover that. And, because the IndexPath's representation of original and derived index clauses doesn't keep a clear association between an original clause and what was derived from it, we also fail to handle some cases where we don't really need to re-test an original clause, see for instance https://www.postgresql.org/message-id/27810.1547651...@sss.pgh.pa.us I think that the original idea here was that we should do as little work as possible "up front", since most index paths will get discarded before we reach createplan.c. But to the extent that that was valid at all, it's gotten overtaken by circumstances. In particular, postponing work to expand_indexqual_conditions (which is called by create_index_path) is just stupid, because these days we typically call that several times with the same index conditions. It's really dubious that postponing commutation to createplan.c is a net win either, considering that it complicates intermediate costing steps. What finally drove me to the breaking point on this was seeing that if we keep this design, we'd have to look up and call an extension operator's planner support function twice, once during match_clause_to_indexcol (to ask whether a lossy conversion is possible) and again in expand_indexqual_conditions (to actually do it). That's just silly. But thinking about how to fix that led me to the conclusion that we need a more wide-ranging refactoring that will also eliminate the inefficiencies cited above. Hence, I propose the attached, which replaces the separate "indexclauses", "indexquals" and "indexqualcols" lists of IndexPaths with a single list of IndexClause nodes. That allows us to keep a clear association between original and derived clauses, and provides us a place to put additional data as needed. In this version I added a "lossy" flag to tell whether the derived clauses are an exact implementation of the original or not, which is enough to fix the boolean-index problem mentioned above. I also added a field to allow storing the index column list for an indexable RowCompareExpr, avoiding the need to re-do creation of that list at createplan time. In this patch I also legislate that commutation of a clause is a form of making a derived clause, and it has to be done up-front and stored explicitly. That's a debatable choice, but I think it's a win because it allows code such as the index cost estimators to not have to deal with un-commuted index clauses, and (after some more refactoring) we'll be able to avoid looking up the commutator operator twice. As best I can tell from microbenchmarking the planner, this patch is about a wash as it stands for simple index clauses. I expect it will come out ahead after I've refactored match_clause_to_indexcol and expand_indexqual_conditions to avoid duplication of effort in the latter. It is already a measurable and very significant win for RowCompareExpr cases, eg planning time for "select * from tenk1 where (thousand, tenthous) < (10,100)" drops by 30%. An interesting side effect, visible in the regression tests, is that EXPLAIN now always shows index clauses with the index column on the left, since commutation happens before we create the "indexqualorig" component of the Plan. This seems all to the good IMO; the old output always struck me as confusing. The next step is to actually do that refactoring inside indxpath.c, but I felt this patch was large enough already, so I'm putting it up for comment as-is. (There's more cleanup and elimination of duplicate work that could happen in the index cost estimators too, I think.) Thoughts? If there's not objections I'd like to push this soon. regards, tom lane diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c index 2385d02..8ed30c0 100644 *** a/src/backend/nodes/nodeFuncs.c --- b/src/backend/nodes/nodeFuncs.c *** expression_tree_walker(Node *node, *** 2192,2197 --- 2192,2208 /* groupClauses are deemed uninteresting */ } break; +
Re: [HACKERS] Block level parallel vacuum
On Thu, Jan 31, 2019 at 10:18 PM Masahiko Sawada wrote:
>
> Thank you. I'll submit the updated patch set.
>
Attached the latest patch set.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
From 021a179d7696183394db60aedbd1acb0301ad4b0 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada
Date: Wed, 23 Jan 2019 16:07:53 +0900
Subject: [PATCH v14 2/2] Add -P option to vacuumdb command
---
doc/src/sgml/ref/vacuumdb.sgml| 22 +++
src/bin/scripts/t/100_vacuumdb.pl | 10 ++-
src/bin/scripts/vacuumdb.c| 58 ++-
3 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 41c7f3d..95ff132 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -227,6 +227,28 @@ PostgreSQL documentation
+ -P workers
+ --parallel=workers
+
+
+Execute parallel vacuum with PostgreSQL's
+workers background workers.
+
+
+This will require background workers, so make sure your
+ setting is
+more than one.
+
+
+
+ This opton is only available for servers runining
+ PostgreSQL 12 and later.
+
+
+
+
+
+
-q
--quiet
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 7f3a9b1..5683ef6 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -3,7 +3,7 @@ use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 44;
+use Test::More tests => 48;
program_help_ok('vacuumdb');
program_version_ok('vacuumdb');
@@ -48,6 +48,14 @@ $node->issues_sql_like(
$node->command_fails(
[ 'vacuumdb', '--analyze-only', '--disable-page-skipping', 'postgres' ],
'--analyze-only and --disable-page-skipping specified together');
+$node->issues_sql_like(
+ [ 'vacuumdb', '-P2', 'postgres' ],
+ qr/statement: VACUUM \(PARALLEL 2\);/,
+ 'vacuumdb -P2');
+$node->issues_sql_like(
+ [ 'vacuumdb', '-P', 'postgres' ],
+ qr/statement: VACUUM \(PARALLEL\);/,
+ 'vacuumdb -P');
$node->command_ok([qw(vacuumdb -Z --table=pg_am dbname=template1)],
'vacuumdb with connection string');
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 5ac41ea..2aee18b 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -45,6 +45,8 @@ typedef struct vacuumingOptions
bool skip_locked;
int min_xid_age;
int min_mxid_age;
+ int parallel_workers; /* -1: disabled, 0: PARALLEL without number of
+ * workers. */
} vacuumingOptions;
@@ -111,6 +113,7 @@ main(int argc, char *argv[])
{"full", no_argument, NULL, 'f'},
{"verbose", no_argument, NULL, 'v'},
{"jobs", required_argument, NULL, 'j'},
+ {"parallel", optional_argument, NULL, 'P'},
{"maintenance-db", required_argument, NULL, 2},
{"analyze-in-stages", no_argument, NULL, 3},
{"disable-page-skipping", no_argument, NULL, 4},
@@ -140,6 +143,7 @@ main(int argc, char *argv[])
/* initialize options to all false */
memset(, 0, sizeof(vacopts));
+ vacopts.parallel_workers = -1;
progname = get_progname(argv[0]);
@@ -147,7 +151,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "vacuumdb", help);
- while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:", long_options, )) != -1)
+ while ((c = getopt_long(argc, argv, "h:p:P::U:wWeqd:zZFat:fvj:", long_options, )) != -1)
{
switch (c)
{
@@ -214,6 +218,25 @@ main(int argc, char *argv[])
exit(1);
}
break;
+ case 'P':
+{
+ int parallel_workers = 0;
+
+ if (optarg != NULL)
+ {
+ parallel_workers = atoi(optarg);
+ if (parallel_workers <= 0)
+ {
+ fprintf(stderr, _("%s: number of parallel workers must be at least 1\n"),
+ progname);
+ exit(1);
+ }
+ }
+
+ /* allow to set 0, meaning PARALLEL without the parallel degree */
+ vacopts.parallel_workers = parallel_workers;
+ break;
+}
case 2:
maintenance_db = pg_strdup(optarg);
break;
@@ -288,9 +311,22 @@ main(int argc, char *argv[])
progname, "disable-page-skipping");
exit(1);
}
+ if (vacopts.parallel_workers >= 0)
+ {
+ fprintf(stderr, _("%s: cannot use the \"%s\" option when performing only analyze\n"),
+ progname, "parallel");
+ exit(1);
+ }
/* allow 'and_analyze' with 'analyze_only' */
}
+ if (vacopts.full && vacopts.parallel_workers >= 0)
+ {
+ fprintf(stderr, _("%s: cannot use the \"%s\" option with \"%s\" option"),
+progname, "full", "parallel");
+ exit(1);
+ }
+
setup_cancel_handler();
/* Avoid opening extra connections. */
@@ -426,6 +462,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
exit(1);
}
+ if (vacopts->parallel_workers > 0 && PQserverVersion(conn) < 12)
+
Re: fast defaults in heap_getattr vs heap_deform_tuple
Hi, On 2019-02-02 05:35:21 -0800, Andres Freund wrote: > This breaks HOT (and probably also foreign keys), when fast default > columns are set to NULL, because HeapDetermineModifiedColumns() gets the > values with heap_getattr(), which returns a spurious NULL for the old > value (instead of the fast default value). That then would compare equal > to the new column value set to NULL. Repro: BEGIN; CREATE TABLE t(); INSERT INTO t DEFAULT VALUES; ALTER TABLE t ADD COLUMN a int default 1; CREATE INDEX ON t(a); UPDATE t SET a = NULL; SET LOCAL enable_seqscan = true; SELECT * FROM t WHERE a IS NULL; SET LOCAL enable_seqscan = false; SELECT * FROM t WHERE a IS NULL; ROLLBACK; output: ... UPDATE 1 SET ┌┐ │ a│ ├┤ │ (null) │ └┘ (1 row) SET ┌───┐ │ a │ ├───┤ └───┘ (0 rows) Greetings, Andres Freund
Re: Able to do ALTER DEFAULT PRIVILEGES from a user who is not the owner
> "rajan" == rajan writes: rajan> --with the student user have no privilege how ALTER DEFAULT PRIVILEGES rajan> works rajan> *learning=> ALTER DEFAULT PRIVILEGES IN SCHEMA additional GRANT INSERT ON rajan> TABLES TO student; This ALTER only affects the default privileges for tables created by the role "student" (because they're the ones executing the ALTER), it does not affect default privileges for tables created by anybody else. -- Andrew (irc:RhodiumToad)
Re: [HACKERS] Block level parallel vacuum
On Sat, Feb 2, 2019 at 4:06 AM Amit Kapila wrote: > > On Fri, Feb 1, 2019 at 2:49 AM Masahiko Sawada wrote: > > > > On Wed, Jan 30, 2019 at 2:06 AM Haribabu Kommi > > wrote: > > > > Thank you. I'll submit the updated patch set. > > > > I don't see any chance of getting this committed in the next few days, > so, moved to next CF. Thanks for working on this and I hope you will > continue work on this project. Thank you! Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: fast defaults in heap_getattr vs heap_deform_tuple
Hi,
On 2019-02-01 08:24:04 -0800, Andres Freund wrote:
> While working on the patch to slotify trigger.c I got somewhat confused
> by the need to expand tuples in trigger.c:
>
> static HeapTuple
> GetTupleForTrigger(EState *estate,
>EPQState *epqstate,
>ResultRelInfo *relinfo,
>ItemPointer tid,
>LockTupleMode lockmode,
>TupleTableSlot **newSlot)
> {
> ...
> if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
> result = heap_expand_tuple(, relation->rd_att);
> else
> result = heap_copytuple();
> ReleaseBuffer(buffer);
>
> There's no explanation why GetTupleForTrigger() needs expanding tuples,
> but most other parts of postgres don't. Normally deforming ought to take
> care of expanding missing attrs.
>
> As far as I can tell, the reason that it's needed is that heap_gettar()
> wasn't updated along the rest of the functions. heap_deform_tuple(),
> heap_attisnull() etc look for missing attrs, but heap_getattr() doesn't.
>
> That, to me, makes no sense. The reason that we see a difference in
> regression test output is that composite_to_json() uses heap_getattr(),
> if it used heap_deform_tuple (which'd be considerably faster), we'd get
> the default value.
>
> Am I missing something?
This breaks HOT (and probably also foreign keys), when fast default
columns are set to NULL, because HeapDetermineModifiedColumns() gets the
values with heap_getattr(), which returns a spurious NULL for the old
value (instead of the fast default value). That then would compare equal
to the new column value set to NULL.
Greetings,
Andres Freund
Re: DNS SRV support for LDAP authentication
On Sat, Feb 2, 2019 at 10:34 PM Graham Leggett wrote: > On 02 Feb 2019, at 01:57, Thomas Munro wrote: > > On Sat, Feb 2, 2019 at 9:25 AM Graham Leggett wrote: > >> Does this support SSL/TLS? > > I didn't try it myself but I found several claims that it works. I > > see complaints that it always looks for _ldap._tcp and not _ldaps._tcp > > as you might expect when using ldascheme=ldaps, but that doesn't seem > > to be a big problem. As for ldaptls=1, that must work because it > > doesn't even negotiate that until after the connection is made. > > If the LDAP server was bound to port 636, how would the client know to use a > direct SSL/TLS connection and not STARTTLS? SRV records don't control that, so it looks like the person configuring pg_hba.conf would simply have to know which of the following formats to use: ldapurl=ldap:///dc=example,dc=net ldapurl=ldap:///dc=example,dc=net ldaptls=1 ldapurl=ldaps:///dc=example,dc=net Only the port and host are obtained from the SRV record, not those protocol details. Nothing in RFC 2782 prevents you from setting up separate "_ldaps._tcp" SRV records (and I can find discussions of that idea on the net) and then writing custom resolver code that knows to look for that, but the OpenLDAP code we're using (for compatibility with the command line tools) is hard coded to use "_ldap._tcp" always[1]. Active Directory apparently automatically creates only "_ldap._tcp" SRV records according to its documentation and that's the user base I was aiming for with this patch, so I think it makes sense to just use the routines they provide, despite this weakness. [1] https://github.com/openldap/openldap/blob/b06f5b0493937fc28f2cc86df1d7f464aa4504d8/libraries/libldap/dnssrv.c#L276 -- Thomas Munro http://www.enterprisedb.com
Re: fast defaults in heap_getattr vs heap_deform_tuple
Him On 2019-02-01 14:49:05 -0800, Andres Freund wrote: > +#ifdef IAM_THE_WRONG_FIX > if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts) > result = heap_expand_tuple(, relation->rd_att); > else > result = heap_copytuple(); > +#else > + result = heap_copytuple(); > +#endif This was added in commit 7636e5c60fea83a9f3cd2ad278c0819b98941c74 Author: Andrew Dunstan Date: 2018-09-24 16:11:24 -0400 Fast default trigger and expand_tuple fixes Ensure that triggers get properly filled in tuples for the OLD value. Also fix the logic of detecting missing null values. The previous logic failed to detect a missing null column before the first missing column with a default. Fixing this has simplified the logic a bit. Regression tests are added to test changes. This should ensure better coverage of expand_tuple(). Original bug reports, and some code and test scripts from Tomas Vondra Backpatch to release 11. Unfortunately the commit doesn't reference the discussion. That appears to have been at https://postgr.es/m/224e4807-395d-9fc5-2934-d5f85130f1f0%402ndQuadrant.com Greetings, Andres Freund
Able to do ALTER DEFAULT PRIVILEGES from a user who is not the owner
Please help to understand the following. Where the User(who is not the owner of a table) is able to ALTER DEFAULT PRIVILEGES and GRANT SELECT rights for all tables Is providing USAGE on schema is enough to do that? How is this secure? learning=> select current_user; current_user -- student (1 row) learning=> \dn List of schemas Name | Owner -+-- academics | head board_exams | head public | postgres (3 rows) learning=> set role head; SET learning=> CREATE SCHEMA additional; CREATE SCHEMA learning=> learning=> \dn List of schemas Name | Owner -+-- academics | head * additional | head* Schema's owner is the user head board_exams | head public | postgres (4 rows) learning=> CREATE TABLE additional.chess(id serial not null, marks varchar); CREATE TABLE learning=> GRANT USAGE ON SCHEMA additional TO student; GRANT learning=> set role student; SET learning=> \z additional.chess Access privileges Schema | Name | Type | Access privileges | Column privileges | Policies +---+---+---+---+-- * additional | chess | table | | |* -- USER student has no privilege on the table (1 row) learning=> SELECT current_user; current_user -- student (1 row) --with the student user have no privilege how ALTER DEFAULT PRIVILEGES works *learning=> ALTER DEFAULT PRIVILEGES IN SCHEMA additional GRANT INSERT ON TABLES TO student; ALTER DEFAULT PRIVILEGES learning=> \ddp Default access privileges Owner | Schema| Type | Access privileges -+-+---+ student | academics | table | student=aD/student student | additional | table | student=a/student student | board_exams | table | student=r/student (3 rows)* learning=> GRANT INSERT ON TABLES TO student; ERROR: relation "tables" does not exist learning=> GRANT INSERT ON TABLE additional.chess TO student; ERROR: permission denied for relation chess learning=> - -- Thanks, Rajan. -- Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: WIP: Avoid creation of the free space map for small tables
On Sat, Feb 2, 2019 at 7:30 AM Amit Kapila wrote:
>
> On Mon, Jan 28, 2019 at 4:40 PM Amit Kapila wrote:
> >
> > On Mon, Jan 28, 2019 at 10:03 AM John Naylor
> > wrote:
>
> In the past few days, we have done a further analysis of each problem
> and tried to reproduce it. We are successful in generating some form
> of reproducer for 3 out of 4 problems in the same way as it was failed
> in the buildfarm. For the fourth symptom, we have tried a lot (even
> Andrew Dunstan has helped us to run the regression tests with the
> faulty commit on Jacana for many hours, but it didn't got reproduced)
> but not able to regenerate a failure in a similar way. However, I
> have a theory as mentioned below why the particular test could fail
> and the fix for the same is done in the patch. I am planning to push
> the latest version of the patch [1] which has fixes for all the
> symptoms.
>
Today, I have spent some more time to generate a test which can
reproduce the failure though it is with the help of breakpoints. See
below:
> >
> > 4. Failure on jacana:
> > ---
> > c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/test/regress/expected/box.out
> > 2018-09-26
> > 17:53:33 -0400
> > +++
> > c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/regress/results/box.out
> > 2019-01-27 23:14:35
> > -0500
> > @@ -252,332 +252,7 @@
> > ('(0,100)(0,infinity)'),
> > ('(-infinity,0)(0,infinity)'),
> > ('(-infinity,-infinity)(infinity,infinity)');
> > -SET enable_seqscan = false;
> > -SELECT * FROM box_temp WHERE f1 << '(10,20),(30,40)';
> > ..
> > ..
> > TRAP: FailedAssertion("!(!(fsm_local_map.nblocks > 0))", File:
> > "c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/backend/storage/freespace/freespace.c",
> > Line:
> > 1118)
> > ..
> > 2019-01-27 23:14:35.495 EST [5c4e81a0.2e28:4] LOG: server process
> > (PID 14388) exited with exit code 3
> > 2019-01-27 23:14:35.495 EST [5c4e81a0.2e28:5] DETAIL: Failed process
> > was running: INSERT INTO box_temp
> > VALUES (NULL),
> >
> > I think the reason for this failure is same as previous (as mentioned
> > in point-3), but this can happen in a different way. Say, we have
> > searched the local map and then try to extend a relation 'X' and in
> > the meantime, another backend has extended such that it creates FSM.
> > Now, we will reuse that page and won't clear local map. Now, say we
> > try to insert in relation 'Y' which doesn't have FSM. It will try to
> > set the local map and will find that it already exists, so will fail.
> > Now, the question is how it can happen in this box.sql test. I guess
> > that is happening for some system table which is being populated by
> > Create Index statement executed just before the failing Insert.
> >
Based on the above theory, the test is as below:
Session-1
---
postgres=# create table test_1(c1 int, c2 char(1500));
CREATE TABLE
postgres=# create table test_2(c1 int, c2 char(1500));
CREATE TABLE
postgres=# insert into test_1 values(generate_series(1,20),'');
INSERT 0 20
postgres=# insert into test_2 values(1,'');
INSERT 0 1
Session-2
postgres=# analyze test_1;
ANALYZE
postgres=# analyze test_2;
ANALYZE
postgres=# select oid, relname, relpages from pg_class where relname
like 'test%';
oid |relname | relpages
---++--
41835 | test_1 |4
41838 | test_2 |1
Till here we can see that test_1 has 4 pages and test2_1 has 1 page.
At this stage, even one more record insertion in test_1 will create a
new page. So, now we have to hit the scenario with the help of
debugger. For session-1, attach debugger and put breakpoint in
RelationGetBufferForTuple().
Session-1
postgres=# insert into test_1 values(21,'');
It will hit the breakpoint in RelationGetBufferForTuple(). Now, add
one more breakpoint on line 542 in hio.c, aka below line:
RelationGetBufferForTuple
{
..
else if (!ConditionalLockRelationForExtension(relation, ExclusiveLock))
Press continue and stop the debugger at line 542. Attach the debugger
for session-2 and add a breakpoint on line 580 in hio.c, aka below
line:
RelationGetBufferForTuple
{
..
buffer = ReadBufferBI(relation, P_NEW, bistate);
Session-2
---
postgres=# insert into test_1 values(22,'');
It will hit the breakpoint in RelationGetBufferForTuple(). Now
proceed with debugging on session-1 by one step. This is to ensure
that session-1 doesn't get a conditional lock. Now, continue the
debugger in session-2 and after that run vacuum test_1 from session-2,
this will ensure that FSM is created for relation test_1. So the
state of session-2 will be as below:
Session-2
postgres=# insert into test_1 values(22,'');
INSERT 0 1
postgres=# vacuum test_1;
VACUUM
postgres=# select oid, relname, relpages from pg_class where relname
like 'test%';
oid |relname | relpages
Re: DNS SRV support for LDAP authentication
On 02 Feb 2019, at 01:57, Thomas Munro wrote: > On Sat, Feb 2, 2019 at 9:25 AM Graham Leggett wrote: >> On 25 Sep 2018, at 04:09, Thomas Munro wrote: >>> Some people like to use DNS SRV records to advertise LDAP servers on >>> their network. Microsoft Active Directory is usually (always?) set up >>> that way. Here is a patch to allow our LDAP auth module to support >>> that kind of discovery. >> >> Does this support SSL/TLS? > > I didn't try it myself but I found several claims that it works. I > see complaints that it always looks for _ldap._tcp and not _ldaps._tcp > as you might expect when using ldascheme=ldaps, but that doesn't seem > to be a big problem. As for ldaptls=1, that must work because it > doesn't even negotiate that until after the connection is made. If the LDAP server was bound to port 636, how would the client know to use a direct SSL/TLS connection and not STARTTLS? Regards, Graham —
Spurious "apparent wraparound" via SimpleLruTruncate() rounding
While testing an xidStopLimit corner case, I got this:
3656710 2019-01-05 00:05:13.910 GMT LOG: automatic aggressive vacuum to
prevent wraparound of table "test.pg_toast.pg_toast_826": index scans: 0
3656710 2019-01-05 00:05:16.912 GMT LOG: could not truncate directory
"pg_xact": apparent wraparound
3656710 2019-01-05 00:05:16.912 GMT DEBUG: transaction ID wrap limit is
4294486400, limited by database with OID 1
3656710 2019-01-05 00:05:16.912 GMT WARNING: database "template1" must be
vacuumed within 481499 transactions
3656710 2019-01-05 00:05:16.912 GMT HINT: To avoid a database shutdown,
execute a database-wide VACUUM in that database.
I think the WARNING was correct about having 481499 XIDs left before
xidWrapLimit, and the spurious "apparent wraparound" arose from this
rounding-down in SimpleLruTruncate():
cutoffPage -= cutoffPage % SLRU_PAGES_PER_SEGMENT;
...
/*
* While we are holding the lock, make an important safety check: the
* planned cutoff point must be <= the current endpoint page. Otherwise
we
* have already wrapped around, and proceeding with the truncation would
* risk removing the current segment.
*/
if (ctl->PagePrecedes(shared->latest_page_number, cutoffPage))
{
LWLockRelease(shared->ControlLock);
ereport(LOG,
(errmsg("could not truncate directory \"%s\":
apparent wraparound",
ctl->Dir)));
We round "cutoffPage" to make ctl->PagePrecedes(segpage, cutoffPage) return
false for the segment containing the cutoff page. CLOGPagePrecedes() (and
most SLRU PagePrecedes methods) implements a circular address space. Hence,
the rounding also causes ctl->PagePrecedes(segpage, cutoffPage) to return true
for the segment furthest in the future relative to the unrounded cutoffPage
(if it exists). That's bad. Such a segment rarely exists, because
xidStopLimit protects 100 XIDs, and the rounding moves truncation by no
more than (BLCKSZ * CLOG_XACTS_PER_BYTE * SLRU_PAGES_PER_SEGMENT - 1) =
1048575 XIDs. Thus, I expect to see this problem at 4.9% of xidStopLimit
values. I expect this is easier to see with multiStopLimit, which protects
only 100 mxid.
The main consequence is the false alarm. A prudent DBA will want to react to
true wraparound, but no such wraparound has occurred. Also, we temporarily
waste disk space in pg_xact. This feels like a recipe for future bugs. The
fix I have in mind, attached, is to change instances of
ctl->PagePrecedes(FIRST_PAGE_OF_SEGMENT, ROUNDED_cutoffPage) to
ctl->PagePrecedes(LAST_PAGE_OF_SEGMENT, cutoffPage). I'm inclined not to
back-patch this; does anyone favor back-patching?
Thanks,
nm
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 3623352..843486a 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -1171,11 +1171,6 @@ SimpleLruTruncate(SlruCtl ctl, int cutoffPage)
int slotno;
/*
- * The cutoff point is the start of the segment containing cutoffPage.
- */
- cutoffPage -= cutoffPage % SLRU_PAGES_PER_SEGMENT;
-
- /*
* Scan shared memory and remove any pages preceding the cutoff page, to
* ensure we won't rewrite them later. (Since this is normally called in
* or just after a checkpoint, any dirty pages should have been flushed
@@ -1320,11 +1315,10 @@ restart:
bool
SlruScanDirCbReportPresence(SlruCtl ctl, char *filename, int segpage, void *data)
{
+ int seg_last_page = segpage + SLRU_PAGES_PER_SEGMENT - 1;
int cutoffPage = *(int *) data;
- cutoffPage -= cutoffPage % SLRU_PAGES_PER_SEGMENT;
-
- if (ctl->PagePrecedes(segpage, cutoffPage))
+ if (ctl->PagePrecedes(seg_last_page, cutoffPage))
return true; /* found one; don't iterate any more */
return false;/* keep going */
@@ -1337,9 +1331,10 @@ SlruScanDirCbReportPresence(SlruCtl ctl, char *filename, int segpage, void *data
static bool
SlruScanDirCbDeleteCutoff(SlruCtl ctl, char *filename, int segpage, void *data)
{
+ int seg_last_page = segpage + SLRU_PAGES_PER_SEGMENT - 1;
int cutoffPage = *(int *) data;
- if (ctl->PagePrecedes(segpage, cutoffPage))
+ if (ctl->PagePrecedes(seg_last_page, cutoffPage))
SlruInternalDeleteSegment(ctl, filename);
return false;/* keep going */
Re: pg_dump multi VALUES INSERT
Hello David, Wondering if you have anything else here? I'm happy for the v13 version to be marked as ready for committer. I still have a few comments. Patch applies cleanly, compiles, global & local make check are ok. Typos and style in the doc: "However, since, by default this option generates ..." "However, since this option, by default, generates ..." I'd suggest a more straightforward to my mind and ear: "However, since by default the option generates ..., ", although beware that I'm not a native English speaker. I do not understand why dump_inserts declaration has left the "flags for options" section. I'd suggest not to rely on "atoi" because it does not check the argument syntax, so basically anything is accepted, eg "1O" is 1; On "if (!dopt->do_nothing) $1 else $2;", I'd rather use a straight condition "if (dopt->do_nothing) $2 else $1;" (two instances). There is a test, that is good! Charater "." should be backslashed in the regexpr. I'd consider also introducing limit cases: empty table, empty columns by creating corresponding tables and using -t repeatedly. -- Fabien.
Re: pgsql: Remove references to Majordomo
On Mon, Jan 28, 2019 at 07:29:39PM +0100, Magnus Hagander wrote: > On Mon, Jan 28, 2019 at 7:26 PM Tom Lane wrote: > > Stephen Frost writes: > > >> On Sun, Jan 27, 2019 at 2:28 AM Noah Misch wrote: > > >>> What are those blocked infrastructure improvements? > > > > > The specific improvements we're talking about are DKIM/DMARC/SPF, which > > > is becoming more and more important to making sure that the email from > > > our lists can actually get through to the subscribers. > > > > Certainly those are pretty critical. But can you give us a quick > > refresher on why dropping the @postgresql.org list aliases is > > necessary for that? I thought we'd already managed to make the > > lists compliant with those specs. > > I believe it doesn't, as Stephen also agreed with upthread. > > We needed to move our *sending* out of the postgresql.org domain in order > to be able to treat them differently. But there is nothing preventing us > from receiving to e.g. pgsql-b...@postgresql.org and internally forward it > to @lists.postgresql.org, where we then deliver from. > > I believe we *can* do the same for all lists, but that part is more a > matter of cleaning up our infrastructure, which has a fair amount of cruft > to deal with those things. We have an easy workaround for a couple of lists > which owuld take only a fairly small amount of traffic over it, but we'd > like to get rid of the cruft to deal with the large batch of them. Ceasing to accept mail at pgsql-...@postgresql.org would cause a concrete, user-facing loss in that users replying to old messages would get a bounce. Also, I find pgsql-...@lists.postgresql.org uglier, since "lists" adds negligible information. (The same is true of "pgsql", alas.) If the cost of keeping pgsql-...@postgresql.org is limited to "cruft", I'd prefer to keep pgsql-...@postgresql.org indefinitely. nm
