Re: [HACKERS] Add min and max execute statement time in pg_stat_statement
On 21 January 2014 19:48, Simon Riggs si...@2ndquadrant.com wrote: On 21 January 2014 12:54, KONDO Mitsumasa kondo.mitsum...@lab.ntt.co.jp wrote: Rebased patch is attached. Does this fix the Windows bug reported by Kumar on 20/11/2013 ? Please respond. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch: option --if-exists for pg_dump
Hello
Second update - I reduced patch by removing not necessary changes.
Attached tests and Makefile
Now --if-exists option is fully consistent with -c option
With some free time I plan to enhance test script about more object types -
now It contains almost all usual types.
Regards
Pavel
2014-01-21 Jeevan Chalke jeevan.cha...@enterprisedb.com
Hi Pavel,
Consider following test scenario:
mydb=# \d emp
Table public.emp
Column | Type | Modifiers
+-+---
empno | integer | not null
deptno | integer |
ename | text|
Indexes:
emp_pkey PRIMARY KEY, btree (empno)
Foreign-key constraints:
emp_deptno_fkey FOREIGN KEY (deptno) REFERENCES dept(deptno)
mydb=# \d dept
Table public.dept
Column | Type | Modifiers
+-+---
deptno | integer | not null
dname | text|
Indexes:
dept_pkey PRIMARY KEY, btree (deptno)
Referenced by:
TABLE emp CONSTRAINT emp_deptno_fkey FOREIGN KEY (deptno)
REFERENCES dept(deptno)
mydb=# \q
jeevan@ubuntu:~/pg_master$ ./install/bin/pg_dump -d mydb --if-exists -c
mydb_ic.dmp
I see following lines in dump which looks certainly wrong:
===
DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_deptno_fkey;
DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_pkey;
DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept_pkey;
When try to restore, as expected it is throwing an error:
===
psql:mydb_ic.dmp:14: ERROR: syntax error at or near FK
LINE 1: DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_d...
^
psql:mydb_ic.dmp:15: ERROR: syntax error at or near CONSTRAINT
LINE 1: DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_p...
^
psql:mydb_ic.dmp:16: ERROR: syntax error at or near CONSTRAINT
LINE 1: DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept...
^
Note:
===
Commands which are in form of ALTER TABLE ... DROP are failing.
You need to test each and every object with DROP .. IF EXISTS command.
Better write small test-case with all objects included.
Following logic has flaw:
===
diff --git a/src/bin/pg_dump/pg_backup_archiver.c
b/src/bin/pg_dump/pg_backup_archiver.c
index 7fc0288..0677712 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -413,8 +413,30 @@ RestoreArchive(Archive *AHX)
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te-namespace);
-/* Drop it */
-ahprintf(AH, %s, te-dropStmt);
+
+if (*te-dropStmt != '\0')
+{
+/* Inject IF EXISTS clause when it is required. */
+if (ropt-if_exists)
+{
+char buffer[40];
+size_t l;
+
+/* But do it only, when it is not there yet. */
+snprintf(buffer, sizeof(buffer), DROP %s IF
EXISTS,
+ te-desc);
+l = strlen(buffer);
+
+if (strncmp(te-dropStmt, buffer, l) != 0)
+{
+ahprintf(AH, DROP %s IF EXISTS %s,
+te-desc,
+te-dropStmt + l);
+}
+else
+ahprintf(AH, %s, te-dropStmt);
+}
+}
}
}
Also:
===
1.
This is still out of sync.
@@ -348,6 +350,8 @@ main(int argc, char *argv[])
appendPQExpBufferStr(pgdumpopts, --binary-upgrade);
if (column_inserts)
appendPQExpBufferStr(pgdumpopts, --column-inserts);
+if (if_exists)
+appendPQExpBufferStr(pgdumpopts, --if-exists);
if (disable_dollar_quoting)
appendPQExpBufferStr(pgdumpopts, --disable-dollar-quoting);
if (disable_triggers)
2.
Spell check required:
+/* skip first n chars, and create a modifieble copy */
modifieble = modifiable
+/* DROP IF EXISTS pattern is not appliable on dropStmt */
appliable = applicable
3.
+/*
+ * Object description is based on dropStmt statement. But
+ * a drop statements can be enhanced about IF EXISTS clause.
+ * We have to increase a offset in this case, IF EXISTS
+ * should not be included on object description.
+ */
Looks like you need to re-phrase these comments line. Something like:
/*
* Object description is based on dropStmt statement which may have
* IF EXISTS clause. Thus we need to update an offset such that it
* won't be included in the object description.
*/
Or as per
Re: [HACKERS] Add min and max execute statement time in pg_stat_statement
2014-01-26 Simon Riggs si...@2ndquadrant.com On 21 January 2014 19:48, Simon Riggs si...@2ndquadrant.com wrote: On 21 January 2014 12:54, KONDO Mitsumasa kondo.mitsum...@lab.ntt.co.jp wrote: Rebased patch is attached. Does this fix the Windows bug reported by Kumar on 20/11/2013 ? Please respond. Oh.. Very sorry... Last day, I tried to find Kumar mail at 20/11/2013. But I couldn't find it... Could you tell me e-mail title? My patch catches up with latest 9.4HEAD. Regards, -- Mitsumasa KONDO
Re: [HACKERS] Add min and max execute statement time in pg_stat_statement
On 23 January 2014 12:43, KONDO Mitsumasa kondo.mitsum...@lab.ntt.co.jp wrote: I tested my patch in pgbench, but I cannot find bottleneck of my latest patch. ... Attached is latest developping patch. It hasn't been test much yet, but sqrt caluclation may be faster. Thank you for reworking this so that the calculation happens outside the lock. Given your testing, and my own observation that the existing code could be reworked to avoid contention if people become concerned, I think this is now ready for commit, as soon as the last point about Windows is answered. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.warn_shadow
On 15 January 2014 00:34, Marko Tiikkaja ma...@joh.to wrote: Hi all! It's me again, trying to find a solution to the most common mistakes I make. This time it's accidental shadowing of variables, especially input variables. I've wasted several hours banging my head against the wall while shouting HOW CAN THIS VARIABLE ALWAYS BE NULL?. I can't believe I'm the only one. To give you a rough idea on how it works: =# set plpgsql.warn_shadow to true; SET =#* create function test(in1 int, in2 int) -#* returns table(out1 int, out2 int) as $$ $#* declare $#* IN1 text; $#* IN2 text; $#* out1 text; $#* begin $#* $#* declare $#* out2 text; $#* in1 text; $#* begin $#* end; $#* end$$ language plpgsql; WARNING: variable in1 shadows a previously defined variable LINE 4: IN1 text; ^ WARNING: variable in2 shadows a previously defined variable LINE 5: IN2 text; ^ WARNING: variable out1 shadows a previously defined variable LINE 6: out1 text; ^ WARNING: variable out2 shadows a previously defined variable LINE 10: out2 text; ^ WARNING: variable in1 shadows a previously defined variable LINE 11: in1 text; ^ CREATE FUNCTION No behaviour change by default. Even setting the GUC doesn't really change behaviour, just emits some warnings when a potentially faulty function is compiled. Let me know what you think so I can either cry or clean the patch up. Looking at the patch and reading comments there is something here of value. Some aspects need further consideration and I would like to include some in 9.4 and push back others to later releases. This is clearly a very useful contribution and the right direction for further work. Suggesting fixes to your own problems is a very good way to proceed... For 9.4, we should cut down the patch so it has plpgsql.warnings = none (default) | all | [individual item list] This syntax can then be extended in later releases to include further individual items, without requiring they be listed individually - which then becomes release dependent behaviour. Also, having plpgsql.warnings_as_errors = off (default) | on makes sense and should be included in 9.4 Putting this and all future options as keywords seems like a poor choice. Indeed, the # syntax proposed isn't even fully described on list here, nor are examples given in tests. My feeling is that nobody even knows that is being proposed and would likely cause more discussion if they did. So I wish to push back the # syntax to a later release when it has had more discussion. It would be good if you could lead that discussion in later releases. Please add further tests, with comments that explain why the WARNING is given. Those should include complex situations like double shadowing, not just the basics. And docs, of course. Last CF, so do this soon so we can commit. Thanks. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: libpq: Support TLS versions beyond TLSv1.
On 01/26/2014 10:13 AM, Alvaro Herrera wrote: Stephen Frost escribió: * Noah Misch (n...@leadboat.com) wrote: +1. If you can upgrade to 9.4, you can also bring your TLS protocol out of the iron age. Agreed- this was going to be my 2c. Anyone w/ an SSL library that old isn't likely to be upgrading to 9.4 of libpq or PG. What about people doing SSL connections through JDBC? As far as I understand, these don't use openssl. That's correct, PgJDBC uses Java's built-in SSL support, which is provided by the underlying JSSE (Java Secure Socket Extension) service in the JVM. From what I can find, it looks like Java 1.4.2 and newer, including Java 5, appear to support TLS 1.0. I haven't found anything definitive for 1.4.2 yet, but 1.5 certainly supports it. That's all we need to care about IMO; 1.4.x users are running unsupported and old PgJDBC versions (we dropped support for 1.4) and they're generally happy living in the stone age. So I don't see Java as a barrier here. Finding a good reference on which Java runtimes support which features is surprisingly hard. Java 6 supports TLS. It took a bit to confirm that 1.5 does too. 1.4.2 may, but we don't need to care. http://docs.oracle.com/javase/1.5.0/docs/guide/security/jsse/JSSERefGuide.html claims: The JSSE implementation in the J2SDK 1.4 and later implements SSL 3.0 and TLS 1.0 ... but in the table Default Enabled Cipher Suites in: http://docs.oracle.com/javase/7/docs/technotes/guides/security/SunProviders.html Java 1.4.2 and newer are shown to support by default: TLS_RSA_WITH_AES_256_CBC_SHA TLS_DHE_RSA_WITH_AES_256_CBC_SHA TLS_DHE_DSS_WITH_AES_256_CBC_SHA TLS_RSA_WITH_AES_128_CBC_SHA TLS_DHE_RSA_WITH_AES_128_CBC_SHA TLS_DHE_DSS_WITH_AES_128_CBC_SHA ... and a bunch of SSL_ stuff. so it looks like TLS support has probably been backpacked to 1.4.2. Java 1.4 is PostgreSQL 7.2 vintage, well into we don't care, go away land. BTW, the JSSE docs also claim that TLS 1.0 is a modest upgrade to the most recent version of SSL, version 3.0. The differences between SSL 3.0 and TLS 1.0 are minor. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Visual Studio 2013 build
On Sun, Jan 26, 2014 at 1:13 AM, Andrew Dunstan and...@dunslane.net wrote: On 12/02/2013 05:12 PM, Brar Piening wrote: Hackers, the attached patch enables Microsoft Visual Studio 2013 as additional build environment. After some tweaking (VS now has got its own rint and a few macro definitions that were previously missing) the build runs without errors or warnings and the product passes the regression tests. I didn't test any special configurations though. I'm using full Visual Studio 2013 actually so I can't conform that everything still works with Visual Studio Express 2013 for Windows Desktop, but there are no documented limitations that make any problems foreseeable. I will add it to the CommitFest 2014-01 so that there is time for testing and tweaking. OK, I have tested this out with the development branch and Visual Studio Express 2013 for Windows Desktop, on Windows Server 2008 R2-SP1 64 bit. With a slight adjustment to make the patch apply it works fine. How far back should we go? About a year ago when we did this we applied it for 9.2 (then the latest stable release) and 9.3dev. Seems reasonable to follow the same pattern, and apply it for 9.3 stable and 9.4dev. The argument being that it's a new build env, and it's not likely that people are going to use that t o build very old versions of postgres. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] alternative back-end block formats
On 01/21/2014 07:43 PM, Christian Convey wrote: Hi all, I'm playing around with Postgres, and I thought it might be fun to experiment with alternative formats for relation blocks, to see if I can get smaller files and/or faster server performance. It's not clear how you'd do this without massively rewriting the guts of Pg. Per the docs on internal structure, Pg has a block header, then tuples within the blocks, each with a tuple header and list of Datum values for the tuple. Each Datum has a generic Datum header (handling varlena vs fixed length values etc) then a type-specific on-disk representation controlled by the type output function for that type. At least, that's my understanding - I haven't had cause to delve into the on-disk format yet. What concrete problem do you mean to tackle? What idea do you want to explore or implement? Does anyone know if this has been done before with Postgres? I would have assumed yes, but I'm not finding anything in Google about people having done this. AFAIK (and I don't know much in this area) the storage manager isn't very pluggable compared to the rest of Pg. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Standalone synchronous master
On 01/24/2014 10:29 PM, Josh Berkus wrote: On 01/24/2014 12:47 PM, Heikki Linnakangas wrote: ISTM the consensus is that we need better monitoring/administration interfaces so that people can script the behavior they want in external tools. Also, a new synchronous apply replication mode would be handy, but that'd be a whole different patch. We don't have a patch on the table that we could consider committing any time soon, so I'm going to mark this as rejected in the commitfest app. I don't feel that we'll never do auto-degrade is determinative; several hackers were for auto-degrade, and they have a good use-case argument. Auto-degrade may make sense together with synchronous apply mentioned by Heikki. I do not see much use for synchronous-(noapply)-if-you-can mode, though it may make some sense in some scenarios if sync failure is accompanied by loud screaming (hey DBA, we are writing checks with no money in the bank, do something fast!) Perhaps some kind of sync-with-timeout mode, where timing out results with a weak error (something between current warning and error) returned to client and/or where it causes and external command to be run which could then be used to flood admins mailbox :) However, we do have consensus that we need more scaffolding than this patch supplies in order to make auto-degrade *safe*. I encourage the submitter to resumbit and improved version of this patch (one with more monitorability) for 9.5 CF1. That'll give us a whole dev cycle to argue about it. Cheers -- Hannu Krosing PostgreSQL Consultant Performance, Scalability and High Availability 2ndQuadrant Nordic OÜ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GIN improvements part2: fast scan
On 2014-01-26 07:24:58 +0100, Tomas Vondra wrote: Not sure how to interpret that, though. For example where did the ginCompareItemPointers go? I suspect it's thanks to inlining, and that it might be related to the performance decrease. Or maybe not. Try recompiling with CFLAGS=-fno-omit-frame-pointers -O2 and then use perf record -g. That gives you a hierarchical profile which often makes such questions easier to answer. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Freezing without write I/O
On 2014-01-25 20:26:16 -0800, Peter Geoghegan wrote: Shouldn't this patch be in the January commitfest? I think we previously concluded that there wasn't much chance to get this into 9.4 and there's significant work to be done on the patch before new reviews are required, so not submitting it imo makes sense. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] INTERVAL overflow detection is terribly broken
On Jan26, 2014, at 03:50 , Bruce Momjian br...@momjian.us wrote: Patch attached. + if ((float)tm-tm_year * MONTHS_PER_YEAR + tm-tm_mon INT_MAX) + return -1; Is this bullet-proof? If float and int are both 32-bit, the float's mantissa will be less than 32-bit (24 or so, I think), and thus won't be able to represent INT_MAX accurately. This means there's a value of tm_year * MONTHS_PER_YEAR which is less than INT_MAX, yet for which tm_year * MONTHS_PER_YEAR + tm_mon will return tm_year * MONTHS_PER_YEAR unmodified if tm_mon is small enough (e.g, 1). But if tm_year * MONTHS_PER_YEAR was close enough to INT_MAX, the actual value of tm_year * MONTHS_PER_YEAR + tm_mon might still be larger than INT_MAX, the floating-point computation just won't detect it. In that case, the check would succeed, yet the actual integer computation would still overflow. It should be safe with double instead of float. + if (SAMESIGN(span1-month, span2-month) + !SAMESIGN(result-month, span1-month)) This assumes wrapping semantics for signed integral types, which isn't guaranteed by the C standard - it says overflows of signed integral types produce undefined results. We currently depend on wrapping semantics for these types in more places, and therefore need GCC's -fwrapv anway, but I still wonder if adding more of these kinds of checks is a good idea. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] effective_cache_size calculation overflow
To test something unrelated, I set my shared_buffers to 7TB on my laptop today (no, unfortunately I don't have that much RAM). That leads to the startup error: FATAL: -536870912 is outside the valid range for parameter effective_cache_size (-1 .. 2147483647) So clearly there is an overflow somewhere in the calculation of effective_cache_size, most likely from the fact that it's now dynamically calculated. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] plpgsql.warn_shadow
On Jan26, 2014, at 10:19 , Simon Riggs si...@2ndquadrant.com wrote: Also, having plpgsql.warnings_as_errors = off (default) | on makes sense and should be included in 9.4 I still think this is a bad idea, for the same reasons I don't like consistent_into (discussed in a separate thread). But these objections would go away if restricted this to function creation time only. So even with warnings_as_errors=on, you could still *call* a function that produces a warning, but not *create* one. We could then integrate this with check_function_bodies, i.e. add a third possible value error_on_warnings to that GUC, instead of having a separate GUC for this. Putting this and all future options as keywords seems like a poor choice. Indeed, the # syntax proposed isn't even fully described on list here, nor are examples given in tests. My feeling is that nobody even knows that is being proposed and would likely cause more discussion if they did. So I wish to push back the # syntax to a later release when it has had more discussion. It would be good if you could lead that discussion in later releases. +1 best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.warn_shadow
2014-01-26 Florian Pflug f...@phlo.org On Jan26, 2014, at 10:19 , Simon Riggs si...@2ndquadrant.com wrote: Also, having plpgsql.warnings_as_errors = off (default) | on makes sense and should be included in 9.4 I still think this is a bad idea, for the same reasons I don't like consistent_into (discussed in a separate thread). But these objections would go away if restricted this to function creation time only. So even with warnings_as_errors=on, you could still *call* a function that produces a warning, but not *create* one. +1 behave - and please, better name Regards Pavel We could then integrate this with check_function_bodies, i.e. add a third possible value error_on_warnings to that GUC, instead of having a separate GUC for this. Putting this and all future options as keywords seems like a poor choice. Indeed, the # syntax proposed isn't even fully described on list here, nor are examples given in tests. My feeling is that nobody even knows that is being proposed and would likely cause more discussion if they did. So I wish to push back the # syntax to a later release when it has had more discussion. It would be good if you could lead that discussion in later releases. +1 best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GIN improvements part2: fast scan
On 01/26/2014 08:24 AM, Tomas Vondra wrote:
Hi!
On 25.1.2014 22:21, Heikki Linnakangas wrote:
Attached is a new version of the patch set, with those bugs fixed.
I've done a bunch of tests with all the 4 patches applied, and it seems
to work now. I've done tests with various conditions (AND/OR, number of
words, number of conditions) and I so far I did not get any crashes,
infinite loops or anything like that.
I've also compared the results to 9.3 - by dumping the database and
running the same set of queries on both machines, and indeed I got 100%
match.
I also did some performance tests, and that's when I started to worry.
For example, I generated and ran 1000 queries that look like this:
SELECT id FROM messages
WHERE body_tsvector @@ to_tsquery('english','(header 53 32
useful dropped)')
ORDER BY ts_rank(body_tsvector, to_tsquery('english','(header 53
32 useful dropped)')) DESC;
i.e. in this case the query always was 5 words connected by AND. This
query is a pretty common pattern for fulltext search - sort by a list of
words and give me the best ranked results.
On 9.3, the script was running for ~23 seconds, on patched HEAD it was
~40. It's perfectly reproducible, I've repeated the test several times
with exactly the same results. The test is CPU bound, there's no I/O
activity at all. I got the same results with more queries (~100k).
Attached is a simple chart with x-axis used for durations measured on
9.3.2, y-axis used for durations measured on patched HEAD. It's obvious
a vast majority of queries is up to 2x slower - that's pretty obvious
from the chart.
Only about 50 queries are faster on HEAD, and 700 queries are more than
50% slower on HEAD (i.e. if the query took 100ms on 9.3, it takes 150ms
on HEAD).
Typically, the EXPLAIN ANALYZE looks something like this (on 9.3):
http://explain.depesz.com/s/5tv
and on HEAD (same query):
http://explain.depesz.com/s/1lI
Clearly the main difference is in the Bitmap Index Scan which takes
60ms on 9.3 and 120ms on HEAD.
On 9.3 the perf top looks like this:
34.79% postgres [.] gingetbitmap
28.96% postgres [.] ginCompareItemPointers
9.36% postgres [.] TS_execute
5.36% postgres [.] check_stack_depth
3.57% postgres [.] FunctionCall8Coll
while on 9.4 it looks like this:
28.20% postgres [.] gingetbitmap
21.17% postgres [.] TS_execute
8.08% postgres [.] check_stack_depth
7.11% postgres [.] FunctionCall8Coll
4.34% postgres [.] shimTriConsistentFn
Not sure how to interpret that, though. For example where did the
ginCompareItemPointers go? I suspect it's thanks to inlining, and that
it might be related to the performance decrease. Or maybe not.
Yeah, inlining makes it disappear from the profile, and spreads that
time to the functions calling it.
The profile tells us that the consistent function is called a lot more
than before. That is expected - with the fast scan feature, we're
calling consistent not only for potential matches, but also to refute
TIDs based on just a few entries matching. If that's effective, it
allows us to skip many TIDs and avoid consistent calls, which
compensates, but if it's not effective, it's just overhead.
I would actually expect it to be fairly effective for that query, so
that's a bit surprising. I added counters to see where the calls are
coming from, and it seems that about 80% of the calls are actually
coming from this little the feature I explained earlier:
In addition to that, I'm using the ternary consistent function to check
if minItem is a match, even if we haven't loaded all the entries yet.
That's less important, but I think for something like rare1 | (rare2
frequent) it might be useful. It would allow us to skip fetching
'frequent', when we already know that 'rare1' matches for the current
item. I'm not sure if that's worth the cycles, but it seemed like an
obvious thing to do, now that we have the ternary consistent function.
So, that clearly isn't worth the cycles :-). At least not with an
expensive consistent function; it might be worthwhile if we pre-build
the truth-table, or cache the results of the consistent function.
Attached is a quick patch to remove that, on top of all the other
patches, if you want to test the effect.
- Heikki
diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index f2f9dc6..76a70a0 100644
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -895,6 +895,25 @@ keyGetItem(GinState *ginstate, MemoryContext tempCtx, GinScanKey key,
GinItemPointerGetBlockNumber(minItem));
/*
+ * We might not have loaded all the entry streams for this TID. We
+ * could call the consistent function, passing MAYBE for those entries,
+ * to see if it can
[HACKERS] running make check with only specified tests
I've often wanted to be able to run make check and just have it run the small number of tests I am interested in. Here's a tiny patch along those lines. It creates a new targe which I have called check-with for want of a better name. And with it I can do: $ make check-with TESTS=json jsonb and have it do the temp install etc and then run just those two tests. Thoughts? cheers andrew diff --git a/GNUmakefile.in b/GNUmakefile.in index 80116a1..32dd9bf 100644 --- a/GNUmakefile.in +++ b/GNUmakefile.in @@ -61,9 +61,9 @@ distclean maintainer-clean: # Garbage from autoconf: @rm -rf autom4te.cache/ -check: all +check check-with: all -check installcheck installcheck-parallel: +check check-with installcheck installcheck-parallel: $(MAKE) -C src/test/regress $@ $(call recurse,check-world,src/test src/pl src/interfaces/ecpg contrib,check) diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile index 94762d5..4165a7d 100644 --- a/src/test/regress/GNUmakefile +++ b/src/test/regress/GNUmakefile @@ -142,6 +142,9 @@ REGRESS_OPTS = --dlpath=. $(EXTRA_REGRESS_OPTS) check: all tablespace-setup $(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(TEMP_CONF) $(EXTRA_TESTS) +check-with: all tablespace-setup + $(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TEMP_CONF) $(TESTS) $(EXTRA_TESTS) + installcheck: all tablespace-setup $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/serial_schedule $(EXTRA_TESTS) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] running make check with only specified tests
2014-01-26 Andrew Dunstan and...@dunslane.net I've often wanted to be able to run make check and just have it run the small number of tests I am interested in. Here's a tiny patch along those lines. It creates a new targe which I have called check-with for want of a better name. And with it I can do: $ make check-with TESTS=json jsonb +1 Pavel and have it do the temp install etc and then run just those two tests. Thoughts? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On Jan26, 2014, at 00:24 , David Rowley dgrowle...@gmail.com wrote: On Sat, Jan 25, 2014 at 3:21 PM, Florian Pflug f...@phlo.org wrote: On Jan24, 2014, at 08:47 , Dean Rasheed dean.a.rash...@gmail.com wrote: The invtrans_minmax patch doesn't contain any patches yet - David, could you provide some for these functions, and also for bool_and and bool_or? We don't need to test every datatype, but a few would be nice. I've added a few regression tests for min, min and bool_or and bool_and. I've pushed these up to here: https://github.com/david-rowley/postgres/commits/invtrans_minmax OK, I've pushed this to github. I haven't produced new patches yet - I think it's probably better to wait for a few things to pile up, to make this less of a moving target. But ultimately, this is up to Dean - Dean, I'll do whatever makes your life as a reviewer easier. I did wonder if you'd want to see uses of FILTER in there as I'm thinking that should really be covered by the core patch and the tests here really should just be checking the inverse transition functions for min and max. I don't mind the FILTER - when this gets committed, the distinction between what was in which patch goes away anyway. What I did realize when merging this is that we currently don't tests the case of a window which lies fully after the current row (i.e. BETWEEN N FOLLOWING AND m FOLLOWING). We do test BETWEEN n PRECEDING AND m PRECEDING, because the array_agg and string_agg tests do that. So maybe some of the MIN/MAX or arithmetic tests could exercise that case? As for the data types tested, I ended just adding tests for int and text for min and max. Let me know if you think that more should be tested. That's sufficient for the regression tests, I think. As for bool_or and bool_and. I didn't think there was much extra that would need tested after I added 1 test. It's pretty simple code and adding anything extra seems like it would be testing something else. Sounds fine to me. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] running make check with only specified tests
On Jan26, 2014, at 17:47 , Andrew Dunstan and...@dunslane.net wrote: I've often wanted to be able to run make check and just have it run the small number of tests I am interested in. Here's a tiny patch along those lines. It creates a new targe which I have called check-with for want of a better name. And with it I can do: $ make check-with TESTS=json jsonb and have it do the temp install etc and then run just those two tests. +1 for the feature (+Inf, actually), but will this work if the tests depend on stuff created by other tests? best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] running make check with only specified tests
On 01/26/2014 12:01 PM, Florian Pflug wrote: On Jan26, 2014, at 17:47 , Andrew Dunstan and...@dunslane.net wrote: I've often wanted to be able to run make check and just have it run the small number of tests I am interested in. Here's a tiny patch along those lines. It creates a new targe which I have called check-with for want of a better name. And with it I can do: $ make check-with TESTS=json jsonb and have it do the temp install etc and then run just those two tests. +1 for the feature (+Inf, actually), but will this work if the tests depend on stuff created by other tests? No, if they do it will be up to you to include those in your test list in the right order. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] running make check with only specified tests
Andrew Dunstan and...@dunslane.net writes: I've often wanted to be able to run make check and just have it run the small number of tests I am interested in. Here's a tiny patch along those lines. It creates a new targe which I have called check-with for want of a better name. And with it I can do: $ make check-with TESTS=json jsonb The vast majority of the regression tests have interdependencies, which would make any feature along these lines fairly useless IME. (And no, I'm not interested in converting the tests to a no-dependencies style.) Also, the tests themselves don't take that long, especially in parallel mode. If you need to speed up repeated testing, it's more profitable to avoid the install/initdb overhead of a make check. I use a small script that just reinstalls the postgres executable and does make installcheck-parallel when I'm doing iterative development. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] In-core regression tests for replication, cascading, archiving, PITR, etc. Michael Paquier
-BEGIN PGP SIGNED MESSAGE- Hash: RIPEMD160 I thought the goal here was to have a testing framework that (a) is portable to every platform we support and (b) doesn't require root privileges to run. None of those options sound like they'll help meet those requirements. FWIW, I hacked up a Perl-based testing system as a proof of concept some time ago. I can dust it off if anyone is interested. Perl has a very nice testing ecosystem and is probably the most portable language we support, other than C. My quick goals for the project were: * allow granular testing (ala Andrew's recent email, which reminded me of this) * allow stackable methods and dependencies * make it very easy to write new tests * test various features that are way too diificult in our existing system (e.g. PITR, fdws) * get some automated code coverage metrics (this one was tricky) * allow future git integration based on subsytems - -- Greg Sabino Mullane g...@turnstep.com End Point Corporation http://www.endpoint.com/ PGP Key: 0x14964AC8 201401261211 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -BEGIN PGP SIGNATURE- iEYEAREDAAYFAlLlQeMACgkQvJuQZxSWSsiYhACggHJgQWB/Q2HEfjGZCwR3yEZg zMsAnAssOStAmMuaJEScCGHGKWYNow1v =zi0Y -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Tablespace options in \db+
Currently, tablespace options (such as random_page_cost) aren't shown in \db or \db+ in psql - the only way to see them is to directly query pg_tablespaces. This seems like an oversight from back when the feature was added. I realize the CF is closed, but would people be ok with me pushing this trivial patch into 9.4? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index 0d4b151..43f1a1c 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -176,6 +176,11 @@ describeTablespaces(const char *pattern, bool verbose) ,\n pg_catalog.shobj_description(oid, 'pg_tablespace') AS \%s\, gettext_noop(Description)); + if (verbose pset.sversion = 9) + appendPQExpBuffer(buf, + ,\n spcoptions AS \%s\, + gettext_noop(Options)); + appendPQExpBufferStr(buf, \nFROM pg_catalog.pg_tablespace\n); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
Sent from my iPad On 26-Jan-2014, at 4:38, Simon Riggs si...@2ndquadrant.com wrote: On 25 January 2014 22:33, Stephen Frost sfr...@snowman.net wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: AFAICT, there was no consensus in this thread on what to do, which probably has something to do with the lack of concrete performance tests presented to back up any particular proposal. This I entirely agree with- more testing and more information on how such a change impacts other workloads would be great. Unfortunately, while I've provided a couple of test cases and seen similar situations on IRC, this is very data-dependent which makes it difficult to have concrete answers for every workload. Still, I'll try and spend some time w/ pg_bench's schema definition and writing up some larger queries to run through it (aiui, the default set of queries won't typically result in a hashjoin) and see what happens there. The case that action of some kind was needed was clear, for me. Hopefully some small improvement can be found from that investigation, even if the greatest gain is in some way I really don't see a way to improve performance in this field except for introducing completely new logic (if we don't hack NTUP_PER_BUCKET). My previous patch added bloom filters as an additional layer which was checked before the actual hash join lookup, hence reducing the number of hash lookups for negatives. The size of bloom filters required for any significant performance gains and over the additional overhead added because of the additional bloom filter lookup was quite big, hence it did not make any sense to add those. Upon my discussion with Steven recently, I thought of redoing the patch based on some new tests but I haven't been really able to work on that. Regards, Atri -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tablespace options in \db+
* Magnus Hagander (mag...@hagander.net) wrote: Currently, tablespace options (such as random_page_cost) aren't shown in \db or \db+ in psql - the only way to see them is to directly query pg_tablespaces. This seems like an oversight from back when the feature was added. I realize the CF is closed, but would people be ok with me pushing this trivial patch into 9.4? It's practically a bugfix, imv. +1 from me for committing it. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Recovery inconsistencies, standby much larger than primary
Hi, On 2014-01-24 19:23:28 -0500, Greg Stark wrote: Since the point release we've run into a number of databases that when we restore from a base backup end up being larger than the primary database was. Sometimes by a large factor. The data below is from 9.1.11 (both primary and standby) but we've seen the same thing on 9.2.6. What's the procedure for creating those standbys? Were they of similar size after being cloned? primary$ for i in 1261982 1364767 1366221 473158 ; do echo -n $i ; du -shc $i* | tail -1 ; done 1261982 29G total 1364767 23G total 1366221 12G total 473158 76G total standby$ for i in 1261982 1364767 1366221 473158 ; do echo -n $i ; du -shc $i* | tail -1 ; done 1261982 55G total 1364767 28G total 1366221 17G total 473158 139G total ... The first three are btrees and the fourth is a haeap btw. Are those all of the same underlying heap relation? We're also seeing log entries about wal contains reference to invalid pages but these errors seem only vaguely correlated. Sometimes we get the errors but the tables don't grow noticeably and sometimes we don't get the errors and the tables are much larger. Uhm. I am a bit confused. You see those in the standby's log? At !debug log levels? That'd imply that the standby is dead and needed to be recloned, no? How do you continue after that? Much of the added space is uninitialized pages as you might expect but I don't understand is how the database can start up without running into the reference to invalid pages panic consistently. We check both that there are no references after consistency is reached *and* that any references before consistency are resolved by a truncate or unlink before consistency. Well, it's pretty easy to get into a situation with lot's of new pages. Lots of concurrent inserts that all fail before logging WAL. The next insert will extend the relation and only initialise that last value. It'd be interesting to look for TRUNCATE records using xlogdump. Could you show those for starters? I'm assuming this is somehow related to the mulixact or transaction wraparound problems but I don't really understand how they could be hitting when both the primary and standby are post-upgrade to the most recent point release which have the fixes That doesn't sound likely. For one the symptoms don't fit, for another, those problems are mostly 9.3+. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] shouldn't we log permission errors when accessing the configured trigger file?
Hi,
For some reason CheckForStandbyTrigger() doesn't report permission
errors when stat()int the trigger file. Shouldn't we fix that?
static bool
CheckForStandbyTrigger(void)
{
...
if (stat(TriggerFile, stat_buf) == 0)
{
ereport(LOG,
(errmsg(trigger file found: %s,
TriggerFile)));
unlink(TriggerFile);
triggered = true;
fast_promote = true;
return true;
}
Imo the stat() should warn about all errors but ENOENT?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] running make check with only specified tests
On 01/26/2014 12:08 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: I've often wanted to be able to run make check and just have it run the small number of tests I am interested in. Here's a tiny patch along those lines. It creates a new targe which I have called check-with for want of a better name. And with it I can do: $ make check-with TESTS=json jsonb The vast majority of the regression tests have interdependencies, which would make any feature along these lines fairly useless IME. (And no, I'm not interested in converting the tests to a no-dependencies style.) Also, the tests themselves don't take that long, especially in parallel mode. If you need to speed up repeated testing, it's more profitable to avoid the install/initdb overhead of a make check. I use a small script that just reinstalls the postgres executable and does make installcheck-parallel when I'm doing iterative development. I have something similar, and prodded by your email I've just improved it a bit ;-) But it doesn't work so well if you're changing the catalog, as you need an initdb anyway. And there are are some cases (the one I happen to be working on being one of them) where the tests have no prior dependencies. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GIN improvements part2: fast scan
On 26.1.2014 17:14, Heikki Linnakangas wrote: I would actually expect it to be fairly effective for that query, so that's a bit surprising. I added counters to see where the calls are coming from, and it seems that about 80% of the calls are actually coming from this little the feature I explained earlier: In addition to that, I'm using the ternary consistent function to check if minItem is a match, even if we haven't loaded all the entries yet. That's less important, but I think for something like rare1 | (rare2 frequent) it might be useful. It would allow us to skip fetching 'frequent', when we already know that 'rare1' matches for the current item. I'm not sure if that's worth the cycles, but it seemed like an obvious thing to do, now that we have the ternary consistent function. So, that clearly isn't worth the cycles :-). At least not with an expensive consistent function; it might be worthwhile if we pre-build the truth-table, or cache the results of the consistent function. Attached is a quick patch to remove that, on top of all the other patches, if you want to test the effect. Indeed, the patch significantly improved the performance. The total runtime is almost exactly the same as on 9.3 (~22 seconds for 1000 queries). The timing chart (patched vs. 9.3) is attached. A table with number of queries with duration ratio below some threshold looks like this: threshold | count | percentage - 0.5 | 3 | 0.3% 0.75 | 45 | 4.5% 0.9 |224 | 22.4% 1.0 |667 | 66.7% 1.05 |950 | 95.0% 1.1 |992 | 99.2% A ratio is just a measure of how much time it took compared to 9.3 ratio = (duration on patched HEAD) / (duration on 9.3) The table is cumulative, e.g. values in the 0.9 row mean that for 224 queries the duration with the patches was below 90% of the duration on 9.3. IMHO the table suggests with the last patch we're fine - majority of queries (~66%) is faster than on 9.3, and the tail is very short. There are just 2 queries that took more than 15% longer, compared to 9.3. And we're talking about 20ms vs. 30ms, so chances are this is just a random noise. So IMHO we can go ahead, and maybe tune this a bit more in the future. regards Tomas attachment: gin_query_durations_fixed.png -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] Client-only installation on Windows
On 01/24/2014 05:36 AM, MauMau wrote: From: Andrew Dunstan and...@dunslane.net Is there any reason why pgbench is listed in @client_program_files as well as @client_contribs? AFAICT it should only be in the latter. Thank you for reviewing the patch. Yes, you are right. I removed pgbench from @client_program_files. In addition, I added some documentation, as well as modifying the usage at the end of install.pl. I'll update the CommitFest entry shortly. Committed, with a little help from perltidy. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Storing pg_stat_statements query texts externally, pg_stat_statements in core
Peter Geoghegan p...@heroku.com writes: On Sat, Jan 25, 2014 at 2:20 PM, Tom Lane t...@sss.pgh.pa.us wrote: Well, it's fairly expensive to generate that text, in the case of a large/complex statement. It's possible that continuing to hold the lock is nonetheless the right thing to do because release+reacquire is also expensive; but there is no proof of that AFAIK, and I believe that release+reacquire is not likely to be expensive unless the lock is heavily contended, which would be exactly the conditions under which keeping it wouldn't be such a good idea anyway. My reason for only acquiring the shared lock once, and performing text normalization with it held was that in practice most query texts are not all that expensive to lex/normalize, and the cost of holding the lock for the vast majority of sessions (that won't experience contention) is nil. Meh. This line of argument seems to reduce to we don't need to worry about performance of this code path because it won't be reached often. I don't particularly buy that; it may be true in some usage patterns but probably not all. And if you *do* buy it, it can be turned around to say that we needn't worry about the costs of an extra locking cycle, anyway. However, the only thing that is really going to settle this argument is data, so here's a simple test case. I made a script that just consisted of many repetitions of \dd SELECT pg_stat_statements_reset(); and ran it in psql while tracing the system with perf. (\dd produces a query that is middlingly complex, but certainly not a patch on those I've seen produced by many real applications. The point of the reset is to force us through the stats-entry-creation code path each time.) The hits above 1%, in a non-assert-enabled backend: 13.92%34576 postmaster [kernel.kallsyms] [k] 0x8103ed21 8.64%21645 postmaster postgres [.] AllocSetAlloc 5.09%12502 postmaster postgres [.] SearchCatCache 2.37% 6025 postmaster postgres [.] MemoryContextStrdup 2.22% 5624 postmaster libc-2.12.so [.] __strcmp_sse42 1.93% 4860 postmaster postgres [.] core_yylex 1.84% 4501 postmaster postgres [.] base_yyparse 1.83% 4601 postmaster postgres [.] ScanKeywordLookup 1.54% 3893 postmaster postgres [.] copyObject 1.54% 3849 postmaster postgres [.] MemoryContextAllocZeroAligned 1.30% 3237 postmaster postgres [.] expression_tree_walker 1.23% 3069 postmaster postgres [.] palloc 1.15% 2903 postmaster libc-2.12.so [.] memcpy 1.04% 2566 postmaster postgres [.] hash_uint32 So 3.76% of the entire runtime is going into core_yylex+ScanKeywordLookup, and presumably half of that can be blamed on generate_normalized_query. On the other hand, the earliest mention of lock-related functions in the profile is 0.17% 411 postmaster postgres [.] LWLockAcquire and LWLockRelease is down here: 0.11% 264 postmaster postgres [.] LWLockRelease So on this example, the *total* cost of LWLocks, across the entire backend, is something like 15% of the cost of generate_normalized_query. This seems to me to be reasonably strong evidence that we should worry about that cost, and that releasing/reacquiring the pgss LWLock is trivial by comparison. Of course, if the lock were contended then the locking cost would go up substantially --- but that would also be a scenario in which it'd be important not to hold the lock unnecessarily. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PoC: Partial sort
On Mon, Jan 20, 2014 at 2:43 PM, Alexander Korotkov
aekorot...@gmail.com wrote:
Another changes in this version of patch:
1) Applied patch to don't compare skipCols in tuplesort by Marti Raudsepp
2) Adjusting sort bound after processing buckets.
Hi,
Here's a patch with some whitespace and coding style fixes for
partial-sort-6.patch
I tried to understand the mergejoin regression, but this code still
looks like Chinese to me. Can anyone else have a look at it?
Test case:
http://www.postgresql.org/message-id/cabrt9rdd-p2rlrdhsmq8rcob46k4a5o+bgz_up2brgeeh4r...@mail.gmail.com
Original report:
http://www.postgresql.org/message-id/CABRT9RCLLUyJ=bkeB132aVA_mVNx5==lvvvqmvuqdgufztw...@mail.gmail.com
Regards,
Marti
From a3cedb922c5a12e43ee94b9d6f5a2aefba701708 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp ma...@juffo.org
Date: Sun, 26 Jan 2014 16:25:45 +0200
Subject: [PATCH 1/2] Whitespace coding style fixes
---
src/backend/executor/nodeSort.c | 17 +
src/backend/optimizer/path/costsize.c | 8
src/backend/optimizer/path/pathkeys.c | 18 +-
src/backend/optimizer/plan/createplan.c | 2 +-
src/backend/optimizer/plan/planner.c| 6 +++---
src/backend/utils/sort/tuplesort.c | 2 +-
6 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index f38190d..2e50497 100644
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -27,13 +27,14 @@
static bool
cmpSortSkipCols(SortState *node, TupleDesc tupDesc, HeapTuple a, TupleTableSlot *b)
{
- int n = ((Sort *)node-ss.ps.plan)-skipCols, i;
+ int i,
+n = ((Sort *)node-ss.ps.plan)-skipCols;
for (i = 0; i n; i++)
{
- Datum datumA, datumB;
- bool isnullA, isnullB;
- AttrNumber attno = node-skipKeys[i].ssup_attno;
+ Datum datumA, datumB;
+ bool isnullA, isnullB;
+ AttrNumber attno = node-skipKeys[i].ssup_attno;
datumA = heap_getattr(a, attno, tupDesc, isnullA);
datumB = slot_getattr(b, attno, isnullB);
@@ -147,7 +148,7 @@ ExecSort(SortState *node)
tuplesort_set_bound(tuplesortstate, node-bound - node-bound_Done);
/*
- * Put next group of tuples where skipCols sort values are equal to
+ * Put next group of tuples where skipCols' sort values are equal to
* tuplesort.
*/
for (;;)
@@ -177,10 +178,10 @@ ExecSort(SortState *node)
}
else
{
-bool cmp;
-cmp = cmpSortSkipCols(node, tupDesc, node-prev, slot);
+bool equal;
+equal = cmpSortSkipCols(node, tupDesc, node-prev, slot);
node-prev = ExecCopySlotTuple(slot);
-if (!cmp)
+if (!equal)
break;
}
}
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 9e79c6d..3a18632 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1331,13 +1331,13 @@ cost_sort(Path *path, PlannerInfo *root,
*/
if (presorted_keys 0)
{
- List *groupExprs = NIL;
- ListCell *l;
- int i = 0;
+ List *groupExprs = NIL;
+ ListCell *l;
+ int i = 0;
foreach(l, pathkeys)
{
- PathKey *key = (PathKey *)lfirst(l);
+ PathKey *key = (PathKey *) lfirst(l);
EquivalenceMember *member = (EquivalenceMember *)
lfirst(list_head(key-pk_eclass-ec_members));
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 55d8ef4..1e1a09a 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -319,10 +319,9 @@ compare_pathkeys(List *keys1, List *keys2)
int
pathkeys_common(List *keys1, List *keys2)
{
- int n;
+ int n = 0;
ListCell *key1,
*key2;
- n = 0;
forboth(key1, keys1, key2, keys2)
{
@@ -460,7 +459,7 @@ get_cheapest_fractional_path_for_pathkeys(List *paths,
num_groups = (double *)palloc(sizeof(double) * list_length(pathkeys));
foreach(l, pathkeys)
{
- PathKey *key = (PathKey *)lfirst(l);
+ PathKey *key = (PathKey *) lfirst(l);
EquivalenceMember *member = (EquivalenceMember *)
lfirst(list_head(key-pk_eclass-ec_members));
@@ -1085,7 +1084,6 @@ find_mergeclauses_for_pathkeys(PlannerInfo *root,
List *mergeclauses = NIL;
ListCell *i;
bool *used = (bool *)palloc0(sizeof(bool) * list_length(restrictinfos));
- int k;
List *unusedRestrictinfos = NIL;
List *usedPathkeys = NIL;
@@ -1103,6 +1101,7 @@ find_mergeclauses_for_pathkeys(PlannerInfo *root,
EquivalenceClass *pathkey_ec = pathkey-pk_eclass;
List *matched_restrictinfos = NIL;
ListCell *j;
+ int k = 0;
/*--
* A mergejoin clause matches a pathkey if it has the same EC.
@@ -1140,7 +1139,6 @@ find_mergeclauses_for_pathkeys(PlannerInfo *root,
* deal with the case in create_mergejoin_plan().
*--
*/
- k = 0;
foreach(j, restrictinfos)
{
RestrictInfo *rinfo = (RestrictInfo *) lfirst(j);
@@ -1182,7 +1180,9 @@
Re: [HACKERS] running make check with only specified tests
Andrew Dunstan and...@dunslane.net writes: On 01/26/2014 12:08 PM, Tom Lane wrote: Also, the tests themselves don't take that long, especially in parallel mode. If you need to speed up repeated testing, it's more profitable to avoid the install/initdb overhead of a make check. I use a small script that just reinstalls the postgres executable and does make installcheck-parallel when I'm doing iterative development. I have something similar, and prodded by your email I've just improved it a bit ;-) But it doesn't work so well if you're changing the catalog, as you need an initdb anyway. True. OTOH, when you're changing the catalogs it seems pretty foolish to not run the whole test suite. Anyway, I have no great objection to the proposed patch, I'm just dubious that it's really worth the trouble. If you do go through with it, I'd suggest adding an installcheck-with variant. In the bikeshedding department, maybe -tests instead of -with? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] Client-only installation on Windows
Andrew Dunstan and...@dunslane.net writes: Committed, with a little help from perltidy. I think you forgot to push to master? The only recent commit I see from you is the Visual Studio 2013 fixes. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] Client-only installation on Windows
On 01/26/2014 03:14 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: Committed, with a little help from perltidy. I think you forgot to push to master? The only recent commit I see from you is the Visual Studio 2013 fixes. Oh, hell and damnation. That's going to make my life VERY difficult. Thanks for letting me know. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] Client-only installation on Windows
Andrew Dunstan and...@dunslane.net schrieb: On 01/26/2014 03:14 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: Committed, with a little help from perltidy. I think you forgot to push to master? The only recent commit I see from you is the Visual Studio 2013 fixes. Oh, hell and damnation. That's going to make my life VERY difficult. Thanks for letting me know. Why would it make things difficult? git fetch; git rebase origin/master; git push. That should be it. --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] Client-only installation on Windows
On 01/26/2014 05:04 PM, Andres Freund wrote: Andrew Dunstan and...@dunslane.net schrieb: On 01/26/2014 03:14 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: Committed, with a little help from perltidy. I think you forgot to push to master? The only recent commit I see from you is the Visual Studio 2013 fixes. Oh, hell and damnation. That's going to make my life VERY difficult. Thanks for letting me know. Why would it make things difficult? git fetch; git rebase origin/master; git push. That should be it. Maybe if I had more facility with git I'd know more what to do. But I'd merged it into another branch and then after I wound this back, rolled forward and recommitted (and this time pushed) the patch I'm not sure not how to fix that branch. I'll manage it somehow I guess. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.warn_shadow
On 26 January 2014 15:53, Pavel Stehule pavel.steh...@gmail.com wrote: 2014-01-26 Florian Pflug f...@phlo.org On Jan26, 2014, at 10:19 , Simon Riggs si...@2ndquadrant.com wrote: Also, having plpgsql.warnings_as_errors = off (default) | on makes sense and should be included in 9.4 I still think this is a bad idea, for the same reasons I don't like consistent_into (discussed in a separate thread). But these objections would go away if restricted this to function creation time only. So even with warnings_as_errors=on, you could still *call* a function that produces a warning, but not *create* one. +1 behave - and please, better name +1 to that. I guess I only saw that way of working because I was thinking of it as a compiler warning. So perhaps we should call it plpgsql.compiler_warnings_as_errors to make that behaviour more clear. plpgsql.error_on_create_warnings -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.warn_shadow
Dne 26. 1. 2014 23:24 Simon Riggs si...@2ndquadrant.com napsal(a): On 26 January 2014 15:53, Pavel Stehule pavel.steh...@gmail.com wrote: 2014-01-26 Florian Pflug f...@phlo.org On Jan26, 2014, at 10:19 , Simon Riggs si...@2ndquadrant.com wrote: Also, having plpgsql.warnings_as_errors = off (default) | on makes sense and should be included in 9.4 I still think this is a bad idea, for the same reasons I don't like consistent_into (discussed in a separate thread). But these objections would go away if restricted this to function creation time only. So even with warnings_as_errors=on, you could still *call* a function that produces a warning, but not *create* one. +1 behave - and please, better name +1 to that. I guess I only saw that way of working because I was thinking of it as a compiler warning. So perhaps we should call it plpgsql.compiler_warnings_as_errors to make that behaviour more clear. plpgsql.error_on_create_warnings I have a problém with joining word error and warning together. Some like stop_on_compilation_warning or strict_compiler sound me more logical Regards Pavel -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] plpgsql.warn_shadow
On 26 January 2014 22:44, Pavel Stehule pavel.steh...@gmail.com wrote: Dne 26. 1. 2014 23:24 Simon Riggs si...@2ndquadrant.com napsal(a): On 26 January 2014 15:53, Pavel Stehule pavel.steh...@gmail.com wrote: 2014-01-26 Florian Pflug f...@phlo.org On Jan26, 2014, at 10:19 , Simon Riggs si...@2ndquadrant.com wrote: Also, having plpgsql.warnings_as_errors = off (default) | on makes sense and should be included in 9.4 I still think this is a bad idea, for the same reasons I don't like consistent_into (discussed in a separate thread). But these objections would go away if restricted this to function creation time only. So even with warnings_as_errors=on, you could still *call* a function that produces a warning, but not *create* one. +1 behave - and please, better name +1 to that. I guess I only saw that way of working because I was thinking of it as a compiler warning. So perhaps we should call it plpgsql.compiler_warnings_as_errors to make that behaviour more clear. plpgsql.error_on_create_warnings I have a problém with joining word error and warning together. Some like stop_on_compilation_warning or strict_compiler sound me more logical Not bothered by the name, so lets wait for Marko to produce a patch and then finalise the naming. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PoC: Duplicate Tuple Elidation during External Sort for DISTINCT
What are my next steps here? I believe the concept is sound, the code is appears to work and doesn't crash, and the result does show a performance win in most cases (sometimes a big win). It's also incomplete, at least insofar as it doesn't interface with the cost models at all, etc... -- Jon -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Recovery inconsistencies, standby much larger than primary
On Sun, Jan 26, 2014 at 9:45 AM, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2014-01-24 19:23:28 -0500, Greg Stark wrote: Since the point release we've run into a number of databases that when we restore from a base backup end up being larger than the primary database was. Sometimes by a large factor. The data below is from 9.1.11 (both primary and standby) but we've seen the same thing on 9.2.6. What's the procedure for creating those standbys? Were they of similar size after being cloned? These are restored from base backup using WAL-E and then started in standby mode. The logs are retrieved using archive_command (which is WAL-E) after it retrieves lots of archived wal the database switches to streaming. We confirmed from size monitoring that the standby database grew substantially before the time it reported reaching consistent state, so I only downloaded the WAL from that range for analysis. primary$ for i in 1261982 1364767 1366221 473158 ; do echo -n $i ; du -shc $i* | tail -1 ; done 1261982 29G total 1364767 23G total 1366221 12G total 473158 76G total standby$ for i in 1261982 1364767 1366221 473158 ; do echo -n $i ; du -shc $i* | tail -1 ; done 1261982 55G total 1364767 28G total 1366221 17G total 473158 139G total ... The first three are btrees and the fourth is a haeap btw. Are those all of the same underlying heap relation? Are you asking whether the relfilenode was reused for a different relation? I doubt it. Or are you asking if the first three indexes are for the same heap (presumably the fourth one)? I don't think so but I can check. We're also seeing log entries about wal contains reference to invalid pages but these errors seem only vaguely correlated. Sometimes we get the errors but the tables don't grow noticeably and sometimes we don't get the errors and the tables are much larger. Uhm. I am a bit confused. You see those in the standby's log? At !debug log levels? That'd imply that the standby is dead and needed to be recloned, no? How do you continue after that? It's possible I'm confusing symptoms from an unrelated problem. But the symptom we saw was that it got this error, recovery crashed, then recovery started again and it replayed fine. I agree that doesn't jive with the code I see in 9.3, I didn't check how long the code was this tense though. Much of the added space is uninitialized pages as you might expect but I don't understand is how the database can start up without running into the reference to invalid pages panic consistently. We check both that there are no references after consistency is reached *and* that any references before consistency are resolved by a truncate or unlink before consistency. Well, it's pretty easy to get into a situation with lot's of new pages. Lots of concurrent inserts that all fail before logging WAL. The next insert will extend the relation and only initialise that last value. It'd be interesting to look for TRUNCATE records using xlogdump. Could you show those for starters? There are no records matching grep -i truncate in any of those extracts for those relfilenodes. I'm grepping the whole xlogdump now but it'll take a while. So far no truncates anywhere. I'm assuming this is somehow related to the mulixact or transaction wraparound problems but I don't really understand how they could be hitting when both the primary and standby are post-upgrade to the most recent point release which have the fixes That doesn't sound likely. For one the symptoms don't fit, for another, those problems are mostly 9.3+. These problems all started to appear after the latest point release btw. That could just be a coincidence of course. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] running make check with only specified tests
Tom Lane wrote: Anyway, I have no great objection to the proposed patch, I'm just dubious that it's really worth the trouble. If you do go through with it, I'd suggest adding an installcheck-with variant. In the bikeshedding department, maybe -tests instead of -with? No objection to the proposed idea either, but I wanted to point out that I had the idea sometime ago that each test would declare which other test it depended on; so if you specify one to run in isolation, the ones it depended on got run beforehand. I never got around to implementing it because running the whole bunch doesn't take that long anyway, but if for some reason you want to run a specific test over and over, it is useful. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Support for pg_stat_archiver view
On Sat, Jan 25, 2014 at 3:19 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Sat, Jan 25, 2014 at 5:41 AM, Fujii Masao masao.fu...@gmail.com wrote:
On Thu, Jan 23, 2014 at 4:10 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Thu, Jan 9, 2014 at 6:36 AM, Gabriele Bartolini
gabriele.bartol...@2ndquadrant.it wrote:
Il 08/01/14 18:42, Simon Riggs ha scritto:
Not sure I see why it needs to be an SRF. It only returns one row.
Attached is version 4, which removes management of SRF stages.
I have been looking at v4 a bit more, and found a couple of small things:
- a warning in pgstatfuncs.c
- some typos and format fixing in the sgml docs
- some corrections in a couple of comments
- Fixed an error message related to pg_stat_reset_shared referring
only to bgwriter and not the new option archiver. Here is how the new
message looks:
=# select pg_stat_reset_shared('popo');
ERROR: 22023: unrecognized reset target: popo
HINT: Target must be bgwriter or archiver
A new version is attached containing those fixes. I played also with
the patch and pgbench, simulating some archive failures and successes
while running pgbench and the reports given by pg_stat_archiver were
correct, so I am marking this patch as Ready for committer.
I refactored the patch further.
* Remove ArchiverStats global variable to simplify pgarch.c.
* Remove stats_timestamp field from PgStat_ArchiverStats struct because
it's not required.
Thanks, pgstat_send_archiver is cleaner now.
I have some review comments:
+s.archived_wals,
+s.last_archived_wal,
+s.last_archived_wal_time,
+s.failed_attempts,
+s.last_failed_wal,
+s.last_failed_wal_time,
The column names of pg_stat_archiver look not consistent at least to me.
What about the followings?
archive_count
last_archived_wal
last_archived_time
fail_count
last_failed_wal
last_failed_time
And what about archived_count and failed_count instead of respectively
archive_count and failed_count? The rest of the names are better now
indeed.
Please find attached an updated patch updated with the new column
names (in context diffs this time).
Regards,
--
Michael
*** a/doc/src/sgml/monitoring.sgml
--- b/doc/src/sgml/monitoring.sgml
***
*** 270,275 postgres: replaceableuser/ replaceabledatabase/
replaceablehost/ re
--- 270,283
/row
row
+
entrystructnamepg_stat_archiver/indextermprimarypg_stat_archiver/primary/indexterm/entry
+ entryOne row only, showing statistics about the
+WAL archiver process's activity. See
+xref linkend=pg-stat-archiver-view for details.
+ /entry
+ /row
+
+ row
entrystructnamepg_stat_bgwriter/indextermprimarypg_stat_bgwriter/primary/indexterm/entry
entryOne row only, showing statistics about the
background writer process's activity. See
***
*** 648,653 postgres: replaceableuser/ replaceabledatabase/
replaceablehost/ re
--- 656,718
/para
/note
+ table id=pg-stat-archiver-view xreflabel=pg_stat_archiver
+titlestructnamepg_stat_archiver/structname View/title
+
+tgroup cols=3
+ thead
+ row
+ entryColumn/entry
+ entryType/entry
+ entryDescription/entry
+ /row
+ /thead
+
+ tbody
+ row
+ entrystructfieldarchived_count//entry
+ entrytypebigint/type/entry
+ entryNumber of WAL files that have been successfully archived/entry
+ /row
+ row
+ entrystructfieldlast_archived_wal//entry
+ entrytypetext/type/entry
+ entryName of the last WAL file successfully archived/entry
+ /row
+ row
+ entrystructfieldlast_archived_time//entry
+ entrytypetimestamp with time zone/type/entry
+ entryTime of the last successful archive operation/entry
+ /row
+ row
+ entrystructfieldfailed_count//entry
+ entrytypebigint/type/entry
+ entryNumber of failed attempts for archiving WAL files/entry
+ /row
+ row
+ entrystructfieldlast_failed_wal//entry
+ entrytypetext/type/entry
+ entryName of the WAL file of the last failed archival
operation/entry
+ /row
+ row
+ entrystructfieldlast_failed_time//entry
+ entrytypetimestamp with time zone/type/entry
+ entryTime of the last failed archival operation/entry
+ /row
+ row
+ entrystructfieldstats_reset//entry
+ entrytypetimestamp with time zone/type/entry
+ entryTime at which these statistics were last reset/entry
+ /row
+ /tbody
+/tgroup
+ /table
+
+ para
+The structnamepg_stat_archiver/structname view will always have a
+single row, containing data about the archiver process of the cluster.
+ /para
+
table id=pg-stat-bgwriter-view xreflabel=pg_stat_bgwriter
titlestructnamepg_stat_bgwriter/structname View/title
***
*** 1613,1618
Re: [HACKERS] ALTER SYSTEM SET typos and fix for temporary file name management
Hi, Please find attached an updated patch (context diffs) improving the comments related to ALTER SYSTEM. This patch does nothing for the suffix tmp/temp used in a couple of places of the code, it only corrects some typos and makes the comments more consistent with current code. The inconsistencies with the temporary file suffixes should be tackled in a separate patch/thread. Thanks, -- Michael *** a/src/backend/utils/misc/guc-file.l --- b/src/backend/utils/misc/guc-file.l *** *** 149,155 ProcessConfigFile(GucContext context) } /* !* Parse postgresql.auto.conf file after postgresql.conf to replace * parameters set by ALTER SYSTEM command. This file is present in * data directory, however when called during initdb data directory is not * set till this point, so use ConfigFile path which will be same. --- 149,155 } /* !* Parse file PG_AUTOCONF_FILENAME after postgresql.conf to replace * parameters set by ALTER SYSTEM command. This file is present in * data directory, however when called during initdb data directory is not * set till this point, so use ConfigFile path which will be same. *** a/src/backend/utils/misc/guc.c --- b/src/backend/utils/misc/guc.c *** *** 6457,6465 flatten_set_variable_args(const char *name, List *args) } /* ! * Writes updated configuration parameter values into ! * postgresql.auto.conf.temp file. It traverses the list of parameters ! * and quote the string values before writing them to temporaray file. */ static void write_auto_conf_file(int fd, const char *filename, ConfigVariable **head_p) --- 6457,6465 } /* ! * Write updated configuration parameter values into a temporary file. ! * this function traverses the list of parameters and quotes the string ! * values before writing them. */ static void write_auto_conf_file(int fd, const char *filename, ConfigVariable **head_p) *** *** 6468,6475 write_auto_conf_file(int fd, const char *filename, ConfigVariable **head_p) StringInfoData buf; initStringInfo(buf); ! appendStringInfoString(buf, # Do not edit this file manually! \n); ! appendStringInfoString(buf, # It will be overwritten by ALTER SYSTEM command. \n); /* * write the file header message before contents, so that if there is no --- 6468,6475 StringInfoData buf; initStringInfo(buf); ! appendStringInfoString(buf, # Do not edit this file manually!\n); ! appendStringInfoString(buf, # It will be overwritten by ALTER SYSTEM command.\n); /* * write the file header message before contents, so that if there is no *** *** 6517,6523 write_auto_conf_file(int fd, const char *filename, ConfigVariable **head_p) /* * This function takes list of all configuration parameters in ! * postgresql.auto.conf and parameter to be updated as input arguments and * replace the updated configuration parameter value in a list. If the * parameter to be updated is new then it is appended to the list of * parameters. --- 6517,6523 /* * This function takes list of all configuration parameters in ! * PG_AUTOCONF_FILENAME and parameter to be updated as input arguments and * replace the updated configuration parameter value in a list. If the * parameter to be updated is new then it is appended to the list of * parameters. *** *** 6595,6607 replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p, * and write them all to the automatic configuration file. * * The configuration parameters are written to a temporary ! * file then renamed to the final name. The template for the ! * temporary file is postgresql.auto.conf.temp. * * An LWLock is used to serialize writing to the same file. * * In case of an error, we leave the original automatic ! * configuration file (postgresql.auto.conf) intact. */ void AlterSystemSetConfigFile(AlterSystemStmt * altersysstmt) --- 6595,6606 * and write them all to the automatic configuration file. * * The configuration parameters are written to a temporary ! * file then renamed to the final name. * * An LWLock is used to serialize writing to the same file. * * In case of an error, we leave the original automatic ! * configuration file (PG_AUTOCONF_FILENAME) intact. */ void AlterSystemSetConfigFile(AlterSystemStmt * altersysstmt) *** *** 6664,6671 AlterSystemSetConfigFile(AlterSystemStmt * altersysstmt) /* !* Use data directory as reference path for postgresql.auto.conf and it's !* corresponding temp file */ join_path_components(AutoConfFileName, data_directory, PG_AUTOCONF_FILENAME); canonicalize_path(AutoConfFileName); --- 6663,6670
Re: [HACKERS] ALTER SYSTEM SET typos and fix for temporary file name management
On Mon, Jan 27, 2014 at 11:29 AM, Michael Paquier michael.paqu...@gmail.com wrote: Hi, Please find attached an updated patch (context diffs) improving the comments related to ALTER SYSTEM. This patch does nothing for the suffix tmp/temp used in a couple of places of the code, it only corrects some typos and makes the comments more consistent with current code. And I almost forgot the second patch simply changing the suffix from temp to tmp for the temporary conf file... Now attached. Regards, -- Michael *** a/src/backend/replication/basebackup.c --- b/src/backend/replication/basebackup.c *** *** 834,840 sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces) /* skip auto conf temporary file */ if (strncmp(de-d_name, ! PG_AUTOCONF_FILENAME .temp, sizeof(PG_AUTOCONF_FILENAME) + 4) == 0) continue; --- 834,840 /* skip auto conf temporary file */ if (strncmp(de-d_name, ! PG_AUTOCONF_FILENAME .tmp, sizeof(PG_AUTOCONF_FILENAME) + 4) == 0) continue; *** a/src/backend/utils/misc/guc.c --- b/src/backend/utils/misc/guc.c *** *** 6671,6677 AlterSystemSetConfigFile(AlterSystemStmt * altersysstmt) canonicalize_path(AutoConfFileName); snprintf(AutoConfTmpFileName, sizeof(AutoConfTmpFileName), %s.%s, AutoConfFileName, !temp); /* * one backend is allowed to operate on postgresql.auto.conf file, to --- 6671,6677 canonicalize_path(AutoConfFileName); snprintf(AutoConfTmpFileName, sizeof(AutoConfTmpFileName), %s.%s, AutoConfFileName, !tmp); /* * one backend is allowed to operate on postgresql.auto.conf file, to -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER SYSTEM SET typos and fix for temporary file name management
On Mon, Jan 27, 2014 at 11:53 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Jan 27, 2014 at 11:29 AM, Michael Paquier michael.paqu...@gmail.com wrote: Hi, Please find attached an updated patch (context diffs) improving the comments related to ALTER SYSTEM. This patch does nothing for the suffix tmp/temp used in a couple of places of the code, it only corrects some typos and makes the comments more consistent with current code. And I almost forgot the second patch simply changing the suffix from temp to tmp for the temporary conf file... Now attached. Regards, Thanks for the patches! Committed. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Standalone synchronous master
On 01/25/2014, Josh Berkus wrote: ISTM the consensus is that we need better monitoring/administration interfaces so that people can script the behavior they want in external tools. Also, a new synchronous apply replication mode would be handy, but that'd be a whole different patch. We don't have a patch on the table that we could consider committing any time soon, so I'm going to mark this as rejected in the commitfest app. I don't feel that we'll never do auto-degrade is determinative; several hackers were for auto-degrade, and they have a good use-case argument. However, we do have consensus that we need more scaffolding than this patch supplies in order to make auto-degrade *safe*. I encourage the submitter to resumbit and improved version of this patch (one with more monitorability) for 9.5 CF1. That'll give us a whole dev cycle to argue about it. I shall rework to improve this patch. Below are the summarization of all discussions, which will be used as input for improving the patch: 1. Method of degrading the synchronous mode: a. Expose the configuration variable to a new SQL-callable functions. b. Using ALTER SYSTEM SET. c. Auto-degrade using some sort of configuration parameter as done in current patch. d. Or may be combination of above, which DBA can use depending on their use-cases. We can discuss further to decide on one of the approach. 2. Synchronous mode should upgraded/restored after at-least one synchronous standby comes up and has caught up with the master. 3. A better monitoring/administration interfaces, which can be even better if it is made as a generic trap system. I shall propose a better approach for this. 4. Send committing clients, a WARNING if they have committed a synchronous transaction and we are in degraded mode. 5. Please add more if I am missing something. Thanks and Regards, Kumar Rajeev Rastogi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Review] inherit support for foreign tables
(2014/01/25 11:27), Shigeru Hanada wrote: 2014/1/23 Etsuro Fujita fujita.ets...@lab.ntt.co.jp: Shigeru Hanada wrote: Though this would be debatable, in current implementation, constraints defined on a foreign table (now only NOT NULL and CHECK are supported) are not enforced during INSERT or UPDATE executed against foreign tables. This means that retrieved data might violates the constraints defined on local side. This is debatable issue because integrity of data is important for DBMS, but in the first cut, it is just documented as a note. I agree with you, but we should introduce a new keyword such as ASSERTIVE or something in 9.4, in preparation for the enforcement of constraints on the local side in a future release? What I'm concerned about is, otherwise, users will have to rewrite those DDL queries at that point. No? In the original thread, whether the new syntax is necessary (maybe ASSERTIVE will not be used though) is under discussion. Anyway, your point, decrease user's DDL rewrite less as possible is important. Could you post review result and/or your opinion as the reply to the original thread, then we include other people interested in this feature can share discussion. OK I'll give my opinion in that thread. For other commands recursively processed such as ANALYZE, foreign tables in the leaf of inheritance tree are ignored. I'm not sure that in the processing of the ANALYZE command, we should ignore child foreign tables. It seems to me that the recursive processing is not that hard. Rather what I'm concerned about is that if the recursive processing is allowed, then autovacuum will probably have to access to forign tables on the far side in order to acquire those sample rows. It might be undesirable from the viewpoint of security or from the viewpoint of efficiency. As you say, it's not difficult to do recursive ANALYZE including foreign tables. The reason why ANALYZE ignores descendant foreign tables is consistent behavior. At the moment, ANALYZE ignores foreign tables in top-level (non-child) when no table name was given, and if we want to ANALYZE foreign tables we need to specify explicitly. I think it's not so bad to show WARNING or INFO message about foreign table ignorance, including which is not a child. Yeah, the consistency is essential for its ease of use. But I'm not sure that inherited stats ignoring foreign tables is actually useful for query optimization. What I think about the consistency is a) the ANALYZE command with no table names skips ANALYZEing inheritance trees that include at least one foreign table as a child, but b) the ANALYZE command with a table name indicating an inheritance tree that includes any foreign tables does compute the inherited stats in the same way as an inheritance tree consiting of only ordinary tables by acquiring the sample rows from each foreign table on the far side. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER SYSTEM SET typos and fix for temporary file name management
On Mon, Jan 27, 2014 at 12:49 PM, Fujii Masao masao.fu...@gmail.com wrote: On Mon, Jan 27, 2014 at 11:53 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Jan 27, 2014 at 11:29 AM, Michael Paquier michael.paqu...@gmail.com wrote: Hi, Please find attached an updated patch (context diffs) improving the comments related to ALTER SYSTEM. This patch does nothing for the suffix tmp/temp used in a couple of places of the code, it only corrects some typos and makes the comments more consistent with current code. And I almost forgot the second patch simply changing the suffix from temp to tmp for the temporary conf file... Now attached. Regards, Thanks for the patches! Committed. Thanks. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] effective_cache_size calculation overflow
Magnus Hagander mag...@hagander.net writes: So clearly there is an overflow somewhere in the calculation of effective_cache_size, most likely from the fact that it's now dynamically calculated. Yeah. Fixed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typo fix in src/backend/catalog/README
Hi, There is a possible typo in src/backend/catalog/README and the attached fixes it. -- Amit diff --git a/src/backend/catalog/README b/src/backend/catalog/README index fce01ea..7e0ddf3 100644 --- a/src/backend/catalog/README +++ b/src/backend/catalog/README @@ -92,7 +92,7 @@ them. Thus, the variable-length fields must all be at the end, and only the variable-length fields of a catalog tuple are permitted to be NULL. For example, if you set pg_type.typrelid to be NULL, a piece of code will likely perform typetup-typrelid (or, worse, -typetyp-typelem, which follows typrelid). This will result in +typetup-typelem, which follows typrelid). This will result in random errors or even segmentation violations. Hence, do NOT insert catalog tuples that contain NULL attributes except in their variable-length portions! (The bootstrapping code is fairly good about -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Storing pg_stat_statements query texts externally, pg_stat_statements in core
On Sun, Jan 26, 2014 at 10:38 AM, Tom Lane t...@sss.pgh.pa.us wrote: Meh. This line of argument seems to reduce to we don't need to worry about performance of this code path because it won't be reached often. I think I may have over-elaborated, giving you the false impression that this was something I felt strongly about. I'm glad that the overhead has been shown to be quite low, and I think that lexing without the lock held will be fine. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] missing windows client only installation
Hello One window user reported unpleasant issue - there is not official client only installation for PostgreSQL for windows similar to Oracle or D2 client only installation. I know so this issue can be solved by pgAdmin or ODBC driver installation, but still it is a issue for users without good PostgreSQL knowledges Regards Pavel Stehule.
Re: [HACKERS] inherit support for foreign tables
(2014/01/22 4:09), Robert Haas wrote: On Mon, Jan 20, 2014 at 9:44 PM, Shigeru Hanada shigeru.han...@gmail.com wrote: Thanks for the comments. 2014/1/21 KaiGai Kohei kai...@ak.jp.nec.com: In addition, an idea which I can't throw away is to assume that all constraints defined on foreign tables as ASSERTIVE. Foreign tables potentially have dangers to have wrong data by updating source data not through foreign tables. This is not specific to an FDW, so IMO constraints defined on foreign tables are basically ASSERTIVE. Of course PG can try to maintain data correct, but always somebody might break it. qu Does it make sense to apply assertive CHECK constraint on the qual of ForeignScan to filter out tuples with violated values at the local side, as if row-level security feature doing. It enables to handle a situation that planner expects only clean tuples are returned but FDW driver is unavailable to anomalies. Probably, this additional check can be turned on/off on the fly, if FDW driver has a way to inform the core system its capability, like FDW_CAN_ENFORCE_CHECK_CONSTRAINT that informs planner to skip local checks. Hmm, IIUC you mean that local users can't (or don't need to) know that data which violates the local constraints exist on remote side. Applying constraints to the data which is modified through FDW would be necessary as well. In that design, FDW is a bidirectional filter which provides these features: 1) Don't push wrong data into remote data source, by applying local constraints to the result of the modifying query executed on local PG. This is not perfect filter, because remote constraints don't mapped automatically or perfectly (imagine constraints which is available on remote but is not supported in PG). 2) Don't retrieve wrong data from remote to local PG, by applying local constraints I have a concern about consistency. It has not been supported, but let's think of Aggregate push-down invoked by a query below. SELECT count(*) FROM remote_table; If this query was fully pushed down, the result is the # of records exist on remote side, but the result would be # of valid records when we don't push down the aggregate. This would confuse users. Besides CHECK constraints, currently NOT NULL constraints are virtually ASSERTIVE (not enforcing). Should it also be noted explicitly? Backward compatibility…. Yep, backward compatibility (especially visible ones to users) should be minimal, ideally zero. NOT NULL [ASSERTIVE] might be an option. Treating [ASSERTIVE | NOT ASSERTIVE] like DEFERRABLE, and allow ingASSERTIVE for only foreign tables? It makes sense, though we need consider exclusiveness . But It needs to default to ASSERTIVE on foreign tables, and NOT ASSERTIVE (means forced) on others. Isn't is too complicated? CREATE FOREIGN TABLE foo ( id int NOT NULL ASSERTIVE CHECK (id 1) ASSERTIVE, … CONSTRAINT chk_foo_name_upper CHECK (upper(name) = name) ASSERTIVE ) SERVER server; BTW, I noticed that this is like push-down-able expressions in JOIN/WHERE. We need to check a CHECK constraint defined on a foreign tables contains only expressions which have same semantics as remote side (in practice, built-in and immutable)? I don't think that that ASSERTIVE is going to fly, because assertive means (sayeth the Google) having or showing a confident and forceful personality, which is not what we mean here. It's tempting to do something like try to replace the keyword check with assume or assert or (stretching) assertion, but that would require whichever one we picked to be a fully-reserved keyword, which I can't think is going to get much support here, for entirely understandable reasons. So I think we should look for another option. Currently, constraints can be marked NO INHERIT (though this seems to have not been fully documented, as the ALTER TABLE page doesn't mention it anywhere) or NOT VALID, so I'm thinking maybe we should go with something along those lines. Some ideas: - NO CHECK. The idea of writing CHECK (id 1) NO CHECK is pretty hilarious, though. - NO VALIDATE. But then people need to understand that NOT VALID means we didn't validate it yet while no validate means we don't ever intend to validate it, which could be confusing. - NO ENFORCE. Requires a new (probably unreserved) keyword. - NOT VALIDATED or NOT CHECKED. Same problems as NO CHECK and NO VALIDATE, respectively, plus now we have to create a new keyword. Another idea is to apply an extensible-options syntax to constraints, like we do for EXPLAIN, VACUUM, etc. Like maybe: CHECK (id 1) OPTIONS (enforced false, valid true) Yet another idea is to consider validity a three-state property: either the constraint is valid (because we have checked it and are enforcing it), or it is not valid (because we are enforcing it but have not checked the pre-existing data), or it is assumed true (because we are not checking or enforcing it but are believing it anyway). So then we
Re: [HACKERS] Typo fix in src/backend/catalog/README
On 01/27/2014 07:22 AM, Amit Langote wrote: There is a possible typo in src/backend/catalog/README and the attached fixes it. Thanks, fixed. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.warn_shadow
Putting this and all future options as keywords seems like a poor choice. Indeed, the # syntax proposed isn't even fully described on list here, nor are examples given in tests. My feeling is that nobody even knows that is being proposed and would likely cause more discussion if they did. So I wish to push back the # syntax to a later release when it has had more discussion. It would be good if you could lead that discussion in later releases. I am returning back to #option syntax a) it should not be plpgsql keywords b) it can be nice if validity can be verified by plpgsql plugins and used by plpgsql plugins much more. Now we can use only GUC for plugin parametrization, but it is not readable as #option it is. Regards Pavel
Re: [HACKERS] Memory leak in PL/pgSQL function which CREATE/SELECT/DROP a temporary table
On 01/25/2014 11:36 PM, Bruce Momjian wrote: On Tue, Jun 18, 2013 at 09:07:59PM +0300, Heikki Linnakangas wrote: Hmm. I could repeat this, and it seems that the catcache for pg_statistic accumulates negative cache entries. Those slowly take up the memory. Digging a bit deeper, this is a rather common problem with negative catcache entries. In general, nothing stops you from polluting the cache with as many negative cache entries as you like. Just do select * from table_that_doesnt_exist for as many non-existent table names as you want, for example. Those entries are useful at least in theory; they speed up throwing the error the next time you try to query the same non-existent table. But there is a crucial difference in this case; the system created a negative cache entry for the pg_statistic row of the table, but once the relation is dropped, the cache entry keyed on the relation's OID, is totally useless. It should be removed. We have this problem with a few other catcaches too, which have what is effectively a foreign key relationship with another catalog. For example, the RELNAMENSP catcache is keyed on pg_class.relname, pg_class.relnamespace, yet any negative entries are not cleaned up when the schema is dropped. If you execute this repeatedly in a session: CREATE SCHEMA foo; SELECT * from foo.invalid; -- throws an error DROP SCHEMA foo; it will leak similarly to the original test case, but this time the leak is into the RELNAMENSP catcache. To fix that, I think we'll need to teach the catalog cache about the relationships between the caches. Is this a TODO? Yes, I think so. Added. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP patch (v2) for updatable security barrier views
Hello,
I checked the latest updatable security barrier view patch.
Even though I couldn't find a major design problem in this revision,
here are two minor comments below.
I think, it needs to be reviewed by committer to stick direction
to implement this feature. Of course, even I know Tom argued the
current design of this feature on the up-thread, it does not seem
to me Dean's design is not reasonable.
Below is minor comments of mine:
@@ -932,9 +938,32 @@ inheritance_planner(PlannerInfo *root)
if (final_rtable == NIL)
final_rtable = subroot.parse-rtable;
else
- final_rtable = list_concat(final_rtable,
+ {
+ List *tmp_rtable = NIL;
+ ListCell *cell1, *cell2;
+
+ /*
+* Planning this new child may have turned some of the original
+* RTEs into subqueries (if they had security barrier quals). If
+* so, we want to use these in the final rtable.
+*/
+ forboth(cell1, final_rtable, cell2, subroot.parse-rtable)
+ {
+ RangeTblEntry *rte1 = (RangeTblEntry *) lfirst(cell1);
+ RangeTblEntry *rte2 = (RangeTblEntry *) lfirst(cell2);
+
+ if (rte1-rtekind == RTE_RELATION
+ rte1-securityQuals != NIL
+ rte2-rtekind == RTE_SUBQUERY)
+ tmp_rtable = lappend(tmp_rtable, rte2);
+ else
+ tmp_rtable = lappend(tmp_rtable, rte1);
+ }
Do we have a case if rte1 is regular relation with securityQuals but
rte2 is not a sub-query? If so, rte2-rtekind == RTE_SUBQUERY should
be a condition in Assert, but the third condition in if-block.
In case when a sub-query is simple enough; no qualifier and no projection
towards underlying scan, is it pulled-up even if this sub-query has
security-barrier attribute, isn't it?
See the example below. The view v2 is defined as follows.
postgres=# CREATE VIEW v2 WITH (security_barrier) AS SELECT * FROM t2 WHERE x %
10 = 5;
CREATE VIEW
postgres=# EXPLAIN SELECT * FROM v2 WHERE f_leak(z);
QUERY PLAN
-
Subquery Scan on v2 (cost=0.00..3.76 rows=1 width=41)
Filter: f_leak(v2.z)
- Seq Scan on t2 (cost=0.00..3.50 rows=1 width=41)
Filter: ((x % 10) = 5)
(4 rows)
postgres=# EXPLAIN SELECT * FROM v2;
QUERY PLAN
---
Seq Scan on t2 (cost=0.00..3.50 rows=1 width=41)
Filter: ((x % 10) = 5)
(2 rows)
The second explain result shows the underlying sub-query is
pulled-up even though it has security-barrier attribute.
(IIRC, it was a new feature in v9.3.)
On the other hand, this kind of optimization was not applied
on a sub-query being extracted from a relation with securityQuals
postgres=# EXPLAIN UPDATE v2 SET z = z;
QUERY PLAN
Update on t2 t2_1 (cost=0.00..3.51 rows=1 width=47)
- Subquery Scan on t2 (cost=0.00..3.51 rows=1 width=47)
- Seq Scan on t2 t2_2 (cost=0.00..3.50 rows=1 width=47)
Filter: ((x % 10) = 5)
(4 rows)
If it has no security_barrier option, the view reference is extracted
in the rewriter stage, it was pulled up as we expected.
postgres=# ALTER VIEW v2 RESET (security_barrier);
ALTER VIEW
postgres=# EXPLAIN UPDATE t2 SET z = z;
QUERY PLAN
---
Update on t2 (cost=0.00..3.00 rows=100 width=47)
- Seq Scan on t2 (cost=0.00..3.00 rows=100 width=47)
(2 rows)
Probably, it misses something to be checked and applied when we extract
a sub-query in prepsecurity.c.
# BTW, which code does it pull up? pull_up_subqueries() is not.
Thanks,
-Original Message-
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Dean Rasheed
Sent: Thursday, January 23, 2014 7:06 PM
To: Kaigai, Kouhei(海外, 浩平)
Cc: Craig Ringer; Tom Lane; PostgreSQL Hackers; Kohei KaiGai; Robert Haas;
Simon Riggs
Subject: Re: [HACKERS] WIP patch (v2) for updatable security barrier views
On 21 January 2014 09:18, Dean Rasheed dean.a.rash...@gmail.com wrote:
Yes, please review the patch from 09-Jan
(http://www.postgresql.org/message-id/CAEZATCUiKxOg=vOOvjA2S6G-sixzzxg
18totggp8zobq6qn...@mail.gmail.com).
After further testing I found a bug --- it involves having a security barrier
view on top of a base relation that has a rule that rewrites the query to
have a different result relation, and possibly also a different command
type, so that the securityQuals are no longer on the result relation, which
is a code path not previously tested and the rowmark handling was wrong.
That's probably a pretty obscure case in the context of security barrier
views, but that code path would be used much more commonly
