Re: [HACKERS] Support UPDATE table SET(*)=...
Tom == Tom Lane t...@sss.pgh.pa.us writes: Tom I spent a fair amount of time cleaning this patch up to get it Tom into committable shape, but as I was working on the documentation Tom I started to lose enthusiasm for it, because I was having a hard Tom time coming up with compelling examples. The originally proposed Tom motivation was It solves the problem of doing UPDATE from a record variable of the same type as the table e.g. update foo set (*) = (select foorec.*) where ...; There are a number of motivating examples for this (which have nothing to do with rules; I doubt anyone cares much about that). The fundamental point is that currently, given a table foo and some column or variable of foo's rowtype, you can do this: insert into foo select foorec.* [from ...] but there is no comparable way to do an update without naming every column explicitly, the closest being update foo set (a,b,...) = (foorec.a, foorec.b, ...) where ... One example that comes up occasionally (and that I've had to do myself more than once) is this: given a table foo and another with identical schema reference_foo, apply appropriate inserts, updates and deletes to table foo to make the content of the two tables identical. This can be done these days with wCTEs: with t_diff as (select o.id as o_id, n.id as n_id, o, n from foo o full outer join reference_foo n on (o.id=n.id) where (o.*) is distinct from (n.*)), ins as (insert into foo select (n).* from t_diff where o_id is null), del as (delete from foo where id in (select o_id from t_diff where n_id is null)), upd as (update foo set (col1,col2,...) = ((n).col1,(n).col2,...) -- XXX from t_diff where foo.id = n_id and o_id = n_id) select count(*) filter (where o_id is null) as num_ins, count(*) filter (where o_id = n_id) as num_upd, count(*) filter (where n_id is null) as num_del from t_diff; (This would be preferred over simply replacing the table content if the table is large and the changes few, you want to audit the changes, you need to avoid interfering with concurrent selects, etc.) The update part of that would be much improved by simply being able to say update all columns of foo with values from (n). The exact syntax isn't a big deal - though since SET (cols...) = ... is in the spec, it seems reasonable to at least keep some kind of consistency with it. Other examples arise from things one might want to do in plpgsql; for example to update a record from an hstore or json value, one can use [json_]populate_record to construct a record variable, but then it's back to naming all the columns in order to actually perform the update statement. [My connection with this patch is only that I suggested it to Atri as a possible project for him to do, because I wanted the feature and knew others did also, and helped explain how the existing MultiAssign worked and some of the criticism. I did not contribute any code.] -- Andrew (irc:RhodiumToad) -- 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] Support UPDATE table SET(*)=...
Tom == Tom Lane t...@sss.pgh.pa.us writes: Now this I think is wrong; I think it's just as robust against schema changes as using the composite value directly would be. Consider the case where foo and reference_foo match with the exception of dropped columns; the code as it is should just work, while a variant that used the composite values would have to explicitly deal with that. Tom AFAICS you've got that backwards. Tom As I illustrated upthread, after parse-time expansion we would Tom have a command that explicitly tells the system to insert or Tom update only the enumerated columns. That will *not* work as Tom desired if columns are added later, Where later is between parse analysis and execution - and if this query is not in a rule, then any such schema change will force a re-analysis if it's a prepared statement, no? and otherwise, we have the tables locked against schema changes anyway? Is there a failure case here that doesn't involve rules? Tom and (if it's in a rule) well, the example I gave is not something that anyone in their right minds would try and put in a rule. ... The alternative of set * = populate_record(foo, new_values) is less satisfactory since it introduces inconsistencies with the case where you _do_ want to specify explicit columns, unless you also allow set (a,b) = row_value which is required by the spec for T641 but which we don't currently have. Tom Right, but we should be trying to move in that direction. I see Tom your point though that (*) is more notationally consistent with Tom that case. Maybe we should be looking at trying to implement T641 Tom in full and then including (*) as a special case of that. I would have liked to have done that, but that would have raised the complexity of the project from Atri can take a stab at this one with negligible supervision to Andrew will have to spend more time than he has conveniently available staring at the raw parser to try and make it work. As I said, the perfect is the enemy of the good. Tom Anyway, my core point here is that we should avoid parse-time Tom expansion of the column set. Parse-time expansion of * is pretty widespread elsewhere. Changing that for this one specific case seems a bit marginal to me - and if the main motivation to do so is to support cases in DML rules, which are already a foot-bazooka, I think it's honestly not worth it. -- Andrew (irc:RhodiumToad) -- 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] Support UPDATE table SET(*)=...
Andrew Gierth wrote: Tom == Tom Lane t...@sss.pgh.pa.us writes: Tom Right, but we should be trying to move in that direction. I see Tom your point though that (*) is more notationally consistent with Tom that case. Maybe we should be looking at trying to implement T641 Tom in full and then including (*) as a special case of that. I would have liked to have done that, but that would have raised the complexity of the project from Atri can take a stab at this one with negligible supervision to Andrew will have to spend more time than he has conveniently available staring at the raw parser to try and make it work. Not to mention that, at this stage, we should be looking at reducing the scope of patches in commitfest rather than enlarge it. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] Support UPDATE table SET(*)=...
Alvaro Herrera alvhe...@2ndquadrant.com writes: Andrew Gierth wrote: Tom == Tom Lane t...@sss.pgh.pa.us writes: Tom Right, but we should be trying to move in that direction. I see Tom your point though that (*) is more notationally consistent with Tom that case. Maybe we should be looking at trying to implement T641 Tom in full and then including (*) as a special case of that. I would have liked to have done that, but that would have raised the complexity of the project from Atri can take a stab at this one with negligible supervision to Andrew will have to spend more time than he has conveniently available staring at the raw parser to try and make it work. Well, we've never considered implementation convenience to be more important than getting it right, and this doesn't seem like a place to start. (FWIW, the raw-parser changes would be a negligible fraction of the work involved to do it like that.) Not to mention that, at this stage, we should be looking at reducing the scope of patches in commitfest rather than enlarge it. I already took it out of the current commitfest ;-). 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] Removal of FORCE option in REINDEX
On 4/8/15 7:04 AM, Fujii Masao wrote: I'm thinking to apply the attached patch. But does anyone want to keep supporting the option? Why? Nuke it. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Support UPDATE table SET(*)=...
Tom == Tom Lane t...@sss.pgh.pa.us writes: One example that comes up occasionally (and that I've had to do myself more than once) is this: given a table foo and another with identical schema reference_foo, apply appropriate inserts, updates and deletes to table foo to make the content of the two tables identical. This can be done these days with wCTEs: with t_diff as (select o.id as o_id, n.id as n_id, o, n from foo o full outer join reference_foo n on (o.id=n.id) where (o.*) is distinct from (n.*)), ins as (insert into foo select (n).* from t_diff where o_id is null), del as (delete from foo where id in (select o_id from t_diff where n_id is null)), upd as (update foo set (col1,col2,...) = ((n).col1,(n).col2,...) -- XXX from t_diff where foo.id = n_id and o_id = n_id) select count(*) filter (where o_id is null) as num_ins, count(*) filter (where o_id = n_id) as num_upd, count(*) filter (where n_id is null) as num_del from t_diff; Tom While I agree that the UPDATE part of that desperately needs Tom improvement, I don't agree that the INSERT part is entirely fine. Tom You're still relying on a parse-time expansion of the (n).* Tom notation, which is inefficient Not in my experience a huge deal given what else is going on... Tom and not at all robust against schema changes (the same problem as Tom with the patch's approach to UPDATE). Now this I think is wrong; I think it's just as robust against schema changes as using the composite value directly would be. Consider the case where foo and reference_foo match with the exception of dropped columns; the code as it is should just work, while a variant that used the composite values would have to explicitly deal with that. (When I've used this kind of operation in practice, reference_foo has just been created using CREATE TEMP TABLE reference_foo (LIKE foo); and then populated via COPY from an external data source. Even if reference_foo were a non-temp table, the logic of the situation requires it to have the same schema as foo as far as SQL statements are concerned.) Tom So if we're taking this as a motivating example, I'd want to see a Tom fix that allows both INSERT and UPDATE directly from a composite Tom value of proper rowtype, without any expansion to individual Tom columns at all. I would argue that this is a case of the perfect being the enemy of the good. Tom Perhaps we could adopt some syntax like Tom INSERT INTO table (*) values-or-select Tom to represent the case that the values-or-select delivers a single Tom composite column of the appropriate type. We could, but I think in all practical cases it'll be nothing more than a small performance optimization rather than something that really benefits people in terms of enhanced functionality. Other examples arise from things one might want to do in plpgsql; for example to update a record from an hstore or json value, one can use [json_]populate_record to construct a record variable, but then it's back to naming all the columns in order to actually perform the update statement. Tom Sure, but the patch as given didn't work very well for that Tom either, Partly that's a limitation resulting from how much can be done with the existing SET (...) = syntax and implementation without ripping it all out and starting over. An incremental improvement seemed to be a more readily achievable goal. In practice it would indeed probably look like: declare new_id integer; new_values hstore; begin /* do stuff */ update foo set (*) = (select * from populate_record(foo, new_values)) where foo.id = new_id; A agree that it would be nicer to do update foo set (*) = populate_record(foo, new_values) where foo.id = new_id; but that would be a substantially larger project. The alternative of set * = populate_record(foo, new_values) is less satisfactory since it introduces inconsistencies with the case where you _do_ want to specify explicit columns, unless you also allow set (a,b) = row_value which is required by the spec for T641 but which we don't currently have. -- Andrew (irc:RhodiumToad) -- 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] Support UPDATE table SET(*)=...
Andrew Gierth and...@tao11.riddles.org.uk writes: Tom == Tom Lane t...@sss.pgh.pa.us writes: Tom I spent a fair amount of time cleaning this patch up to get it Tom into committable shape, but as I was working on the documentation Tom I started to lose enthusiasm for it, because I was having a hard Tom time coming up with compelling examples. One example that comes up occasionally (and that I've had to do myself more than once) is this: given a table foo and another with identical schema reference_foo, apply appropriate inserts, updates and deletes to table foo to make the content of the two tables identical. This can be done these days with wCTEs: with t_diff as (select o.id as o_id, n.id as n_id, o, n from foo o full outer join reference_foo n on (o.id=n.id) where (o.*) is distinct from (n.*)), ins as (insert into foo select (n).* from t_diff where o_id is null), del as (delete from foo where id in (select o_id from t_diff where n_id is null)), upd as (update foo set (col1,col2,...) = ((n).col1,(n).col2,...) -- XXX from t_diff where foo.id = n_id and o_id = n_id) select count(*) filter (where o_id is null) as num_ins, count(*) filter (where o_id = n_id) as num_upd, count(*) filter (where n_id is null) as num_del from t_diff; While I agree that the UPDATE part of that desperately needs improvement, I don't agree that the INSERT part is entirely fine. You're still relying on a parse-time expansion of the (n).* notation, which is inefficient and not at all robust against schema changes (the same problem as with the patch's approach to UPDATE). So if we're taking this as a motivating example, I'd want to see a fix that allows both INSERT and UPDATE directly from a composite value of proper rowtype, without any expansion to individual columns at all. Perhaps we could adopt some syntax like INSERT INTO table (*) values-or-select to represent the case that the values-or-select delivers a single composite column of the appropriate type. Other examples arise from things one might want to do in plpgsql; for example to update a record from an hstore or json value, one can use [json_]populate_record to construct a record variable, but then it's back to naming all the columns in order to actually perform the update statement. Sure, but the patch as given didn't work very well for that either, at least not if you wanted to avoid multiple evaluation of the composite-returning function. You'd have to adopt some obscure syntax like UPDATE target SET (*) = (SELECT * FROM composite_function(...)). With what I'm thinking about now you could do UPDATE target SET * = composite_function(...) which is a good deal less notation, and with a bit of luck it would not require disassembling and reassembling the function's output tuple. 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] Precedence of NOT LIKE, NOT BETWEEN, etc
On Tue, Feb 24, 2015 at 5:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Also, it strikes me that we could significantly reduce, maybe even fully eliminate, the funny behaviors around the existing base_yylex() substitutions if we made them use the same idea, ie replace the leading token with something special but keep the second token's separate identity. Unless somebody sees a hole in this idea, I'll probably go do that and then come back to the precedence issues. IIRC that's exactly what the earlier patch for this did. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Precedence of NOT LIKE, NOT BETWEEN, etc
Greg Stark st...@mit.edu writes: On Tue, Feb 24, 2015 at 5:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Also, it strikes me that we could significantly reduce, maybe even fully eliminate, the funny behaviors around the existing base_yylex() substitutions if we made them use the same idea, ie replace the leading token with something special but keep the second token's separate identity. Unless somebody sees a hole in this idea, I'll probably go do that and then come back to the precedence issues. IIRC that's exactly what the earlier patch for this did. Right, see d809fd0008a2e26de463f47b7aba0365264078f3 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] INSERT ... ON CONFLICT IGNORE (and UPDATE) 3.0
On Tue, Apr 7, 2015 at 10:59 PM, Peter Geoghegan p...@heroku.com wrote: The documentation has been updated to reflect all of this. By the way, for the convenience of reviewers I continue to maintain a mirror of pre-built documentation as outlined here: https://wiki.postgresql.org/wiki/UPSERT#Documentation -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Row security violation error is misleading
On 8 April 2015 at 16:27, Stephen Frost sfr...@snowman.net wrote: 2). In prepend_row_security_policies(), I think it is better to have any table RLS policies applied before any hook policies, so that a hook cannot be used to bypass built-in RLS. While I agree that we want to include the RLS policies defined against the table prior to any policies which are added by the hook, I don't quite follow what you mean by cannot be used to bypass built-in RLS. If we allow, as intended, extensions to define their own policies then it's entirely possible for the extension to return a allow all policy, and I believe that's perfectly fine. That doesn't match what the code currently does: * Also, allow extensions to add their own policies. * * Note that, as with the internal policies, if multiple policies are * returned then they will be combined into a single expression with * all of them OR'd together. However, to avoid the situation of an * extension granting more access to a table than the internal policies * would allow, the extension's policies are AND'd with the internal * policies. In other words - extensions can only provide further * filtering of the result set (or further reduce the set of records * allowed to be added). which seems reasonable, and means that if there are both internal and external policies, an allow all external policy would be a no-op. All the patch does is to ensure that the quals from the external policies are on the outer security barrier, so that if they contain leaky functions they cannot leak data that doesn't pass the internal policies (like a SB view on top of another SB view). Regards, Dean -- 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] Tuple visibility within a single XID
Qingqing Zhou zhouqq.postg...@gmail.com writes: Not sure if I understand correctly: in uniqueness check, we see all possible tuples with a dirty snapshot. For a tuple version inserted and updated by myself, it is surely dead no matter how the transaction ends. So I interpret that we can safely pruning the version. It may be dead in the future, but unless you can prove that your session does not still contain a snapshot that could see the row, you can't prune it. Considering only the current query is a fundamental error. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Making src/test/ssl more robust
Hi all, I noticed two things while looking at the SSL test suite: 1) When running the tests, some logs are generated in client-log, but this log file has no entry in .gitignore... A patch is attached. 2) cp is used with a wildcard and system_or_bail in ServerSetup.pm: system_or_bail cp ssl/server-*.crt '$tempdir'/pgdata; system_or_bail cp ssl/server-*.key '$tempdir'/pgdata; system_or_bail chmod 0600 '$tempdir'/pgdata/server-*.key; system_or_bail cp ssl/root+client_ca.crt '$tempdir'/pgdata; system_or_bail cp ssl/root+client.crl '$tempdir'/pgdata; This does not look very portable to me. Wouldn't it be better to use glob to get a list of the files and then copy each matching entry? Thoughts? -- Michael diff --git a/src/test/ssl/.gitignore b/src/test/ssl/.gitignore new file mode 100644 index 000..3bc46b5 --- /dev/null +++ b/src/test/ssl/.gitignore @@ -0,0 +1,2 @@ +# Generated by tests +/client-log -- 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] Proposal : REINDEX xxx VERBOSE
On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? +1 Changed status to Ready for Committer. The patch adds new syntax like REINDEX ... WITH VERBOSE, i.e., () is not added after WITH clause. Did we reach the consensus about this syntax? The last email from Robert just makes me think that () should be added into the syntax. Thank you for taking time for this patch! This was quite complicated issue since we already have a lot of style command currently. We can think many of style from various perspective: kind of DDL, new or old command, maintenance command. And each command has each its story. I believe we have reached the consensus with this style at least once (please see previous discussion), but we might needs to discuss more... Regards, --- Sawada Masahiko -- 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] Row security violation error is misleading
Dean Rasheed dean.a.rash...@gmail.com wrote: Re-using the SQLSTATE 44000 is a bit iffy too. We should probably define something to differentiate this, like: 44P01 ROW SECURITY WRITE POLICY VIOLATION Yes, that sounds sensible. I would be more inclined to use: 42501 ERRCODE_INSUFFICIENT_PRIVILEGE I know this is used 173 other places where a user attempts to do something they are not authorized to do, so you would not be able to differentiate the specific cause based on SQLSTATE if this is used -- but why don't we feel that way about the other 173 causes? Why does this security violation require a separate SQLSTATE? -- Kevin Grittner EDB: 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] Row security violation error is misleading
* Kevin Grittner (kgri...@ymail.com) wrote: Dean Rasheed dean.a.rash...@gmail.com wrote: Re-using the SQLSTATE 44000 is a bit iffy too. We should probably define something to differentiate this, like: 44P01 ROW SECURITY WRITE POLICY VIOLATION Yes, that sounds sensible. I would be more inclined to use: 42501 ERRCODE_INSUFFICIENT_PRIVILEGE I know this is used 173 other places where a user attempts to do something they are not authorized to do, so you would not be able to differentiate the specific cause based on SQLSTATE if this is used -- but why don't we feel that way about the other 173 causes? Why does this security violation require a separate SQLSTATE? I tend to agree with this and it feels more consistent. SQLSTATE is already a very generic response system and knowing that it's a policy violation instead of a GRANT violations strikes me as unlikely to be terribly interesting at the level where you're just looking at the SQLSTATE code. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Row security violation error is misleading
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: On 7 April 2015 at 16:21, Stephen Frost sfr...@snowman.net wrote: Agreed and we actually have a patch from Dean already to address this, it's just been waiting on me (with a couple of other ones). It'd certainly be great if you have time to take a look at those, though, generally speaking, I feel prety happy about where those are and believe they really just need to be reviewed/tested and maybe a bit of word- smithing around the docs. The first of those patches [1] has bit-rotted somewhat, due to the recent changes to the way rowmarks are handled, so here's an updated version. It wasn't a trivial merge, because commit cb1ca4d800621dcae67ca6c799006de99fa4f0a5 made a change to ExecBuildAuxRowMark() without a matching change to preprocess_targetlist(), and one of the new RLS-with-inheritance tests fell over that. Thanks! 1). In prepend_row_security_policies(), defaultDeny was always true, so if there were any hook policies, the RLS policies on the table would just get discarded. That was certainly unintentional (and wasn't the case at one point.. I recall specifically checking that), thanks for addressing it. 2). In prepend_row_security_policies(), I think it is better to have any table RLS policies applied before any hook policies, so that a hook cannot be used to bypass built-in RLS. While I agree that we want to include the RLS policies defined against the table prior to any policies which are added by the hook, I don't quite follow what you mean by cannot be used to bypass built-in RLS. If we allow, as intended, extensions to define their own policies then it's entirely possible for the extension to return a allow all policy, and I believe that's perfectly fine. The idea has come up a couple of times to also have restrictive policies and I agree that's an interesting idea and, once implemented, we would want to allow extensions to define both kinds and make sure that we apply restrictive policies defined in table-level policies in addition to policies from extensions, but we're not quite there yet. 3). The infinite recursion detection in fireRIRrules() didn't properly manage the activeRIRs list in the case of WCOs, so it would incorrectly report infinite recusion if the same relation with RLS appeared more than once in the rtable, for example UPDATE t ... FROM t Right, thanks for working through that and providing a fix. 4). The RLS expansion code in fireRIRrules() was handling RLS in the main loop through the rtable. This lead to RTEs being visited twice if they contained sublink subqueries, which prepend_row_security_policies() attempted to handle by exiting early if the RTE already had securityQuals. That didn't work, however, since if the query involved a security barrier view on top of a table with RLS, the RTE would already have securityQuals (from the view) by the time fireRIRrules() was invoked, and so the table's RLS policies would be ignored. This is most easily fixed in fireRIRrules() by handling RLS in a separate loop at the end, after dealing with any other sublink subqueries, thus ensuring that each RTE is only visited once for RLS expansion. Agreed. 5). The inheritance planner code didn't correctly handle non-target relations with RLS, which would get turned into subqueries during planning. Thus an update of the form UPDATE t1 ... FROM t2 ... where t1 has inheritance and t2 has RLS quals would fail. Urgh. Thanks for the testing and fix for that. 6). process_policies() was adding WCOs to non-target relations, which is unnecessary, and could lead to a lot of wasted time in the rewriter and the planner. WCOs are only needed on the result relation. Ah, yes, that makes sense. The second patch [2] is the one that is actually relevant to this thread. This patch is primarily to apply the RLS checks earlier, before an update is attempted, more like a regular permissions check. This adds a new enum to classify the kinds of WCO, a side benefit of which is that it allows different error messages when RLS checks are violated, as opposed to WITH CHECK OPTIONs on views. Right. I actually re-used the sql status code 42501 - ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the parallel with permissions checks, but I quite like Craig's idea of inventing a new status code for this, so that it can be more easily distinguished from a lack of GRANTed privileges. As I mentioned to Kevin, I'm not sure that this is really a useful distinction. I'm quite curious if other systems provide that distinction between grant violations and policy violations. If they do then that would certainly bolster the argument to provide the distinction in PG. There's another side benefit to this patch, which is that the new enum could be extended to include a new kind of WCO for a check of the USING quals of a RLS UPDATE policy on the update path of an INSERT..ON
Re: [HACKERS] Sloppy SSPI error reporting code
On Fri, Apr 3, 2015 at 08:32:08PM -0400, Noah Misch wrote: On Thu, Apr 02, 2015 at 07:31:52AM -0400, Bruce Momjian wrote: On Thu, Apr 2, 2015 at 01:44:59AM -0400, Noah Misch wrote: On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote: On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote: While looking at fe-auth.c I noticed quite a few places that weren't bothering to make error messages localizable (ie, missing libpq_gettext calls), and/or were failing to add a trailing newline as expected in libpq error messages. Perhaps these are intentional but I doubt it. Most of the instances seemed to be SSPI-related. I have no intention of fixing these myself, but whoever committed that code should take a second look. I looked through that file and only found two cases; patch attached. Tom mentioned newline omissions, which you'll find in pg_SSPI_error(). Oh, I accidentally saw the backend version of that function, which looked fine. I have attached a patch for that. That patch looks reasonable. Patch applied. Thanks. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] New error code to track unsupported contexts
Pushed this. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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] Support UPDATE table SET(*)=...
Andrew Gierth and...@tao11.riddles.org.uk writes: Tom == Tom Lane t...@sss.pgh.pa.us writes: Tom and not at all robust against schema changes (the same problem as Tom with the patch's approach to UPDATE). Now this I think is wrong; I think it's just as robust against schema changes as using the composite value directly would be. Consider the case where foo and reference_foo match with the exception of dropped columns; the code as it is should just work, while a variant that used the composite values would have to explicitly deal with that. AFAICS you've got that backwards. As I illustrated upthread, after parse-time expansion we would have a command that explicitly tells the system to insert or update only the enumerated columns. That will *not* work as desired if columns are added later, and (if it's in a rule) I would expect the system to have to fail a DROP command trying to drop one of those named columns, the same way you can't drop a column referenced in a view/rule today. On the other hand, if it's a composite-type assignment, then dealing with things like dropped columns becomes the system's responsibility, and we already have code for that sort of thing. ... The alternative of set * = populate_record(foo, new_values) is less satisfactory since it introduces inconsistencies with the case where you _do_ want to specify explicit columns, unless you also allow set (a,b) = row_value which is required by the spec for T641 but which we don't currently have. Right, but we should be trying to move in that direction. I see your point though that (*) is more notationally consistent with that case. Maybe we should be looking at trying to implement T641 in full and then including (*) as a special case of that. Anyway, my core point here is that we should avoid parse-time expansion of the column set. The *only* reason for doing that is that it makes the patch simpler, not that there is some functional advantage; in fact there are considerable functional disadvantages as we've just been through. So I do not want to commit a patch that institutionalizes those disadvantages just for short-term implementation simplicity. 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] Providing catalog view to pg_hba.conf file - Patch submission
I'm not sure what the best way to handle the hand-off from patch contribution to reviewer/committer. If I start tweaking things then you send in a new version it's actually more work to resolve the conflicts. I think at this point it's easiest if I just take it from here. I'm puzzled about the change from pg_hba_settings to pg_hba_conf. They're both ok but if pressed I would have preferred the original pg_hba_settings since that's consistent with the pg_settings view. There's no reason to quote tokens, that defeats the whole point of breaking the keywords into a separate column. Also there's no point in breaking out all but then still leaving +foo in the same column. My version had that moved into a new column as well called recursive_roles. We could call that just roles or groups but I was opting to the more explicit. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] rejected vs returned with feedback in new CF app
On Wed, Apr 8, 2015 at 4:57 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Apr 7, 2015 at 3:35 PM, Peter Eisentraut pete...@gmx.net wrote: On 4/7/15 3:33 PM, Tom Lane wrote: I tried to mark the UPDATE SET (*) patch as returned with feedback, but the CF app informed me that if I did that the patch would automatically be moved to the next commitfest. That seems completely stupid. There is no need to reconsider it unless a new version of the patch is forthcoming (which there may or may not ever be, but that's beside the point for now). When and if the author does submit a new patch, that would be the time to include it in the next commitfest, no? I noticed that as well and have avoided closing some patches because of it. Several people, including me, have complained about this before. I hope that Magnus will fix it soon. Yeah, I think my doing so is more or less down to one of the hardest problems in IT - naming things. As in, what should we call that level. Right now we have Committed, Returned with feedback and Rejected as the statuses that indicates something is done for this commitfest. I do think we want to add another one of those to differentiate between these two states -- we could flag it as just returned with feedback and not move it, but if we do that we loose the ability to do statistics on it for example, and in order to figure out what happened you have to go look at the history details int he box at the bottom. So i think we need a specific label for it. Any suggestions for what it should be? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] deparsing utility commands
On 4/7/15 4:32 PM, Alvaro Herrera wrote: Executive summary: There is now a CommandDeparse_hook; deparse_utility_command is provided as an extension, intended for 9.6; rest of patch would be pushed to 9.5. Long version: I've made command deparsing hookable. Attached there are three patches: the first patch contains changes to core that just add the command list stuff, so that on ddl_command_end there is access to what has been executed. This includes the OID of the object just created, the command tag, and assorted other details; the deparsed command in JSON format is not immediately part of the result. I'm looking forward to testing this with pg_audit. I think the first patch alone will give us the bulk of the desired functionality. The third patch contains all the deparse code, packaged as a contrib module and extension named ddl_deparse. Essentially, it's everything that was previously in tcop/deparse_utility.c and utils/adt/ddl_json.c: the stuff that takes the parsenode and OID of a command execution and turns it into a JSON blob, and also the support function that takes the JSON blob and converts back into the plain text rendering of the command. The second patch contains some changes to core code that support the ddl_deparse extension; mainly some ruleutils.c changes. What links patches 0001 and 0003 is a hook, CommandDeparse_hook. If unset, the pg_event_trigger_ddl_commands function returns some boilerplate text like no deparse function installed; if the extension is installed, the JSON rendering is returned instead and can be used with the ddl_deparse_expand_command() function. I would prefer for the column to be NULL or to have a boolean column that indicates if JSON output is available (or both). I'd rather not do string matching if I can help it (or test for invalid JSON). The rationale for doing things this way is that it will be useful to have 9.5 expose the pg_event_trigger_ddl_commands() function for various uses, while we refine the JSON bits some more and get it committed for 9.6. In reviews, it's clear that there's some more bits to fiddle so that it can be as general as possible. I think we should label the whole DDL command reporting as experimental in 9.5 and subject to change, so that we can just remove the hook in 9.6 when the ddl_deparse thing becomes part of core. I'm completely on board with this. I think deparse is a cool piece of code and I see a bunch of potential uses for it, but at the same time I'm not sure it has finished baking yet. This is a smart and safe choice. Is there a particular commit you'd recommend for applying the deparse patches (especially the first set)? -- - David Steele da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] TABLESAMPLE patch
On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek p...@2ndquadrant.com wrote: On 06/04/15 14:30, Petr Jelinek wrote: On 06/04/15 11:02, Simon Riggs wrote: Are we ready for a final detailed review and commit? I plan to send v12 in the evening with some additional changes that came up from Amit's comments + some improvements to error reporting. I think it will be ready then. Ok so here it is. Changes vs v11: - changed input parameter list to expr_list - improved error reporting, particularly when input parameters are wrong - fixed SELECT docs to show correct syntax and mention that there can be more sampling methods - added name of the sampling method to the explain output - I don't like the code much there as it has to look into RTE but on the other hand I don't want to create new scan node just so it can hold the name of the sampling method for explain - made views containing TABLESAMPLE clause not autoupdatable - added PageIsAllVisible() check before trying to check for tuple visibility - some typo/white space fixes Compiler warnings: explain.c: In function 'ExplainNode': explain.c:861: warning: 'sname' may be used uninitialized in this function Docs spellings: PostgreSQL distrribution extra r. The optional parameter REPEATABLE acceps accepts. But I don't know that 'accepts' is the right word. It makes the seed value sound optional to REPEATABLE. each block having same chance should have the before same. Both of those sampling methods currently I think it should be these not those, as this sentence is immediately after their introduction, not at a distance. ...tuple contents and decides if to return in, or zero if none Something here is confusing. return it, not return in? Other comments: Do we want tab completions for psql? (I will never remember how to spell BERNOULLI). Needs a Cat version bump. Cheers, Jeff
Re: [HACKERS] rejected vs returned with feedback in new CF app
On April 8, 2015 9:28:50 PM GMT+02:00, Magnus Hagander mag...@hagander.net wrote: On Wed, Apr 8, 2015 at 4:57 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Apr 7, 2015 at 3:35 PM, Peter Eisentraut pete...@gmx.net wrote: On 4/7/15 3:33 PM, Tom Lane wrote: I tried to mark the UPDATE SET (*) patch as returned with feedback, but the CF app informed me that if I did that the patch would automatically be moved to the next commitfest. That seems completely stupid. There is no need to reconsider it unless a new version of the patch is forthcoming (which there may or may not ever be, but that's beside the point for now). When and if the author does submit a new patch, that would be the time to include it in the next commitfest, no? I noticed that as well and have avoided closing some patches because of it. Several people, including me, have complained about this before. I hope that Magnus will fix it soon. Yeah, I think my doing so is more or less down to one of the hardest problems in IT - naming things. As in, what should we call that level. Right now we have Committed, Returned with feedback and Rejected as the statuses that indicates something is done for this commitfest. I do think we want to add another one of those to differentiate between these two states -- we could flag it as just returned with feedback and not move it, but if we do that we loose the ability to do statistics on it for example, and in order to figure out what happened you have to go look at the history details int he box at the bottom. So i think we need a specific label for it. Any suggestions for what it should be? I'm not convinced we really need a version that closes and moves a entry. But if we indeed want it we can just name it moved. --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] rejected vs returned with feedback in new CF app
On 04/08/2015 03:28 PM, Magnus Hagander wrote: On Wed, Apr 8, 2015 at 4:57 AM, Robert Haas robertmh...@gmail.com mailto:robertmh...@gmail.com wrote: On Tue, Apr 7, 2015 at 3:35 PM, Peter Eisentraut pete...@gmx.net mailto:pete...@gmx.net wrote: On 4/7/15 3:33 PM, Tom Lane wrote: I tried to mark the UPDATE SET (*) patch as returned with feedback, but the CF app informed me that if I did that the patch would automatically be moved to the next commitfest. That seems completely stupid. There is no need to reconsider it unless a new version of the patch is forthcoming (which there may or may not ever be, but that's beside the point for now). When and if the author does submit a new patch, that would be the time to include it in the next commitfest, no? I noticed that as well and have avoided closing some patches because of it. Several people, including me, have complained about this before. I hope that Magnus will fix it soon. Yeah, I think my doing so is more or less down to one of the hardest problems in IT - naming things. As in, what should we call that level. Right now we have Committed, Returned with feedback and Rejected as the statuses that indicates something is done for this commitfest. I do think we want to add another one of those to differentiate between these two states -- we could flag it as just returned with feedback and not move it, but if we do that we loose the ability to do statistics on it for example, and in order to figure out what happened you have to go look at the history details int he box at the bottom. So i think we need a specific label for it. Any suggestions for what it should be? If we're moving it to the next commitfest, maybe Delayed with feedback. Returned with feedback should be putting the ball back in the submitter's court for further action. 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] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/04/08 7:44, Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: To support ROW_MARK_REFERENCE on (postgres_fdw) foreign tables, I'd like to propose the following FDW APIs: RowMarkType GetForeignRowMarkType(Oid relid, LockClauseStrength strength); Decide which rowmark type to use for a foreign table (that has strength = LCS_NONE), ie, ROW_MARK_REFERENCE or ROW_MARK_COPY. (For now, the second argument takes LCS_NONE only, but is intended to be used for the possible extension to the other cases.) This is called during select_rowmark_type() in the planner. Why would you restrict that to LCS_NONE? Seems like the point is to give the FDW control of the rowmark behavior, not only partial control. The reason is because I think it's comparatively more promissing to customize the ROW_MARK type for LCS_NONE than other cases since in many workloads no re-fetch is needed, and because I think other cases would need more discussions. So, as a first step, I restricted that to LCS_NONE. But I've got the point, and agree that we should give the full control to the FDW. (For the same reason I disagree with the error check in the patch that restricts which ROW_MARK options this function can return. If the FDW has TIDs, seems like it could reasonably use whatever options tables can use.) Will fix. void BeginForeignFetch(EState *estate, ExecRowMark *erm, List *fdw_private, int eflags); Begin a remote fetch. This is called during InitPlan() in the executor. The begin/end functions seem like useless extra mechanism. Why wouldn't the FDW just handle this during its regular scan setup? It could look to see whether the foreign table is referenced by any ExecRowMarks (possibly there's room to add some convenience functions to help with that). What's more, that design would make it simpler if the basic row fetch needs to be modified. The reason is just because it's easy to understand the structure at least to me since the begin/exec/end are all done in a higher level of the executor. What's more, the begin/end can be done once per foreign scan node for the multi-table update case. But I feel inclined to agree with you on this point also. And I'd also like to propose to add a table/server option, row_mark_reference, to postgres_fdw. When a user sets the option to true for eg a foreign table, ROW_MARK_REFERENCE will be used for the table, not ROW_MARK_COPY. Why would we leave that in the hands of the user? Hardly anybody is going to know what to do with the option, or even that they should do something with it. It's basically only of value for debugging AFAICS, Agreed. (When begining to update postgres_fdw docs, I also started to think so.) I'll update the patch, which will contain only an infrastructure for this in the PG core, and will not contain any postgres_fdw change. Thank you for taking the time to review the patch! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Gracefully Reload SSL Certificates
On Wed, Apr 8, 2015 at 11:48:11AM -0400, Donald Stufft wrote: Currently replacing the SSL certificates for PostgreSQL requires a full server restart. However in the infrastructure for www.python.org (and in the future, pypi.python.org as well) we use short lived certificates (1 day) that automatically get rotated when 75% of their lifetime is used up. This means that we end up needing to do a full restart of PostgreSQL once a day or so which is a disruptive action that causes the site to generate errors while PostgreSQL shuts down and starts back up. It would be great if PostgreSQL could load a new SSL certificate with a graceful reload. This would solve our use case perfectly. In the interim I'm attempting to work around this problem by sticking stunnel inbetween PostgreSQL and the clients and use that to terminate TLS since it *does* support gracefully reloading certificates. This has been discussed before and seemed reasonable: http://www.postgresql.org/message-id/flat/caas3tyljcv-m0cqfmrrxujwa9_fksckuakt9_l41wnujzyw...@mail.gmail.com#caas3tyljcv-m0cqfmrrxujwa9_fksckuakt9_l41wnujzyw...@mail.gmail.com -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] New error code to track unsupported contexts
2015-04-09 3:45 GMT+09:00 Alvaro Herrera alvhe...@2ndquadrant.com: Pushed this. Thanks! -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? +1 Changed status to Ready for Committer. The patch adds new syntax like REINDEX ... WITH VERBOSE, i.e., () is not added after WITH clause. Did we reach the consensus about this syntax? The last email from Robert just makes me think that () should be added into the syntax. Thank you for taking time for this patch! I removed the FORCE option from REINDEX, so you would need to update the patch. This was quite complicated issue since we already have a lot of style command currently. We can think many of style from various perspective: kind of DDL, new or old command, maintenance command. And each command has each its story. I believe we have reached the consensus with this style at least once (please see previous discussion), but we might needs to discuss more... Okay, another question is that; WITH must be required whenever the options are specified? Or should it be abbreviatable? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Removal of FORCE option in REINDEX
On Thu, Apr 9, 2015 at 12:33 AM, Jim Nasby jim.na...@bluetreble.com wrote: On 4/8/15 7:04 AM, Fujii Masao wrote: I'm thinking to apply the attached patch. But does anyone want to keep supporting the option? Why? Nuke it. There seem no objections, so just pushed. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] rejected vs returned with feedback in new CF app
On Apr 9, 2015, at 1:08 AM, Andres Freund and...@anarazel.de wrote: I'm not convinced we really need a version that closes and moves a entry. But if we indeed want it we can just name it moved. +1. ...Robert -- 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] rejected vs returned with feedback in new CF app
On Tue, Apr 7, 2015 at 12:33 PM, Tom Lane t...@sss.pgh.pa.us wrote: I tried to mark the UPDATE SET (*) patch as returned with feedback, but the CF app informed me that if I did that the patch would automatically be moved to the next commitfest. That seems completely stupid. There is no need to reconsider it unless a new version of the patch is forthcoming (which there may or may not ever be, but that's beside the point for now). When and if the author does submit a new patch, that would be the time to include it in the next commitfest, no? I ended up marking it rejected instead, but that seems a bit harsh. While that is one possibility, given that it shows in the next CF as Waiting on Author, and lack of any other obvious place to put the CF entry while it is in limbo, I'm not sure there is a problem here - though I'm sure I and others can envision additional capabilities to make tracking committer vs author responsibility easier. I could see adding a Moved to ToDo status that denotes that we got tired of Waiting on Author and decided to move the item to the ToDo list. The same could be used to simply indicate that while the idea is solid the current implementation is lacking. Quite a few ToDo items fall into that category - and saying the patch is rejected while the concept is accepted is valid feedback. Whether we want to distinguish between Abandoned - moved to ToDo and Unacceptable Implementation - moved to ToDo is something to consider once the idea of using the ToDo as a limbo area, in addition to the next CF, is agreed upon. Put another way, the logical conclusion to Tom's sentiment is to simply remove everything Waiting on Author since there is never any guarantee that a response will be forthcoming and then, if one is, the entry can be added back into the current CF at that time. Leaving open items in the prior CS doesn't seem to make sense and I do not know enough about the application to determine how feasible it is to be a closed item from a previous CFs back to life. David J.
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
Hanada-san, In addition to your comments, I removed useless code that retrieves ForeignPath from outer/inner RelOptInfo and store them into ForeignPath#fdw_private. Now postgres_fdw’s join pushd-down is free from existence of ForeignPath under the join relation. This means that we can support the case Robert mentioned before, that whole (huge JOIN large) JOIN small” can be pushed down even if “(huge JOIN large)” is dominated by another join path. Yes, it's definitely reasonable design, and fits intention of the interface. I should point out it from the beginning. :-) l of the first SELECT represents a whole-row reference. However, underlying SELECT contains system columns in its target- list. Is it available to construct such a query? SELECT l.a1, l.a2 FROM (SELECT (id,a,atext), ctid) l (a1, a2) ... ^^ Simple relation reference such as l is not sufficient for the purpose, yes. But putting columns in parentheses would not work when a user column is referenced in original query. I implemented deparseProjectionSql to use ROW(...) expression for a whole-row reference in the target list, in addition to ordinary column references for actually used columns and ctid. Please see the test case for mixed use of ctid and whole-row reference to postgres_fdw’s regression tests. Now a whole-row reference in the remote query looks like this: It seems to me that deparseProjectionSql() works properly. -- ctid with whole-row reference EXPLAIN (COSTS false, VERBOSE) SELECT t1.ctid, t1, t2 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10; - Limit Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1 - Sort Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1 Sort Key: t1.c3, t1.c1 - Foreign Scan Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1 Remote SQL: SELECT l.a1, l.a2, l.a3, l.a4, r.a1 FROM (SELECT l.a7, ROW(l.a10, l.a11, l.a12, l.a13, l.a14, l.a15, l.a16, l.a17), l.a12, l.a10 FROM (SELECT C 1 a10, c2 a11, c3 a12, c4 a13, c5 a14, c6 a15, c7 a1 (8 rows) In fact l.a12 and l.a10, for t1.c3 and t1.c1, are redundant in transferred data, but IMO this would simplify the code for deparsing. I agree. Even if we can reduce networking cost a little, tuple construction takes CPU cycles. Your decision is fair enough for me. * merge_fpinfo() It seems to me fpinfo-rows should be joinrel-rows, and fpinfo-width also should be joinrel-width. No need to have special intelligence here, isn't it? Oops. They are vestige of my struggle which disabled SELECT clause optimization (omit unused columns). Now width and rows are inherited from joinrel. Besides that, fdw_startup_cost and fdw_tuple_cost seem wrong, so I fixed them to use simple summary, not average. Does fpinfo-fdw_startup_cost represent a cost to open connection to remote PostgreSQL, doesn't it? postgres_fdw.c:1757 says as follows: /* * Add some additional cost factors to account for connection overhead * (fdw_startup_cost), transferring data across the network * (fdw_tuple_cost per retrieved row), and local manipulation of the data * (cpu_tuple_cost per retrieved row). */ If so, does a ForeignScan that involves 100 underlying relation takes 100 times heavy network operations on startup? Probably, no. I think, average is better than sum, and max of them will reflect the cost more correctly. Also, fdw_tuple_cost introduce the cost of data transfer over the network. I thinks, weighted average is the best strategy, like: fpinfo-fdw_tuple_cost = (fpinfo_o-width / (fpinfo_o-width + fpinfo_i-width) * fpinfo_o-fdw_tuple_cost + (fpinfo_i-width / (fpinfo_o-width + fpinfo_i-width) * fpinfo_i-fdw_tuple_cost; That's just my suggestion. Please apply the best way you thought. * explain output EXPLAIN output may be a bit insufficient to know what does it actually try to do. postgres=# explain select * from ft1,ft2 where a = b; QUERY PLAN Foreign Scan (cost=200.00..212.80 rows=1280 width=80) (1 row) Even though it is not an immediate request, it seems to me better to show users joined relations and remote ON/WHERE clause here. Like this? Foreign Scan on ft1 INNER JOIN ft2 ON ft1.a = ft2.b (cost=200.00..212.80 rows=1280 width=80) … No. This line is produced by ExplainScanTarget(), so it requires the backend knowledge to individual FDW. Rather than the backend, postgresExplainForeignScan() shall produce the output. It might produce a very long line in a case of joining many tables because it
Re: [HACKERS] rejected vs returned with feedback in new CF app
On Thu, Apr 9, 2015 at 9:20 AM, Robert Haas robertmh...@gmail.com wrote: On Apr 9, 2015, at 1:08 AM, Andres Freund and...@anarazel.de wrote: I'm not convinced we really need a version that closes and moves a entry. But if we indeed want it we can just name it moved. +1. +1. Sounds like a good idea. It would be good to get something in this area before the virtual deadline of 4/15, switching the current CF to money time... -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Gracefully Reload SSL Certificates
Currently replacing the SSL certificates for PostgreSQL requires a full server restart. However in the infrastructure for www.python.org (and in the future, pypi.python.org as well) we use short lived certificates (1 day) that automatically get rotated when 75% of their lifetime is used up. This means that we end up needing to do a full restart of PostgreSQL once a day or so which is a disruptive action that causes the site to generate errors while PostgreSQL shuts down and starts back up. It would be great if PostgreSQL could load a new SSL certificate with a graceful reload. This would solve our use case perfectly. In the interim I'm attempting to work around this problem by sticking stunnel inbetween PostgreSQL and the clients and use that to terminate TLS since it *does* support gracefully reloading certificates. --- Donald Stufft PGP: 7C6B 7C5D 5E2B 6356 A926 F04F 6E3C BCE9 3372 DCFA signature.asc Description: Message signed with OpenPGP using GPGMail
