Re: [HACKERS] RangeVarGetRelid()
On Thu, Nov 24, 2011 at 10:54:50AM -0500, Robert Haas wrote:
> All right, here's an updated patch, and a couple of follow-on patches.
Your committed patch looks great overall. A few cosmetic points:
> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c
> @@ -309,6 +313,19 @@ RangeVarGetRelid(const RangeVar *relation, LOCKMODE
> lockmode, bool missing_ok,
> }
>
> /*
> + * Invoke caller-supplied callback, if any.
> + *
> + * This callback is a good place to check permissions: we
> haven't taken
> + * the table lock yet (and it's really best to check
> permissions before
> + * locking anything!), but we've gotten far enough to know what
> OID we
> + * think we should lock. Of course, concurrent DDL might
> things while
That last sentence needs a word around "might things".
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -767,15 +775,13 @@ RemoveRelations(DropStmt *drop)
>*/
> AcceptInvalidationMessages();
The above call can go away, now.
> @@ -843,6 +804,74 @@ RemoveRelations(DropStmt *drop)
> }
>
> /*
> + * Before acquiring a table lock, check whether we have sufficient rights.
> + * In the case of DROP INDEX, also try to lock the table before the index.
> + */
> +static void
> +RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid
> oldRelOid,
> + void *arg)
> +{
> + HeapTuple tuple;
> + struct DropRelationCallbackState *state;
> + charrelkind;
> + Form_pg_class classform;
> +
> + state = (struct DropRelationCallbackState *) arg;
> + relkind = state->relkind;
> +
> + /*
> + * If we previously locked some other index's heap, and the name we're
> + * looking up no longer refers to that relation, release the now-useless
> + * lock.
> + */
> + if (relOid != oldRelOid && OidIsValid(state->heapOid))
> + {
> + UnlockRelationOid(state->heapOid, AccessExclusiveLock);
> + state->heapOid = InvalidOid;
> + }
> +
> + /* Didn't find a relation, so need for locking or permission checks. */
That sentence needs a word around "so need".
Thanks,
nm
--
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 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. Yes, but I wasn't aware of that - I thought local_preload_libraries is defined in postgresql.conf so it seemed fine (yes, it was ugly but it did not seem that dangerous). > Why are you using pg_stat_activity for this anyway? Searching the > ProcArray seems much safer ... see CountDBBackends for an example. Because I've never user ProcArray before, but I use pg_stat_activity quite frequently, so it seemed very natural. Thanks for pointing to ProcArray/CountDBBackends, I'll see how to use that. 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] Inlining comparators as a performance optimisation
On 4 December 2011 19:17, Tom Lane 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. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services results_server_new.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] why local_preload_libraries does require a separate directory ?
Tomas Vondra 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. 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] planner fails on HEAD
Pavel Stehule 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. 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); 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] planner fails on HEAD
2011/12/4 Tom Lane : > Pavel Stehule writes: >> 2011/12/4 Tom Lane : >>> [ scratches head ... ] Given that it got past the previous assertions, >>> surely that ought to be impossible. Could we see the values of >>> cost_mergejoin's local variables, please? > >> It is strange > >> when I put a fprintf(stderr, "const literal") to exactly before or >> somewhere after assertion, then assertion is ok. Without fprintf >> assertion fails again > >> 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 > > Hmm. I'm betting that gcc has flushed one value to memory but the other > one is still in a register that's wider than memory, creating a roundoff > hazard. Can you look at the generated assembly code? I can, but tomorrow evening, I'll send a code Regards Pavel > > 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] planner fails on HEAD
Pavel Stehule writes: > 2011/12/4 Tom Lane : >> [ scratches head ... ] Given that it got past the previous assertions, >> surely that ought to be impossible. Could we see the values of >> cost_mergejoin's local variables, please? > It is strange > when I put a fprintf(stderr, "const literal") to exactly before or > somewhere after assertion, then assertion is ok. Without fprintf > assertion fails again > 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 Hmm. I'm betting that gcc has flushed one value to memory but the other one is still in a register that's wider than memory, creating a roundoff hazard. Can you look at the generated assembly code? 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] logging in high performance systems.
On 11/24/2011 06:33 PM, Theo Schlossnagle wrote: > I see the next steps being: > 1) agreeing that a problem exists (I know one does, but I suppose > consensus is req'd) +1, high volume logging is also a huge problem here at Skype. Some of the issues that immediately come to mind: - extracting usage statistics from logs - extracting error messages from logs - pushing the logs to remote log server (and no, syslog is not actually usable for that ;) > 2) agreeing that "hooks" are the right approach, if not propose a > different approach. (fwiw, it's incredible common) +1 for hook based approach. While the whole logging system could possibly use an overhaul, this is not likely going to happen for 9.2. Meanwhile hooks provide the flexibility to implement custom log collectors for those who really need it. > 3) reworking the implementation to fit in the project; I assume the > implementation I proposed will, at best, vaguely resemble anything > that gets integrated. It was just a PoC. > I'd vote for a hook to be added to EmitErrorReport. That way all logged messages pass through the hook, which could then decide whether to actually write the message to server log or to discard it. As an added bonus, the message is broken down into pieces so there is no need to parse the messages for severity, context, etc. Patch attached. regards, Martin *** a/src/backend/utils/error/elog.c --- b/src/backend/utils/error/elog.c *** *** 136,141 static int errordata_stack_depth = -1; /* index of topmost active frame */ --- 136,143 static int recursion_depth = 0; /* to detect actual recursion */ + emit_log_hook_type emit_log_hook = NULL; /* hook for log interception */ + /* buffers for formatted timestamps that might be used by both * log_line_prefix and csv logs. */ *** *** 1276,1281 EmitErrorReport(void) --- 1278,1286 CHECK_STACK_DEPTH(); oldcontext = MemoryContextSwitchTo(ErrorContext); + if (emit_log_hook) + emit_log_hook(edata); + /* Send to server log, if enabled */ if (edata->output_to_server) send_message_to_server_log(edata); *** a/src/include/utils/elog.h --- b/src/include/utils/elog.h *** *** 327,332 typedef struct ErrorData --- 327,334 int saved_errno; /* errno at entry */ } ErrorData; + typedef void (*emit_log_hook_type)(ErrorData *edata); + extern void EmitErrorReport(void); extern ErrorData *CopyErrorData(void); extern void FreeErrorData(ErrorData *edata); *** *** 347,352 typedef enum --- 349,355 extern int Log_error_verbosity; extern char *Log_line_prefix; extern int Log_destination; + extern emit_log_hook_type emit_log_hook; /* Log destination bitmap */ #define LOG_DESTINATION_STDERR 1 -- 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 4.12.2011 22:16, Tom Lane wrote: > Tomas Vondra writes: >> On 3.12.2011 18:53, Tom Lane wrote: >>> Security: it lets the DBA constrain which libraries are loadable this way. > >> But local_preload_libraries can be set only in postgresql.conf, right? > > No. It's PGC_BACKEND, which means it can be set at connection start by > a client (eg, via PGOPTIONS). > >> The problem I'm trying to solve right now is that I do have an extension >> that needs to install two .so libraries - one loaded using >> shared_preload_libraries, one loaded using local_preload_libraries. > > Um ... why would you design it like that? Well, the purpose of the extension is to limit number of connections by db/user/IP. It's using pg_stat_activity and auth hook, and the rules are loaded from a file into a shared memory segment. So I need to: 1) allocate memory to keep the rules (so it has to be loaded using shared_preload_libraries) 2) check the rules (this is done in auth hook) using pg_stat_activity 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. 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] planner fails on HEAD
2011/12/4 Tom Lane : > Pavel Stehule writes: >> #3 0x083a1dfe in ExceptionalCondition (conditionName=0x8505474 >> "!(innerstartsel <= innerendsel)", errorType=0x83db178 >> "FailedAssertion", fileName=0x8505140 "costsize.c", lineNumber=1937) >> at assert.c:57 > > [ scratches head ... ] Given that it got past the previous assertions, > surely that ought to be impossible. Could we see the values of > cost_mergejoin's local variables, please? It is strange when I put a fprintf(stderr, "const literal") to exactly before or somewhere after assertion, then assertion is ok. Without fprintf assertion fails again 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 when I put elog before calculation outerstartsel, innerstartsel,outerendsel and innerendsel then it fails the output is (last elog result) outer_skip_rows: 0.0 inner_skip_rows: 1.0 outer_rows: 208.000 inner_rows: 1.0 when I append elog to show selectivity, then it work again - related selectivity is outerstartsel: 0.000 outerendsel: 1.000 innerstartsel: 0.17 innerendsel: 0.17 Regards Pavel Stehule > > 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?
Tomas Vondra 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. 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] why local_preload_libraries does require a separate directory ?
Tomas Vondra writes: > On 3.12.2011 18:53, Tom Lane wrote: >> Security: it lets the DBA constrain which libraries are loadable this way. > But local_preload_libraries can be set only in postgresql.conf, right? No. It's PGC_BACKEND, which means it can be set at connection start by a client (eg, via PGOPTIONS). > The problem I'm trying to solve right now is that I do have an extension > that needs to install two .so libraries - one loaded using > shared_preload_libraries, one loaded using local_preload_libraries. Um ... why would you design it like that? 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 4.12.2011 17:10, Tom Lane wrote: > Tomas Vondra writes: >> Anyway, the code I posted does not fail because of pg_stat_activity, it >> fails because it attempts for find the dbname/username for the backends >> (read from pg_stat_activity). > > Well, get_database_name tries to do a syscache lookup, and the syscache > infrastructure isn't working yet. It is possible to read a shared > catalog at this stage, but you have to use lower-level access mechanisms > --- for an example with some comments, look at GetDatabaseTuple in > postinit.c. Great, this seems to work perfectly. 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)? Tomas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [PATCH] Caching for stable expressions with constant arguments v3
On 25.09.2011 05:09, Marti Raudsepp wrote: This is the third version of my CacheExpr patch. This seems to have bitrotted, thanks to the recent refactoring in eval_const_expressions(). For explanation about design decisions, please read these earlier messages: http://archives.postgresql.org/pgsql-hackers/2011-09/msg00579.php http://archives.postgresql.org/pgsql-hackers/2011-09/msg00812.php http://archives.postgresql.org/pgsql-hackers/2011-09/msg00833.php 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(). This comment in RelationGetExpressions() also worries me: /* * Run the expressions through eval_const_expressions. This is not just an * optimization, but is necessary, because the planner will be comparing * them to similarly-processed qual clauses, and may fail to detect valid * matches without this. We don't bother with canonicalize_qual, however. */ result = (List *) eval_const_expressions(NULL, (Node *) result); Do the injected CacheExprs screw up that equality? Or the constraint exclusion logic in predtest.c? -- 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] planner fails on HEAD
Pavel Stehule writes: > #3 0x083a1dfe in ExceptionalCondition (conditionName=0x8505474 > "!(innerstartsel <= innerendsel)", errorType=0x83db178 > "FailedAssertion", fileName=0x8505140 "costsize.c", lineNumber=1937) > at assert.c:57 [ scratches head ... ] Given that it got past the previous assertions, surely that ought to be impossible. Could we see the values of cost_mergejoin's local variables, please? 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] Adding Node support in outfuncs.c and readfuncs.c
Magnus Hagander writes: >> I can also maintain that in a separate git repository on github, but >> that only reduces the already very thin population that could find it >> useful. > > Since people seem to be less than super-enthusiastic about putting > into the core distro, perhaps it would at least be a good > startingpoint to do this? Should we perhaps consider a "postgres > developer tools" common repository with just a random bunch of tools > that people come up with (I assume there are more than just one of > them sitting around peoples environments..) I'm now thinking this script will be happy being on its own on github. There's already peg over there that targets developers, and pgbench-tools too, by Greg. And setting pgsrc.el as a separate repository will make it easier to integrate into el-get (well, I've just done that, so if you already use el-get, install pgsrc-el and you're done, if interested). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et 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] Inlining comparators as a performance optimisation
Robert Haas writes:
> OK, so I tried to code this up. Adding the new amproc wasn't too
> difficult (see attached). It wasn't obvious to me how to tie it into
> the tuplesort infrastructure, though, so instead of wasting time
> guessing what a sensible approach might be I'm going to use one of my
> lifelines and poll the audience (or is that ask an expert?).
Here's another cut at this. I only went as far as converting the
heap-sort code path in tuplesort.c, so there's lots more to do, but
this does sort integers successfully. Before expanding the patch
to do more, I think we need to have consensus on some API details,
in particular:
* I invented a "SortKey" struct that replaces ScanKey for tuplesort's
purposes. Right now that's local in tuplesort.c, but we're more than
likely going to need it elsewhere as well. Should we just define it
in sortsupport.h? Or perhaps we should just add the few additional
fields to SortSupportInfoData, and not bother with two struct types?
Before you answer, consider the next point.
* I wonder whether it would be worthwhile to elide inlineApplyComparator
altogether, pushing what it does down to the level of the
datatype-specific functions. That would require changing the
"comparator" API to include isnull flags, and exposing the
reverse/nulls_first sort flags to the comparators (presumably by
including them in SortSupportInfoData). The main way in which that
could be a win would be if the setup function could choose one of four
comparator functions that are pre-specialized for each flags
combination; but that seems like it would add a lot of code bulk, and
the bigger problem is that we need to be able to change the flags after
sort initialization (cf. the reversedirection code in tuplesort.c), so
we'd also need some kind of "re-select the comparator" call API. On the
whole this doesn't seem promising, but maybe somebody else has a
different idea.
* We're going to want to expose PrepareSortSupportComparisonShim
for use outside tuplesort.c too, and possibly refactor
tuplesort_begin_heap so that the SortKey setup logic inside it
can be extracted for use elsewhere. Shall we just add those to
tuplesort's API, or would it be better to create a sortsupport.c
with these sorts of functions?
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.
regards, tom lane
diff --git a/doc/src/sgml/xindex.sgml b/doc/src/sgml/xindex.sgml
index 28324361a950f31737c0cbb6909726d02ce1af5f..51f847c9f907756a823e1f76ef0275bceed7 100644
*** a/doc/src/sgml/xindex.sgml
--- b/doc/src/sgml/xindex.sgml
*** DEFAULT FOR TYPE int4 USING btree FAMILY
*** 758,764
OPERATOR 3 = ,
OPERATOR 4 >= ,
OPERATOR 5 > ,
! FUNCTION 1 btint4cmp(int4, int4) ;
CREATE OPERATOR CLASS int2_ops
DEFAULT FOR TYPE int2 USING btree FAMILY integer_ops AS
--- 758,765
OPERATOR 3 = ,
OPERATOR 4 >= ,
OPERATOR 5 > ,
! FUNCTION 1 btint4cmp(int4, int4) ,
! FUNCTION 2 btint4sortsupport(internal) ;
CREATE OPERATOR CLASS int2_ops
DEFAULT FOR TYPE int2 USING btree FAMILY integer_ops AS
diff --git a/src/backend/access/nbtree/nbtcompare.c b/src/backend/access/nbtree/nbtcompare.c
index 23f2b61fe902cff7eebcf8526cf8116d90be75e8..62d03f96de07cbbe14dedefb1596220af398caea 100644
*** a/src/backend/access/nbtree/nbtcompare.c
--- b/src/backend/access/nbtree/nbtcompare.c
***
*** 49,54
--- 49,55
#include "postgres.h"
#include "utils/builtins.h"
+ #include "utils/sortsupport.h"
Datum
*** btint4cmp(PG_FUNCTION_ARGS)
*** 83,88
--- 84,112
PG_RETURN_INT32(-1);
}
+ static int
+ btint4fastcmp(Datum x, Datum y, SortSupportInfo sinfo)
+ {
+ int32 a = DatumGetInt32(x);
+ int32 b = DatumGetInt32(y);
+
+ if (a > b)
+ return 1;
+ else if (a == b)
+ return 0;
+ else
+ return -1;
+ }
+
+ Datum
+ btint4sortsupport(PG_FUNCTION_ARGS)
+ {
+ SortSupportInfo sinfo = (SortSupportInfo) PG_GETARG_POINTER(0);
+
+ sinfo->comparator = btint4fastcmp;
+ PG_RETURN_VOID();
+ }
+
Datum
btint8cmp(PG_FUNCTION_ARGS)
{
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index cb341b8db679382c3ab8de31ff7a0bb57bc8dc50..cb6a38bcfd29fb17fc62040eef79d34aee9fe802 100644
*** a/src/backend/utils/cache/lsyscache.c
--- b/src/backend/utils/cache/lsyscache.c
*** get_compare_function_for_ordering_op(Oid
*** 287,292
--- 287,348
}
/*
+ * get_sort_function_for_ordering_op
+ * Get the OID of the datatype-specific btree sort support function,
+ * or if there is none, the btree comparison function,
+ * associated with an ordering operator (a "<" or ">" operator).
+ *
+ * *sortfunc receives the support or comparison function OID.
+ * *issupport is set TRUE if it's a support func, FALSE if a comparison func.
+ * *reverse is set FALSE if the operator is "<", TR
Re: [HACKERS] Adding Node support in outfuncs.c and readfuncs.c
On Thu, Nov 17, 2011 at 09:50, Dimitri Fontaine wrote: > Tom Lane writes: >> What you've got here could be useful >> to people who use emacs and understand they've got to hand-check the >> results. I'm not sure how much further it'd be useful to go. > > Agreed. That's the reason why I'm proposing src/tools/editors in the > first place. I find that it's enough for most of the Nodes I've been > dealing with recently (all the ones that initdb uses, for starters), and > for the other ones it helps a lot in adding the to-be-hand-edited code > at the right place in the right files. > > The goal for this tool is to be more useful an advice to Emacs users > than the usual "pick another patch that added syntax in the past and try > to reproduce what it did as far as nodes support functions goes". > > I can also maintain that in a separate git repository on github, but > that only reduces the already very thin population that could find it > useful. Since people seem to be less than super-enthusiastic about putting into the core distro, perhaps it would at least be a good startingpoint to do this? Should we perhaps consider a "postgres developer tools" common repository with just a random bunch of tools that people come up with (I assume there are more than just one of them sitting around peoples environments..) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] Review of VS 2010 support patches
On 11/29/2011 04:32 PM, Brar Piening wrote: Andrew Dunstan wrote: Some minor nitpicks: Do we really need to create all those VSProject.pm and VSSolution.pm files? They are all always included anyway. Why not just stash all the packages in Solution.pm and Project.pm? We certainly don't *need* them. Having different files separates the tasks of generating different target file formats into different source files. In my opinion this makes it easier to find the code that is actually generating the files that get used in a specific build environment. While the VSSolution.pm and VC200nProject.pm files are indeed not much more than stubs that could eventually be extended in future (and probably never will) VC2010Project.pm contains the whole code for generating the new file format which would significantly bloat up the code in Project.pm that currently contains the common code for generating the old file formats. Anyhow - this is just my opinion and my intention is to help improving the Windows build process and not forcing my design into the project. Well, I do also dislike the asymmetry of it. Here's what I suggest: for the Solution files, we'll just put the object packages in Solution.pm. There really doesn't seem like any need for those to have tiny files on their own. For the Project files, factor out the 2005/2008 specific parts from Project.pm into a new file, and have a new file for the equivalent parts of your new VC2010Project.pm. Then we'll add packages to Project.pm to create objects just like I'm suggesting above for Solution.pm. The result is then more symmetrical and we'll have three new files instead of seven (counting VSObjectFactory.pm). Perhaps, too, this has all got sufficiently complicated that adding some descritpion of what's going on here to README would be in order. I suspect some of my fellow committers tend to look at the whole thing and scratch their heads a bit, and that means expecting other people to make sense if it is probably a bit much ;-) cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [DOCS] Moving tablespaces
Andrew Dunstan writes: > On 12/04/2011 11:41 AM, Tom Lane wrote: >> Hm, how portable is symlink-reading? If we can actually do that >> without big headaches, then +1. > I wondered that, specifically about Windows junction points, but we seem > to have support for it already in dirmod.c::pgreadlink(). Surely there's > no other currently supported platform where it would even be a question? readlink is required by Single Unix Spec v2 (1997), which is what we've been treating as our baseline expectation for Unix-oid platforms for awhile now. Given that we dealt with the Windows side already, I don't see a problem with making this assumption. At worst we'd end up needing a couple more emulations in src/port, since surely there's *some* way to do it on any platform with symlinks. 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] [DOCS] Moving tablespaces
On 12/04/2011 11:41 AM, Tom Lane wrote: 1) Remove the column. Rely on the symlink. Create a pg_get_tablespace_location(oid) function, that could be used by pg_dumpall and friends, that just reads the symlink. Hm, how portable is symlink-reading? If we can actually do that without big headaches, then +1. I wondered that, specifically about Windows junction points, but we seem to have support for it already in dirmod.c::pgreadlink(). Surely there's no other currently supported platform where it would even be a question? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql setenv command
On 11/28/2011 09:19 PM, Josh Kupershmidt wrote: Other than those small gripes, the patch looks fine. Attached is an updated version to save you some keystrokes. Thanks. Committed. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [DOCS] Moving tablespaces
On Sun, Dec 4, 2011 at 17:41, Tom Lane wrote: > Magnus Hagander writes: >> And IIRC, we don't actually *use* spclocation anywhere. > > Just for pg_dump, I think. pg_dumpall :-) It's also used in pg_upgrade and pg_basebackup, but those are easily dealt with if we define a function for it. >> How about we >> just get rid of them as independents? We could either: > >> 1) Remove the column. Rely on the symlink. Create a >> pg_get_tablespace_location(oid) function, that could be used by >> pg_dumpall and friends, that just reads the symlink. > > Hm, how portable is symlink-reading? If we can actually do that > without big headaches, then +1. We already use readlink in a couple of places - including getting called from find_my_exec. It's also used in pg_basebackup. The use in find_my_exec is conditional on HAVE_READLINK, but not the one in backend/replication/basebackup.c. An oversight from my side when putting that in, but it shows that every single bf member we have now has readlink - else the whole compilation would fail, AFAICT. It's not directly portable to win32, but we have a wrapper function for it in port/ already. So I think we're fairly committed to having readlink already. >> 2) Forcibly update the spclocation column when we start the server to >> be whatever the symlink points to. That will at least automatically >> restore the system to being consistent. > > -1, running a transaction at that level will be a giant pita. > And how would you do it at all on a hot standby slave? yeah, it would break that usage scenario, so I'm -1 for that one as well. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] [DOCS] Moving tablespaces
Magnus Hagander writes: > And IIRC, we don't actually *use* spclocation anywhere. Just for pg_dump, I think. > How about we > just get rid of them as independents? We could either: > 1) Remove the column. Rely on the symlink. Create a > pg_get_tablespace_location(oid) function, that could be used by > pg_dumpall and friends, that just reads the symlink. Hm, how portable is symlink-reading? If we can actually do that without big headaches, then +1. > 2) Forcibly update the spclocation column when we start the server to > be whatever the symlink points to. That will at least automatically > restore the system to being consistent. -1, running a transaction at that level will be a giant pita. And how would you do it at all on a hot standby slave? 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] Command Triggers
Andres Freund writes: > I have two questions now: > First, does anybody think it would be worth getting rid of the duplication > from OpenIntoRel (formerly from execMain.c) in regard to DefineRelation()? That's probably reasonable to do, since as you say it would remove the opportunity for bugs-of-omission in the CTAS table creation step. OTOH, if you find yourself having to make any significant changes to DefineRelation, then maybe not. > Secondly, I am currently wondering whether it would be a good idea to use the > ModifyTable infrastructure for doing the insertion instead an own > DestReceiver > infrastructure thats only used for CTAS. I think this is probably a bad idea; it will complicate matters and buy little. There's not a lot of stuff needed for the actual data insertion step, since we know the table can't have any defaults, constraints, triggers, etc as yet. 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] [DOCS] Moving tablespaces
On Sun, Dec 4, 2011 at 17:12, Bruce Momjian wrote: > Magnus Hagander wrote: >> On Sun, Dec 4, 2011 at 00:43, Bruce Momjian wrote: >> > Do we have any documentation about how to move a tablespace to a new >> > directory? ?If not, I think we should write some. >> >> Do we have any support for doing it? (Yes, it works, but anything that >> requires manual hacking of system catalogs really can't be considered >> supported, can it?) > > True. It is something we just don't support? They have to dump, edit > the dump, and reload, to change a tablespace directory? Yikes. Is that > the state we are in? Has no one complained about this? They just use > symlinks? AFAIK, we don't. What you can do is take the db offline, change the symlink, start it up agin and manually change pg_tablespace.spclocation. That seems quite ugly though. And if you forget one step, everything seems to work, but you have two inconsistent definitions of the tablespace. And IIRC, we don't actually *use* spclocation anywhere. How about we just get rid of them as independents? We could either: 1) Remove the column. Rely on the symlink. Create a pg_get_tablespace_location(oid) function, that could be used by pg_dumpall and friends, that just reads the symlink. 2) Forcibly update the spclocation column when we start the server to be whatever the symlink points to. That will at least automatically restore the system to being consistent. Option 1 would also make it a lot easier to in a supported way allow tablespaces to have different locations on replica masters and slaves. A tool like pg_basebackup could easily provide for something like --relocate-tablespace mytblspc=/new/path and just rewrite the symlink on the fly. But we cannot modify the pg_tablespace system catalog to be different on the slave and the master... It does seem rather obvious to me that this would be a win, so I'm most likely missing something here. So please shoot a hole in the theory for me ;) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] [PATCH] PostgreSQL fails to build with 32bit MinGW-w64
On 12/04/2011 06:31 AM, Magnus Hagander wrote: On Sun, Dec 4, 2011 at 09:14, NISHIYAMA Tomoaki 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) #include 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
Re: [HACKERS] cannot read pg_class without having selected a database / is this a bug?
Tomas Vondra writes: > On 4.12.2011 05:19, Tom Lane wrote: >> It should be possible to access shared catalogs from an auth hook. >> pg_stat_activity is neither shared nor a catalog. Like Robert, >> I find it astonishing that this works ever, because the info needed >> simply isn't available until you've connected to a particular database. >> The fact that the view is actually defined the same in every database >> doesn't enter into that ... > Hmmm, I do admit this is the first time I play with these things > (relcache, catalogs ...) so closely. so there are obviously things I'm > not aware of. For example I'm a bit confused what is / is not a shared > catalogue. Thanks in advance for your patience. See pg_class.relisshared. > Anyway, the code I posted does not fail because of pg_stat_activity, it > fails because it attempts for find the dbname/username for the backends > (read from pg_stat_activity). Well, get_database_name tries to do a syscache lookup, and the syscache infrastructure isn't working yet. It is possible to read a shared catalog at this stage, but you have to use lower-level access mechanisms --- for an example with some comments, look at GetDatabaseTuple in postinit.c. 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 Sat, Sep 24, 2011 at 9:09 PM, Marti Raudsepp wrote: > Hi list! > > This is the third version of my CacheExpr patch. i wanted to try this, but it no longer applies in head... specially because of the change of IF's for a switch in src/backend/optimizer/util/clauses.c it also needs adjustment for the rangetypes regress test -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cannot read pg_class without having selected a database / is this a bug?
On 4.12.2011 05:19, Tom Lane wrote:
> Tomas Vondra writes:
>> That might explain why it fails at first and then works just fine,
>> although it's a bit strange. Wouldn't that mean you can't access any
>> catalogs from the auth hook?
>
> It should be possible to access shared catalogs from an auth hook.
> pg_stat_activity is neither shared nor a catalog. Like Robert,
> I find it astonishing that this works ever, because the info needed
> simply isn't available until you've connected to a particular database.
> The fact that the view is actually defined the same in every database
> doesn't enter into that ...
Hmmm, I do admit this is the first time I play with these things
(relcache, catalogs ...) so closely. so there are obviously things I'm
not aware of. For example I'm a bit confused what is / is not a shared
catalogue. Thanks in advance for your patience.
Anyway, the code I posted does not fail because of pg_stat_activity, it
fails because it attempts for find the dbname/username for the backends
(read from pg_stat_activity).
I've removed pg_stat_activity (see the code below) and it still fails.
The reason is that get_database_name attempts to read pg_database, but
once it gets to ScanPgRelation in relcache.c it notices MyDatabaseID=0
and so the check fails
This is the simplified code:
if (status == STATUS_OK)
{
char * db;
LWLockAcquire(lock, LW_EXCLUSIVE);
sleep(1);
if (MyBackendId > 2) {
db = get_database_name(17000);
}
sleep(4);
LWLockRelease(lock);
}
If you start two backends at the same time, the first one gets ID=2 and
skips the get_database_name call, the second one gets ID=3 and calls the
function (and it fails). Subsequent calls work, because the first
backend initializes the relcache or something.
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 Sun, Dec 4, 2011 at 09:14, NISHIYAMA Tomoaki 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) > #include > 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. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] [PATCH] PostgreSQL fails to build with 32bit MinGW-w64
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) #include 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. On 2011/12/04, at 12:55, NISHIYAMA Tomoaki wrote: > Hi, > > On 2011/12/04, at 9:45, Andrew Dunstan wrote: >>> Yes, but there's a deal more work to do here. This whole thing is falling >>> over in my build environment (64 bit Windows 7, MinGW/MSys, the machine >>> that runs pitta on the buildfarm.) >>> >>> This is a long way from a done deal. >> >> In particular, it's a major mess because it does this (or at least the >> version I'm using does): >> >> #define stat _stat64 >> >> which plays merry hell with pgwin32_safestat(). Working around that looks >> very unpleasant indeed. > > > Thank you for testing and reporting. > Would you mind giving me a bit more information on the environment? > --especially for the MinGW/MSYS versions and any other component that is > required. > > I tested on a virtual machine running 64 bit Windows 7 SP1, > installing MinGW/MSYS in the clean state and then compile. > The versions coming with pre-packaged repository catalogues of 20110802 > (gcc 4.5.2) and latest catalogue (gcc-4.6.1-2) compiles successfully. > > > On 2011/12/04, at 9:45, Andrew Dunstan wrote: > >> >> >> On 12/03/2011 06:12 PM, Andrew Dunstan wrote: >>> >>> >>> On 12/03/2011 09:59 AM, Magnus Hagander wrote: On Sat, Dec 3, 2011 at 15:49, NISHIYAMA Tomoaki wrote: > Hi, > >> Have you verified if tihs affects _MSC_VER< 1400? Suddently that >> branch would care about HAVE_CRTDEFS_H, and I'm not sure if that's >> something we need to worry about. > > I have no MSVC. In that sense it is not verified in fact, and I hope > those who knows well would kindly comment on it. > > However, it appears that pg_config.h is not created through > configure, but just copied from pg_config.h.win32 in those > compilers and thus HAVE_CRTDEFS_H will not be defined. > So, I think this code fragment will not be enabled in > _MSC_VER< 1400 Hmm, true. Unless HAVE_CRTDEFS_H is defined by the sytem, which it likely isn't - I was confusing it with the kind of defines that MSVC tends to stick in their own headerfiles, and thought that's what you were testing for. >>> >>> >>> Yes, but there's a deal more work to do here. This whole thing is falling >>> over in my build environment (64 bit Windows 7, MinGW/MSys, the machine >>> that runs pitta on the buildfarm.) >>> >>> This is a long way from a done deal. >> >> >> In particular, it's a major mess because it does this (or at least the >> version I'm using does): >> >> #define stat _stat64 >> >> >> which plays merry hell with pgwin32_safestat(). Working around that looks >> very unpleasant indeed. >> >> >> 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 > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
