Re: [HACKERS] foreign key locks, 2nd attempt
On Thu, Nov 10, 2011 at 8:17 PM, Christopher Browne cbbro...@gmail.com wrote: On Sun, Nov 6, 2011 at 2:28 AM, Jeroen Vermeulen j...@xs4all.nl wrote: On 2011-11-04 01:12, Alvaro Herrera wrote: I would like some opinions on the ideas on this patch, and on the patch itself. If someone wants more discussion on implementation details of each part of the patch, I'm happy to provide a textual description -- please just ask. Jumping in a bit late here, but thanks for working on this: it looks like it could solve some annoying problems for us. I do find myself idly wondering if those problems couldn't be made to go away more simply given some kind of “I will never ever update this key” constraint. I'm having trouble picturing the possible lock interactions as it is. :-) +1 on that, though I'd make it more general than that. There's value in having an immutability constraint on a column, where, in effect, you're not allowed to modify the value of the column, once assigned. That certainly doesn't prevent issuing DELETE + INSERT to get whatever value you want into place, but that's a big enough hoop to need to jump through to get rid of some nonsensical updates. And if the target of a foreign key constraint consists of immutable columns, then, yes, indeed, UPDATE on that table no longer conflicts with references. In nearly all cases, I'd expect that SERIAL would be reasonably followed by IMMUTABLE. create table something_assigned ( something_id serial immutable primary key, something_identifier text not null unique ); This is a good idea but doesn't do what KEY LOCKS are designed to do so. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign key locks, 2nd attempt
On Thu, Nov 3, 2011 at 6:12 PM, Alvaro Herrera alvhe...@alvh.no-ip.org wrote: So Noah Misch proposed using the FOR KEY SHARE syntax, and that's what I have implemented here. (There was some discussion that instead of inventing new SQL syntax we could pass the necessary lock mode internally in the ri_triggers code. That can still be done of course, though I haven't done so in the current version of the patch.) FKs are a good short hand, but they aren't the only constraint people implement. It can often be necessary to write triggers to enforce complex constraints. So user triggers need access to the same facilities that ri triggers uses. Please keep the syntax. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Core Extensions relocation
On 11/18/2011 03:36 PM, Josh Berkus wrote: Of course, packagers may then reasonably ask why that code is not just part of the core? Let me step back from the implementation ideas for a minute and talk about this, and how it ties into release philosophy. The extensions infrastructure for PostgreSQL is one of its strongest features. We can use it as a competitive advantage over other databases, one that can make this database stable and continuously innovating at the same time. But that's not happening enough yet; I feel this change is a major step in that direction. There's no demonstration that extensions are edible dog food like the core database visibly eating a can. To see why this matters so much, let's compare two stereotypes of PostgreSQL users at different extremes of upgrade tolerance. First we have the classic enterprise DBA. Relative to this person's expectations, PostgreSQL releases are way too fast. They can't upgrade their database every year; that's madness. This is the person who we yell at about how they should upgrade to the latest minor point release, because once they have a working system they touch *nothing*. For this user, the long beta period of new PostgreSQL releases, and its general conservative development model, are key components to PostgreSQL being suitable for them. At the other extreme, we have the software developer with a frantic development/release schedule, the one who's running the latest stable version of every tool they use. This person expects some bugs in them, and the first reaction to running into one is to ask is this fixed in the next version? You'll find at least one component in their stack that's labeled compiled from the latest checkout because that was the only way to get a working version. And to them, the yearly release cycle of PostgreSQL is glacial. When they run into a limitation that is impacting a user base that's doubling every few months, they can't wait a year for a fix; they could easily go out of business by then. The key to satisfying both these extremes at once is a strong extensions infrastructure, led by the example of serious tools that are provided that way in the PostgreSQL core. For this to catch on, we need the classic DBAs to trust core extensions enough to load them into production. They don't do that now because the current contrib description sounds too scary, and they may not even have loaded that package onto the server. And we need people who want more frequent database core changes to see that extensions are a viable way to build some pretty extensive server hacks. I've submitted two changes to this CommitFest that are enhancing features in this core extensions set. Right now I have multiple customers who are desperate for both of those features. With extensions, I can give them changes that solve their immediate crisis right now, almost a full year before they could possibly appear in a proper release of PostgreSQL. And then I can push those back toward community PostgreSQL, with any luck landing in the next major version. Immediate gratification for the person funding development, and peer reviewed code that goes through a long beta and release cycle. That's the vision I have for a PostgreSQL that is simultaneously stable and agile. The easiest way to get there it is to lead by example--by having extensions that provide necessary, visible components to core, while still being obviously separate components. That's the best approach for proving this development model works and is suitable for everyone. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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] foreign key locks, 2nd attempt
Simon Riggs si...@2ndquadrant.com writes: On Thu, Nov 3, 2011 at 6:12 PM, Alvaro Herrera alvhe...@alvh.no-ip.org wrote: So Noah Misch proposed using the FOR KEY SHARE syntax, and that's what I have implemented here. (There was some discussion that instead of inventing new SQL syntax we could pass the necessary lock mode internally in the ri_triggers code. That can still be done of course, though I haven't done so in the current version of the patch.) FKs are a good short hand, but they aren't the only constraint people implement. It can often be necessary to write triggers to enforce complex constraints. So user triggers need access to the same facilities that ri triggers uses. Please keep the syntax. It's already the case that RI triggers require access to special executor features that are not accessible at the SQL level. I don't think the above argument is a compelling reason for exposing more such features at the SQL level. All we need is that C-coded functions can get at them somehow. 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] EXPLAIN (plan off, rewrite off) for benchmarking
Andres Freund and...@anarazel.de writes: Explain is just a vehicle here, I admit that. But on what else should I bolt it? If you don't like CREATE RULE, try having your test program send just Parse messages, and not Bind/Execute. I still dislike the idea of exposing a fundamentally-broken-and-useless variant of EXPLAIN in order to have a test harness for a variant of performance testing that hardly anyone cares about. (There is no real-world case where the performance of the parser matters in isolation.) If we do that, it will be a feature that we have to support forever, and possibly fix bugs in --- what if the system crashes because the rewriter wasn't invoked, for 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] COUNT(*) and index-only scans
Thom Brown t...@linux.com writes: So is there a chance of getting bitmap index-only scans? Don't hold your breath. It seems like a huge increment of complexity for probably very marginal gains. The point of a bitmap scan (as opposed to regular indexscan) is to reduce heap accesses by combining visits to the same page; but it's not clear how useful that is if you're not making heap accesses at all. Robert's sketch of how this could work, full of don't-know-how-to-do-this as it was, still managed to omit a whole lot of reasons why it wouldn't work. Notably the fact that the index AM API for bitmap scans is to return a bitmap, not index-tuple data; and trying to do the latter would break a lot of the performance advantages that exist now for bitmap scans. 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] COUNT(*) and index-only scans
On 19 November 2011 16:08, Tom Lane t...@sss.pgh.pa.us wrote: Thom Brown t...@linux.com writes: So is there a chance of getting bitmap index-only scans? Don't hold your breath. It seems like a huge increment of complexity for probably very marginal gains. The point of a bitmap scan (as opposed to regular indexscan) is to reduce heap accesses by combining visits to the same page; but it's not clear how useful that is if you're not making heap accesses at all. Well consider: pgbench=# explain analyse select count(*) from pgbench_accounts where aid between 143243 and 374825 or aid between 523242 and 712111; QUERY PLAN --- Aggregate (cost=83016.38..83016.39 rows=1 width=0) (actual time=1039.282..1039.282 rows=1 loops=1) - Bitmap Heap Scan on pgbench_accounts (cost=7934.54..81984.58 rows=412719 width=0) (actual time=243.012..977.946 rows=420453 loops=1) Recheck Cond: (((aid = 143243) AND (aid = 374825)) OR ((aid = 523242) AND (aid = 712111))) - BitmapOr (cost=7934.54..7934.54 rows=423802 width=0) (actual time=228.934..228.934 rows=0 loops=1) - Bitmap Index Scan on pgbench_accounts_pkey (cost=0.00..4299.40 rows=235782 width=0) (actual time=134.410..134.410 rows=231583 loops=1) Index Cond: ((aid = 143243) AND (aid = 374825)) - Bitmap Index Scan on pgbench_accounts_pkey (cost=0.00..3428.78 rows=188020 width=0) (actual time=94.520..94.520 rows=188870 loops=1) Index Cond: ((aid = 523242) AND (aid = 712111)) Total runtime: 1039.598 ms (9 rows) Since I can't get this to use an index-only scan, it will always visit the heap. Instead I'd be forced to write: pgbench=# explain analyse select count(*) from (select aid from pgbench_accounts where aid between 143243 and 374825 union all select aid from pgbench_accounts where aid between 523242 and 712111) x; QUERY PLAN -- Aggregate (cost=17263.72..17263.73 rows=1 width=0) (actual time=232.053..232.053 rows=1 loops=1) - Append (cost=0.00..16204.22 rows=423802 width=0) (actual time=10.925..195.134 rows=420453 loops=1) - Subquery Scan on *SELECT* 1 (cost=0.00..9015.04 rows=235782 width=0) (actual time=10.925..90.116 rows=231583 loops=1) - Index Only Scan using pgbench_accounts_pkey on pgbench_accounts (cost=0.00..6657.22 rows=235782 width=4) (actual time=10.924..61.822 rows=231583 loops=1) Index Cond: ((aid = 143243) AND (aid = 374825)) - Subquery Scan on *SELECT* 2 (cost=0.00..7189.18 rows=188020 width=0) (actual time=0.062..62.953 rows=188870 loops=1) - Index Only Scan using pgbench_accounts_pkey on pgbench_accounts (cost=0.00..5308.98 rows=188020 width=4) (actual time=0.061..40.343 rows=188870 loops=1) Index Cond: ((aid = 523242) AND (aid = 712111)) Total runtime: 232.291 ms (9 rows) These 2 queries are equal only because the ranges being checked don't overlap, so if arbitrary values were being substituted, and the ranges did happen to overlap, that last method couldn't work. I'd have to use a UNION ALL in that particular case, which adds a lot of overhead due to de-duplication. While I accept that maybe adapting the existing bitmap index scan functionality isn't necessarily desirable, would it be feasible to create a corresponding bitmap index-only scan method. -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: 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] Core Extensions relocation
On 11/18/2011 09:35 AM, Tom Lane wrote: Subdividing/rearranging contrib makes the packager's life more complicated, *and* makes his users' lives more complicated, if only because things aren't where they were before. It seems unlikely to happen, at least in the near term. Users are visibly suffering by the current packaging. Production DBAs are afraid to install contrib because it's described as untrustworthy. They are hit by emergencies that the inspection tools would help with, but cannot get contrib installed easily without root permissions. They have performance issues that the contrib modules I'm trying to relocate into the server package would help with, but company policies related to post-deployment installation mean they cannot use them. They have to always be installed to make this class of problem go away. If you feel there are more users that would be negatively impacted by some directories moving than what I'm describing above, we are a very fundamental disagreement here. The status quote for all of these extensions is widely understood to be unusable in common corporate environments. Packagers should be trying to improve the PostgreSQL experience, and I'm trying to help with that. In the face of pushback from packagers, I can roll my own packages that are designed without this problem; I'm being pushed into doing that instead of working on community PostgreSQL already. But I'd really prefer to see this very common problem identified as so important it should get fixed everywhere instead. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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] EXPLAIN (plan off, rewrite off) for benchmarking
On Saturday, November 19, 2011 04:52:10 PM Tom Lane wrote:
Andres Freund and...@anarazel.de writes:
Explain is just a vehicle here, I admit that. But on what else should I
bolt it?
If you don't like CREATE RULE, try having your test program send just
Parse messages, and not Bind/Execute.
That sounds like a plan. Except that I would prefer to use pgbench. To avoid
the planning overhead...
I see it correctly that I would need to
I tpgbench is a more appropriate place to add such an option...
If we do that, it will be a
feature that we have to support forever, and possibly fix bugs in ---
what if the system crashes because the rewriter wasn't invoked, for
example?
rewrite=off aborts before planning, so that specific problem I don't see. The
name is rather bad I admit. Its mostly there to avoid the copyObject() which
skews results considerably:
* Because the rewriter and planner tend to scribble on the input, we
make
* a preliminary copy of the source querytree. This prevents problems
in
* the case that the EXPLAIN is in a portal or plpgsql function and is
* executed repeatedly. (See also the same hack in DECLARE CURSOR and
* PREPARE.) XXX FIXME someday.
*/
rewritten = QueryRewrite((Query *) copyObject(stmt-query));
I still dislike the idea of
exposing a fundamentally-broken-and-useless variant of EXPLAIN in order
to have a test harness for a variant of performance testing that hardly
anyone cares about. (There is no real-world case where the performance
of the parser matters in isolation.)
I absolutely cannot agree on the fact that the speed parse-analyze is
irrelevant though. In several scenarios using prepared statements is not a
viable/simple option. Think transaction-level pooling for example. Or the
queries generated by all those ORMs out there.
When executing many small statments without prepared statments a rather large
portion of time is spent parsing.
Consider:
statement: EXPLAIN SELECT * FROM pg_class WHERE oid = 1000;
pgbench -M simple -f ~/.tmp/simple1.sql -T 3
tps = 16067.780407
pgbench -M prepared -f ~/.tmp/simple1.sql -T 3
tps = 24348.244630
In both variants the queries are fully planned as far as I can see. The only
difference there is parsing. I do find the difference in speed rather notable.
That does represent measurements from realworld profiles I gathered were
functions related to parsing + parse analysis contribute up to 1/3 of the
runtime.
Obviously nobody needs to benchmark the parser alone in a production scenario.
But if you want to improve the overall performance of some workload analysing
bits and pieces alone to get a useful, more detailed profile is a pretty sane
approach.
Why should that be a variant of performance testing that nobody cares about?
Andres
From c5251309a3eb3e826eab9bf792d4ba84fca63738 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Wed, 9 Nov 2011 17:52:38 +0100
Subject: [PATCH] Add new EXPLAIN options plan and rewrite
Those commands are useful to make it easier to gauge the costs of those
respective steps when introducing new features or optimizing existing code.
Another useful thing would be to be able to separate the parse and
parse-analyze steps, but that is more invasive.
---
src/backend/commands/explain.c | 56 +--
src/include/commands/explain.h |2 +
2 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e38de5c..2987d3b 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -133,6 +133,10 @@ ExplainQuery(ExplainStmt *stmt, const char *queryString,
es.costs = defGetBoolean(opt);
else if (strcmp(opt-defname, buffers) == 0)
es.buffers = defGetBoolean(opt);
+ else if (strcmp(opt-defname, plan) == 0)
+ es.plan = defGetBoolean(opt);
+ else if (strcmp(opt-defname, rewrite) == 0)
+ es.rewrite = defGetBoolean(opt);
else if (strcmp(opt-defname, format) == 0)
{
char *p = defGetString(opt);
@@ -158,11 +162,25 @@ ExplainQuery(ExplainStmt *stmt, const char *queryString,
opt-defname)));
}
+
if (es.buffers !es.analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg(EXPLAIN option BUFFERS requires ANALYZE)));
+ if ((!es.plan || !es.rewrite) es.analyze)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(EXPLAIN option !PLAN contradicts ANALYZE)));
+ /*
+ * XXX: we could check for !plan rewrite but that would mean !rewrite
+ * would have to explicitly specified when disabling planning...
+ */
+
+ /* emit opening boilerplate */
+ ExplainBeginOutput(es);
+
+ Assert(IsA(stmt-query, Query));
/*
* Parse analysis was done already, but we still have to run the rule
* rewriter. We do not do AcquireRewriteLocks: we assume the query either
@@ -175,11 +193,13 @@
[HACKERS] Singleton range constructors versus functional coercion notation
The singleton range constructors don't work terribly well.
regression=# select int4range(42); -- ok
int4range
---
[42,43)
(1 row)
regression=# select int4range(null);-- not so ok
int4range
---
(1 row)
regression=# select int4range('42');-- clearly not ok
ERROR: malformed range literal: 42
LINE 1: select int4range('42');
^
DETAIL: Missing left parenthesis or bracket.
The second of these might at first glance seem all right; until you
remember that range_constructor1 is not strict and throws an error
on null input. So it's not getting called.
What is actually happening in both cases 2 and 3 is that
func_get_detail() is interpreting the syntax as equivalent to
'literal'::int4range. We do not have a whole lot of room to maneuver
here, because that equivalence is of very long standing; and as
mentioned in the comments in that function, we can't easily alter its
priority relative to other interpretations.
I don't immediately see a solution that's better than dropping the
single-argument range constructors. Even if you don't care that much
about the NULL case, things like this are pretty fatal from a usability
standpoint:
regression=# select daterange('2011-11-18');
ERROR: malformed range literal: 2011-11-18
LINE 1: select daterange('2011-11-18');
^
DETAIL: Missing left parenthesis or bracket.
I'm not sure that singleton ranges are so useful that we need to come up
with a short-form input method for them. (Yeah, I know that this case
could be fixed with an explicit cast, but if we leave it like this we'll
get a constant stream of bug reports about it.)
For that matter, the zero-argument range constructors seem like
mostly a waste of catalog space too ... what's wrong with writing
'empty'::int4range when you need 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] RFC: list API / memory allocations
Andres, * Andres Freund (and...@anarazel.de) wrote: For that I added new functions/defines which allocate all the needed memory in one hunk: list_immutable_make$n(), List *list_new_immutable_n(NodeTag t, size_t n); List *list_make_n(NodeTag t, size_t n, ...); A while back, I posted a patch to try and address this same issue. The approach that I took was to always pre-allocate a certain (#defined) amount (think it was 5 or 10 elements). There were a number of places that caused problems with that approach because they hacked on the list element structures directly (instead of using the macros/functions)- you'll want to watch out for those areas in any work on lists. That patch is here: http://archives.postgresql.org/pgsql-hackers/2011-05/msg01213.php The thread on it might also be informative. I do like your approach of being able to pass the ultimate size of the list in.. Perhaps the two approaches could be merged? I was able to make everything work with my approach, provided all the callers used the list API (I did that by making sure the links, etc, actually pointed to the right places in the pre-allocated array). One downside was that the size ended up being larger that it might have been in some cases. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] RFC: list API / memory allocations
* Tom Lane (t...@sss.pgh.pa.us) wrote: Now, if you could do something that *doesn't* restrict what operations could be applied to the lists, that would be good. If the API is followed, I believe my previous patch works for everything, but it wasn't variable about the size of the new list. Perhaps we could combine the two approaches, though there would be more overhead from dealing with a variable bitmap for what's currently used. I've wished for a long while that we could allocate the list header and the first list cell in a single palloc cycle. You've mentioned that before and, to be honest, I could have sworn that we're doing that already.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] EXPLAIN (plan off, rewrite off) for benchmarking
Andres Freund and...@anarazel.de writes: On Saturday, November 19, 2011 04:52:10 PM Tom Lane wrote: If you don't like CREATE RULE, try having your test program send just Parse messages, and not Bind/Execute. That sounds like a plan. Except that I would prefer to use pgbench. Well, how about plan C: write a small C function that consists of a loop around calling just the part of the parser you want to test? I've done that in the past when I wanted fine-grained profiles, and it works a whole lot better than anything involving per-query messages from the client side. 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] pgsql_fdw, FDW for PostgreSQL server
Hanada-san, I'm still under reviewing of your patch, so the comment is not overall, sorry. I'm not sure whether the logic of is_foreign_expr() is appropriate. It checks oid of the function within FuncExpr and OpExpr to disallow to push down user-defined functions. However, if a user-defined operator is implemented based on built-in functions with different meaning, is it really suitable to push-down? E.g) It is available to define '=' operator using int4ne, even though quite nonsense. So, I'd like to suggest to check oid of the operator; whether it is a built-in, or not. On the other hand, this hard-wired restriction may damage to the purpose of this module; that enables to handle a query on multiple nodes in parallel. I'm not sure whether it is right design that is_foreign_expr() returns false when the supplied expr contains mutable functions. Probably, here are two perspectives. The one want to make sure functions works with same manner in all nodes. The other want to distribute processing of queries as possible as we can. Here is a trade-off between these two perspectives. So, how about an idea to add a guc variable to control the criteria of pushing-down. Thanks, 2011年11月15日17:55 Shigeru Hanada shigeru.han...@gmail.com: Hi, Attached are revised version of pgsql_fdw patches. fdw_helper_funcs_v2.patch provides some functions which would be useful to implement FDW, and document about FDW helper functions including those which exist in 9.1. They are not specific to pgsql_fdw, so I separated it from pgsql_fdw patch. pgsql_fdw_v4.patch provides a FDW for PostgreSQL. This patch requires fdw_helper_funcs_v2.patch has been applied. Changes done since last version are: * Default of fetch_count option is increased to 1, as suggested by Pavel Stehule. * Remove unnecessary NULL check before PQresultStatus. * Evaluate all conditions on local side even if some of them has been pushed down to remote side, to ensure that results are correct. * Use fixed costs for queries which contain external parameter, because such query can't be used in EXPLAIN command. Regards, -- Shigeru Hanada -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
2011/11/18 Robert Haas robertmh...@gmail.com: On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: Part-2) Groundworks on objectaddress.c This patch adds necessary groundworks for Part-3 and Part-4. It adds ObjectPropertyType of objectaddress.c index-oid and cache-id for name lookup and attribute number of object name; these field is used to detect namespace conflict on object_exists_namespace() that shall be called on refactored ALTER SET SCHEMA/RENAME TO. It also reduce some arguments of check_object_ownership(), and allows to call this function without knowledge of ObjectType and text representation. It allows to check ownership using this function from the code path of AlterObjectNamespace_oid() that does not have (or need to look up catalog to obtain) ObjectType information. I spent some time wading through the code for parts 2 through 4, and I guess I'm not sure this is headed in the right direction. If we need this much infrastructure in order to consolidate the handling of ALTER BLAH .. SET SCHEMA, then maybe it's not worth doing. But I'm not sure why we do. My thought here was that we should extended the ObjectProperty array in objectaddress.c so that AlterObjectNamespace can get by with fewer arguments - specifically, it seems like the following ought to be things that can be looked up via the ObjectProperty mechanism: int oidCacheId, int nameCacheId, int Anum_name, int Anum_namespace, int Anum_owner, AclObjectKind acl_kind Thanks for your reviewing, and sorry for not a timely response. I tried to add these items into ObjectProperty and replace existing caller of AlterObjectNamespace, however, it seemed to me these members (especially AclObjectKind) were too specific with current implementation of the AlterObjectNamespace. I think, SET SCHEMA, RENAME TO and OWNER TO are candidate to consolidate similar codes using facilities in objectaddress.c. Even though SET SCHEMA is mostly consolidated with AlterObjectNamespace, RENAME TO and OWNER TO are implemented individual routines for each object types. So, I thought it is preferable way to provide groundwork to be applied these routines also. In the part-2 patch, I added object_exists_namespace() to check namespace conflicts commonly used to SET SCHEMA and RENAME TO, although we have individual routines for RENAME TO. And, I also modified check_ownership() to eliminate objtype/object/objarg; that allows to invoke this function from code paths without these information, such as shdepReassignOwned() or AlterObjectNamespace_oid(). The advantage of that, or so it seems to me, is that all this information is in a table in objectaddress.c where multiple clients can get at it at need, as opposed to the current system where it's passed as arguments to AlterObjectNamespace(), and all that would need to be duplicated if we had another function that did something similar. Yes, correct. Now, what you have here is a much broader reworking. And that's not necessarily bad, but at the moment I'm not really seeing how it benefits us. In my point, if individual object types need to have its own handler for alter commands, points of the code to check permissions are also distributed for each object types. It shall be a burden to maintain hooks that allows modules (e.g sepgsql) to have permission checks. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] range_adjacent and discrete ranges
On Fri, 2011-11-18 at 14:47 -0500, Tom Lane wrote: Yeah, probably not. However, I don't like the idea of '(3,4)'::int4range throwing an error, as it currently does, because it seems to require the application to have quite a lot of knowledge of the range semantics to avoid having errors sprung on it. OK, then let's make '(3,4)'::int4range the empty range. (3,3) might be OK as well (for any range type), because at least it's consistent. The one that I find strange is [3,3), but I think that needs to work for the range_adjacent idea to work. Seeing it as useful in the context of range_adjacent might mean that it's useful elsewhere, too, so now I'm leaning toward supporting [3,3) as an empty range. Regards, Jeff Davis -- 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] Singleton range constructors versus functional coercion notation
On Sat, 2011-11-19 at 12:27 -0500, Tom Lane wrote: The singleton range constructors don't work terribly well. ... I don't immediately see a solution that's better than dropping the single-argument range constructors. We could change the name, I suppose, but that seems awkward. I'm hesitant to remove them because the alternative is significantly more verbose: numrange(1.0, 1.0, '[]'); But I don't have any particularly good ideas to save them, either. Regarding the zero-argument (empty) constructors, I'd be fine removing them. They don't seem to cause problems, but the utility is also very minor. Regards, Jeff Davis -- 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 for Add permission check on SELECT INTO
Thanks for your reviewing. The reason of this strange message was bug is the patch. CREATE TABLE public.copy1(a, b) AS SELECT * FROM public.test; ERROR: whole-row update is not implemented When it constructs a fake RangeTblEntry, it marked modifiedCols for attribute 0 to (tupdesc-natts - 1), although it should be 1 to natts. InvalidAttrNumber equals 0, so ExecCheckRTPerms got confused. The attached patch is a revised version. It fixed up this bug, and revised test cases to ensure permission check error shall be raised due to the new table. +SET SESSION AUTHORIZATION selinto_user; +SELECT * INTO TABLE selinto_schema.tmp1 + FROM pg_class WHERE relname like '%a%'; -- Error +ERROR: permission denied for relation tmp1 Thanks, 2011/11/18 Albe Laurenz laurenz.a...@wien.gv.at: The patch is in context diff format and applies cleanly. The functionality is needed because it keeps users from circumventing privilege restrictions, as they can currently do in this case. There is no documentation, which I think is OK since it changes behaviour to work as documented. The patch compiles without warning. It contains a test case, but that test case does not test the feature at all! The expected (and encountered) result is: CREATE SCHEMA selinto_schema; CREATE USER selinto_user; ALTER DEFAULT PRIVILEGES IN SCHEMA selinto_schema REVOKE INSERT ON TABLES FROM selinto_user; SET SESSION AUTHORIZATION selinto_user; SELECT * INTO TABLE selinto_schema.tmp1 FROM onek WHERE onek.unique1 2; -- Error ERROR: permission denied for relation onek RESET SESSION AUTHORIZATION; DROP SCHEMA selinto_schema CASCADE; DROP USER selinto_user; This does not fail because selinto_user lacks INSERT permission on selinto_schema.tmp1 (he doesn't!), but because he lacks SELECT permission on onek (as the error message indicates). Setting default privileges on a schema can never revoke default privileges. The patch works as advertised in that it causes SELECT ... INTO and CREATE TABLE ... AS to fail, however the error message is misleading. Here a (correct) test case: CREATE ROLE laurenz LOGIN; CREATE TABLE public.test(id, val) AS VALUES (1, 'eins'), (2, 'zwei'); GRANT SELECT ON public.test TO laurenz; ALTER DEFAULT PRIVILEGES FOR ROLE laurenz REVOKE INSERT ON TABLES FROM laurenz; SET ROLE laurenz; CREATE TABLE public.copy1(a, b) AS SELECT * FROM public.test; ERROR: whole-row update is not implemented CREATE TABLE public.copy1(a) AS SELECT id FROM public.test; ERROR: whole-row update is not implemented SELECT * INTO public.copy2 FROM public.test; ERROR: whole-row update is not implemented RESET ROLE; ALTER DEFAULT PRIVILEGES FOR ROLE laurenz GRANT ALL ON TABLES TO laurenz; DROP TABLE test; DROP ROLE laurenz; The correct error message would be: ERROR: permission denied for relation copy1 It seems like a wrong code path is taken in ExecCheckRTEPerms, maybe there's something wrong with rte-modifiedCols. I'll mark the patch as Waiting on Author until these problems are addressed. Yours, Laurenz Albe -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.2-add-select-into-checks.v2.patch 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] Singleton range constructors versus functional coercion notation
2011/11/19 Jeff Davis pg...@j-davis.com: On Sat, 2011-11-19 at 12:27 -0500, Tom Lane wrote: The singleton range constructors don't work terribly well. ... I don't immediately see a solution that's better than dropping the single-argument range constructors. We could change the name, I suppose, but that seems awkward. I'm hesitant to remove them because the alternative is significantly more verbose: numrange(1.0, 1.0, '[]'); one parameter range should be confusing. Single parameter range constructors is useless Regards Pavel But I don't have any particularly good ideas to save them, either. Regarding the zero-argument (empty) constructors, I'd be fine removing them. They don't seem to cause problems, but the utility is also very minor. Regards, Jeff Davis -- 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] Singleton range constructors versus functional coercion notation
Jeff Davis pg...@j-davis.com writes: On Sat, 2011-11-19 at 12:27 -0500, Tom Lane wrote: I don't immediately see a solution that's better than dropping the single-argument range constructors. We could change the name, I suppose, but that seems awkward. Yeah, something like int4range_1(42) would work, but it seems rather ugly. I'm hesitant to remove them because the alternative is significantly more verbose: numrange(1.0, 1.0, '[]'); Right. The question is, does the case occur in practice often enough to justify a shorter notation? I'm not sure. One thing I've been thinking for a bit is that for discrete ranges, I find the '[)' canonical form to be surprising/confusing. If we were to fix range_adjacent along the lines suggested by Florian, would it be practical to go over to '[]' as the canonical form? One good thing about that approach is that canonicalization wouldn't have to involve generating values that might overflow. 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] range_adjacent and discrete ranges
Jeff Davis pg...@j-davis.com writes: On Fri, 2011-11-18 at 14:47 -0500, Tom Lane wrote: Yeah, probably not. However, I don't like the idea of '(3,4)'::int4range throwing an error, as it currently does, because it seems to require the application to have quite a lot of knowledge of the range semantics to avoid having errors sprung on it. OK, then let's make '(3,4)'::int4range the empty range. (3,3) might be OK as well (for any range type), because at least it's consistent. The one that I find strange is [3,3), but I think that needs to work for the range_adjacent idea to work. Seeing it as useful in the context of range_adjacent might mean that it's useful elsewhere, too, so now I'm leaning toward supporting [3,3) as an empty range. OK, so the thought is that the only actual error condition would be lower bound value upper bound value? Otherwise, if the range specification represents an empty set, that's fine, we just normalize it to 'empty'. Seems consistent to me. 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] SQLDA fix for ECPG
Hi,
2011-11-17 14:53 keltezéssel, Michael Meskes írta:
On Mon, Nov 14, 2011 at 09:06:30AM +0100, Boszormenyi Zoltan wrote:
Yes, you are right. For timestamp and interval, the safe alignment is int64.
Patch is attached.
Applied, thanks.
Michael
thanks.
Hopefully last turn in this topic. For NUMERIC types, the safe minimum
alignment is a pointer because there are 5 int members followed by
two pointer members in this struct. I got a crash from this with a lucky
query and dataset. The DECIMAL struct is safe because the digits[] array
is embedded there.
After fixing this, I got another one at an innocent looking memcpy():
memcpy((char *) sqlda + offset, num-buf, num-ndigits + 1);
It turned out that when the server sends 0.00, PGTYPESnumeric_from_asc()
constructs a numeric structure with:
ndigits == 0
buf == NULL
digits == NULL.
This makes memcpy() crash and burn. Let's also fix this case.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
--- postgresql-9.1.1/src/interfaces/ecpg/ecpglib/sqlda.c.orig 2011-11-19
21:13:25.699169076 +0100
+++ postgresql-9.1.1/src/interfaces/ecpg/ecpglib/sqlda.c2011-11-19
22:50:13.895653223 +0100
@@ -110,7 +110,7 @@
* int Unfortunately we need to do double work
here to compute
* the size of the space needed for the numeric
structure.
*/
- ecpg_sqlda_align_add_size(offset, sizeof(int),
sizeof(numeric), offset, next_offset);
+ ecpg_sqlda_align_add_size(offset,
sizeof(NumericDigit *), sizeof(numeric), offset, next_offset);
if (!PQgetisnull(res, row, i))
{
char *val = PQgetvalue(res, row,
i);
@@ -119,7 +119,8 @@
num = PGTYPESnumeric_from_asc(val,
NULL);
if (!num)
break;
- ecpg_sqlda_align_add_size(next_offset,
sizeof(int), num-ndigits + 1, offset, next_offset);
+ if (num-ndigits)
+
ecpg_sqlda_align_add_size(next_offset, sizeof(int), num-ndigits + 1, offset,
next_offset);
PGTYPESnumeric_free(num);
}
break;
@@ -323,7 +324,7 @@
set_data = false;
- ecpg_sqlda_align_add_size(offset,
sizeof(int), sizeof(numeric), offset, next_offset);
+ ecpg_sqlda_align_add_size(offset,
sizeof(NumericDigit *), sizeof(numeric), offset, next_offset);
sqlda-sqlvar[i].sqldata = (char *)
sqlda + offset;
sqlda-sqlvar[i].sqllen =
sizeof(numeric);
@@ -343,11 +344,14 @@
memcpy(sqlda-sqlvar[i].sqldata, num,
sizeof(numeric));
- ecpg_sqlda_align_add_size(next_offset,
sizeof(int), num-ndigits + 1, offset, next_offset);
- memcpy((char *) sqlda + offset,
num-buf, num-ndigits + 1);
+ if (num-ndigits)
+ {
+
ecpg_sqlda_align_add_size(next_offset, sizeof(int), num-ndigits + 1, offset,
next_offset);
+ memcpy((char *) sqlda + offset,
num-buf, num-ndigits + 1);
- ((numeric *)
sqlda-sqlvar[i].sqldata)-buf = (NumericDigit *) sqlda + offset;
- ((numeric *)
sqlda-sqlvar[i].sqldata)-digits = (NumericDigit *) sqlda + offset +
(num-digits - num-buf);
+ ((numeric *)
sqlda-sqlvar[i].sqldata)-buf = (NumericDigit *) sqlda + offset;
+ ((numeric *)
sqlda-sqlvar[i].sqldata)-digits = (NumericDigit *) sqlda + offset +
(num-digits - num-buf);
+ }
PGTYPESnumeric_free(num);
@@ -509,7 +513,7 @@
set_data = false;
- ecpg_sqlda_align_add_size(offset,
sizeof(int), sizeof(numeric), offset, next_offset);
+ ecpg_sqlda_align_add_size(offset,
sizeof(NumericDigit *), sizeof(numeric), offset, next_offset);
Re: [HACKERS] range_adjacent and discrete ranges
On Nov19, 2011, at 22:03 , Tom Lane wrote: Jeff Davis pg...@j-davis.com writes: On Fri, 2011-11-18 at 14:47 -0500, Tom Lane wrote: Yeah, probably not. However, I don't like the idea of '(3,4)'::int4range throwing an error, as it currently does, because it seems to require the application to have quite a lot of knowledge of the range semantics to avoid having errors sprung on it. OK, then let's make '(3,4)'::int4range the empty range. (3,3) might be OK as well (for any range type), because at least it's consistent. The one that I find strange is [3,3), but I think that needs to work for the range_adjacent idea to work. Seeing it as useful in the context of range_adjacent might mean that it's useful elsewhere, too, so now I'm leaning toward supporting [3,3) as an empty range. OK, so the thought is that the only actual error condition would be lower bound value upper bound value? Otherwise, if the range specification represents an empty set, that's fine, we just normalize it to 'empty'. Seems consistent to me. +1. Making the error condition independent from the boundary type makes it easy for callers to avoid the error conditions. And it should still catch mistakes that stem from swapping the lower and upper bound. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Singleton range constructors versus functional coercion notation
On Nov19, 2011, at 21:57 , Tom Lane wrote: One thing I've been thinking for a bit is that for discrete ranges, I find the '[)' canonical form to be surprising/confusing. If we were to fix range_adjacent along the lines suggested by Florian, would it be practical to go over to '[]' as the canonical form? One good thing about that approach is that canonicalization wouldn't have to involve generating values that might overflow. I have argued against that in the past, mostly because 1) It's better to be consistent 2) While it seems intuitive for integer ranges to use [] and float ranges to use [), things are far less clear once you take other base types into account. For example, we'd end up making ranges over DATE use canonicalize to [], but ranges over TIMESTAMP to [). Which, at least IMHO, is quite far from intuitive. And if we start fixing these issues by making exception from the discrete - [], continuous - [) rule, we'll end up with essentially arbitrary behaviour pretty quickly. At which point (1) kicks it ;-) And then there also ranges over NUMERIC, which can be both discrete and continuous, depending on the typmod. Which I think is a good reason in itself to make as little behaviour as possible depend on the continuity or non-continuity of range types. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inlining comparators as a performance optimisation
On 19 November 2011 02:55, Robert Haas robertmh...@gmail.com wrote: Maybe we should look at trying to isolate that a bit better. Indeed. Fortunately, GCC has options to disable each optimisation. Here's potentially relevant flags that we're already implicitly using at -02: -finline-small-functions -- this is the one that inlined functions without an inline keyword -findirect-inlining -finline-functions-called-once -fearly-inlining -fipa-sra (Perform interprocedural scalar replacement of aggregates, removal of unused parameters and replacement of parameters passed by reference by parameters passed by value). In an effort to better isolate the effects of inlining, I tried this: ./configure CFLAGS=-fno-inline -fno-inline-small-functions (I could have disabled more -02 optimisations, but this proved sufficient to make my point) Unsurprisingly, this makes things slower than regular -02. With the patch, the same query (once again, using explain analyze) with the same fast path quicksort stabilises around 92ms +/- 5ms (recall that the original figure was ~82ms). Our gains evaporate, and then some. Take away the additional CLAGS, and we're predictably back to big gains, with the query taking ~52ms just as before. What happens to this query when we build an unmodified postgres with these same CFLAGS? Well, we see the query take ~116ms after a few runs. It seems that the impedance mismatch matters, but inlining and other optimisations look to be at least as important. This isn't surprising to me, given what I was able to do with the isolated test. Maybe I should have tried it with additional disabling of optimisations named above, but that would have perhaps made things less clear. I'd probably have been better off directly measuring qsort speed and only passing those flags when compiling tuplesort.c (maybe impedance mismatch issues would have proven to have been even less relevant), but I wanted to do something that could be easily recreated, plus it's late. It strikes me that we could probably create an API that would support doing either of these things depending on the wishes of the underlying datatype. For example, imagine that we're sorting with (int4, int4). We associate a PGPROC-callable function with that operator that returns internal, really a pointer to a struct. The first element of the struct is a pointer to a comparison function that qsort() (or a tape sort) can invoke without a trampoline; the second is a wholesale replacement for qsort(); either or both can be NULL. Given that, it seems to me that we could experiment with this pretty easily, and if it turns out that only one of them is worth doing, it's easy to drop one element out of the structure. Or do you have another plan for how to do this? I haven't given it much thought. Let me get back to you on that next week. Have you done any benchmarks where this saves seconds or minutes, rather than milliseconds? That would certainly make it more exciting, at least to me. Right. Well, I thought I'd use pgbench to generate a large table in a re-creatable way. That is: pgbench -i -s 60 This puts pgbench_accounts at 769MB. Then, having increased work_mem to 1GB (enough to qsort) and maintenance_work_mem to 756mb, I decided to test this query with the patch: explain analyze select * from pgbench_accounts order BY abalance; This stabilised at ~3450ms, through repeatedly being executed. How does this compare to unpatched postgres? Well, it stabilised at about ~3780ms for the same query. This patch is obviously less of a win as the number of tuples to sort goes up. That's probably partly explained by the cost of everything else going up at a greater rate than the number of comparisons. I suspect that if we measure qsort in isolation, we'll see better results, so we may still see a good win on index creation time as a result of this work. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inlining comparators as a performance optimisation
On 20 November 2011 03:29, Peter Geoghegan pe...@2ndquadrant.com wrote: ./configure CFLAGS=-fno-inline -fno-inline-small-functions (I could have disabled more -02 optimisations, but this proved sufficient to make my point) I'll isolate this further to tuplesort.c soon, which ought to be a lot more useful. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
