Re: [HACKERS] Command Triggers
Hi Peter, On Sunday, December 04, 2011 08:01:34 PM Andres Freund wrote: > On Sunday, December 04, 2011 05:34:44 PM Tom Lane wrote: > > 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. > Building a CreateStmt seems to work well enough so far. > The only problem with that approach so far that I found is that: > CREATE TABLE collate_test2 > ( >a int, > b text COLLATE "POSIX" > ); > > CREATE TABLE collate_test1 > ( >a int, > b text COLLATE "C" NOT NULL > ); > > CREATE TABLE test_u AS SELECT a, b FROM collate_test1 UNION ALL SELECT a, > b FROM collate_test2; -- fail > > failed with: > ERROR: no collation was derived for column "b" with collatable type text > HINT: Use the COLLATE clause to set the collation explicitly. > "works" now. Could you explain why the above should fail? After all the UNION is valid outside the CREATE TABLE and you can even sort on b. That tidbit is he only thing I couldn't quickly solve since the last submission... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_stat_statements with query tree based normalization
On 10 December 2011 13:56, Greg Smith wrote: > I heard about some bitrot creeping in here too, but it seems gone now; I had > no problem merging Peter's development branch against master. I've attached > a newer patch of the main code, which fixes most of the earlier issues there > were disclaimers about. I'm aware of some further bugs in the patch that Greg posted regarding the synchronisation of executor and planner plugins, so please bear with me while I squash them. -- 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] static or dynamic libpgport
Andrew Dunstan writes: > On 12/09/2011 06:27 PM, Bruce Momjian wrote: >> I am not against shipping a dynamic libpgport, but I will just point out >> that this was never intended or anticipated. Are there any symbols in >> there that might conflict with other software? > Possibly. Below is a list of symbols from a recent build. This doesn't seem like much of an issue to me, since anything wanting to link against libpgport would be designed to work with whatever it provides, no? > The other > thing is we'd need to turn on flags that make the object suitable for a > dynamic library (e.g. -fpic). Right now, libpq laboriously rebuilds all the .o files it needs from src/port/ so as to get them with -fpic. It would be nice if we could clean that up while we're doing this. It might be all right to always build the client-side version of libpgport with -fpic, though I'd be sad if that leaked into the server-side build. 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 : Allow toast tables to be moved to a different tablespace
On Mon, Nov 28, 2011 at 1:32 PM, Julien Tachoires wrote: > Hi Jaime, > > Please find a new version. > cool >> 2) after CLUSTER the index of the toast table gets moved to the same >> tablespace as the main table > there is still a variant of this one, i created 3 tablespaces (datos_tblspc): """ create table t1 ( i serial primary key, t text ) tablespace datos_tblspc; ALTER TABLE t1 SET TOAST TABLESPACE pg_default; CLUSTER t1 USING t1_pkey; """ >> >> now, if we are now supporting this variants >> ALTER TABLE SET TABLE TABLESPACE >> ALTER TABLE SET TOAST TABLESPACE >> >> why not also support ALTER TABLE SET INDEX TABLESPACE which should >> have the same behaviour as ALTER INDEX SET TABLESPACE... just an idea, >> and of course not necessary for this patch >> any opinion about this? maybe i can make a patch for that if there is consensus that it could be good for symettry -- 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] [PATCH] Caching for stable expressions with constant arguments v3
On Sat, Dec 10, 2011 at 16:50, Greg Smith wrote: > Or should we wait for a new update based on the > feedback that Heikki and Tom have already provided first, perhaps one that > proposes fixes for these two test cases? Yes, I will post and updated and rebased version tomorrow. > Now that you're settling into the git workflow, you might consider > publishing updates to a site like Github in the future too. It's in the 'cache' branch on my Github: https://github.com/intgr/postgres/commits/cache (I linked to it in the v3 posting, but forgot to mention it later) Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review of VS 2010 support patches
On 12/10/2011 12:58 PM, Brar Piening wrote: I'm currently trying to find some time window in my before chrismas schedule but it seems like I can't guarantee anything. Anyhow I'll try to make it happen within "this year". That's fair, and Andrew or something else may get an itch to just plow forward and do it themselves. I'm going to mark this one returned with feedback for now. So long as we get an update from you before the January 15th CommitFest, this should still be feasible to slip into 9.2. -- 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
[HACKERS] CommitFest 2011-11 Update
All of the information on the CommitFest app is as accurate as I could make it now, I made a pass over every open thread there to look for major changes that hadn't gotten message ID archive links. Since the official start 13 patches have been committed and 6 bounced out. Reminder notes have gone out to most of the people who there's something waiting for, mostly off-list nagging, and several people have already gotten back to say they're planning to hash out open issues over this weekend. Of the 34 patches still waiting for review or their author, there's a couple of the usual suspects responsible for a good chunk of them. Greg Smith: 10 patches that troublemaker is meddling with in some form. I'm clearly not going anywhere this upcoming week, as I've had "panic over closing of CommitFest in the middle of December" on my calendar for a while. General plan of attack is: -Try and break the deadlock over what to do with pg_last_xact_insert_timestamp (done with that for now) -Update "Configuration include directory" to fix all of Noah's suggestions -Make a similar pass over "includeifexists in configuration file", which is a little cut and paste from the first one and may have some of the same errors. -Revisit the "unite recovery.conf and postgresql.conf" from a new perspective that presumes those two features are coming. I suspect we can implement some of the "what I'd like is this" requests people wanted here with these features, to chop away enough edge cases to make this more tractable. -Mixed with the bigger above bits, during smaller chunks of time help rework "pg_terminate_backend and pg_cancel_backend by not administrator user", "Measuring relation free space", and "Separate pg_stat_activity into current_query into state and query columns" features to commit quality. -Resume slogging through the controversy around "Core extensions relocation" and see if that goes anywhere. -Join in on any benchmarking help I can provide for some of Robert's patches, starting with "Make pgBufferUsage track dirty buffers" -Continued work with Peter G on pg_stat_statements normalization. I consider a major feature in the "Performance Release" theme 9.2 has taken on. There's a normal sized plate lined up for Robert since he's been keeping up better; in no particular order: -More review on the always a new surprise "Fix Leaky Views Problem, again" -Extra fun with "Tuplesort comparison overhead reduction" -His own "avoid taking two snapshots per query" and "FlexLocks" improvements And then Dimitri is on: -His "Command Triggers" -Review on "Prep object creation hooks" -Another likely pass over Robert's "avoid taking two snapshots per query" Other people in similarly loaded and overlapping lists with the above include Fujii Masao and KaiGai Kohei. So about half of the open items are from contributors who have a track record of just coming back for more every time. It's not like we're going to wonder off based on whether our submission goes in now or we have to keep chugging along. I think most of the above is achievable in 9.2. What I'm happy about is that I'm not seeing any giant controversial things here, the sort that tend to hit the last CF and then push its boundary back too. Last year at this time, patches on the table at this point were things like Extensions, Sync Rep, Per-column collation, and KNN-gist. Those had the bad mix of "we want this feature for the relase" and "this is hard". That pushed some of them into early March before they got committed. I think only Dimitri's Command Triggers has that sort of potential this time. And I've been talking with him enough about the plan there to feel it chunks into useful pieces pretty well if the target feature set has to scale back. We'll have to see if anyone tries to sneak one of the more complicated things into the final CF. -- 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] %TYPE and array declaration patch review
On Sat, 10 Dec 2011 10:22:54 -0500 Greg Smith wrote: > Tom's concerns about the grammar rewrite and way parsing is handled > here seem the worst blockers for committing this, and I can't imagine > how those could be resolved before this CommitFest is over. I'm > going to mark this one as returned with that and Pavel's feedback. > Perhaps Wojciech or someone else will come up with a clever way to > address this, one that has less impact on existing code. Adding more > parser complexity than absolutely necessary is a much bigger problem > than the regression tests being too long. My first attempt to fix the issue was very short & simple, I slightly modified existing C code to handle additional suffixes, see http://archives.postgresql.org/pgsql-hackers/2011-08/msg00998.php Pavel reviewed this and pointed that would be great if we were fix related issues. Thus, the "second" patch try to resolve: 1. handling array indices; 2. copying types; 3. handling %TYPE/%ROWTYPE in procedure definition. Ad 1 - IMHO first attempt is good enough :) (except coding style) Ad 2 - this is broken now, just few cases are handled correctly. My code handled more cases manually, but now I see this have to be solved with kind of generic pattern-matching procedure (maybe with backtracking) - I hope shorter, cleaner and easier to understand and maintain. Tom Lane wrote it would be good if resolving types were handled in core, but IMO only PL/pgsql code will use this procedure. Ad 3 - minor issue, could be fixed anytime So, my proposition is to drop current approach and back to the first proposition. Copying types have to be considered as separate issue. regards Wojciech Muła -- 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
Andrew Dunstan wrote: In the absence of reaction to this I've marked the patch as "waiting on author", but if/when I have time I'll work on rearranging things as above. Sorry for my non-reaction. I'm currently trying to find some time window in my before chrismas schedule but it seems like I can't guarantee anything. Anyhow I'll try to make it happen within "this year". Regards, Brar -- 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] [v9.2] Fix Leaky View Problem
2011/12/9 Robert Haas : > On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai wrote: >> My first impression remind me an idea that I proposed before, even >> though it got negative response due to user visible changes. >> It requires superuser privilege to create new operators, since we >> assume superuser does not set up harmful configuration. > > I don't think that's acceptable from a usability point of view; this > feature is important, but not important enough to go start ripping out > other features that people are already using, like non-superuser > operators. I'm also pretty skeptical that it would fix the problem, > because the superuser might fail to realize that creating an operator > was going to create this type of security exposure. After all, you > and I also failed to realize that, so it's obviously a fairly subtle > problem. > OK, I agree with your opinion. It may stand on a fiction story that superuser understand all effects and risk of his operations. If this assumption get broken, system's security is also broken. > I feel like there must be some logic in the planner somewhere that is > "looking through" the subquery RTE and figuring out that safe_foo.a is > really the same variable as foo.a, and which therefore feels entitled > to apply !!'s selectivity estimator to foo.a's statistics. If that's > the case, it might be possible to handicap that logic so that when the > security_barrier flag is set, it doesn't do that, and instead treats > safe_foo.a as a black box. That would, obviously, degrade the quality > of complex plans involving security views, but I think we should focus > on getting something that meets our security goals first and then try > to improve performance later. > I tried to investigate the code around size-estimation, and it seems to me here is two candidates to put this type of checks. The one is examine_simple_variable() that is used to pull a datum from statistic information, but it locates on the code path restriction estimator of operators; so user controlable, although it requires least code changes just after if (rte->rtekind == RTE_SUBQUERY). The other is clause_selectivity(). Its code path is not user controlable, so we can apply necessary checks to prevent qualifier that reference variable come from sub-query with security-barrier. In my sense, clause_selectivity() is better place to apply this type of checks. But, on the other hand, it provides get_relation_stats_hook to allow extensions to control references to statistic data. So, I wonder whether the PG core assumes this routine covers all the code path here? In addition, I also consider the case when we add a functionality that forcibly adds restriction on WHERE clause of regular tables in the future version, like: SELECT * FROM t WHERE a > 0; ==> SELECT * FROM t WHERE sepgsql_policy(selinux_label) AND a > 0; Probably, same solution will be available to avoid unintentional references to pg_statistic; as long as security_barrier is set on rte of regular tables. Thanks, -- KaiGai Kohei -- 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 line number reporting from stdin
On fre, 2011-12-09 at 13:44 -0500, Tom Lane wrote: > Peter Eisentraut writes: > > The problem is, this breaks the regression tests, because first the > > actual output changes, and second the line numbers get included, which > > will create a mess every time you edit a test. Not sure whether we can > > work around that. Ideas? > > Ugh, that's pretty nearly a deal-breaker. Would it be sane to have a > command line switch the regression test driver could specify to prevent > inclusion of this info? Perhaps. I was thinking we could use an environment variable when running under pg_regress. This could also help, e.g., ecpg. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_cancel_backend by non-superuser
On 10/02/2011 05:32 PM, Tom Lane wrote: I'm with Noah on this. If allowing same-user cancels is enough to solve 95% or 99% of the real-world use cases, let's just do that. And we're back full circle. This is basically where Josh Kuperschmidt started in early 2010: http://archives.postgresql.org/message-id/4ec1cf761002051455i6e702999y7cf4699b4eb48...@mail.gmail.com Then Torello's patch initially more ambitious patch got pruned down to the same sort of feature set. Next, the day after the November CommitFest started (so it got kind of lost), Edward Muller took a shot at coding exactly this too, which he tells me happened without even knowing the other two were already floating around: http://archives.postgresql.org/message-id/cabm0hdx+xuc3dsncnb2z2mertw3crcc5kjmvh6kwhb7jnix...@mail.gmail.com The picture of what people really want here is pretty clear now, after different people wanted same-user cancels (or more) badly enough to submit a patch adding it, in three cases now. -- 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] static or dynamic libpgport
On 12/09/2011 06:27 PM, Bruce Momjian wrote: In the Fedora world, a static lib would go in postgresql-devel, but a dynamic lib would go in postgresql-libs, which is also where libpq is shipped. I am not against shipping a dynamic libpgport, but I will just point out that this was never intended or anticipated. Are there any symbols in there that might conflict with other software? Possibly. Below is a list of symbols from a recent build. The other thing is we'd need to turn on flags that make the object suitable for a dynamic library (e.g. -fpic). I'm not sure if that has any significant impact - probably not or else people would avoid using them far more. I think we should deal with this. Just Peter's and Steve's responses upthread indicate a demand, and I came into this because a customer encountered enormous difficulty in doing an out of tree build of a well known piece of support software, so this is clearly a pain point. If we're not going to do it, we should probably think about adjusting people's expectations. [andrew@diego port]$ nm libpgport.a | grep ' T ' T strlcat T strlcpy T getpeereid T pg_get_encoding_from_locale T pgfnames 01a0 T pgfnames_cleanup 01e0 T rmtree 0240 T find_my_exec 05f0 T find_other_exec 0500 T pclose_check 0760 T set_pglocale_pgservice T inet_net_ntop 0030 T pg_set_block T pg_set_noblock 0280 T canonicalize_path 0110 T first_dir_separator 0140 T first_path_var_separator 0800 T get_doc_path 0720 T get_etc_path 0860 T get_home_path 0820 T get_html_path 0740 T get_include_path 0780 T get_includeserver_path 07a0 T get_lib_path 07e0 T get_locale_path 0840 T get_man_path 08e0 T get_parent_directory 0760 T get_pkginclude_path 07c0 T get_pkglib_path 0690 T get_progname 0700 T get_share_path 01b0 T join_path_components 0170 T last_dir_separator 01a0 T make_native_path 0560 T path_contains_parent_reference 0630 T path_is_prefix_of_path 0610 T path_is_relative_and_below_cwd T pg_check_dir T pg_mkdir_p T pg_usleep 0330 T pg_ascii_tolower 0310 T pg_ascii_toupper T pg_strcasecmp 0100 T pg_strncasecmp 0290 T pg_tolower 0210 T pg_toupper 00d0 T pg_qsort 00f0 T qsort_arg T simple_prompt 0010 T pqGetpwuid T pqStrerror 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] plpython SPI cursors
On mån, 2011-12-05 at 13:12 -0500, Bruce Momjian wrote: > Jan Urbański wrote: > > On 05/12/11 18:58, Peter Eisentraut wrote: > > > On ons, 2011-11-23 at 19:58 +0100, Jan Urba?ski wrote: > > >> On 20/11/11 19:14, Steve Singer wrote: > > >> Responding now to all questions and attaching a revised patch based on > > >> your comments. > > > > > > Committed > > > > > > Please refresh the other patch. > > > > Great, thanks! > > > > I'll try to send an updated version of the other patch this evening. > > I assume this is _not_ related to this TODO item: > > Add a DB-API compliant interface on top of the SPI interface No, but this is: http://petereisentraut.blogspot.com/2011/11/plpydbapi-db-api-for-plpython.html -- 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] %TYPE and array declaration patch review
2011/12/10 Greg Smith : > On 11/30/2011 10:42 AM, Pavel Stehule wrote: >> >> Regress tests are really large - it is question if about 900 lines is >> necessary - should be more compact >> > > > Can't recall the last time I heard a complaint about having too many > regression tests for new code. We've got some bit rot, code convention > suggestions, test scope concerns, and some fundamental design questions > hanging over this one. I don't think anyone expected this TODO item was > going to turn into a 2626 line patch, but discovering a bunch of unexpected > complexity underneath one of those is a regular event. Stuff that's > straighforward to implement tends to just get done instead of added there. > this is just my opinion, it's not blocker (about regress tests length) but maybe this patch should be divided to some smaller parts Regards Pavel > Tom's concerns about the grammar rewrite and way parsing is handled here > seem the worst blockers for committing this, and I can't imagine how those > could be resolved before this CommitFest is over. I'm going to mark this > one as returned with that and Pavel's feedback. Perhaps Wojciech or someone > else will come up with a clever way to address this, one that has less > impact on existing code. Adding more parser complexity than absolutely > necessary is a much bigger problem than the regression tests being too long. > > -- > Greg Smith 2ndQuadrant US g...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: SP-GiST, Space-Partitioned GiST
I wrote: > ... the leaf tuple datatype is hard-wired to be > the same as the indexed column's type. Why is that? It seems to me > to be both confusing and restrictive. For instance, if you'd designed > the suffix tree opclass just a little differently, it would be wanting > to store "char" not text in leaf nodes. Shouldn't we change this to > allow the leaf data type to be specified explicitly? After another day's worth of hacking, I now understand the reason for the above: when an index is less than a page and an incoming value would still fit on the root page, the incoming value is simply dumped into a leaf tuple without ever calling any opclass-specific function at all. To allow the leaf datatype to be different from the indexed column, we'd need at least one more opclass support function, and it's not clear that the potential gain is worth any extra complexity. However, I now have another question: what is the point of the allTheSame mechanism? It seems to add quite a great deal of complexity, without saving much of any space. At least for the node key types used so far, the null bitmap that's added to the node tuples eats just as much space as storing a normal key would. We could probably avoid that by using custom tuple construction code instead of index_form_tuple, but on the whole I think it'd be better to remove the concept. For one thing, it's giving me fits while attempting to fix the limitation on storing long indexed values. There's no reason why a suffix tree representation shouldn't work for long strings, but you have to be willing to cap the length of any given inner tuple's prefix to something that will fit on a page --- and that breaks the badly-underdocumented allTheSame logic in spgtextproc.c. You can't choose to just drop the node key (i.e., the next byte in the original string) unless there isn't any because you reached the end of the string. In general it's not clear to me why it's sensible to drop a node key ever. I'm also still wondering what your thoughts are on storing null values versus full-index-scan capability. I'm leaning towards getting rid of the dead code, but if you have an idea how to remove the limitation, maybe we should do that instead. 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] %TYPE and array declaration patch review
On 11/30/2011 10:42 AM, Pavel Stehule wrote: Regress tests are really large - it is question if about 900 lines is necessary - should be more compact Can't recall the last time I heard a complaint about having too many regression tests for new code. We've got some bit rot, code convention suggestions, test scope concerns, and some fundamental design questions hanging over this one. I don't think anyone expected this TODO item was going to turn into a 2626 line patch, but discovering a bunch of unexpected complexity underneath one of those is a regular event. Stuff that's straighforward to implement tends to just get done instead of added there. Tom's concerns about the grammar rewrite and way parsing is handled here seem the worst blockers for committing this, and I can't imagine how those could be resolved before this CommitFest is over. I'm going to mark this one as returned with that and Pavel's feedback. Perhaps Wojciech or someone else will come up with a clever way to address this, one that has less impact on existing code. Adding more parser complexity than absolutely necessary is a much bigger problem than the regression tests being too long. -- 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] patch for type privileges
On 2011-12-07 19:59, Peter Eisentraut wrote: Two excellent finds. Here is an updated patch with fixes. Thanks.. I'm sorry I cannot yet provide a complete review, but since the end of the commitfest is near, I decided to mail them anyway instead of everything on dec 15. * ExecGrant_type() prevents 'grant usage on domain' on a type, but the converse is possible. postgres=# create domain myint as int2; CREATE DOMAIN postgres=# grant usage on type myint to public; GRANT * Cannot restrict access to array types. After revoking usage from the element type, the error is perhaps a bit misleading. (smallint[] vs smallint) postgres=> create table a (a int2[]); ERROR: permission denied for type smallint[] * The patch adds the following text explaining the USAGE privilege on types. For types and domains, this privilege allow the use of the type or domain in the definition of tables, functions, and other schema objects. Since other paragraphs in USAGE use the word 'creation' instead of 'definition', I believe here the word 'creation' should be used too. IMHO it would also be good to describe what the USAGE privilege is not, but might be expected since it is such a generic term. USAGE on type: use of the type while creating new dependencies to the type, not usage in the sense of instantiating values of the type. If there are existing dependencies, revoking usage privileges will not return any warning and the dependencies still exist. Also other kinds of exceptions could be noted, such as the exception for array types and casts. The example you gave in the top mail about why restricting access to types can be useful, such as preventing that owners are prevented changing their types because others have 'blocked' them by their usage, is something that could also help readers of the documentation understand why privileges on types are useful for them (or not). * The information schema view 'attributes' has this additional condition: AND (pg_has_role(t.typowner, 'USAGE') OR has_type_privilege(t.oid, 'USAGE')); What happens is that attributes in a composite type are shown, or not, if the current user has USAGE rights. The strange thing here, is that the attribute in the type being show or not, doesn't match being able to use it (in the creation of e.g. a table). Maybe that is not intended, but I would expect it matching: postgres=# create user c; CREATE ROLE postgres=# create type t as (a int2); CREATE TYPE postgres=# \c - c You are now connected to database "postgres" as user "c". postgres=> select udt_name,attribute_name from information_schema.attributes; udt_name | attribute_name --+ t| a (1 row) postgres=> \c - You are now connected to database "postgres" as user "c". postgres=> \c - postgres You are now connected to database "postgres" as user "postgres". postgres=# revoke usage on type int2 from public; REVOKE postgres=# \c - c You are now connected to database "postgres" as user "c". postgres=> select udt_name,attribute_name from information_schema.attributes; udt_name | attribute_name --+ (0 rows) postgres=> create table m (a t); CREATE TABLE postgres=> insert into m values (ROW(10)); INSERT 0 1 postgres=> Conversely: postgres=# grant usage on type int2 to public; GRANT postgres=# revoke usage on type t from public; REVOKE postgres=# \c - c You are now connected to database "postgres" as user "c". postgres=> select udt_name,attribute_name from information_schema.attributes; udt_name | attribute_name --+ t| a (1 row) postgres=> create table m2 (a t); ERROR: permission denied for type t postgres=> regards, Yeb Havinga -- 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 12/07/2011 04:58 PM, Marti Raudsepp wrote: PS: I forgot to mention that 2 test cases covering the two above query types are deliberately left failing in the v4-wip patch. It's not completely clear what happens next with this. Are you hoping code churn here has calmed down enough for Jaime or someone else to try and look at this more already? Or should we wait for a new update based on the feedback that Heikki and Tom have already provided first, perhaps one that proposes fixes for these two test cases? One general suggestion about the fight with upstream changes you've run into here. Now that you're settling into the git workflow, you might consider publishing updates to a site like Github in the future too. That lets testing of the code at the point you wrote it always possible. Given just the patch, reviewers normally must reconcile any bit rot before they can even compile your code to try it. That gets increasingly sketchy the longer your patch waits before the next CommitFest considers it. With a published git working tree, reviewers can pull that for some hands-on testing whenever, even if a merge wouldn't actually work out at that point. You just need to be careful not to push an update that isn't self-consistent to the world. I normally attach the best formatted patch I can and publish to Github. Then reviewers can use whichever they find easier, and always have the option of postponing a look at merge issues if they just want to play with the program. -- 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: GiST for range types (was Re: [HACKERS] Range Types - typo + NULL string constructor)
On 12/02/2011 06:48 AM, Alexander Korotkov wrote: Rebased with head. Could you comment a little more on what changed? There were a couple of areas Tom commented on: -General code fixes -"pull out and apply the changes related to the RANGE_CONTAIN_EMPTY flag, and also remove the <> opclass entry" -Subdiff issues The third one sounded hard to deal with, so presumably nothing there. I'm not sure if your updated rebase addresses either of those first two yet or not, or if it was just fixing bitrot from upstream code changes. -- 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] [REVIEW] pg_last_xact_insert_timestamp
On 10/02/2011 07:10 AM, Robert Haas wrote: Your proposals involve sending additional information from the master to the slave, but the slave already knows both its WAL position and the timestamp of the transaction it has most recently replayed, because the startup process on the slave tracks that information and publishes it in shared memory. On the master, however, only the WAL position is centrally tracked; the transaction timestamps are not. This seems to be the question that was never really answered well enough to satisfy anyone, so let's rewind to here for a bit. I wasn't following this closely until now, so I just did my own review from scratch against the latest patch. I found a few issues, and I don't think all of them have been vented here yet: -It adds overhead at every commit, even for people who aren't using it. Probably not enough to matter, but it's yet another thing going through the often maligned as too heavy pgstat system, often. -In order to measure lag this way, you need access to both the master and the standby. Yuck, dblink or application code doing timestamp math, either idea makes me shudder. It would be nice to answer "how many seconds of lag do have?" directly from the standby. Ideally I would like both the master and standby to know those numbers. -After the server is restarted, you get a hole in the monitoring data until the first transaction is committed or aborted. The way the existing pg_last_xact_replay_timestamp side of this computation goes NULL for some unpredictable period after restart is going to drive monitoring systems batty. Building this new facility similarly will force everyone who writes a lag monitor to special case for that condition on both sides. Sure, that's less user hostile than the status quo, but it isn't going to help PostgreSQL's battered reputation in the area of monitoring either. -The transaction ID advancing is not a very good proxy for real-world lag. You can have a very large amount of writing in between commits. The existing lag monitoring possibilities focus on WAL position instead, which is better correlated with the sort of activity that causes lag. Making one measurement of lag WAL position based (the existing ones) and another based on transaction advance (this proposal) is bound to raise some "which of these is the correct lag?" questions, when the two diverge. Large data loading operations come to mind as a not unusual at all situation where this would happen. I'm normally a fan of building the simplest thing that will do something useful, and this patch succeeds there. But the best data to collect needs to have a timestamp that keeps moving forward in a way that correlates reasonably well to the system WAL load. Using the transaction ID doesn't do that. Simon did some hand waving around sending a timestamp every checkpoint. That would allow the standby to compute its own lag, limit overhead to something much lighter than per-transaction, and better track write volume. There could still be a bigger than normal discontinuity after server restart, if the server was down a while, but at least there wouldn't ever be a point where the value was returned by the master but was NULL. But as Simon mentioned in passing, it will bloat the WAL streams for everyone. Here's the as yet uncoded mini-proposal that seems to have slid by uncommented upon: "We can send regular special messages from WALSender to WALReceiver that do not form part of the WAL stream, so we don't bulk up WAL archives. (i.e. don't use "w" messages)." Here's my understanding of how this would work. Each time a commit/abort record appears in the WAL, that updates XLogCtl with the associated timestamp. If WALReceiver received regular updates containing the master's clock timestamp and stored them similarly--either via regular streaming or the heartbeat--it could compute lag with the same resolution as this patch aims to do, for the price of two spinlocks: just subtract the two timestamps. No overhead on the master, and lag can be directly computed and queried from each standby. If you want to feed that data back to the master so it can appear in pg_stat_replication in both places, send it periodically via the same channel sync rep and standby feedback use. I believe that will be cheap in many cases, since it can piggyback on messages that will still be quite small relative to minimum packet size on most networks. (Exception for compressed/encrypted networks where packets aren't discrete in this way doesn't seem that relevant, presuming that if you're doing one of those I would think this overhead is the least of your worries) Now, there's still one problem here. This doesn't address the "lots of write volume but no commit" problem any better than the proposed patch does. Maybe there's some fancy way to inject it along with the received WAL on the standby, I'm out of brai
Re: [HACKERS] [PATCH] Support for foreign keys with arrays
Hi Noah, thanks for your feedback. Il 20/11/11 14:05, Noah Misch ha scritto: What about making ON UPDATE CASCADE an error? That way, we can say that ARRAY always applies to array elements, and plain always applies to entire rows. SET DEFAULT should now be fine to allow. It's ARRAY SET DEFAULT, in your new terminology, that wouldn't make sense. I have tried to gather your ideas with Gianni's and come to a compromise, which I hope you can both agree on. The reason why I would be inclined to leave CASCADE act on rows (rather than array elements as Gianni suggests) is for backward compatibility (people that are already using referential integrity based on array values). For the same reason, I am not sure whether we should raise an error on update, but will leave this for later. So, here is a summary: --- - - | ON| ON| Action | DELETE | UPDATE | --- - - CASCADE| Row | Error | SET NULL | Row | Row | SET DEFAULT| Row | Row | ARRAY CASCADE | Element | Element | ARRAY SET NULL | Element | Element | NO ACTION |-|-| RESTRICT |-|-| --- - - If that's fine with you guys, Marco and I will refactor the development based on these assumptions. Thanks, Gabriele -- Gabriele Bartolini - 2ndQuadrant Italia PostgreSQL Training, Services and Support gabriele.bartol...@2ndquadrant.it | www.2ndQuadrant.it -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers