[HACKERS] Detecting loss of client connection (was Re: Bug in walsender when calling out to do_pg_stop_backup (and others?))
Thinking about this some more, why don't we just elog(FATAL) in
internal_flush(), if the write fails, instead of setting the flag and
waiting for the next CHECK_FOR_INTERRUPTS(). That sounds scary at first,
but why not?
There's this comment in internal_flush():
/*
* Careful: an ereport() that tries to write to the
client would
* cause recursion to here, leading to stack overflow
and core
* dump! This message must go *only* to the postmaster
log.
That's understandable.
* If a client disconnects while we're in the midst of
output, we
* might write quite a bit of data before we get to a
safe query
* abort point. So, suppress duplicate log messages.
But what about this? Tracing back the callers, I don't see any that
would be upset if we just threw an error there. One scary aspect is if
you're within a critical section, but I don't think we currently send
any messages while in a critical section. And we could refrain from
throwing the error if we're in a critical section, to be safe.
*/
if (errno != last_reported_send_errno)
{
last_reported_send_errno = errno;
ereport(COMMERROR,
(errcode_for_socket_access(),
errmsg(could not send data to
client: %m)));
}
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
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] Notes on implementing URI syntax for libpq
On Tue, Nov 29, 2011 at 12:02 PM, Alexander Shulgin
a...@commandprompt.com wrote:
Excerpts from Alexander Shulgin's message of Sat Nov 26 22:07:21 +0200 2011:
So how about this:
postgresql:ssl://user:pw@host:port/dbname?sslmode=...
The postgresql:ssl:// designator would assume sslmode=require, if not
overriden in extra parameters and postgresql:// would imply
sslmode=prefer. And to disable SSL you would pick either designator and
append sslmode=disable.
The JDBC's ssl=true will translate to sslmode=require.
Hey, I'm going to assume no objections equals positive feedback and
continue hacking in this direction.
A couple of cents on this:
I think the current direction is fine, although as Robert Haas has
said, I am not really at all inclined to view JDBC compatibility as
any kind of a plus. JDBC URLs are weird, and do the drivers actually
link libpq anyway? That world is unto itself. Looking like a normal
URL to the greater body of URL-dom seems like a much more desirable
design trait to me, and after that decreasing the number of ways to
write the same thing. So a weak -1 from me on adding two ways to do
the same thing, of which the way to do it is weird by URL standards.
At best I'd try to avoid choosing any notations that clash or visually
confuse the URLs with their JDBC doppelgangers, and I think the more
URL-ish notation is polite to JDBC in that regard.
Here's a couple of URIs used by projects that do generally end up
delegating to libpq-based drivers. It is precisely this slight
fragmentation that I'd like to see put to rest by this feature in
libpq.
Sequel, for Ruby (all valid):
DB = Sequel.connect('postgres://user:password@localhost/blog') # Uses
the postgres adapter
DB = Sequel.connect('postgres://localhost/blog?user=userpassword=password')
DB = Sequel.connect('postgres://localhost/blog' :user='user',
:password='password')
Amazingly, I don't find it trivial to find the format ActiveRecord
(part of Rails) spelled out, but here's one thing that works, at least
(same as Sequel, format one)
postgres://username:password@localhost/myrailsdb
Django: Doesn't use URLs at all, preferring dictionary structures, as
far as I can tell.
SQLAlchemy, Python:
dialect+driver://user:password@host/dbname[?key=value..]
I'm not familiar enough with the Perl and TCL worlds to comment, nor
some of the newcomers like Golang or Node.js.
Dialect would be 'postgresql', driver 'psycopg2', but at the libpq
level clearly that wouldn't make much sense, so it's basically Sequel
format one-compatible, except it goes by postgresql rather than
postgres for the dialect.
I do really like the attention paid to somehow making AF_UNIX sockets
work; they're great for development. I agree a different scheme would
be hard to swallow, but unfortunately the options to pack that
information into URL notation is at least a little ugly, as far as I
can tell. Using a different scheme is probably not worth the cost of
an ugly URL string, in my book.
Are there any provisions for choosing X.509/cert authentication? I
imagine not, but out-of-band presentation of that information is the
norm there, and I'm not sure if is any room for improvement within
reach.
If we can decide on this, we should also put reasonable effort into making
JDBC support the same syntax.
What would be our plan on this? Since the syntax proposed here is strictly a
superset of the existing JDBC syntax, I would think this qualifies as an
improvement and it would be backwards compatible with any previous version of
the JDBC connector.
I suppose that is nice, but is this designed, or coincidental? Is
there any fundamental reason why the JDBC driver will remain so
similar to libpq in the future? Will people realistically be able to
use one URL across their Java and libpq projects in most situations,
now and in the forseeable future, including the keyword options?
Because as soon as one encounters the need to maintain two URLs for
any reason, the otherwise real convenience regresses into bloat.
So there's my pile of opinions.
--
fdr
--
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] Patch for cursor calling with named parameters
Kevin, Thank you for your review! On 2011-12-03 21:53, Kevin Grittner wrote: (1) Doc changes: (a) These contain some unnecessary whitespace changes which make it harder to see exactly what changed. This is probably caused because I used emacs's fill-paragraph (alt+q) command, after some changes. If this is against policy, I could change the additions in such a way as to cause minor differences, however I believe that the benefit of that ends immediately after review. (b) There's one point where cursors should be possessive cursor's. (c) I think it would be less confusing to be consistent about mentioning positional and named in the same order each time. Mixing it up like that is harder to read, at least for me. Good point, both changed. (2) The error for mixing positional and named parameters should be moved up. That seems more important than too many arguments or provided multiple times if both are true. I moved the error up, though I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. This is also in line with calling functions, where mixing named and positional is allowed, as long as the positional arguments are first. Personally I have no opinion what is best, include the feature or give an error, and I removed the possibility during the previous commitfest. (3) The -- END ADDITIONAL TESTS comment shouldn't be added to the regression tests. This is removed, also the -- START ADDITIONAL TESTS, though I kept the tests between those to comments. The way this parses out the named parameters and then builds the positional list, with some code from read_sql_construct() repeated in read_cursor_args() seems a little bit clumsy to me, but I couldn't think of a better way to do it. Same here. The new patch is attached. regards Yeb Havinga diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml new file mode 100644 index f33cef5..a3e6540 *** a/doc/src/sgml/plpgsql.sgml --- b/doc/src/sgml/plpgsql.sgml *** OPEN curs1 FOR EXECUTE 'SELECT * FROM ' *** 2823,2833 /para /sect3 ! sect3 titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional; /synopsis para --- 2823,2833 /para /sect3 ! sect3 id=plpgsql-open-bound-cursor titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional; /synopsis para *** OPEN replaceablebound_cursorvar/repla *** 2847,2856 --- 2847,2867 /para para + Cursors may be opened using either firsttermpositional/firstterm + or firsttermnamed/firstterm notation. In contrast with calling + functions, described in xref linkend=sql-syntax-calling-funcs, it + is not allowed to mix positional and named notation. In positional + notation, all arguments are specified in order. In named notation, + each argument's name is specified using literal:=/literal to + separate it from the argument expression. + /para + + para Examples (these use the cursor declaration examples above): programlisting OPEN curs2; OPEN curs3(42); + OPEN curs3(key := 42); /programlisting /para *** COMMIT; *** 3169,3186 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP replaceablestatements/replaceable END LOOP optional replaceablelabel/replaceable /optional; /synopsis The cursor variable must have been bound to some query when it was ! declared, and it emphasiscannot/ be open already. The ! commandFOR/ statement automatically opens the cursor, and it closes ! the cursor again when the loop exits. A list of actual argument value ! expressions must appear if and only if the cursor was declared to take ! arguments. These values will be substituted in the query, in just ! the same way as during an commandOPEN/. The variable replaceablerecordvar/replaceable is automatically defined as type typerecord/ and exists only inside the loop (any existing definition of the variable name is ignored within the loop). --- 3180,3203 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable
Re: [HACKERS] Inlining comparators as a performance optimisation
On 05.12.2011 02:14, Peter Geoghegan wrote:
On 4 December 2011 19:17, Tom Lanet...@sss.pgh.pa.us wrote:
I have not done any performance testing on this patch, but it might be
interesting to check it with the same test cases Peter's been using.
I've attached a revision of exactly the same benchmark run to get the
results in results_server.ods .
You'll see very similar figures to results_server.ods for HEAD and for
my patch, as you'd expect. I think the results speak for themselves. I
maintain that we should use specialisations - that's where most of the
benefit is to be found.
I'm still not convinced the big gain is in inlining the comparison
functions. Your patch contains a few orthogonal tricks, too:
* A fastpath for the case of just one scankey. No reason we can't do
that without inlining.
* inlining the swap-functions within qsort. Thaẗ́'s different from
inlining the comparison functions, and could be done regardless of the
data type. The qsort element size in tuplesort.c is always sizeof(SortTuple)
And there's some additional specializations we can do, not implemented
in your patch, that don't depend on inlining the comparison functions:
* A fastpath for the case of no NULLs
* separate comparetup functions for ascending and descending sorts (this
allows tail call elimination of the call to type-specific comparison
function in the ascending version)
* call CHECK_FOR_INTERRUPTS() less frequently.
To see how much difference those things make, I hacked together the
attached patch. I didn't base this on Robert's/Tom's patch, but instead
just added a quick dirty FunctionCallInvoke-overhead-skipping version
of btint4cmp. I believe that aspect of this patch it's similar in
performance characteristics, though.
In my quick tests, it gets quite close in performance to your inlining
patch, when sorting int4s and the input contains no NULLs. But please
give it a try in your test environment, to get numbers comparable with
your other tests.
Perhaps you can get even more gain by adding the no-NULLs, and asc/desc
special cases to your inlining patch, too, but let's squeeze all the fat
out of the non-inlined version first. One nice aspect of many of these
non-inlining optimizations is that they help with external sorts, too.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 3505236..85bec20 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -114,6 +114,8 @@
#include utils/rel.h
#include utils/tuplesort.h
+#include utils/builtins.h
+
/* sort-type codes for sort__start probes */
#define HEAP_SORT 0
@@ -273,6 +275,8 @@ struct Tuplesortstate
int memtupcount; /* number of tuples currently present */
int memtupsize; /* allocated length of memtuples array */
+ bool hasnulls_initial; /* any NULLs in the first key col? */
+
/*
* While building initial runs, this is the current output run number
* (starting at 0). Afterwards, it is the number of initial runs we made.
@@ -341,6 +345,8 @@ struct Tuplesortstate
TupleDesc tupDesc;
ScanKey scanKeys; /* array of length nKeys */
+ int (*comparator) (Datum, Datum);
+
/*
* These variables are specific to the CLUSTER case; they are set by
* tuplesort_begin_cluster. Note CLUSTER also uses tupDesc and
@@ -459,6 +465,11 @@ static unsigned int getlen(Tuplesortstate *state, int tapenum, bool eofOK);
static void markrunend(Tuplesortstate *state, int tapenum);
static int comparetup_heap(const SortTuple *a, const SortTuple *b,
Tuplesortstate *state);
+static int comparetup_heap_1key_asc_nonulls(const SortTuple *a, const SortTuple *b,
+Tuplesortstate *state);
+static int comparetup_heap_1key_asc_nonulls_btint4cmp(const SortTuple *a, const SortTuple *b,
+Tuplesortstate *state);
+static int compare_int4(Datum first, Datum second);
static void copytup_heap(Tuplesortstate *state, SortTuple *stup, void *tup);
static void writetup_heap(Tuplesortstate *state, int tapenum,
SortTuple *stup);
@@ -551,6 +562,7 @@ tuplesort_begin_common(int workMem, bool randomAccess)
state-availMem = state-allowedMem;
state-sortcontext = sortcontext;
state-tapeset = NULL;
+ state-hasnulls_initial = false;
state-memtupcount = 0;
state-memtupsize = 1024; /* initial guess */
@@ -1247,11 +1259,41 @@ tuplesort_performsort(Tuplesortstate *state)
* amount of memory. Just qsort 'em and we're done.
*/
if (state-memtupcount 1)
+ {
+int (*comparetup) (const SortTuple *a, const SortTuple *b,
+ Tuplesortstate *state);
+
+/* If there is only one key col, the input contains no NULLs,
+ * and it's ascending, we can use a fastpath version of
+ * comparetup_heap that skips the over those things.
+ */
+if (state-comparetup == comparetup_heap
+ state-nKeys == 1
+ !(state-scanKeys[0].sk_flags SK_BT_DESC)
+
Re: [HACKERS] planner fails on HEAD
On Sun, Dec 4, 2011 at 4:55 PM, Tom Lane t...@sss.pgh.pa.us wrote: Pavel Stehule pavel.steh...@gmail.com writes: it looks like gcc bug - gcc 4.5.1 20100924 (Red Hat 4.5.1) It was configured just with --enable-debug and --enable-cassert Is this x86? I can't reproduce it on x86_64. reading all the comments in the gcc bug report, this is because x86 targets the x87 fpu by default which is where the bug is -- it's a hardware problem. x86_64 targets sse which has stricter standards for rounding. most x86 processors support sse -- is there a reason why we don't target sse? merlin -- 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] PostgreSQL fails to build with 32bit MinGW-w64
Hi, If we are not to use 64 bit file size (and time), #undef stat may be sufficient. The #undef should be before the prototype of pgwin32_safestat because the #define stat _stat64 affect both the function and struct stat. The #undef stat necessitate #undef fstat as the parameter struct stat * is changed. Additional change are for the macro redefinition warnings. (Suppress warnings, but perhaps not very different) The patch is tested to compile on x86_64-w64-mingw32-gcc 4.7.0 20111203 (experimental) and gcc version 4.6.1 on MingW/MSYS --- a/src/include/port.h +++ b/src/include/port.h @@ -334,6 +334,12 @@ extern bool rmtree(const char *path, bool rmtopdir); */ #if defined(WIN32) !defined(__CYGWIN__) !defined(UNSAFE_STAT_OK) #include sys/stat.h +#ifdef stat +#undef stat +#endif +#ifdef fstat +#undef fstat +#endif extern int pgwin32_safestat(const char *path, struct stat * buf); #define stat(a,b) pgwin32_safestat(a,b) If this is not sufficient, we might need to change all call of stat, lstat, and fstat to some wrapper functions? : It's theoretically doable, but could be quite difficult for a huge software. pgsql-mingw64-patch.diff Description: Binary data On 2011/12/05, at 1:10, Andrew Dunstan wrote: On 12/04/2011 06:31 AM, Magnus Hagander wrote: On Sun, Dec 4, 2011 at 09:14, NISHIYAMA Tomoaki tomoa...@staff.kanazawa-u.ac.jp wrote: Hi, I found error on #define stat _stat64 occurs on Mingw-w64 (x86_64-w64-mingw32) gcc version 4.7.0 20111203 (experimental) (GCC) The code can be compiled with diff --git a/src/include/port.h b/src/include/port.h index eceb4bf..78d5c92 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -332,7 +332,7 @@ extern bool rmtree(const char *path, bool rmtopdir); * Some frontends don't need the size from stat, so if UNSAFE_STAT_OK * is defined we don't bother with this. */ -#if defined(WIN32) !defined(__CYGWIN__) !defined(UNSAFE_STAT_OK) +#if defined(WIN32_ONLY_COMPILER) !defined(UNSAFE_STAT_OK) #includesys/stat.h extern int pgwin32_safestat(const char *path, struct stat * buf); but, surely we need to know if it is ok or not as the comment before says: * stat() is not guaranteed to set the st_size field on win32, so we * redefine it to our own implementation that is. Is there any simple test program that determines if the pgwin32_safestat is required or the library stat is sufficient? I presume the stat is a library function and therefore it depends on the compiler rather than the WIN32 platform as a whole. No, stat() is unreliable because it is implemented on top of FindNextFile(), and *that's* where the actual problem is at. And that's an OS API function, not a library function. See the discussion at http://archives.postgresql.org/pgsql-hackers/2008-03/msg01181.php In theory, if mingw implemented their stat() without using FindNextFile(), it might work - but I don't see how they'd do it in that case. And I can't see us going into details to remove such a simple workaround even if they do - it's better to ensure we work the same way with different compilers. Yeah. This is only a problem because, with this compiler, configure finds this: checking for _FILE_OFFSET_BITS value needed for large files... 64 checking size of off_t... 8 whereas on the mingw-w64 compiler pitta is using it finds this: checking for _FILE_OFFSET_BITS value needed for large files... unknown checking for _LARGE_FILES value needed for large files... unknown checking size of off_t... 4 It's the setting of _FILE_OFFSET_BITS that causes the offending macro definition. Can we just turn off largefile support for this compiler, or maybe for all mingw builds, possibly by just disabling the checks in configure.in? I note it's turned off for MSVC in all flavors apparently. pgwin32_safestat() isn't safe for large files anyway, so there would be good grounds for doing so quite apart from this, ISTM. (Of course, we should work out how to handle large files properly on Windows, but that's a task for another day.) (BTW, someone asked me the other day why anyone would want to do 32 bit builds. One answer is that often the libraries you want to link with are only available in 32 bit versions.) 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 -- 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] Command Triggers
On Sat, Dec 03, 2011 at 01:26:22AM +0100, Andres Freund wrote: On Saturday, December 03, 2011 01:09:48 AM Alvaro Herrera wrote: Excerpts from Andres Freund's message of vie dic 02 19:09:47 -0300 2011: Hi all, There is also the point about how permission checks on the actual commands (in comparison of modifying command triggers) and such are handled: BEFORE and INSTEAD will currently be called independently of the fact whether the user is actually allowed to do said action (which is inconsistent with data triggers) and indepentent of whether the object they concern exists. I wonder if anybody considers that a problem? Hmm, we currently even have a patch (or is it already committed?) to avoid locking objects before we know the user has permission on the object. Getting to the point of calling the trigger would surely be even worse. Well, calling the trigger won't allow them to lock the object. It doesn't even confirm the existance of the table. didn't I see a discussion in passing about the possibility of using these command triggers to implement some aspects of se-pgsql? In that case, you'd need the above behavior. Ross -- Ross Reedstrom, Ph.D. reeds...@rice.edu Systems Engineer Admin, Research Scientistphone: 713-348-6166 Connexions http://cnx.orgfax: 713-348-3665 Rice University MS-375, Houston, TX 77005 GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE -- 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] planner fails on HEAD
Merlin Moncure mmonc...@gmail.com writes: On Sun, Dec 4, 2011 at 4:55 PM, Tom Lane t...@sss.pgh.pa.us wrote: Pavel Stehule pavel.steh...@gmail.com writes: it looks like gcc bug - gcc 4.5.1 20100924 (Red Hat 4.5.1) It was configured just with --enable-debug and --enable-cassert Is this x86? I can't reproduce it on x86_64. reading all the comments in the gcc bug report, this is because x86 targets the x87 fpu by default which is where the bug is -- it's a hardware problem. x86_64 targets sse which has stricter standards for rounding. most x86 processors support sse -- is there a reason why we don't target sse? Well, older machines won't have sse, and in any case I think x86 is not the only architecture with the issue, just the most popular one. Floating-point registers that are wider than standard double are hardly an unusual idea. 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] [REVIEW] Patch for cursor calling with named parameters
Yeb Havinga yebhavi...@gmail.com wrote: On 2011-12-03 21:53, Kevin Grittner wrote: (1) Doc changes: (a) These contain some unnecessary whitespace changes which make it harder to see exactly what changed. This is probably caused because I used emacs's fill-paragraph (alt+q) command, after some changes. If this is against policy, I could change the additions in such a way as to cause minor differences, however I believe that the benefit of that ends immediately after review. I don't know whether it's a hard policy, but people usually minimize whitespace changes in such situations, to make it easier for the reviewer and committer to tell what really changed. The committer can always reformat after looking that over, if they feel that's useful. The issue is small enough here that I'm not inclined to consider it a blocker for sending to the committer. (2) The error for mixing positional and named parameters should be moved up. That seems more important than too many arguments or provided multiple times if both are true. I moved the error up, though I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. This is also in line with calling functions, where mixing named and positional is allowed, as long as the positional arguments are first. Personally I have no opinion what is best, include the feature or give an error, and I removed the possibility during the previous commitfest. If there's no technical reason not to allow them to be mixed, I would tend to favor consistency with parameter calling rules. Doing otherwise seems like to result in confusion and bug reports. How do others feel? -Kevin -- 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] cannot read pg_class without having selected a database / is this a bug?
On Sun, Dec 4, 2011 at 4:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: Tomas Vondra t...@fuzzy.cz writes: What about the pg_stat_activity - is it safe to access that from auth hook or is that just a coincidence that it works (and might stop working in the future)? It doesn't seem like a good thing to rely on. There's certainly no testing being done that would cause us to notice if it stopped working so early in backend startup. I'm still puzzled that Tomas got it working at all. If MyDatabaseId hasn't been set yet, the how did we manage to build a relcache entry for anything - let alone an unshared catalog? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] missing rename support
On Sat, Dec 3, 2011 at 4:46 PM, Peter Eisentraut pete...@gmx.net wrote: I noticed the following object types don't have support for an ALTER ... RENAME command: DOMAIN (but ALTER TYPE works) FOREIGN DATA WRAPPER OPERATOR RULE SERVER Are there any restrictions why these couldn't be added? I don't think so. There's no ALTER RULE command; should we add one (matching ALTER TRIGGER) or make this part of ALTER TABLE? I don't think constraints can be renamed either, which should probably be addressed along with rules. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] cannot read pg_class without having selected a database / is this a bug?
Robert Haas robertmh...@gmail.com writes: I'm still puzzled that Tomas got it working at all. If MyDatabaseId hasn't been set yet, the how did we manage to build a relcache entry for anything - let alone an unshared catalog? Well, he wasn't actually issuing a SQL query, just calling some of the pgstat.c subroutines that underlie the view. It happens that the pgstat module has no backend-local initialization (at the moment, and discounting the issue of making the process's own pgstat_activity entry), so they were happy enough. It was the syscache stuff that was spitting up. 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] missing rename support
Robert Haas robertmh...@gmail.com writes: I don't think so. There's no ALTER RULE command; should we add one (matching ALTER TRIGGER) or make this part of ALTER TABLE? I don't think constraints can be renamed either, which should probably be addressed along with rules. Note that renaming an index-based constraint should also rename the index. I believe the other direction works already. 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] cannot read pg_class without having selected a database / is this a bug?
On Mon, Dec 5, 2011 at 10:46 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I'm still puzzled that Tomas got it working at all. If MyDatabaseId hasn't been set yet, the how did we manage to build a relcache entry for anything - let alone an unshared catalog? Well, he wasn't actually issuing a SQL query, just calling some of the pgstat.c subroutines that underlie the view. It happens that the pgstat module has no backend-local initialization (at the moment, and discounting the issue of making the process's own pgstat_activity entry), so they were happy enough. It was the syscache stuff that was spitting up. Oh, I see. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Patch for cursor calling with named parameters
Kevin Grittner kevin.gritt...@wicourts.gov wrote: Yeb Havinga yebhavi...@gmail.com wrote: I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. This is also in line with calling functions, where mixing named and positional is allowed, as long as the positional arguments are first. Personally I have no opinion what is best, include the feature or give an error, and I removed the possibility during the previous commitfest. If there's no technical reason not to allow them to be mixed, I would tend to favor consistency with parameter calling rules. Doing otherwise seems like to result in confusion and bug reports. Er, that was supposed to read I would tend to favor consistency with function calling rules. As stated here: http://www.postgresql.org/docs/9.1/interactive/sql-syntax-calling-funcs.html | PostgreSQL also supports mixed notation, which combines positional | and named notation. In this case, positional parameters are | written first and named parameters appear after them. How do others feel? If there are no objections, I suggest that Yeb implement the mixed notation for cursor parameters. -Kevin -- 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] Inlining comparators as a performance optimisation
On 5 December 2011 13:23, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: I'm still not convinced the big gain is in inlining the comparison functions. Your patch contains a few orthogonal tricks, too: * A fastpath for the case of just one scankey. No reason we can't do that without inlining. True, though Tom did seem to not like that idea very much. * inlining the swap-functions within qsort. Thaẗ́'s different from inlining the comparison functions, and could be done regardless of the data type. The qsort element size in tuplesort.c is always sizeof(SortTuple) Sure. I think that Tom mostly objects to hard-coded intelligence about which datatypes are used, rather than specialisations per se. And there's some additional specializations we can do, not implemented in your patch, that don't depend on inlining the comparison functions: * A fastpath for the case of no NULLs * separate comparetup functions for ascending and descending sorts (this allows tail call elimination of the call to type-specific comparison function in the ascending version) * call CHECK_FOR_INTERRUPTS() less frequently. All of those had occurred to me, except the last - if you look at the definition of that macro, it looks like more trouble than it's worth to do less frequently. I didn't revise my patch with them though, because the difference that they made, while clearly measurable, seemed fairly small, or at least relatively so. I wasn't about to add additional kludge for marginal benefits given the existing controversy, though I have not dismissed the idea entirely - it could be important on some other machine. Perhaps you can get even more gain by adding the no-NULLs, and asc/desc special cases to your inlining patch, too, but let's squeeze all the fat out of the non-inlined version first. As I said, those things are simple enough to test, and were not found to make a significant difference, at least to my exact test case + hardware. One nice aspect of many of these non-inlining optimizations is that they help with external sorts, too. I'd expect the benefits to be quite a lot smaller there, but fair point. Results from running the same test on your patch are attached. I think that while you're right to suggest that the inlining of the comparator proper, rather than any other function or other optimisation isn't playing a dominant role in these optimisations, I think that you're underestimating the role of locality of reference. To illustrate this, I've also included results for when I simply move the comparator to another translation unit, logtape.c . There's clearly a regression from doing so (I ran the numbers twice, in a clinical environment). The point is, there is a basically unknown overhead paid for not inlining - maybe inlining is not worth it, but that's a hard call to make. I didn't try to make the numbers look worse by putting the comparator proper in some other translation unit, but it may be that I'd have shown considerably worse numbers by doing so. Why the strong aversion to what I've done? I accept that it's ugly, but is it really worth worrying about that sort of regression in maintainability for something that was basically untouched since 2006, and will probably remain untouched after this work concludes, whatever happens? -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services results_server_w_heikki.ods Description: application/vnd.oasis.opendocument.spreadsheet -- 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] Caching for stable expressions with constant arguments v3
On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: This seems to have bitrotted, thanks to the recent refactoring in eval_const_expressions(). Luckily the conflicts are mostly whitespace changes, so shouldn't be hard to fix. I'll try to come up with an updated patch today or tomorrow. I wonder if it would be better to add the CacheExpr nodes to the tree as a separate pass, instead of shoehorning it into eval_const_expressions? I think would be more readable that way, even though a separate pass would be more expensive. And there are callers of eval_const_expressions() that have no use for the caching, like process_implied_equality(). Per Tom's comment, I won't split out the cache insertion for now. The context struct has a boolean 'cache' attribute that controls whether caching is desired or not. I think this could be exposed to the caller as an eval_const_expressions() argument. This comment in RelationGetExpressions() also worries me: [...] Do the injected CacheExprs screw up that equality? Or the constraint exclusion logic in predtest.c? I suspect these cases are guaranteed not to produce any CacheExprs. They're always immutable expressions. If they contain Var references they're stored as is (not cachable); if not, they're folded to a constant. But I will have to double-check all the callers; it might be a good idea to disable caching anyway in these cases. Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] exit() calls in libraries
Debian's package policy and quality check tool lintian reports the following (among other things) on the postgresql-9.1 (and earlier) packages: X: libpq5: shlib-calls-exit usr/lib/libpq.so.5.4 X: libecpg6: shlib-calls-exit usr/lib/libecpg.so.6.3 which is explained as I: shlib-calls-exit N: N: The listed shared library calls the C library exit() or _exit() N: functions. N: N: In the case of an error, the library should instead return an N: appropriate error code to the calling program which can then determine N: how to handle the error, including performing any required clean-up. [snip] The report on libecpg is actually a false positive, because the exit() call comes from get_progname() in path.c, which is not called from functions in libecpg. The cases in libpq are * various places in fe-print.c calling exit(1) when malloc fails, presumably having run out of memory, and * in libpq-int.h the macro PGTHREAD_ERROR, which is called in several places in fe-connect.c and fe-secure.c. Are these appropriate behaviors? The fe-print.c stuff probably isn't used much anymore. But the threading stuff is, and it encroaches on the exit status space of the libpq-using program. And does it even make sense to call exit() if the thread locking is busted? Maybe abort() would work better? -- 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] Caching for stable expressions with constant arguments v3
Marti Raudsepp ma...@juffo.org writes: On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: This comment in RelationGetExpressions() also worries me: [...] Do the injected CacheExprs screw up that equality? Or the constraint exclusion logic in predtest.c? I suspect these cases are guaranteed not to produce any CacheExprs. They're always immutable expressions. If they contain Var references they're stored as is (not cachable); if not, they're folded to a constant. But I will have to double-check all the callers; it might be a good idea to disable caching anyway in these cases. I think if you have some call sites inject CacheExprs and others not, it will get more difficult to match up expressions that should be considered equal. On the whole this seems like a bad idea. What is the reason for having such a control boolean in the first place? 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] Inlining comparators as a performance optimisation
Peter Geoghegan pe...@2ndquadrant.com writes: Why the strong aversion to what I've done? I accept that it's ugly, but is it really worth worrying about that sort of regression in maintainability for something that was basically untouched since 2006, and will probably remain untouched after this work concludes, whatever happens? Maintainability is only part of the issue --- though it's definitely one part, since this code has hardly gone untouched since 2006, cf http://git.postgresql.org/gitweb/?p=postgresql.git;a=history;f=src/backend/utils/sort/tuplesort.c;hb=HEAD What is bothering me is that this approach is going to cause substantial bloat of the executable code, and such bloat has got distributed costs, which we don't have any really good way to measure but for sure micro-benchmarks addressing only sort speed aren't going to reveal them. Cache lines filled with sort code take cycles to flush and replace with something else. I think it's possibly reasonable to have inlined copies of qsort for a small number of datatypes, but it seems much less reasonable to have multiple copies per datatype in order to obtain progressively tinier micro-benchmark speedups. We need to figure out what the tradeoff against executable size really is, but so far it seems like you've been ignoring the fact that there is any such tradeoff at all. 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] plpython SPI cursors
On ons, 2011-11-23 at 19:58 +0100, Jan Urbański wrote: On 20/11/11 19:14, Steve Singer wrote: On 11-10-15 07:28 PM, Jan Urbański wrote: Hi, attached is a patch implementing the usage of SPI cursors in PL/Python. Currently when trying to process a large table in PL/Python you have slurp it all into memory (that's what plpy.execute does). J I found a few bugs (see my testing section below) that will need fixing + a few questions about the code Responding now to all questions and attaching a revised patch based on your comments. Committed Please refresh the other patch. -- 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] plpython SPI cursors
On 05/12/11 18:58, Peter Eisentraut wrote: On ons, 2011-11-23 at 19:58 +0100, Jan Urbański wrote: On 20/11/11 19:14, Steve Singer wrote: Responding now to all questions and attaching a revised patch based on your comments. Committed Please refresh the other patch. Great, thanks! I'll try to send an updated version of the other patch this evening. Cheers, Jan -- 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] exit() calls in libraries
Excerpts from Peter Eisentraut's message of lun dic 05 14:27:41 -0300 2011: The cases in libpq are * various places in fe-print.c calling exit(1) when malloc fails, presumably having run out of memory, and * in libpq-int.h the macro PGTHREAD_ERROR, which is called in several places in fe-connect.c and fe-secure.c. Are these appropriate behaviors? The fe-print.c stuff probably isn't used much anymore. But the threading stuff is, and it encroaches on the exit status space of the libpq-using program. And does it even make sense to call exit() if the thread locking is busted? Maybe abort() would work better? Having had to battle some exit() calls in the PHP interpreter back when I was working in PL/php, I agree that they shouldn't be there -- abort() seems more appropriate if the system is truly busted. As for the fr-print.c code, I'm not really sure why don't we just remove it. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] plpython SPI cursors
Jan Urba??ski wrote: On 05/12/11 18:58, Peter Eisentraut wrote: On ons, 2011-11-23 at 19:58 +0100, Jan Urba?ski wrote: On 20/11/11 19:14, Steve Singer wrote: Responding now to all questions and attaching a revised patch based on your comments. Committed Please refresh the other patch. Great, thanks! I'll try to send an updated version of the other patch this evening. I assume this is _not_ related to this TODO item: Add a DB-API compliant interface on top of the SPI interface -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] plpython SPI cursors
On 05/12/11 19:12, Bruce Momjian wrote: Jan Urbański wrote: On 05/12/11 18:58, Peter Eisentraut wrote: On ons, 2011-11-23 at 19:58 +0100, Jan Urba?ski wrote: On 20/11/11 19:14, Steve Singer wrote: Responding now to all questions and attaching a revised patch based on your comments. Committed Please refresh the other patch. Great, thanks! I'll try to send an updated version of the other patch this evening. I assume this is _not_ related to this TODO item: Add a DB-API compliant interface on top of the SPI interface No, not related. -- 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] planner fails on HEAD
On Mon, Dec 5, 2011 at 12:17 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello 2011/12/4 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: it looks like gcc bug - gcc 4.5.1 20100924 (Red Hat 4.5.1) It was configured just with --enable-debug and --enable-cassert Is this x86? I can't reproduce it on x86_64. yes, this is x86 platform uname -a Linux nemesis 2.6.35.14-106.fc14.i686.PAE #1 SMP Wed Nov 23 13:39:51 UTC 2011 i686 i686 i386 GNU/Linux [pavel@nemesis ~]$ cat /proc/cpuinfo processor : 0 vendor_id : GenuineIntel cpu family : 6 model : 15 model name : Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz stepping : 11 cpu MHz : 800.000 cache size : 4096 KB physical id : 0 siblings : 2 core id : 0 cpu cores : 2 apicid : 0 initial apicid : 0 fdiv_bug : no hlt_bug : no f00f_bug : no coma_bug : no fpu : yes fpu_exception : yes cpuid level : 10 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe nx lm constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm ida tpr_shadow vnmi flexpriority bogomips : 4785.76 clflush size : 64 cache_alignment : 64 address sizes : 36 bits physical, 48 bits virtual power management: processor : 1 vendor_id : GenuineIntel cpu family : 6 model : 15 model name : Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz stepping : 11 cpu MHz : 800.000 cache size : 4096 KB physical id : 0 siblings : 2 core id : 1 cpu cores : 2 apicid : 1 initial apicid : 1 fdiv_bug : no hlt_bug : no f00f_bug : no coma_bug : no fpu : yes fpu_exception : yes cpuid level : 10 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe nx lm constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm ida tpr_shadow vnmi flexpriority bogomips : 4786.60 clflush size : 64 cache_alignment : 64 address sizes : 36 bits physical, 48 bits virtual power management: it is Dell latitude D830 It's fairly easy to get a set of values such that innerstartsel *should* equal innerendsel; but if one value has been rounded to memory precision and the other hasn't, the assert could certainly fail. Some digging around yields the information that the gcc hackers do not consider this a bug, or at least adamantly refuse to do anything about it: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=323 Comment 47 is particularly relevant to our situation: To summarize, this defect effectively states that: assert( (x/y) == (x/y) ) may cause an assertion if compiled with optimization. Also, http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45691#c4 indicates that an explicit cast to double should help. Would you check if the problem goes away if the Asserts are changed to Assert((double) outerstartsel = (double) outerendsel); Assert((double) innerstartsel = (double) innerendsel); it doesn't help regards, tom lane assambler list is attached how about: Assert((volatile double) outerstartsel = (volatile double) outerendsel); etc merlin -- 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] planner fails on HEAD
2011/12/5 Merlin Moncure mmonc...@gmail.com: On Mon, Dec 5, 2011 at 12:17 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello 2011/12/4 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: it looks like gcc bug - gcc 4.5.1 20100924 (Red Hat 4.5.1) It was configured just with --enable-debug and --enable-cassert Is this x86? I can't reproduce it on x86_64. yes, this is x86 platform uname -a Linux nemesis 2.6.35.14-106.fc14.i686.PAE #1 SMP Wed Nov 23 13:39:51 UTC 2011 i686 i686 i386 GNU/Linux [pavel@nemesis ~]$ cat /proc/cpuinfo processor : 0 vendor_id : GenuineIntel cpu family : 6 model : 15 model name : Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz stepping : 11 cpu MHz : 800.000 cache size : 4096 KB physical id : 0 siblings : 2 core id : 0 cpu cores : 2 apicid : 0 initial apicid : 0 fdiv_bug : no hlt_bug : no f00f_bug : no coma_bug : no fpu : yes fpu_exception : yes cpuid level : 10 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe nx lm constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm ida tpr_shadow vnmi flexpriority bogomips : 4785.76 clflush size : 64 cache_alignment : 64 address sizes : 36 bits physical, 48 bits virtual power management: processor : 1 vendor_id : GenuineIntel cpu family : 6 model : 15 model name : Intel(R) Core(TM)2 Duo CPU T7700 @ 2.40GHz stepping : 11 cpu MHz : 800.000 cache size : 4096 KB physical id : 0 siblings : 2 core id : 1 cpu cores : 2 apicid : 1 initial apicid : 1 fdiv_bug : no hlt_bug : no f00f_bug : no coma_bug : no fpu : yes fpu_exception : yes cpuid level : 10 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe nx lm constant_tsc arch_perfmon pebs bts aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm ida tpr_shadow vnmi flexpriority bogomips : 4786.60 clflush size : 64 cache_alignment : 64 address sizes : 36 bits physical, 48 bits virtual power management: it is Dell latitude D830 It's fairly easy to get a set of values such that innerstartsel *should* equal innerendsel; but if one value has been rounded to memory precision and the other hasn't, the assert could certainly fail. Some digging around yields the information that the gcc hackers do not consider this a bug, or at least adamantly refuse to do anything about it: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=323 Comment 47 is particularly relevant to our situation: To summarize, this defect effectively states that: assert( (x/y) == (x/y) ) may cause an assertion if compiled with optimization. Also, http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45691#c4 indicates that an explicit cast to double should help. Would you check if the problem goes away if the Asserts are changed to Assert((double) outerstartsel = (double) outerendsel); Assert((double) innerstartsel = (double) innerendsel); it doesn't help regards, tom lane assambler list is attached how about: Assert((volatile double) outerstartsel = (volatile double) outerendsel); doesn't help too Regards Pavel etc merlin -- 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] Caching for stable expressions with constant arguments v3
On Mon, Dec 5, 2011 at 19:31, Tom Lane t...@sss.pgh.pa.us wrote: I think if you have some call sites inject CacheExprs and others not, it will get more difficult to match up expressions that should be considered equal. On the whole this seems like a bad idea. What is the reason for having such a control boolean in the first place? It's needed for correctness with PL/pgSQL simple expressions. This seems a bit of a kludge, but I considered it the least bad solution. Here's what I added to planner.c standard_planner(): /* * glob-isSimple is a hint to eval_const_expressions() and PL/pgSQL that * this statement is potentially a simple expression -- it contains no * table references, no subqueries and no join clauses. * * We need this here because this prevents insertion of CacheExpr, which * would break simple expressions in PL/pgSQL. Such queries wouldn't * benefit from constant caching anyway. * * The actual definition of a simple statement is more strict, but we * don't want to spend that checking overhead here. * * Caveat: Queries with set-returning functions in SELECT list could * still potentially benefit from caching, but we don't check that now. */ glob-isSimple = (parse-commandType == CMD_SELECT parse-jointree-fromlist == NIL parse-hasSubLinks == FALSE parse-cteList == NIL); I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't remember right now why I rejected that approach (sorry, it's been 2 months). Currently I'm also disabling CacheExpr nodes in estimate_expression_value() since we know for a fact that the planner only evaluates it once. But that probably doesn't make much of a difference. Regards, Marti -- 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] Caching for stable expressions with constant arguments v3
On 05.12.2011 20:53, Marti Raudsepp wrote: I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't remember right now why I rejected that approach (sorry, it's been 2 months). Yet another idea would be to leave the CacheExprs there, but provide a way to reset the caches. PL/pgSQL could then reset the caches between every invocation. Or pass a flag to ExecInitExpr() to skip through the CacheExprs. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- 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] Re: [PATCH] Caching for stable expressions with constant arguments v3
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: I wonder if it would be better to add the CacheExpr nodes to the tree as a separate pass, instead of shoehorning it into eval_const_expressions? I think would be more readable that way, even though a separate pass would be more expensive. A separate pass would be very considerably more expensive, because (1) it would require making a whole new copy of each expression tree, and (2) it would require looking up the volatility status of each function and operator. eval_const_expressions already has to do the latter, or has to do it in a lot of cases anyway, so I think it's probably the best place to add this. If it weren't for (2) I would suggest adding the work to setrefs.c instead, but as it is I think we'd better suck it up and deal with any fallout in the later stages of the planner. 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] hiding variable-length fields from Form_pg_* structs
On sön, 2011-11-27 at 18:20 -0500, Tom Lane wrote:
The low-tech way would be
CATALOG(pg_attribute,1249) BKI_BOOTSTRAP ...
{
...
int4attinhcount;
Oid attcollation;
aclitem attacl[1];
#ifdef CATALOG_VARLEN_FIELDS
textattoptions[1];
textattfdwoptions[1];
#endif
} FormData_pg_attribute;
Good enough.
To clarify, I believe the rule is that the first variable-length field
can be accessed as a struct field. Are there any exceptions to this?
This kind of comment is pretty confusing:
CATALOG(pg_rewrite,2618)
{
NameDatarulename;
Oid ev_class;
int2ev_attr;
charev_type;
charev_enabled;
boolis_instead;
/* NB: remaining fields must be accessed via heap_getattr */
pg_node_tree ev_qual;
pg_node_tree ev_action;
} FormData_pg_rewrite;
Also, this code in relcache.c accesses indclass, which is after an
int2vector and an oidvector field:
/* Check to see if it is a unique, non-partial btree index on OID */
if (index-indnatts == 1
index-indisunique index-indimmediate
index-indkey.values[0] == ObjectIdAttributeNumber
index-indclass.values[0] == OID_BTREE_OPS_OID
heap_attisnull(htup, Anum_pg_index_indpred))
oidIndex = index-indexrelid;
--
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] planner fails on HEAD
Pavel Stehule pavel.steh...@gmail.com writes: 2011/12/4 Tom Lane t...@sss.pgh.pa.us: Is this x86? I can't reproduce it on x86_64. yes, this is x86 platform uname -a Linux nemesis 2.6.35.14-106.fc14.i686.PAE #1 SMP Wed Nov 23 13:39:51 UTC 2011 i686 i686 i386 GNU/Linux I reproduced this with gcc 4.6.0 on Fedora 15 x86, too. Also, http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45691#c4 indicates that an explicit cast to double should help. Would you check if the problem goes away if the Asserts are changed to Assert((double) outerstartsel = (double) outerendsel); Assert((double) innerstartsel = (double) innerendsel); it doesn't help Hmm ... I'm inclined to think this actually *is* a bug, since Jakub is on record as saying it should work. Nonetheless, we need a workaround, since gcc versions behaving this way are going to be widespread for a long time even if we convince them to do something about it (which I suspect they wouldn't given their imperviousness to complaints about the main issue). I'm now thinking the best solution is just to drop these two Asserts. They're not adding anything very useful given the previous ones (which should be safe since those involve quantities rounded to integers). 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] Caching for stable expressions with constant arguments v3
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 05.12.2011 20:53, Marti Raudsepp wrote: I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't remember right now why I rejected that approach (sorry, it's been 2 months). Yet another idea would be to leave the CacheExprs there, but provide a way to reset the caches. PL/pgSQL could then reset the caches between every invocation. We're likely to need a way to reset these caches anyway, at some point... Or pass a flag to ExecInitExpr() to skip through the CacheExprs. Not sure what you mean by that --- are you imagining that the ExprState tree would have structure not matching the Expr tree? That seems just about guaranteed to break something somewhere. 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] Caching for stable expressions with constant arguments v3
On 05.12.2011 21:36, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@enterprisedb.com writes: Or pass a flag to ExecInitExpr() to skip through the CacheExprs. Not sure what you mean by that --- are you imagining that the ExprState tree would have structure not matching the Expr tree? Yes. Or it could just set a flag in the CacheExprState nodes to not do the caching. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- 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] pg_upgrade automatic testing
On sön, 2011-11-27 at 18:17 -0500, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: I've committed it now, and some buildfarm members are failing with lack of shared memory, semaphores, or disk space. Don't know what to do with that or why so many are failing like that. We could create a way to omit the test if it becomes a problem. I believe the issue is that those BF members have kernel settings that only support running one postmaster at a time. The way you've got this set up, it launches a new private postmaster during a make installcheck; which is not only problematic from a resource consumption standpoint, but seems to me to violate the spirit of make installcheck, because what it's testing is not the installed postmaster but a local instance. Can you confine the test to only occur in make check mode, not make installcheck, please? FWIW, the original definition of installcheck is that it tests the already installed programs, which is what this does (did). But I agree that the difference is minimal in this case. -- 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] hiding variable-length fields from Form_pg_* structs
Peter Eisentraut pete...@gmx.net writes: To clarify, I believe the rule is that the first variable-length field can be accessed as a struct field. Are there any exceptions to this? If it is known not null, yes, but I wonder just how many places actually depend on that. It might be better to remove all varlena fields from C visibility and require use of the accessor functions. We should at least look into what that would cost us. Also, this code in relcache.c accesses indclass, which is after an int2vector and an oidvector field: /* Check to see if it is a unique, non-partial btree index on OID */ if (index-indnatts == 1 index-indisunique index-indimmediate index-indkey.values[0] == ObjectIdAttributeNumber index-indclass.values[0] == OID_BTREE_OPS_OID heap_attisnull(htup, Anum_pg_index_indpred)) oidIndex = index-indexrelid; Hmm, that does look mighty broken, doesn't it ... but somehow it works, else GetNewOid would be bleating all the time. (Thinks about that for a bit) Oh, it accidentally fails to fail because the C declarations for int2vector and oidvector are actually correct if there is a single element in the arrays, which we already verified with the indnatts test. But yeah, this seems horribly fragile, and it should not be performance critical because we only go through here when loading up a cache entry. So let's change it. 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] hiding variable-length fields from Form_pg_* structs
On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Eisentraut pete...@gmx.net writes: To clarify, I believe the rule is that the first variable-length field can be accessed as a struct field. Are there any exceptions to this? If it is known not null, yes, but I wonder just how many places actually depend on that. It might be better to remove all varlena fields from C visibility and require use of the accessor functions. We should at least look into what that would cost us. My impression is that all the varlena fields also allow nulls. So I think there's no point in trying to keep the first one C-accessible. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] pg_upgrade automatic testing
On sön, 2011-11-27 at 19:12 -0500, Andrew Dunstan wrote: Contrib tests are only run by the buildfarm in installcheck mode, so that will probably turn the tests off for the buildfarm, so +1 on that Does the new buildfarm modular thing support that some members could run the checks through explicit configuration? I think these tests are probably somewhat ill-conceived. Note also that shell scripts are not portable, and so these tests would not be able to run on MSVC buildfarm members, even if they had been enabled in the MSVC regression driver, which they have not. If we need a regression driver script it needs to be written in Perl. Anyone is free to rewrite the thing in a different language. Also note that the test as written is likely to add significantly to buildfarm run times, as it will run the entire base regression suite again, possibly several times. Are there any restrictions on how long a buildfarm run is supposed to take? Finally, I think that this is probably not what we really need. What do you base your thinking on? This test suite has already found a number of bugs in the pg_upgrade procedure that no one else was able to find. By that measure, it's exactly what we need. I have already started work (as I mentioned some weeks ago) on having the buildfarm stash away a successful build and data directory, to be used later in cross-version upgrade testing, which seems to me much more what we need from something like the buildfarm. Maybe we could discuss how to run something like that. That is one part of the puzzle. But once you have stashed away the old source and data directory, you still need a test runner, which is exactly what this provides you. But note, cross-version pg_upgrade checks will not give you the full value, even assuming that you can make them work at all in an unattended way, because by default you won't be able to get a clean dumps match result, at least without a lot of additional work to mangle the dump output. Most (or all) of the bugs found so far with this test suite were for upgrades *from* whatever was the current version. If we don't have a current-to-current upgrade test suite, then we would only find those years from now. -- 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] hiding variable-length fields from Form_pg_* structs
Robert Haas robertmh...@gmail.com writes: On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Eisentraut pete...@gmx.net writes: To clarify, I believe the rule is that the first variable-length field can be accessed as a struct field. Are there any exceptions to this? If it is known not null, yes, but I wonder just how many places actually depend on that. My impression is that all the varlena fields also allow nulls. See MARKNOTNULL in bootstrap.c. I think the exceptions were designed to protect direct accesses to pg_index. 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] JSON for PG 9.2
Where are we with adding JSON for Postgres 9.2? We got bogged down in the data representation last time we discussed this. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] hiding variable-length fields from Form_pg_* structs
On Mon, Dec 5, 2011 at 3:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Dec 5, 2011 at 2:47 PM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Eisentraut pete...@gmx.net writes: To clarify, I believe the rule is that the first variable-length field can be accessed as a struct field. Are there any exceptions to this? If it is known not null, yes, but I wonder just how many places actually depend on that. My impression is that all the varlena fields also allow nulls. See MARKNOTNULL in bootstrap.c. I think the exceptions were designed to protect direct accesses to pg_index. Hmm, OK. rhaas=# select r.relname, a.attname, a.atttypid::regtype from pg_class r, pg_attribute a where relnamespace=11 and relkind='r' and attrelid = r.oid and a.attnotnull and a.attlen0; relname | attname| atttypid +--+ pg_proc| proargtypes | oidvector pg_index | indkey | int2vector pg_index | indcollation | oidvector pg_index | indclass | oidvector pg_index | indoption| int2vector pg_trigger | tgattr | int2vector (6 rows) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Command Triggers
Hi, Please find an update attached, v4, fixing most remaining items. Next steps are better docs and more commands support (along with finishing currently supported ones), and a review locking behavior. If you want to just scroll over the patch to get an impression of what's in there rather than try out the attachment, follow this URL: https://github.com/dimitri/postgres/compare/master...command_triggers Dimitri Fontaine dimi...@2ndquadrant.fr writes: Will look into qualifying names. I'm now qualifying relation names even if they have not been entered with a namespace qualifier. What do you think? The other components are left alone, I think the internal APIs for qualifying all kind of objects from the parse tree and current context are mostly missing. As an alternative it would be possible to pass the current search path but that doesn't seem to the best approach to me; The trigger runs with the same search_path settings as the command, right? Maybe that's good enough for command triggers? Command triggers should only be allowed for the database owner. Yes, that needs to happen soon, along with pg_dump and psql support. All three are implemented in the attached new revision of the patch. Imo the current callsite in ProcessUtility isn't that good. I think it would make more sense moving it into standard_ProcessUtility. It should be *after* the check_xact_readonly check in there in my opinion. Makes sense, will fix in the next revision. Done too. It's better this way, thank you. I am also don't think its a good idea to run the ExecBeforeOrInsteadOfCommandTriggers stuff there because thats allso the path that COMMIT/BEGIN and other stuff take. Those are pretty fast path. I suggest making two switches in standard_ProcessUtility, one for the non- command trigger stuff which returns on success and one after that. Only after the first triggers are checked. Agreed. That's about the way I've done it. Please note that doing it this way means that a ProcessUtility_hook can decide whether or not the command triggers are going to be fired or not, and that's true for BEFORE, AFTER and INSTEAD OF command triggers. I think that's the way to go, though. * Either tgenable's datatype or its readfunc is wrong (watch the compiler ;)) * ruleutils.c: * generic routine for IF EXISTS, CASCADE, ... Will have to wait until next revision. Fixed. Well, the generic routine would only be called twice and would only share a rather short expression, so will have to wait until I add support for more commands. There's a regression tests gotcha. Namely that the parallel running of triggers against inheritance makes it impossible to predict if the trigger on the command CREATE TABLE will spit out a notice in the inherit tests. I don't know how that is usually avoided, but I guess it involves picking some other command example that don't conflict with the parallel tests of that section? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support command-trigger.v4.patch.gz Description: Binary data -- 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] pg_upgrade and regclass
Bruce Momjian wrote: In Postgres 8.4, pg_upgrade preserved pg_class relfilenodes by creating files in the file system. In Postgres 9.0, we changed this by creating pg_upgrade_support functions which allow us to directly preserve pg_class.oids. Unfortunately, check.c was not updated to reflect this and clusters using regclass were prevented from being upgraded by pg_upgrade. I have developed the attached patch to allow clusters using regclass to be upgraded. I plan to apply it to PG 9.0, 9.1, and HEAD. I have applied the attached patch to all relevant releases. I did a more modest single-line code change for back branches. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/doc/src/sgml/pgupgrade.sgml b/doc/src/sgml/pgupgrade.sgml new file mode 100644 index 460d06b..ac3f99b *** a/doc/src/sgml/pgupgrade.sgml --- b/doc/src/sgml/pgupgrade.sgml *** psql --username postgres --file script.s *** 557,563 applicationpg_upgrade/ does not support upgrading of databases containing these typereg*/ OID-referencing system data types: typeregproc/, typeregprocedure/, typeregoper/, !typeregoperator/, typeregclass/, typeregconfig/, and typeregdictionary/. (typeregtype/ can be upgraded.) /para --- 557,563 applicationpg_upgrade/ does not support upgrading of databases containing these typereg*/ OID-referencing system data types: typeregproc/, typeregprocedure/, typeregoper/, !typeregoperator/, typeregconfig/, and typeregdictionary/. (typeregtype/ can be upgraded.) /para diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c new file mode 100644 index d32a84c..7185f13 *** a/contrib/pg_upgrade/check.c --- b/contrib/pg_upgrade/check.c *** check_for_isn_and_int8_passing_mismatch( *** 611,621 /* * check_for_reg_data_type_usage() * pg_upgrade only preserves these system values: ! * pg_class.relfilenode * pg_type.oid * pg_enum.oid * ! * Most of the reg* data types reference system catalog info that is * not preserved, and hence these data types cannot be used in user * tables upgraded by pg_upgrade. */ --- 611,621 /* * check_for_reg_data_type_usage() * pg_upgrade only preserves these system values: ! * pg_class.oid * pg_type.oid * pg_enum.oid * ! * Many of the reg* data types reference system catalog info that is * not preserved, and hence these data types cannot be used in user * tables upgraded by pg_upgrade. */ *** check_for_reg_data_type_usage(ClusterInf *** 653,668 NOT a.attisdropped AND a.atttypid IN ( 'pg_catalog.regproc'::pg_catalog.regtype, ! 'pg_catalog.regprocedure'::pg_catalog.regtype, 'pg_catalog.regoper'::pg_catalog.regtype, ! 'pg_catalog.regoperator'::pg_catalog.regtype, ! 'pg_catalog.regclass'::pg_catalog.regtype, /* regtype.oid is preserved, so 'regtype' is OK */ ! 'pg_catalog.regconfig'::pg_catalog.regtype, ! 'pg_catalog.regdictionary'::pg_catalog.regtype) AND ! c.relnamespace = n.oid AND ! n.nspname != 'pg_catalog' AND ! n.nspname != 'information_schema'); ntups = PQntuples(res); i_nspname = PQfnumber(res, nspname); --- 653,668 NOT a.attisdropped AND a.atttypid IN ( 'pg_catalog.regproc'::pg_catalog.regtype, ! 'pg_catalog.regprocedure'::pg_catalog.regtype, 'pg_catalog.regoper'::pg_catalog.regtype, ! 'pg_catalog.regoperator'::pg_catalog.regtype, ! /* regclass.oid is preserved, so 'regclass' is OK */ /* regtype.oid is preserved, so 'regtype' is OK */ ! 'pg_catalog.regconfig'::pg_catalog.regtype, ! 'pg_catalog.regdictionary'::pg_catalog.regtype) AND ! c.relnamespace = n.oid AND ! n.nspname != 'pg_catalog' AND ! n.nspname != 'information_schema'); ntups = PQntuples(res); i_nspname = PQfnumber(res, nspname); -- 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] pg_upgrade and regclass
Bruce Momjian wrote: Bruce Momjian wrote: In Postgres 8.4, pg_upgrade preserved pg_class relfilenodes by creating files in the file system. In Postgres 9.0, we changed this by creating pg_upgrade_support functions which allow us to directly preserve pg_class.oids. Unfortunately, check.c was not updated to reflect this and clusters using regclass were prevented from being upgraded by pg_upgrade. I have developed the attached patch to allow clusters using regclass to be upgraded. I plan to apply it to PG 9.0, 9.1, and HEAD. I have applied the attached patch to all relevant releases. I did a more modest single-line code change for back branches. Oh, I forgot to mention that this bug report came to me privately via EntepriseDB testing. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_upgrade and relkind filtering
Pg_upgrade has the following check to make sure the cluster is safe for upgrading: res = executeQueryOrDie(conn, SELECT n.nspname, c.relname, a.attname FROM pg_catalog.pg_class c, pg_catalog.pg_namespace n, pg_catalog.pg_attribute a WHERE c.oid = a.attrelid AND NOT a.attisdropped AND a.atttypid IN ( 'pg_catalog.regproc'::pg_catalog.regtype, 'pg_catalog.regprocedure'::pg_catalog.regtype, 'pg_catalog.regoper'::pg_catalog.regtype, 'pg_catalog.regoperator'::pg_catalog.regtype, /* regclass.oid is preserved, so 'regclass' is OK */ /* regtype.oid is preserved, so 'regtype' is OK */ 'pg_catalog.regconfig'::pg_catalog.regtype, 'pg_catalog.regdictionary'::pg_catalog.regtype) AND c.relnamespace = n.oid AND n.nspname != 'pg_catalog' AND n.nspname != 'information_schema'); Based on a report from EnterpriseDB, I noticed that we check all pg_class entries, while there are cases where this is unnecessary because there is no data behind the entry, e.g. views. Here are the relkinds supported: #define RELKIND_RELATION'r' /* ordinary table */ #define RELKIND_INDEX 'i' /* secondary index */ #define RELKIND_SEQUENCE'S' /* sequence object */ #define RELKIND_TOASTVALUE 't' /* for out-of-line values */ #define RELKIND_VIEW'v' /* view */ #define RELKIND_COMPOSITE_TYPE 'c' /* composite type */ #define RELKIND_FOREIGN_TABLE 'f' /* foreign table */ #define RELKIND_UNCATALOGED 'u' /* not yet cataloged */ What types, other than views, can we skip in this query? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] why local_preload_libraries does require a separate directory ?
On 5.12.2011 00:06, Tom Lane wrote: Tomas Vondra t...@fuzzy.cz writes: On 4.12.2011 22:16, Tom Lane wrote: Um ... why would you design it like that? The backends are added to pg_stat_activity after the auth hook finishes, which means possible race conditions (backends executed at about the same time don't see each other in pg_stat_activity). So I use an exclusive lock that's acquired before reading pg_stat_activity and released after the pg_stat_activity is updated. That's the only thing the library loaded using local_preload_libraries does - it releases the lock. That's an unbelievably ugly, and dangerous, kluge. All you need is one backend not loading the second library (and remember, local_preload_libraries is user-settable) and you've just locked up the system. Why are you using pg_stat_activity for this anyway? Searching the ProcArray seems much safer ... see CountDBBackends for an example. Thanks, reading ProcArray works fine, although the ProcArrayStruct is private to procarray.c so I had to create a local copy. That sounds a bit too fragile I guess - whenever it changes, the extension will break without a warning. Tomas -- 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] PostgreSQL fails to build with 32bit MinGW-w64
On 12/05/2011 09:31 AM, NISHIYAMA Tomoaki wrote: Hi, If we are not to use 64 bit file size (and time), #undef stat may be sufficient. The #undef should be before the prototype of pgwin32_safestat because the #define stat _stat64 affect both the function and struct stat. The #undef stat necessitate #undef fstat as the parameter struct stat * is changed. I don't think I'm going to do it that way, but leave this with me, I can take it from here. Right now I'm down to the following interesting regression failure: $ cat regression.diffs *** C:/MinGW/msys/1.0/home/pgrunner/bf/root32/HEAD/pgsql/src/test/regress/expected/float8-exp-three-digits-win32.out Fri Nov 25 14:24:49 2011 --- C:/MinGW/msys/1.0/home/pgrunner/bf/root32/HEAD/pgsql/src/test/regress/results/float8.out Mon Dec 5 18:17:36 2011 *** *** 382,388 SET f1 = FLOAT8_TBL.f1 * '-1' WHERE FLOAT8_TBL.f1 '0.0'; SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f; ! ERROR: value out of range: overflow SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f; ERROR: value out of range: overflow SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5; --- 382,396 SET f1 = FLOAT8_TBL.f1 * '-1' WHERE FLOAT8_TBL.f1 '0.0'; SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f; ! bad | ?column? ! -+-- ! |0 ! | -3.484e+201 ! | -1.0043e+203 ! |-Infinity ! | -1.2345678901234 ! (5 rows) ! SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f; ERROR: value out of range: overflow SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5; == 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
[HACKERS] pull_up_simple_subquery
While working on KaiGai's leaky views patch, I found myself
scrutinizing the behavior of the function named in the subject line;
and specifically the retest of is_simple_subquery(). I've been unable
to make that fail. For example, the following patch fails to break
the regression tests:
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -718,6 +718,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, Ran
}
else
{
+ elog(ERROR, croak and die);
/*
* Give up, return unmodified RangeTblRef.
*
This logic was originally introduced by the following commit:
commit e439fef6fc3e81aeb865f2c5a77c6faa2ee2a931
Author: Tom Lane t...@sss.pgh.pa.us
Date: Sat Jan 10 00:30:21 2004 +
Fix subquery pullup logic to not be fooled when a view that appears
'simple' references another view that is not simple. Must recheck
conditions after performing recursive pullup. Per example from
Laurent Perez, 9-Jan-04.
However, despite my best efforts, I can't figure out what scenario
it's protecting us against, at least not on current sources. The
original bug report is here:
http://archives.postgresql.org/pgsql-general/2004-01/msg00375.php
Tom's reply indicates that the v4 example shouldn't get flattened, but
it looks to me like current sources do flatten it and I really don't
see why they shouldn't. Poking around with git bisect and the patch
shown above, I see that the test case stopped tickling this code with
commit e6ae3b5dbf2c07bceb737c5a0ff199b1156051d1, which introduced
PlaceHolderVars, apparently for the precise purpose of allowing joins
of this type to be flattened. But this code survived that commit,
leaving the question of whether there are still cases where it's
needed (in which case we should probably add a comment or regression
test case, since it's not at all obvious) or whether we can rip it out
and save a few cycles.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] xlog location arithmetic
Hi,
A while ago when blogging about WAL [1], I noticed a function to deal with
xlog location arithmetic is wanted. I remembered Depez [2] mentioning it and
after some questions during trainings and conferences I decided to translate
my shell script function in C.
The attached patch implements the function pg_xlog_location_diff (bikeshed
colors are welcome). It calculates the difference between two given
transaction log locations. Now that we have pg_stat_replication view, it will
be easy to get the lag just passing columns as parameters. Also, the
monitoring tools could take advantage of it instead of relying on a fragile
routine to get the lag.
I noticed that pg_xlogfile_name* functions does not sanity check the xrecoff
boundaries but that is material for another patch.
[1] http://eulerto.blogspot.com/2011/11/understanding-wal-nomenclature.html
[2]
http://www.depesz.com/index.php/2011/01/24/waiting-for-9-1-pg_stat_replication/
--
Euler Taveira de Oliveira - Timbira http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index ddfb29a..cce218a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14342,6 +14342,9 @@ SELECT set_config('log_statement_stats', 'off', false);
indexterm
primarypg_xlogfile_name_offset/primary
/indexterm
+ indexterm
+primarypg_xlog_location_diff/primary
+ /indexterm
para
The functions shown in xref
@@ -14414,6 +14417,13 @@ SELECT set_config('log_statement_stats', 'off', false);
entrytypetext/, typeinteger//entry
entryConvert transaction log location string to file name and decimal byte offset within file/entry
/row
+ row
+ entry
+literalfunctionpg_xlog_location_diff(parameterlocation/ typetext/, parameterlocation/ typetext/)/function/literal
+/entry
+ entrytypebigint//entry
+ entryCalculate the difference between two transaction log locations/entry
+ /row
/tbody
/tgroup
/table
@@ -14507,6 +14517,13 @@ postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
/para
para
+ functionpg_xlog_location_diff/ calculates the difference in bytes
+ between two transaction log locations. It can be used with
+ structnamepg_stat_replication/structname or some functions shown in
+ xref linkend=functions-admin-backup-table to get the replication lag.
+ /para
+
+ para
For details about proper usage of these functions, see
xref linkend=continuous-archiving.
/para
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 22c6ca0..09e8369 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -26,6 +26,7 @@
#include replication/walreceiver.h
#include storage/smgr.h
#include utils/builtins.h
+#include utils/int8.h
#include utils/guc.h
#include utils/timestamp.h
@@ -465,3 +466,57 @@ pg_is_in_recovery(PG_FUNCTION_ARGS)
{
PG_RETURN_BOOL(RecoveryInProgress());
}
+
+/*
+ * Compute the difference in bytes between two WAL locations.
+ */
+Datum
+pg_xlog_location_diff(PG_FUNCTION_ARGS)
+{
+ text *location1 = PG_GETARG_TEXT_P(0);
+ text *location2 = PG_GETARG_TEXT_P(1);
+ char *str1, *str2;
+ uint32 xlogid1, xrecoff1;
+ uint32 xlogid2, xrecoff2;
+ int64 tmp;
+ int64 result;
+
+ /*
+ * Read and parse input
+ */
+ str1 = text_to_cstring(location1);
+ str2 = text_to_cstring(location2);
+
+ if (sscanf(str1, %8X/%8X, xlogid1, xrecoff1) != 2)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(could not parse transaction log location \%s\, str1)));
+ if (sscanf(str2, %8X/%8X, xlogid2, xrecoff2) != 2)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(could not parse transaction log location \%s\, str2)));
+
+ /*
+ * Sanity check
+ */
+ if (xrecoff1 XLogFileSize)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(xrecoff \%X\ is out of valid range, 0..%X, xrecoff1, XLogFileSize)));
+ if (xrecoff2 XLogFileSize)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(xrecoff \%X\ is out of valid range, 0..%X, xrecoff2, XLogFileSize)));
+
+ /*
+ * Use the int8 functions mainly for overflow detection
+ *
+ * result = XLogFileSize * (xlogid1 - xlogid2) + xrecoff1 - xrecoff2
+ */
+ tmp = DirectFunctionCall2(int8mi, xlogid1, xlogid2);
+ tmp = DirectFunctionCall2(int8mul, XLogFileSize, tmp);
+ tmp = DirectFunctionCall2(int8pl, tmp, xrecoff1);
+ result = DirectFunctionCall2(int8mi, tmp, xrecoff2);
+
+ PG_RETURN_INT64(result);
+}
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index cb43879..3e7340b 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -279,5 +279,6 @@ extern Datum pg_is_in_recovery(PG_FUNCTION_ARGS);
extern Datum
Re: [HACKERS] Configuration include directory
Hi Greg,
On Tue, Nov 15, 2011 at 11:53:53PM -0500, Greg Smith wrote:
Two years ago Magnus submitted a patch to parse all the configuration
files in a directory. After some discussion I tried to summarize what I
thought the most popular ideas were for moving that forward:
http://archives.postgresql.org/pgsql-hackers/2009-10/msg01452.php
http://archives.postgresql.org/pgsql-hackers/2009-10/msg01631.php
What a thread. I think the earlier patch was more controversial due to its
introduction of policy, a single include directory searched by default. This
latest patch is just infrastructure through which individual sites can build
all manner of configuration strategies. Many projects implement similar
directives for their configuration files, so I'm quite comfortable assuming
that some sites/packagers will like this.
-If it's not an absolute path, considers that relative to the -D option
(if specified) or PGDATA, the same logic used to locate the
postgresql.conf (unless a full path to it is used)
Let's instead mimic the behavior of the include directive, which finds its
target relative to the file containing the directive. This also removes the
warts you mentioned in your final two paragraphs:
No docs in here yet. There's one ugly bit of code here I was hoping
(but failed) to avoid. Right now the server doesn't actually save the
configuration directory anywhere. Once you leave the initial read in
SelectConfigFiles, that information is gone, and you only have the
configfile. I decided to make that configdir into a global value.
Seemed easier than trying to pass it around, given how many SIGHUP paths
could lead to this new code.
SelectConfigFiles() still does free(configdir). Due to that, in my testing,
SIGHUP reloads do not re-find relative includedirs.
I can see some potential confusion here in one case. Let's say someone
specifies a full path to their postgresql.conf file. They might assume
that the includedir was relative to the directory that file is in.
Let's say configfile is /etc/sysconfig/pgsql/postgresql.conf ; a user
might think that includedir conf.d from there would reference
/etc/sysconfig/pgsql/conf.d instead of the $PGDATA/conf.d you actually
get. Wavering on how to handle that is one reason I didn't try
documenting this yet, the decision I made here may not actually be the
right one.
For this patch, the documentation is perhaps more important than the code.
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
*** ParseConfigFp(FILE *fp, const char *conf
*** 599,604
--- 616,727
return OK;
}
+ static int
+ comparestr(const void *a, const void *b)
+ {
+ return strcmp(*(char **) a, *(char **) b);
+ }
+
+ /*
+ * Read and parse all config files in a subdirectory in alphabetical order
+ */
+ bool
+ ParseConfigDirectory(const char *includedir,
+ const char *calling_file,
+ int depth, int elevel,
+ ConfigVariable **head_p,
+ ConfigVariable **tail_p)
+ {
+ DIR *d;
+ struct dirent *de;
+ char directory[MAXPGPATH];
+ char **filenames = NULL;
+ int num_filenames = 0;
+ int size_filenames = 0;
+ bool status;
+
+ if (is_absolute_path(includedir))
+ sprintf(directory, %s, includedir);
+ else
+ sprintf(directory, %s/%s, configdir, includedir);
You need a length-limiting copy, and this won't cut it on Windows. I suggest
extracting and reusing the comparable logic in ParseConfigFile().
+ d = AllocateDir(directory);
+ if (d == NULL)
+ {
+ /*
+ * Not finding the configuration directory is not fatal,
because we
+ * still have the main postgresql.conf file. Return true so the
+ * complete config parsing doesn't fail in this case. Also avoid
+ * logging this, since it can be a normal situtation.
+ */
+ return true;
I can't see much to recommend this; it's morally equivalent to silently
ignoring include somefile or work_mem = 'foobar'.
+ }
+
+ /*
+ * Read the directory and put the filenames in an array, so we can sort
+ * them prior to processing the contents.
+ */
+ while ((de = ReadDir(d, directory)) != NULL)
+ {
+ struct stat st;
+ char filename[MAXPGPATH];
+
+ /*
+ * Only parse files with names ending in .conf.
+ * This automatically excludes things like . and .., as well
+ * as backup files and editor debris.
+ */
+ if (strlen(de-d_name) 6)
+ continue;
I would probably allow the literal file name .conf as well, mainly to avoid
documenting the need for a nonempty prefix.
+
