Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW
On 2016/02/22 20:13, Rushabh Lathia wrote: I did another round of review for the latest patch and well as performed the sanity test, and haven't found any functional issues. Found couple of issue, see in-line comments for the same. Thanks! On Thu, Feb 18, 2016 at 3:15 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: On 2016/02/12 21:19, Etsuro Fujita wrote: + /* Check point 1 */ + if (operation == CMD_INSERT) + return false; + + /* Check point 2 */ + if (nodeTag(subplan) != T_ForeignScan) + return false; + + /* Check point 3 */ + if (subplan->qual != NIL) + return false; + + /* Check point 4 */ + if (operation == CMD_UPDATE) These comments are referring to something in the function header further up, but you could instead just delete the stuff from the header and mention the actual conditions here. Will fix. Done. The patch doesn't allow the postgres_fdw to push down an UPDATE/DELETE on a foreign join, so I added one more condition here not to handle such cases. (I'm planning to propose a patch to handle such cases, in the next CF.) I think we should place the checking foreign join condition before the target columns, as foreign join condition is less costly then the target columns. Agreed. Other changes: * I keep Rushabh's code change that we call PlanDMLPushdown only when all the required APIs are available with FDW, but for CheckValidResultRel, I left the code as-is (no changes to that function), to match the docs saying that the FDW needs to provide the DML pushdown callback functions together with existing table-updating functions such as ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete. I think we should also update the CheckValidResultRel(), because even though ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete not present, FDW still can perform UPDATE/DELETE/INSERT using DML Pushdown APIs. Lets take committer's view on this. OK PFA update patch, which includes changes into postgresPlanDMLPushdown() to check for join condition before target columns and also fixed couple of whitespace issues. Thanks again for updating the patch and fixing the issues! 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] postgres_fdw vs. force_parallel_mode on ppc
Robert Haas writes: > On Tue, Feb 23, 2016 at 2:06 AM, Tom Lane wrote: >> Robert Haas writes: >>> Foreign tables are supposed to be categorically excluded from >>> parallelism. Not sure why that's not working in this instance. >> BTW, I wonder where you think that's supposed to be enforced, because >> I sure can't find any such logic. > RTEs are checked in set_rel_consider_parallel(), and I thought there > was a check there related to foreign tables, but there isn't. Oops. Even if there were, it would not fix this bug, because AFAICS the only thing that set_rel_consider_parallel is chartered to do is set the per-relation consider_parallel flag. The failure that is happening in that regression test with force_parallel_mode turned on happens because standard_planner plasters a Gather node at the top of the plan, causing the whole plan including the FDW access to happen inside a parallel worker. The only way to prevent that is to clear the wholePlanParallelSafe flag, which as far as I can tell (not that any of this is documented worth a damn) isn't something that set_rel_consider_parallel is supposed to do. It looks to me like there is a good deal of fuzzy thinking here about the difference between locally parallelizable and globally parallelizable plans, ie Gather at the top vs Gather somewhere else. I also note with dismay that turning force_parallel_mode on seems to pretty much disable any testing of local parallelism. > In view of 69d34408e5e7adcef8ef2f4e9c4f2919637e9a06, we shouldn't > blindly assume that foreign scans are not parallel-safe, but we can't > blindly assume the opposite either. Maybe we should assume that the > foreign scan is parallel-safe only if one or more of the new methods > introduced by the aforementioned commit are set, but actually that > doesn't seem quite right. That would tell us whether the scan itself > can be parallelized, not whether it's safe to run serially but within > a parallel worker. I think maybe we need a new FDW API that gets > called from set_rel_consider_parallel() with the root, rel, and rte as > arguments and which can return a Boolean. If the callback is not set, > assume false. Meh. As things stand, postgres_fdw would have to aver that it can't ever be safely parallelized, which doesn't seem like a very satisfactory answer even if there are other FDWs that work differently (and which would those be? None that use a socket-style connection to an external server.) The commit you mention above seems to me to highlight the dangers of accepting hook patches with no working use-case to back them up. AFAICT it's basically useless for typical FDWs because of this multiple-connection problem. 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] postgres_fdw vs. force_parallel_mode on ppc
On Tue, Feb 23, 2016 at 2:06 AM, Tom Lane wrote: > Robert Haas writes: >>> Foreign tables are supposed to be categorically excluded from >>> parallelism. Not sure why that's not working in this instance. > > BTW, I wonder where you think that's supposed to be enforced, because > I sure can't find any such logic. > > I suppose that has_parallel_hazard() would be the logical place to > notice foreign tables, but it currently doesn't even visit RTEs, > much less contain any code to check if their tables are foreign. > Or did you have another place in mind to do that? RTEs are checked in set_rel_consider_parallel(), and I thought there was a check there related to foreign tables, but there isn't. Oops. In view of 69d34408e5e7adcef8ef2f4e9c4f2919637e9a06, we shouldn't blindly assume that foreign scans are not parallel-safe, but we can't blindly assume the opposite either. Maybe we should assume that the foreign scan is parallel-safe only if one or more of the new methods introduced by the aforementioned commit are set, but actually that doesn't seem quite right. That would tell us whether the scan itself can be parallelized, not whether it's safe to run serially but within a parallel worker. I think maybe we need a new FDW API that gets called from set_rel_consider_parallel() with the root, rel, and rte as arguments and which can return a Boolean. If the callback is not set, assume false. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw vs. force_parallel_mode on ppc
Thomas Munro writes: > On Tue, Feb 23, 2016 at 4:03 AM, Tom Lane wrote: >> I've not looked at the test case to see if this is exactly what's >> going wrong, but it's pretty easy to see how there might be a problem: >> consider a STABLE user-defined function that does a SELECT from a foreign >> table. If that function call gets pushed down into a parallel worker >> then it would fail as described. > I thought user defined functions were not a problem since it's the > user's responsibility to declare functions' parallel safety correctly. > The manual says: "In general, if a function is labeled as being safe > when it is restricted or unsafe, or if it is labeled as being > restricted when it is in fact unsafe, it may throw errors or produce > wrong answers when used in a parallel query"[1]. Hm. I'm not terribly happy with this its-the-users-problem approach to things, mainly because I have little confidence that somebody couldn't figure out a security exploit based on it. > The case of a plain old SELECT (as seen in the failing regression > test) is definitely a problem though and FDW access there needs to be > detected automatically. Yes, the problem we're actually seeing in that regression test is not dependent on a function wrapper. 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] [PATH] Correct negative/zero year in to_date/to_timestamp
On 2/22/16, Thomas Munro wrote: > On Tue, Feb 23, 2016 at 11:58 AM, Vitaly Burovoy > wrote: >> Hello, Hackers! >> >> I'm writing another patch and while I was trying to cover corner cases >> I found that to_date and to_timestamp work wrong if year in input >> value is zero or negative: >> >> postgres=# SELECT >> postgres-# y || '-06-01' as src >> postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN >> ('00'||(-y)||'-06-01 BC') END::date >> postgres-# ,to_date(y || '-06-01', '-MM-DD') >> postgres-# ,to_timestamp(y || '-06-01', '-MM-DD') >> postgres-# FROM (VALUES(2),(1),(0),(-1),(-2))t(y); >>src| date |to_date| to_timestamp >> --+---+---+--- >> 2-06-01 | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00 >> 1-06-01 | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00 >> 0-06-01 | | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC >> -1-06-01 | 0001-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC >> -2-06-01 | 0002-06-01 BC | 0003-06-01 BC | 0003-06-01 00:00:00+00 BC >> (5 rows) >> >> Zero year (and century) is accepted and negative years differs by 1 >> from what they should be. >> >> >> I've written a patch fixes that. With it results are correct: >> postgres=# SELECT >> postgres-# y || '-06-01' as src >> postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN >> ('00'||(-y)||'-06-01 BC') END::date >> postgres-# ,to_date(y || '-06-01', '-MM-DD') >> postgres-# ,to_timestamp(y || '-06-01', '-MM-DD') >> postgres-# FROM (VALUES(2),(1),(-1),(-2))t(y); >>src| date |to_date| to_timestamp >> --+---+---+--- >> 2-06-01 | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00 >> 1-06-01 | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00 >> -1-06-01 | 0001-06-01 BC | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC >> -2-06-01 | 0002-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC >> (4 rows) >> >> >> When year "0" is given, it raises an ERROR: >> postgres=# SELECT to_timestamp('*01*01', '*MM*DD'); >> ERROR: invalid input string for "" >> DETAIL: Year cannot be 0. >> >> >> Also I change behavior for era indicator when negatives century or >> year are given. In such case era indicator is ignored (for me it is >> obvious signs should be OR-ed): >> postgres=# SELECT to_timestamp('-0010*01*01 BC', '*MM*DD BC') >> postgres-# ,to_timestamp(' 0010*01*01 BC', '*MM*DD BC'); >>to_timestamp| to_timestamp >> ---+--- >> 0010-01-01 00:00:00+00 BC | 0010-01-01 00:00:00+00 BC >> (1 row) >> >> >> Testings, complains, advice, comment improvements are very appreciated. > > This seems to be a messy topic. The usage of "AD" and "BC" imply that > TO_DATE is using the anno domini system which doesn't have a year 0, > but in the DATE type perhaps we are using the ISO 8601 model[2] where > 1 BC is represented as , leading to the difference of one in all > years before 1 AD? > > [1] https://en.wikipedia.org/wiki/Anno_Domini > [2] https://en.wikipedia.org/wiki/ISO_8601#Years > > -- > Thomas Munro > http://www.enterprisedb.com Thank you for fast reply and for the link[2]. Be honest I didn't know ISO8601 specifies 1 BC as +. But the documentation[3] doesn't points that ISO8601 is used for "", but it is mentioned for "IYYY", and it is slightly deceiving. Also I remember that the other part of the documentation says[4] that "Keep in mind there is no 0 AD" that's why I decided it is impossible to pass for . Currently behavior with =0 is still surprising in some cases: postgres=# SELECT postgres-# to_date('20 -01-01', 'CC -MM-DD'), postgres-# to_date('20 0001-01-01', 'CC -MM-DD'); to_date | to_date + 1901-01-01 | 0001-01-01 (1 row) but the documentation[3] says "In conversions from string to timestamp or date, the CC (century) field is ignored if there is a YYY, or Y,YYY field." So is it shared opinion that ISO8601 is used for ""? [3]http://www.postgresql.org/docs/devel/static/functions-formatting.html [4]http://www.postgresql.org/docs/devel/static/functions-datetime.html -- Best regards, Vitaly Burovoy -- 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] Writing new unit tests with PostgresNode
On Tue, Feb 23, 2016 at 12:29 AM, Alvaro Herrera wrote: > Craig Ringer wrote: > >> > +=pod >> > + >> > +=head2 Set up a node >> > pod format... Do we really want that? Considering that those modules >> > are only aimed at being dedicated for in-core testing, I would say no. >> >> If it's plain comments you have to scan through massive piles of verbose >> Perl to find what you want. If it's pod you can just perldoc >> /path/to/module it and get a nice summary of the functions etc. >> >> If these are intended to become usable facilities for people to write tests >> with then I think it's important that the docs be reasonably accessible. > > Yes, I think adding POD here is a good idea. I considered doing it > myself back when I was messing with PostgresNode ... OK, withdrawal from here. If there are patches to add that to the existing tests, I'll review them, and rebase what I have depending on what gets in first. Could a proper patch split be done please? -- 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: format() changes discussion (was: Re: [HACKERS] psql metaqueries with \gexec)
On 2/22/16 5:16 PM, Corey Huinker wrote: (One thing I had to come up with was processing of arrays, which you also see in that example JSON -- it's the specifiers that have a colon inside the {}. The part after the colon is used as separator between the array elements, and each element is expanded separately.) I'm splitting the subject line because it seems like two very different patches may come out of this. Oops. Just saw this. I don't think it'd make sense to make format() itself as capable as what Alvaro did for event triggers. Something that geared towards dealing with catalog objects should be it's own thing. What would be very useful IMHO is a version of format() that accepted hstore as it's second argument and then let you use names in the format specification. IE: format( 'blahblah name1%I blahblah literal_a%L blah "some string"%s' , 'name1 => name, literal_a => a, "some string" => string ) I suggest hstore over json because it's easier to work with by hand than json is, and there's no ambiguity with things like objects or arrays. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL 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] psql metaqueries with \gexec
On 2/22/16 5:13 PM, Alvaro Herrera wrote: Jim Nasby wrote: On 2/22/16 11:47 AM, Alvaro Herrera wrote: Pavel Stehule wrote: The design of the "format" function is not closed. Try to send prototype and patch. The possibility to do PostgreSQL customization was strong reason why we didn't implemented "sprintf" and we implemented "format". Probably not terribly useful here, but for the DDL-deparse patch I came up with a syntax to format JSON objects, which used %-escapes; each escaped element corresponds to a string literal, or to an object. So you'd have %{table}D where the "table" element in the JSON object could be a simple string which is expanded verbatim (plus quoting if necessary), or it could be a JSON object with something like { schema => "public", name => "students" }, where each element is expanded and quoted as necessary; if the "schema" is null or it doesn't exist, it expands only the name, obviously omitting the dot. Where did the "D" in "%{table}D" come from? The I in %{foo}I was for "identifier" (of course) and I *think* the D was for "double identifiers" (that is, qualified). I expanded the idea afterwards to allow for a third name for things like catalog.schema.name, so I guess it's a misnomer already. It's not released code yet. You can see an example here https://www.postgresql.org/message-id/%3C20150224175152.GI5169%40alvh.no-ip.org%3E just scroll down a few hundred lines to about 7/16ths of the page (yes, really) (One thing I had to come up with was processing of arrays, which you also see in that example JSON -- it's the specifiers that have a colon inside the {}. The part after the colon is used as separator between the array elements, and each element is expanded separately.) Ahh, very interesting. Something that would probably be helpful for these kind of things is if we had a set of complex types available that represented things like the arguments to a function. Something like (parameter_mode enum(IN, OUT, INOUT), parameter_name name, parameter_type regtype, parameter_default text). A function might be represented by (function_schema name, function_name name, function_parameters ..., function_language, function_options, function_body). In any case, having anything along these lines in core would be useful, assuming that the individual facility was exposed as well (as opposed to only being available inside an event trigger). -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL 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] [PATH] Correct negative/zero year in to_date/to_timestamp
On Tue, Feb 23, 2016 at 11:58 AM, Vitaly Burovoy wrote: > Hello, Hackers! > > I'm writing another patch and while I was trying to cover corner cases > I found that to_date and to_timestamp work wrong if year in input > value is zero or negative: > > postgres=# SELECT > postgres-# y || '-06-01' as src > postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN > ('00'||(-y)||'-06-01 BC') END::date > postgres-# ,to_date(y || '-06-01', '-MM-DD') > postgres-# ,to_timestamp(y || '-06-01', '-MM-DD') > postgres-# FROM (VALUES(2),(1),(0),(-1),(-2))t(y); >src| date |to_date| to_timestamp > --+---+---+--- > 2-06-01 | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00 > 1-06-01 | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00 > 0-06-01 | | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC > -1-06-01 | 0001-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC > -2-06-01 | 0002-06-01 BC | 0003-06-01 BC | 0003-06-01 00:00:00+00 BC > (5 rows) > > Zero year (and century) is accepted and negative years differs by 1 > from what they should be. > > > I've written a patch fixes that. With it results are correct: > postgres=# SELECT > postgres-# y || '-06-01' as src > postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN > ('00'||(-y)||'-06-01 BC') END::date > postgres-# ,to_date(y || '-06-01', '-MM-DD') > postgres-# ,to_timestamp(y || '-06-01', '-MM-DD') > postgres-# FROM (VALUES(2),(1),(-1),(-2))t(y); >src| date |to_date| to_timestamp > --+---+---+--- > 2-06-01 | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00 > 1-06-01 | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00 > -1-06-01 | 0001-06-01 BC | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC > -2-06-01 | 0002-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC > (4 rows) > > > When year "0" is given, it raises an ERROR: > postgres=# SELECT to_timestamp('*01*01', '*MM*DD'); > ERROR: invalid input string for "" > DETAIL: Year cannot be 0. > > > Also I change behavior for era indicator when negatives century or > year are given. In such case era indicator is ignored (for me it is > obvious signs should be OR-ed): > postgres=# SELECT to_timestamp('-0010*01*01 BC', '*MM*DD BC') > postgres-# ,to_timestamp(' 0010*01*01 BC', '*MM*DD BC'); >to_timestamp| to_timestamp > ---+--- > 0010-01-01 00:00:00+00 BC | 0010-01-01 00:00:00+00 BC > (1 row) > > > Testings, complains, advice, comment improvements are very appreciated. This seems to be a messy topic. The usage of "AD" and "BC" imply that TO_DATE is using the anno domini system which doesn't have a year 0, but in the DATE type perhaps we are using the ISO 8601 model[2] where 1 BC is represented as , leading to the difference of one in all years before 1 AD? [1] https://en.wikipedia.org/wiki/Anno_Domini [2] https://en.wikipedia.org/wiki/ISO_8601#Years -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Convert pltcl from strings to objects
On 2/18/16 6:26 AM, Victor Wagner wrote: On Tue, 9 Feb 2016 16:23:21 -0600 There is suspicious place at the end of compile_pltcl_fuction function, where you've put comment that old prodesc cannot be deallocated, because it might be used by other call. It seems that reference counting mechanism which Tcl already has, can help here. Would there be serious performance impact if you'll use Tcl list instead of C structure to store procedure description? If not, you can rely on Tcl_IncrRefCount/Tcl_DecrRefCount to free a procedure description when last active call finishes. Are you referring to this comment? / * Add the proc description block to the hashtable. Note we do not * attempt to free any previously existing prodesc block. This is * annoying, but necessary since there could be active calls using * the old prodesc. / That is not changed by the patch, and I don't think it's in the scope of this patch. I agree it would be nice to get rid of that and the related malloc() call, as well as what perm_fmgr_info() is doing, but those are unrelated to this work. (BTW, I also disagree about using a Tcl list for prodesc. That's an object in a *Postgres* hash table; as such it needs to be managed by Postgres, not Tcl. AFAIK the Postgres ResourceOwner stuff would be the correct way to do that (but I'm not exactly an expert on that area). Function pltcl_elog have a big switch case to convert enum logpriority, local to this function to PostgreSql log levels. It seems not a most readable solution, because this enumeration is desined to be passed to Tcl_GetIndexFromObj, so it can be used and is used to index an array (logpriorities array of string representation). You can define simular array with PostgreSQL integer constant and replace page-sized switch with just two lines - this array definition and getting value from it by index static CONST84 int loglevels[]={DEBUG,LOG,INFO,NOTICE,WARNING,ERROR,FATAL}; Tcl_GetIndexFromObj( level=loglevels[priIndex]; Done. It seems that you are losing some diagnostic information by extracting just message field from ErrorData, which you do in pltcl_elog and pltcl_subtrans_abort. Tcl has mechanisms for passing around additional error information. See Tcl_SetErrorCode and Tcl_AddErrorInfo functions pltcl_process_SPI_result might benefit from providing SPI result code in machine readable way via Tcl_SetErrorCode too. Is there any backwards compatibility risk to these changes? Could having that new info break someone's existing code? In some cases you use Tcl_SetObjResult(interp, Tcl_NewStringObj("error message",-1)) to report error with constant message, where in other places Tcl_SetResult(interp,"error message",TCL_STATIC) left in place. I see no harm in using old API with static messages, especially if Tcl_AppendResult is used, but mixing two patterns above seems to be a bit inconsistent. Switched everything to using the new API. As far as I can understand, using Tcl_StringObj to represent all postgresql primitive (non-array) types and making no attempt to convert tuple data into integer, boolean or double objects is design decision. It really can spare few unnecessary type conversions. Yeah, that's on the list. But IMHO it's outside the scope of this patch. Thanks for your effort. Thanks for the review! -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL 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] Sanity checking for ./configure options?
On 2/5/16 10:08 AM, David Fetter wrote: On Wed, Feb 03, 2016 at 06:02:57PM -0600, Jim Nasby wrote: I just discovered that ./configure will happily accept '--with-pgport=' (I was actually doing =$PGPORT, and didn't realize $PGPORT was empty). What you end up with is a compile error in guc.c, with no idea why it's broken. Any reason not to have configure or at least make puke if pgport isn't valid? That seems like a good idea. Patch attached. I've verified it with --with-pgport=, =0, =7 and =1. It catches what you'd expect it to. As the comment states, it doesn't catch things like --with-pgport=1a in configure, but the compile error you get with that isn't too hard to figure out, so I think it's OK. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com diff --git a/configure b/configure index b3f3abe..2beee31 100755 --- a/configure +++ b/configure @@ -3099,6 +3099,14 @@ cat >>confdefs.h <<_ACEOF _ACEOF +# It's worth testing for this because it creates a very confusing error +if test "$default_port" == ""; then + as_fn_error $? "Invalid empty string supplied for \$PGPORT or --with-pgport" "$LINENO" 5 +# This won't catch something like "PGPORT=11a" but that produces a pretty easy +# to understand compile error. +elif test "$default_port" -lt "1" -o "$default_port" -gt "65535"; then + as_fn_error $? "port must be between 1 and 65535" "$LINENO" 5 +fi # # '-rpath'-like feature can be disabled diff --git a/configure.in b/configure.in index 0bd90d7..54e9a16 100644 --- a/configure.in +++ b/configure.in @@ -164,6 +164,14 @@ but it's convenient if your clients have the right default compiled in. AC_DEFINE_UNQUOTED(DEF_PGPORT_STR, "${default_port}", [Define to the default TCP port number as a string constant.]) AC_SUBST(default_port) +# It's worth testing for this because it creates a very confusing error +if test "$default_port" == ""; then + AC_MSG_ERROR([Invalid empty string supplied for \$PGPORT or --with-pgport]) +# This won't catch something like "PGPORT=11a" but that produces a pretty easy +# to understand compile error. +elif test "$default_port" -lt "1" -o "$default_port" -gt "65535"; then + AC_MSG_ERROR([port must be between 1 and 65535]) +fi # # '-rpath'-like feature can be disabled -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
format() changes discussion (was: Re: [HACKERS] psql metaqueries with \gexec)
> > (One thing I had to come up with was processing of arrays, which you > also see in that example JSON -- it's the specifiers that have a colon > inside the {}. The part after the colon is used as separator between > the array elements, and each element is expanded separately.) > > I'm splitting the subject line because it seems like two very different patches may come out of this.
Re: [HACKERS] psql metaqueries with \gexec
Jim Nasby wrote: > On 2/22/16 11:47 AM, Alvaro Herrera wrote: > >Pavel Stehule wrote: > > > >>The design of the "format" function is not closed. Try to send prototype > >>and patch. The possibility to do PostgreSQL customization was strong reason > >>why we didn't implemented "sprintf" and we implemented "format". > > > >Probably not terribly useful here, but for the DDL-deparse patch I came > >up with a syntax to format JSON objects, which used %-escapes; each > >escaped element corresponds to a string literal, or to an object. So > >you'd have %{table}D where the "table" element in the JSON object could > >be a simple string which is expanded verbatim (plus quoting if > >necessary), or it could be a JSON object with something like { schema => > >"public", name => "students" }, where each element is expanded and > >quoted as necessary; if the "schema" is null or it doesn't exist, it > >expands only the name, obviously omitting the dot. > > Where did the "D" in "%{table}D" come from? The I in %{foo}I was for "identifier" (of course) and I *think* the D was for "double identifiers" (that is, qualified). I expanded the idea afterwards to allow for a third name for things like catalog.schema.name, so I guess it's a misnomer already. It's not released code yet. You can see an example here https://www.postgresql.org/message-id/%3C20150224175152.GI5169%40alvh.no-ip.org%3E just scroll down a few hundred lines to about 7/16ths of the page (yes, really) (One thing I had to come up with was processing of arrays, which you also see in that example JSON -- it's the specifiers that have a colon inside the {}. The part after the colon is used as separator between the array elements, and each element is expanded separately.) -- Á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] postgres_fdw vs. force_parallel_mode on ppc
On Tue, Feb 23, 2016 at 4:03 AM, Tom Lane wrote: > Robert Haas writes: >> On Mon, Feb 22, 2016 at 11:02 AM, Thomas Munro >> wrote: >>> The postgres_fdw failure is a visibility-of-my-own-uncommitted-work >>> problem. The first command in a transaction updates a row via an FDW, >>> and then the SELECT expects to see the effects, but when run in a >>> background worker it creates a new FDW connection that can't see the >>> uncommitted UPDATE. > >> Foreign tables are supposed to be categorically excluded from >> parallelism. Not sure why that's not working in this instance. > > I've not looked at the test case to see if this is exactly what's > going wrong, but it's pretty easy to see how there might be a problem: > consider a STABLE user-defined function that does a SELECT from a foreign > table. If that function call gets pushed down into a parallel worker > then it would fail as described. I thought user defined functions were not a problem since it's the user's responsibility to declare functions' parallel safety correctly. The manual says: "In general, if a function is labeled as being safe when it is restricted or unsafe, or if it is labeled as being restricted when it is in fact unsafe, it may throw errors or produce wrong answers when used in a parallel query"[1]. Uncommitted changes on foreign tables are indeed invisible to functions declared as PARALLEL SAFE, when run with force_parallel_mode = on, max_parallel_degree = 2, but the default is UNSAFE and in that case the containing query is never parallelised. Perhaps the documentation could use a specific mention of this subtlety with FDWs in the PARALLEL section? The case of a plain old SELECT (as seen in the failing regression test) is definitely a problem though and FDW access there needs to be detected automatically. I also thought that has_parallel_hazard_walker might be the right place for that logic, as you suggested in your later message. [1] http://www.postgresql.org/docs/devel/static/sql-createfunction.html -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATH] Correct negative/zero year in to_date/to_timestamp
On 2/22/16, Vitaly Burovoy wrote: > Testings, complains, advice, comment improvements are very appreciated. The patch seems simple, but it can lead to a discussion, so I've added it to CF. [CF]https://commitfest.postgresql.org/9/533/ -- Best regards, Vitaly Burovoy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATH] Correct negative/zero year in to_date/to_timestamp
Hello, Hackers! I'm writing another patch and while I was trying to cover corner cases I found that to_date and to_timestamp work wrong if year in input value is zero or negative: postgres=# SELECT postgres-# y || '-06-01' as src postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN ('00'||(-y)||'-06-01 BC') END::date postgres-# ,to_date(y || '-06-01', '-MM-DD') postgres-# ,to_timestamp(y || '-06-01', '-MM-DD') postgres-# FROM (VALUES(2),(1),(0),(-1),(-2))t(y); src| date |to_date| to_timestamp --+---+---+--- 2-06-01 | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00 1-06-01 | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00 0-06-01 | | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC -1-06-01 | 0001-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC -2-06-01 | 0002-06-01 BC | 0003-06-01 BC | 0003-06-01 00:00:00+00 BC (5 rows) Zero year (and century) is accepted and negative years differs by 1 from what they should be. I've written a patch fixes that. With it results are correct: postgres=# SELECT postgres-# y || '-06-01' as src postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN ('00'||(-y)||'-06-01 BC') END::date postgres-# ,to_date(y || '-06-01', '-MM-DD') postgres-# ,to_timestamp(y || '-06-01', '-MM-DD') postgres-# FROM (VALUES(2),(1),(-1),(-2))t(y); src| date |to_date| to_timestamp --+---+---+--- 2-06-01 | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00 1-06-01 | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00 -1-06-01 | 0001-06-01 BC | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC -2-06-01 | 0002-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC (4 rows) When year "0" is given, it raises an ERROR: postgres=# SELECT to_timestamp('*01*01', '*MM*DD'); ERROR: invalid input string for "" DETAIL: Year cannot be 0. Also I change behavior for era indicator when negatives century or year are given. In such case era indicator is ignored (for me it is obvious signs should be OR-ed): postgres=# SELECT to_timestamp('-0010*01*01 BC', '*MM*DD BC') postgres-# ,to_timestamp(' 0010*01*01 BC', '*MM*DD BC'); to_timestamp| to_timestamp ---+--- 0010-01-01 00:00:00+00 BC | 0010-01-01 00:00:00+00 BC (1 row) Testings, complains, advice, comment improvements are very appreciated. -- Best regards, Vitaly Burovoy negative_years_in_to_date.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql metaqueries with \gexec
On 2/22/16 11:47 AM, Alvaro Herrera wrote: Pavel Stehule wrote: The design of the "format" function is not closed. Try to send prototype and patch. The possibility to do PostgreSQL customization was strong reason why we didn't implemented "sprintf" and we implemented "format". Probably not terribly useful here, but for the DDL-deparse patch I came up with a syntax to format JSON objects, which used %-escapes; each escaped element corresponds to a string literal, or to an object. So you'd have %{table}D where the "table" element in the JSON object could be a simple string which is expanded verbatim (plus quoting if necessary), or it could be a JSON object with something like { schema => "public", name => "students" }, where each element is expanded and quoted as necessary; if the "schema" is null or it doesn't exist, it expands only the name, obviously omitting the dot. Where did the "D" in "%{table}D" come from? BTW, the syntax I chose for [1] is similar to format's, except I elected to stick with % instead of $. So you do %parameter_name%type where type is s, L or I. I don't think it'd be hard to support an object with 'schema' and 'name' keys. [1] https://github.com/decibel/trunklet-format/blob/master/doc/trunklet-format.asc#2-template-specification -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL 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] about google summer of code 2016
On 22/02/16 23:34, Tom Lane wrote: =?UTF-8?Q?=c3=81lvaro_Hern=c3=a1ndez_Tortosa?= writes: On 22/02/16 05:10, Tom Lane wrote: Another variable is that your answers might depend on what format you assume the client is trying to convert from/to. (It's presumably not text JSON, but then what is it?) As I mentioned before, there are many well-known JSON serialization formats, like: - http://ubjson.org/ - http://cbor.io/ - http://msgpack.org/ - BSON (ok, let's skip that one hehehe) - http://wiki.fasterxml.com/SmileFormatSpec Ah, the great thing about standards is there are so many to choose from :-( So I guess part of the GSoC project would have to be figuring out which one of these would make the most sense for us to adopt. regards, tom lane Yes. And unless I'm mistaken, there's an int16 to identify the data format. Apart from the chosen format, others may be provided as an alternative using different data formats. Or alternatives (like compressed text json). Of course, this may be better suited for a next GSoC project, of course. Álvaro -- Álvaro Hernández Tortosa --- 8Kdata -- 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] about google summer of code 2016
=?UTF-8?Q?=c3=81lvaro_Hern=c3=a1ndez_Tortosa?= writes: > On 22/02/16 05:10, Tom Lane wrote: >> Another variable is that your answers might depend on what format you >> assume the client is trying to convert from/to. (It's presumably not >> text JSON, but then what is it?) > As I mentioned before, there are many well-known JSON serialization > formats, like: > - http://ubjson.org/ > - http://cbor.io/ > - http://msgpack.org/ > - BSON (ok, let's skip that one hehehe) > - http://wiki.fasterxml.com/SmileFormatSpec Ah, the great thing about standards is there are so many to choose from :-( So I guess part of the GSoC project would have to be figuring out which one of these would make the most sense for us to adopt. 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] about google summer of code 2016
On 22/02/16 05:10, Tom Lane wrote: Heikki Linnakangas writes: On 19/02/16 10:10, Ã�lvaro Hernández Tortosa wrote: Oleg and I discussed recently that a really good addition to a GSoC item would be to study whether it's convenient to have a binary serialization format for jsonb over the wire. Seems a bit risky for a GSoC project. We don't know if a different serialization format will be a win, or whether we want to do it in the end, until the benchmarking is done. It's also not clear what we're trying to achieve with the serialization format: smaller on-the-wire size, faster serialization in the server, faster parsing in the client, or what? Another variable is that your answers might depend on what format you assume the client is trying to convert from/to. (It's presumably not text JSON, but then what is it?) As I mentioned before, there are many well-known JSON serialization formats, like: - http://ubjson.org/ - http://cbor.io/ - http://msgpack.org/ - BSON (ok, let's skip that one hehehe) - http://wiki.fasterxml.com/SmileFormatSpec Having said that, I'm not sure that risk is a blocking factor here. History says that a large fraction of our GSoC projects don't result in a commit to core PG. As long as we're clear that "success" in this project isn't measured by getting a feature committed, it doesn't seem riskier than any other one. Maybe it's even less risky, because there's less of the success condition that's not under the GSoC student's control. Agreed :) Álvaro -- Álvaro Hernández Tortosa --- 8Kdata -- 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] about google summer of code 2016
On 21/02/16 21:15, Heikki Linnakangas wrote: On 19/02/16 10:10, Álvaro Hernández Tortosa wrote: Oleg and I discussed recently that a really good addition to a GSoC item would be to study whether it's convenient to have a binary serialization format for jsonb over the wire. Some argue this should be benchmarked first. So the scope for this project would be to benchmark and analyze the potential improvements and then agree on which format jsonb could be serialized to (apart from the current on-disk format, there are many json or nested k-v formats that could be used for sending over the wire). Seems a bit risky for a GSoC project. We don't know if a different serialization format will be a win, Over the current serialization (text) is hard to believe there will be no wins. or whether we want to do it in the end, until the benchmarking is done. It's also not clear what we're trying to achieve with the serialization format: smaller on-the-wire size, faster serialization in the server, faster parsing in the client, or what? Probably all of them (it would be ideal if it could be selectable). Some may favor small on-the-wire size (which can be significant with several serialization formats) or faster decoding (de-serialization takes a significant execution time). Of course, all this should be tested and benchmarked before, but we're not alone here. This is a significant request from many, at least from the Java users, where it has been discussed many times. Specially if wire format adheres to one well-known (or even Standard) format, so that the receiving side and the drivers could expose an API based on that format --one of the other big pains today in this side. I think it fits very well for a GSoC! :) Álvaro -- Álvaro Hernández Tortosa --- 8Kdata -- 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] postgres_fdw vs. force_parallel_mode on ppc
Robert Haas writes: >> Foreign tables are supposed to be categorically excluded from >> parallelism. Not sure why that's not working in this instance. BTW, I wonder where you think that's supposed to be enforced, because I sure can't find any such logic. I suppose that has_parallel_hazard() would be the logical place to notice foreign tables, but it currently doesn't even visit RTEs, much less contain any code to check if their tables are foreign. Or did you have another place in mind to do that? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] checkpointer continuous flushing - V18
Random updates on 16 tables which total to 1.1GB of data, so this is in buffer, no significant "read" traffic. (1) with 16 tablespaces (1 per table) on 1 disk : 680.0 tps per second avg, stddev [ min q1 median d3 max ] <=300tps 679.6 ± 750.4 [0.0, 317.0, 371.0, 438.5, 2724.0] 19.5% (2) with 1 tablespace on 1 disk : 956.0 tps per second avg, stddev [ min q1 median d3 max ] <=300tps 956.2 ± 796.5 [3.0, 488.0, 583.0, 742.0, 2774.0] 2.1% Interesting. That doesn't reflect my own tests, even on rotating media, at all. I wonder if it's related to: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=23d0127096cb91cb6d354bdc71bd88a7bae3a1d5 If you use your 12.04 kernel, that'd not be fixed. Which might be a reason to do it as you suggest. Could you share the exact details of that workload? See attached scripts (sh to create the 16 tables in the default or 16 table spaces, small sql bench script, stat computation script). The per-second stats were computed with: grep progress: pgbench.out | cut -d' ' -f4 | avg.py --length=1000 --limit=300 Host is 8 cpu 16 GB, 2 HDD in RAID 1. -- Fabien. ts_create.sh Description: Bourne shell script ts_test.sql Description: application/sql #! /usr/bin/env python # -*- coding: utf-8 -*- # # $Id: avg.py 1242 2016-02-06 14:44:02Z coelho $ # import argparse ap = argparse.ArgumentParser(description='show stats about data: count average stddev [min q1 median q3 max]...') ap.add_argument('--median', default=True, action='store_true', help='compute median and quartile values') ap.add_argument('--no-median', dest='median', default=True, action='store_false', help='do not compute median and quartile values') ap.add_argument('--more', default=False, action='store_true', help='show some more stats') ap.add_argument('--limit', type=float, default=None, help='set limit for counting below limit values') ap.add_argument('--length', type=int, default=None, help='set expected length, assume 0 if beyond') ap.add_argument('--precision', type=int, default=1, help='floating point precision') ap.add_argument('file', nargs='*', help='list of files to process') opt = ap.parse_args() # option consistency if opt.limit != None: opt.more = True if opt.more: opt.median = True # reset arguments for fileinput import sys sys.argv[1:] = opt.file import fileinput n, skipped, vals = 0, 0, [] k, vmin, vmax = None, None, None sum1, sum2 = 0.0, 0.0 for line in fileinput.input(): try: v = float(line) if opt.median: # keep track only if needed vals.append(v) if k is None: # first time k, vmin, vmax = v, v, v else: # next time vmin = min(vmin, v) vmax = max(vmax, v) n += 1 vmk = v - k sum1 += vmk sum2 += vmk * vmk except ValueError: # float conversion failed skipped += 1 if n == 0: # avoid ops on None below k, vmin, vmax = 0.0, 0.0, 0.0 if opt.length: assert "some data seen", n > 0 missing = int(opt.length) - len(vals) assert "positive number of missing data", missing >= 0 if missing > 0: print("warning: %d missing data, expanding with zeros" % missing) if opt.median: vals += [ 0.0 ] * missing vmin = min(vmin, 0.0) sum1 += - k * missing sum2 += k * k * missing n += missing assert len(vals) == int(opt.length) if opt.median: assert "consistent length", len(vals) == n # five numbers... # numpy.percentile requires numpy at least 1.9 to use 'midpoint' # statistics.median requires python 3.4 (?) def median(vals, start, length): if len(vals) == 1: start, length = 0, 1 m, odd = divmod(length, 2) #return 0.5 * (vals[start + m + odd - 1] + vals[start + m]) return vals[start + m] if odd else \ 0.5 * (vals[start + m-1] + vals[start + m]) # return ratio of below limit (limit included) values def below(vals, limit): # hmmm... short but generates a list #return float(len([ v for v in vals if v <= limit ])) / len(vals) below_limit = 0 for v in vals: if v <= limit: below_limit += 1 return float(below_limit) / len(vals) # float prettyprint with precision def f(v): return ('%.' + str(opt.precision) + 'f') % v # output if skipped: print("warning: %d lines skipped" % skipped) if n > 0: # show result (hmmm, precision is truncated...) from math import sqrt avg, stddev = k + sum1 / n, sqrt((sum2 - (sum1 * sum1) / n) / n) if opt.median: vals.sort() med = median(vals, 0, len(vals)) # not sure about odd/even issues here... q3 needs fixing if len is 1 q1 = median(vals, 0, len(vals) // 2) q3 = median(vals, (len(vals)+1) // 2, len(vals) // 2) # build summary message msg = "avg over %d: %s ± %s [%s, %s, %s, %s, %s]" % \ (n, f(avg), f(stddev), f(vmin), f(q1), f(med), f(q3), f(vmax)) if opt.more: limit = opt.limit if opt.limit != None else 0.1 * med # msg += " <=%s:" % f(limit) msg += " %s%%" % f(100.0 * below(vals, limit)) else: msg = "avg over %d: %s ± %s [%s, %s]" % \ (n, f(avg), f(stddev), f(vmin), f(vmax)) else: msg = "no data se
Re: [HACKERS] Add generate_series(date,date) and generate_series(date,date,integer)
> > > > > Given that counterexample, I think we not only shouldn't back-patch such > a > > change but should reject it altogether. > > Ouch, good point. The overflows are a different problem that we had > better address though (still on my own TODO list)... > So I should remove the bounds checking from generate_series(date,date,[int]) altogether?
Re: [HACKERS] psql metaqueries with \gexec
> > In the mean time, update patch attached. > > Really attached this time. diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 9750a5b..5ca769f 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -849,6 +849,13 @@ exec_command(const char *cmd, status = PSQL_CMD_SEND; } + /* \gexec -- send query and treat every result cell as a query to be executed */ + else if (strcmp(cmd, "gexec") == 0) + { + pset.gexec_flag = true; + status = PSQL_CMD_SEND; + } + /* \gset [prefix] -- send query and store result into variables */ else if (strcmp(cmd, "gset") == 0) { diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 2cb2e9b..54b7790 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -710,6 +710,46 @@ StoreQueryTuple(const PGresult *result) return success; } +/* + * ExecQueryTuples: assuming query result is OK, execute every query + * result as its own statement + * + * Returns true if successful, false otherwise. + */ +static bool +ExecQueryTuples(const PGresult *result) +{ + boolsuccess = true; + int nrows = PQntuples(result); + int ncolumns = PQnfields(result); + int r, c; + + for (r = 0; r < nrows; r++) + { + for (c = 0; c < ncolumns; c++) + { + if (! PQgetisnull(result, r, c)) + { + if ( ! SendQuery(PQgetvalue(result, r, c)) ) + { + if (pset.on_error_stop) + { + return false; + } + else + { + success = false; + } + } + } + } + } + + /* Return true if all queries were successful */ + return success; +} + + /* * ProcessResult: utility function for use by SendQuery() only @@ -903,8 +943,14 @@ PrintQueryResults(PGresult *results) switch (PQresultStatus(results)) { case PGRES_TUPLES_OK: - /* store or print the data ... */ - if (pset.gset_prefix) + /* execute or store or print the data ... */ + if (pset.gexec_flag) + { + /* Turn off gexec_flag to avoid infinite loop */ + pset.gexec_flag = false; + ExecQueryTuples(results); + } + else if (pset.gset_prefix) success = StoreQueryTuple(results); else success = PrintQueryTuples(results); diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index 59f6f25..251dd1e 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -173,6 +173,7 @@ slashUsage(unsigned short int pager) fprintf(output, _("General\n")); fprintf(output, _(" \\copyright show PostgreSQL usage and distribution terms\n")); fprintf(output, _(" \\g [FILE] or ; execute query (and send results to file or |pipe)\n")); + fprintf(output, _(" \\gexec execute query and treat every result cell as a query to be executed )\n")); fprintf(output, _(" \\gset [PREFIX] execute query and store results in psql variables\n")); fprintf(output, _(" \\q quit psql\n")); fprintf(output, _(" \\watch [SEC] execute query every SEC seconds\n")); diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h index 20a6470..9f1e94b 100644 --- a/src/bin/psql/settings.h +++ b/src/bin/psql/settings.h @@ -91,6 +91,9 @@ typedef struct _psqlSettings char *gfname; /* one-shot file output argument for \g */ char *gset_prefix;/* one-shot prefix argument for \gset */ + boolgexec_flag; /* true if query results are to be treated as +* queries to be executed. Set by \gexec */ + boolnotty; /* stdin or stdout is not a tty (as determined * on startup) */ enum trivalue getPassword; /* prompt the user for a username and password */ diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 5f27120..0f87f29 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-com
Re: [HACKERS] psql metaqueries with \gexec
On Mon, Feb 22, 2016 at 11:30 AM, Corey Huinker wrote: > On Mon, Feb 22, 2016 at 10:08 AM, Daniel Verite > wrote: > >> Corey Huinker wrote: >> >> > ...and query text visibility, and result visibility, and error handling, >> > etc. In this case, we're leveraging the psql environment we'd already >> set >> > up, and if there's an error, \set ECHO queries shows us the errant SQL >> as >> > if we typed it ourselves.. >> >> BTW, about error handling, shouldn't it honor ON_ERROR_STOP ? >> >> With the patch when trying this: >> >> => set ON_ERROR_STOP on >> => select * from (values ('select 1/0', 'select 1/0')) AS n \gexec >> >> it produces two errors: >> ERROR: division by zero >> ERROR: division by zero >> >> I'd rather have the execution stop immediately after the first error, >> like it's the case with successive queries entered normally via the >> query buffer: >> >> => \set ON_ERROR_STOP on >> => select 1/0; select 1/0; >> ERROR: division by zero >> >> as opposed to: >> >> => \set ON_ERROR_STOP off >> => select 1/0; select 1/0; >> ERROR: division by zero >> ERROR: division by zero >> >> > Yes, I would like it to honor ON_ERROR_STOP. I'll look into that. > > Well, that was easy enough. Turns out that pset.on_error_stop is checked in MainLoop, whereas the other pset.on_* vars are checked in SendQuery(). My original idea had been to push each cell into a in-memory temp file handle and call MainLoop() on each. Pavel suggested that temp files of any sort were a bad idea, hence using SendQuery instead. It's probably for the best. # select 'select 1,2,3', 'select 1/0', 'select 4,5,6' ... # \gexec ?column? | ?column? | ?column? --+--+-- 1 |2 |3 (1 row) Time: 0.151 ms ERROR: 22012: division by zero LOCATION: int4div, int.c:719 Time: 0.528 ms ?column? | ?column? | ?column? --+--+-- 4 |5 |6 (1 row) Time: 0.139 ms Time: 0.595 ms # \set ON_ERROR_STOP 1 # select 'select 1,2,3', 'select 1/0', 'select 4,5,6' \gexec ?column? | ?column? | ?column? --+--+-- 1 |2 |3 (1 row) Time: 0.137 ms ERROR: 22012: division by zero LOCATION: int4div, int.c:719 Time: 0.165 ms Time: 0.284 ms Does \set ON_ERROR_STOP mess up regression tests? If not, I'll add the test above (minus the \set VERBOSITY verbose-isms) to the regression. In the mean time, update patch attached.
Re: [HACKERS] psql metaqueries with \gexec
Pavel Stehule wrote: > The design of the "format" function is not closed. Try to send prototype > and patch. The possibility to do PostgreSQL customization was strong reason > why we didn't implemented "sprintf" and we implemented "format". Probably not terribly useful here, but for the DDL-deparse patch I came up with a syntax to format JSON objects, which used %-escapes; each escaped element corresponds to a string literal, or to an object. So you'd have %{table}D where the "table" element in the JSON object could be a simple string which is expanded verbatim (plus quoting if necessary), or it could be a JSON object with something like { schema => "public", name => "students" }, where each element is expanded and quoted as necessary; if the "schema" is null or it doesn't exist, it expands only the name, obviously omitting the dot. -- Á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] psql metaqueries with \gexec
On Mon, Feb 22, 2016 at 10:08 AM, Daniel Verite wrote: > Corey Huinker wrote: > > > ...and query text visibility, and result visibility, and error handling, > > etc. In this case, we're leveraging the psql environment we'd already set > > up, and if there's an error, \set ECHO queries shows us the errant SQL as > > if we typed it ourselves.. > > BTW, about error handling, shouldn't it honor ON_ERROR_STOP ? > > With the patch when trying this: > > => set ON_ERROR_STOP on > => select * from (values ('select 1/0', 'select 1/0')) AS n \gexec > > it produces two errors: > ERROR: division by zero > ERROR: division by zero > > I'd rather have the execution stop immediately after the first error, > like it's the case with successive queries entered normally via the > query buffer: > > => \set ON_ERROR_STOP on > => select 1/0; select 1/0; > ERROR: division by zero > > as opposed to: > > => \set ON_ERROR_STOP off > => select 1/0; select 1/0; > ERROR: division by zero > ERROR: division by zero > > Yes, I would like it to honor ON_ERROR_STOP. I'll look into that.
Re: [HACKERS] checkpointer continuous flushing - V18
On 2016-02-22 11:05:20 -0500, Tom Lane wrote: > Andres Freund writes: > > Interesting. That doesn't reflect my own tests, even on rotating media, > > at all. I wonder if it's related to: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=23d0127096cb91cb6d354bdc71bd88a7bae3a1d5 > > > If you use your 12.04 kernel, that'd not be fixed. Which might be a > > reason to do it as you suggest. > > Hmm ... that kernel commit is less than 4 months old. Would it be > reflected in *any* production kernels yet? Probably not - so far I though it mainly has some performance benefits on relatively extreme workloads; where without the patch, flushing still is better performancewise than not flushing. But in the scenario Fabien has brought up it seems quite possible that sync_file_range emitting "storage cache flush" instructions, could explain the rather large performance difference between his and my experiments. Regards, 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] Typo in bufmgr.c that result in waste of memory
On Sat, Feb 20, 2016 at 01:55:55PM +0530, Robert Haas wrote: > On Fri, Feb 19, 2016 at 7:20 PM, Andres Freund wrote: > > On February 19, 2016 2:42:08 PM GMT+01:00, Tom Lane > > wrote: > >>> I think we should fix it, but not backpatch. > >> > >>I don't think that's particularly good policy. It's a clear bug, why > >>would we not fix it? Leaving it as-is in the back branches can have > >>no good effect, and what it does do is create a merge hazard for other > >>back-patchable bug fixes in the same area. > > > > Agreed. > > +1. I think this is clearly a back-patchable fix. Fix applied to head and 9.5. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- 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] checkpointer continuous flushing - V18
Andres Freund writes: > Interesting. That doesn't reflect my own tests, even on rotating media, > at all. I wonder if it's related to: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=23d0127096cb91cb6d354bdc71bd88a7bae3a1d5 > If you use your 12.04 kernel, that'd not be fixed. Which might be a > reason to do it as you suggest. Hmm ... that kernel commit is less than 4 months old. Would it be reflected in *any* production kernels yet? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] checkpointer continuous flushing - V18
On 2016-02-22 14:11:05 +0100, Fabien COELHO wrote: > > >I did a quick & small test with random updates on 16 tables with > >checkpoint_flush_after=16 checkpoint_timeout=30 > > Another run with more "normal" settings and over 1000 seconds, so less > "quick & small" that the previous one. > > checkpoint_flush_after = 16 > checkpoint_timeout = 5min # default > shared_buffers = 2GB # 1/8 of available memory > > Random updates on 16 tables which total to 1.1GB of data, so this is in > buffer, no significant "read" traffic. > > (1) with 16 tablespaces (1 per table) on 1 disk : 680.0 tps > per second avg, stddev [ min q1 median d3 max ] <=300tps > 679.6 ± 750.4 [0.0, 317.0, 371.0, 438.5, 2724.0] 19.5% > > (2) with 1 tablespace on 1 disk : 956.0 tps > per second avg, stddev [ min q1 median d3 max ] <=300tps > 956.2 ± 796.5 [3.0, 488.0, 583.0, 742.0, 2774.0] 2.1% Interesting. That doesn't reflect my own tests, even on rotating media, at all. I wonder if it's related to: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=23d0127096cb91cb6d354bdc71bd88a7bae3a1d5 If you use your 12.04 kernel, that'd not be fixed. Which might be a reason to do it as you suggest. Could you share the exact details of that workload? Greetings, Andres Freund -- 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: Failover Slots
Hi, > On 16 Feb 2016, at 09:11, Craig Ringer wrote: > > > > Revision attached. There was a file missing from the patch too. > All attached patches apply normally. I only took a look at first 2, but also tried to run the Patroni with the modified version to check whether the basic replication works. What it’s doing is calling pg_basebackup first to initialize the replica, and that actually failed with: _basebackup: unexpected termination of replication stream: ERROR: requested WAL segment 0001 has already been removed The segment name definitely looks bogus to me. The actual command causing the failure was an attempt to clone the replica using pg_basebackup, turning on xlog streaming: pg_basebackup --pgdata data/postgres1 --xlog-method=stream --dbname="host=localhost port=5432 user=replicator” I checked the same command against the git master without the patches applied and could not reproduce this problem there. On the code level, I have no comments on 0001, it’s well documented and I have no questions about the approach, although I might be not too knowledgable to judge the specifics of the implementation. On the 0002, there are a few rough edges: slots.c:294 elog(LOG, "persistency is %i", (int)slot->data.persistency); Should be changed to DEBUG? slot.c:468 Why did you drop “void" as a parameter type of ReplicationSlotDropAcquired? walsender.c: 1509 at PhysicalConfirmReceivedLocation I’ve noticed a comment stating that we don’t need to call ReplicationSlotSave(), but that pre-dated the WAL-logging of replication slot changes. Don’t we need to call it now, the same way it’s done for the logical slots in logical.c:at LogicalConfirmReceivedLocation? Kind regards, -- Oleksii
Re: [HACKERS] Writing new unit tests with PostgresNode
Craig Ringer wrote: > > +=pod > > + > > +=head2 Set up a node > > pod format... Do we really want that? Considering that those modules > > are only aimed at being dedicated for in-core testing, I would say no. > > If it's plain comments you have to scan through massive piles of verbose > Perl to find what you want. If it's pod you can just perldoc > /path/to/module it and get a nice summary of the functions etc. > > If these are intended to become usable facilities for people to write tests > with then I think it's important that the docs be reasonably accessible. Yes, I think adding POD here is a good idea. I considered doing it myself back when I was messing with PostgresNode ... -- Á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] psql metaqueries with \gexec
Corey Huinker wrote: > ...and query text visibility, and result visibility, and error handling, > etc. In this case, we're leveraging the psql environment we'd already set > up, and if there's an error, \set ECHO queries shows us the errant SQL as > if we typed it ourselves.. BTW, about error handling, shouldn't it honor ON_ERROR_STOP ? With the patch when trying this: => set ON_ERROR_STOP on => select * from (values ('select 1/0', 'select 1/0')) AS n \gexec it produces two errors: ERROR: division by zero ERROR: division by zero I'd rather have the execution stop immediately after the first error, like it's the case with successive queries entered normally via the query buffer: => \set ON_ERROR_STOP on => select 1/0; select 1/0; ERROR: division by zero as opposed to: => \set ON_ERROR_STOP off => select 1/0; select 1/0; ERROR: division by zero ERROR: division by zero Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> ... However, this is one of the big problems that >> we'd have to have a solution for before we ever consider allowing >> read-write parallelism. > Having such a blocker for read-write parallelism would be unfortunate, > though perhaps there isn't much help for it, and having read-only query > parallelism is certainly far better than nothing. IIUC, this is not the only large problem standing between us and read-write parallelism, and probably not even the biggest one. So I'm basically just asking that it gets documented while it's fresh in mind, rather than leaving it for some future hackers to rediscover the hard way. (Wouldn't be bad to doc the other known stumbling blocks, too.) 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] postgres_fdw vs. force_parallel_mode on ppc
Robert Haas writes: > On Mon, Feb 22, 2016 at 11:02 AM, Thomas Munro > wrote: >> The postgres_fdw failure is a visibility-of-my-own-uncommitted-work >> problem. The first command in a transaction updates a row via an FDW, >> and then the SELECT expects to see the effects, but when run in a >> background worker it creates a new FDW connection that can't see the >> uncommitted UPDATE. > Foreign tables are supposed to be categorically excluded from > parallelism. Not sure why that's not working in this instance. I've not looked at the test case to see if this is exactly what's going wrong, but it's pretty easy to see how there might be a problem: consider a STABLE user-defined function that does a SELECT from a foreign table. If that function call gets pushed down into a parallel worker then it would fail as described. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Robert Haas writes: > > On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane wrote: > >> I just had a rather disturbing thought to the effect that this entire > >> design --- ie, parallel workers taking out locks for themselves --- is > >> fundamentally flawed. As far as I can tell from README.parallel, > >> parallel workers are supposed to exit (and, presumably, release their > >> locks) before the leader's transaction commits. Releasing locks before > >> commit is wrong. Do I need to rehearse why? > > > No, you don't. I've spent a good deal of time thinking about that problem. > > [ much snipped ] > > Unless I'm missing something, though, this is a fairly obscure > > problem. Early release of catalog locks is desirable, and locks on > > scanned tables should be the same locks (or weaker) than already held > > by the master. Other cases are rare. I think. It would be good to > > know if you think otherwise. > > After further thought, what I think about this is that it's safe so long > as parallel workers are strictly read-only. Given that, early lock > release after user table access is okay for the same reasons it's okay > after catalog accesses. However, this is one of the big problems that > we'd have to have a solution for before we ever consider allowing > read-write parallelism. Having such a blocker for read-write parallelism would be unfortunate, though perhaps there isn't much help for it, and having read-only query parallelism is certainly far better than nothing. > So what distresses me about the current situation is that this is a large > stumbling block that I don't see documented anywhere. It'd be good if you > transcribed some of this conversation into README.parallel. Agreed. > (BTW, I don't believe your statement that transferring locks back to the > master would be deadlock-prone. If the lock system treats locks held by > a lock group as effectively all belonging to one entity, then granting a > lock identical to one already held by another group member should never > fail. I concur that it might be expensive performance-wise, though it > hardly seems like this would be a dominant factor compared to all the > other costs of setting up and tearing down parallel workers.) This is only when a parallel worker is finished, no? Isn't there already some indication of when a parallel worker is done that the master handles, where it could also check the shared lock table and see if any locks were transferred to it on worker exit? Only following this thread from afar, so take my suggestions with a grain of salt. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] FDW: should GetFdwRoutine be called when drop table?
Robert Haas writes: > On Sat, Feb 20, 2016 at 1:43 AM, Tom Lane wrote: >> Yes, that's exactly the problem: you'd need some sort of atomic commit >> mechanism to make this work safely. >> >> It's possible we could give FDWs a bunch of hooks that would let them >> manage post-commit cleanup the same way smgr does, but it's a far larger >> project than it might have seemed. > I've been thinking about the idea of letting foreign data wrappers > have either (a) a relfilenode that is not zero, representing local > storage; or perhaps even (b) an array of relfilenodes. The > relfilenode, or relfilenodes, would be automatically dropped. It > seems like this would be handy for things like cstore_fdw or the > problem mentioned here, where you do want to manage local storage. Hmm, mumble. This assumes that the "FDW" is willing to keep its data in something that looks externally just like a Postgres heap file (as opposed to, say, keeping it somewhere else in the filesystem). That pretty much gives up the notion that this is "foreign" data access and instead means that you're trying to force-fit an alternate storage manager into our FDW-shaped slot. I doubt it will fit very well. For one thing, we have never supposed that FDWs were 100% responsible for managing the data they access, which is why they are not hooked up to either DROP or a boatload of other maintenance activities like VACUUM, CLUSTER, REINDEX, etc. Not to mention that an alternate storage manager might have its own maintenance activities that don't really fit any of those concepts. > If you then also had the generic XLOG patch, maybe you could make it > WAL-logged, too, if you wanted... While I've not paid close attention, I had the idea that the "generic XLOG" patches that have been discussed would still be restricted to dealing with data that fits into Postgres-style pages (because, for example, it would have to pass bufmgr's page sanity checks). That's a restriction that an alternate storage manager would likely not want. My point remains that building anything actually useful in this space is not a small task. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: > On Mon, Feb 22, 2016 at 2:56 AM, Tom Lane wrote: > !held by the indicated process. False indicates that this process is > !currently waiting to acquire this lock, which implies that at > least one other > !process is holding a conflicting lock mode on the same lockable object. > I know you're just updating existing language here, but this is false. > It only implies that one other process is holding *or waiting for* a > conflicting lock mode on the same lockable object. True. I had considered whether to fix that point as well, and decided that it might just be overcomplicating matters. But since you complain, I'll add "or waiting for". It also occurred to me last night that pg_blocking_pids() needs a disclaimer similar to the existing one for pg_locks about how using it a lot could put a performance drag on the system. Other than adjusting those points, I think this is ready to go, and will commit later today if I hear no objections. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: > On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane wrote: >> I just had a rather disturbing thought to the effect that this entire >> design --- ie, parallel workers taking out locks for themselves --- is >> fundamentally flawed. As far as I can tell from README.parallel, >> parallel workers are supposed to exit (and, presumably, release their >> locks) before the leader's transaction commits. Releasing locks before >> commit is wrong. Do I need to rehearse why? > No, you don't. I've spent a good deal of time thinking about that problem. > [ much snipped ] > Unless I'm missing something, though, this is a fairly obscure > problem. Early release of catalog locks is desirable, and locks on > scanned tables should be the same locks (or weaker) than already held > by the master. Other cases are rare. I think. It would be good to > know if you think otherwise. After further thought, what I think about this is that it's safe so long as parallel workers are strictly read-only. Given that, early lock release after user table access is okay for the same reasons it's okay after catalog accesses. However, this is one of the big problems that we'd have to have a solution for before we ever consider allowing read-write parallelism. So what distresses me about the current situation is that this is a large stumbling block that I don't see documented anywhere. It'd be good if you transcribed some of this conversation into README.parallel. (BTW, I don't believe your statement that transferring locks back to the master would be deadlock-prone. If the lock system treats locks held by a lock group as effectively all belonging to one entity, then granting a lock identical to one already held by another group member should never fail. I concur that it might be expensive performance-wise, though it hardly seems like this would be a dominant factor compared to all the other costs of setting up and tearing down parallel workers.) 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] Support for N synchronous standby servers - take 2
On Tue, Feb 16, 2016 at 4:19 PM, Masahiko Sawada wrote: > On Mon, Feb 15, 2016 at 2:54 PM, Michael Paquier > wrote: >> On Mon, Feb 15, 2016 at 2:11 PM, Kyotaro HORIGUCHI wrote: >>> Surprizingly yes. The list is handled as an identifier list and >>> parsed by SplitIdentifierString thus it can accept double-quoted >>> names. >> > > Attached latest version patch which has only feature logic so far. > I'm writing document patch about this feature now, so this version > patch doesn't have document and regression test patch. Thanks for updating the patch! When I changed s_s_names to 'hoge*' and reloaded the configuration file, the server crashed unexpectedly with the following error message. This is obviously a bug. FATAL: syntax error 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] FDW: should GetFdwRoutine be called when drop table?
On Sat, Feb 20, 2016 at 1:43 AM, Tom Lane wrote: > Andres Freund writes: >> On 2016-02-19 14:18:19 -0500, Peter Eisentraut wrote: >>> On 2/19/16 12:21 PM, Feng Tian wrote: I have an fdw that each foreign table will acquire some persisted resource. > >>> But foreign data wrappers are meant to be wrappers around data managed >>> elsewhere, not their own storage managers (although that is clearly >>> tempting), so there might well be other places where this breaks down. > >> Sounds like even a BEGIN;DROP TABLE foo;ROLLBACK; will break this >> approach. > > Yes, that's exactly the problem: you'd need some sort of atomic commit > mechanism to make this work safely. > > It's possible we could give FDWs a bunch of hooks that would let them > manage post-commit cleanup the same way smgr does, but it's a far larger > project than it might have seemed. I've been thinking about the idea of letting foreign data wrappers have either (a) a relfilenode that is not zero, representing local storage; or perhaps even (b) an array of relfilenodes. The relfilenode, or relfilenodes, would be automatically dropped. It seems like this would be handy for things like cstore_fdw or the problem mentioned here, where you do want to manage local storage. If you then also had the generic XLOG patch, maybe you could make it WAL-logged, too, if you wanted... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Mon, Feb 22, 2016 at 2:56 AM, Tom Lane wrote: > I wrote: >> Robert Haas writes: >>> As for the patch itself, I'm having trouble grokking what it's trying >>> to do. I think it might be worth having a comment defining precisely >>> what we mean by "A blocks B". I would define "A blocks B" in general >>> as either A holds a lock which conflicts with one sought by B >>> (hard-blocked) or A awaits a lock which conflicts with one sought by B >>> and precedes it in the wait queue (soft-blocked). > >> Yes, that is exactly what I implemented ... and it's something you can't >> find out from pg_locks. I'm not sure how that view could be made to >> expose wait-queue ordering. > > Here's an updated version of this patch, now with user-facing docs. > > I decided that "pg_blocking_pids()" is a better function name than > "pg_blocker_pids()". The code's otherwise the same, although I > revisited some of the comments. > > I also changed quite a few references to "transaction" into "process" > in the discussion of pg_locks. The previous choice to conflate > processes with transactions was never terribly wise in my view, and > it's certainly completely broken by parallel query. !held by the indicated process. False indicates that this process is !currently waiting to acquire this lock, which implies that at least one other !process is holding a conflicting lock mode on the same lockable object. I know you're just updating existing language here, but this is false. It only implies that one other process is holding *or waiting for* a conflicting lock mode on the same lockable object. Other than that, I think the documentation changes look good. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane wrote: > I just had a rather disturbing thought to the effect that this entire > design --- ie, parallel workers taking out locks for themselves --- is > fundamentally flawed. As far as I can tell from README.parallel, > parallel workers are supposed to exit (and, presumably, release their > locks) before the leader's transaction commits. Releasing locks before > commit is wrong. Do I need to rehearse why? No, you don't. I've spent a good deal of time thinking about that problem. In typical cases, workers are going to be acquiring either catalog locks (which are released before commit) or locks on relations which the leader has already locked (in which case the leader will still hold the lock - or possibly a stronger one - even after the worker releases that lock). Suppose, however, that you write a function which goes and queries some other table not involved in the query, and therefore acquires a lock on it. If you mark that function PARALLEL SAFE and it runs only in the worker and not in in the leader, then you could end up with a parallel query that releases the lock before commit where a non-parallel version of that query would have held the lock until transaction commit. Of course, one answer to this problem is - if the early lock release is apt to be a problem for you - don't mark such functions PARALLEL SAFE. I've thought about engineering a better solution. Two possible designs come to mind. First, we could have the worker send to the leader a list of locks that it holds at the end of its work, and the leader could acquire all of those before confirming to the worker that it is OK to terminate. That has some noteworthy disadvantages, like being prone to deadlock and requiring workers to stick around potentially quite a bit longer than they do at present, thus limiting the ability of other processes to access parallel query. Second, we could have the workers reassign all of their locks to the leader in the lock table (unless the leader already holds that lock). The problem with that is that then the leader is in the weird situation of having locks in the shared lock table that it doesn't know anything about - they don't appear in it's local lock table. How does the leader decide which resource owner they go with? Unless I'm missing something, though, this is a fairly obscure problem. Early release of catalog locks is desirable, and locks on scanned tables should be the same locks (or weaker) than already held by the master. Other cases are rare. I think. It would be good to know if you think otherwise. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Writing new unit tests with PostgresNode
On 22 February 2016 at 20:21, Michael Paquier wrote: > On Mon, Feb 22, 2016 at 7:55 PM, Craig Ringer > wrote: > > Er, the patch is attached this time. > > top_builddir = ../.. > include $(top_builddir)/src/Makefile.global > > -SUBDIRS = regress isolation modules > +SUBDIRS = regress isolation modules example_suite > > I have no problem with a test suite to be included in SUBDIRS, but if > that's a template test it would be better if added in ALWAYS_SUBDIRS. > Fair enough. My thinking in adding it to SUBDIRS is that it's trivial enough to cost practically nothing, and things that aren't run by default tend to bitrot. > I think as well that having it in src/test/perl/example_suite would > make more sense. > Yeah, it probably would. > --- /dev/null > +++ b/src/test/README > @@ -0,0 +1,37 @@ > +This directory contains a variety of test infrastructure as well as some > of the > +tests in PostgreSQL. Not all tests are here - in particular, there are > more in > +individual contrib/ modules and in src/bin. > The usual format for README files is more like that: > Can do. > > +use strict; > +use warnings; > +use 5.8.8; > +use PostgresNode; > +use TestLib; > I am -1 for including a forced version number in the list of > inclusions. We have been careful about not using routines that are not > supported with perl < 5.8.8. Let's continue like that. Simply > mentioning in the README this minimal requirement is enough IMO. > Ok, no complaint from me. > > + > +=pod > + > +=head2 Set up a node > pod format... Do we really want that? Considering that those modules > are only aimed at being dedicated for in-core testing, I would say no. > If it's plain comments you have to scan through massive piles of verbose Perl to find what you want. If it's pod you can just perldoc /path/to/module it and get a nice summary of the functions etc. If these are intended to become usable facilities for people to write tests with then I think it's important that the docs be reasonably accessible. You don't write Test::More tests by reading Test/More.pm, you read its perldoc. Same deal. SGML ghastly to write and would separate the test docs from the tests themselves, so I think it's pretty much the worst of all worlds. So. Yes. I do think pod is desirable. Plus anything but the most extremely primitive editor highlights it nicely and it's trivial to write even if, like me, you haven't really used Perl in five years. Have a look at perldoc src/test/perl/PostgresNode.pm (and imagine there was a SYNOPSIS and EXAMPLES section, which there isn't yet). I think that's a *lot* more readable than dredging through the sources, and it's only going to get more so as the functionality grows. diff --git a/src/test/mb/.gitignore b/src/test/mb/.gitignore > new file mode 100644 > index 000..6628455 > --- /dev/null > +++ b/src/test/mb/.gitignore > @@ -0,0 +1 @@ > +/results/ > This should be a separate patch. > Eh, sure. > This patch is going a bit more far than I expected first, I thought > that this would be only a set of README files added into core. Yeah, as I said I realised while writing the "how to write tests" and "how to write modules" parts that I'd basically be writing code-in-a-README that was guaranteed to bitrot into uselessness since it couldn't be run and tested automatically. A sample module with some comments/pod describing what it does is a much, much more useful way to achieve that end and the README can just refer to it. > And it > is not completely clear what we would gain by using perlpod in the > case of those in-core modules As above, readability when you're authoring tests not hacking on the test framework. I really don't like writing Perl. The last thing I want to do is read through reams of Perl to find out if TestLib or PostgresNode have some functionality I need in a test or if I need to add it / do it in my own code. I certainly don't want to be doing that repeatedly when I'm just trying to look up the signature for methods as I write tests. It might make more sense if you think of it as infrastructure that lets people write tests, not the end product its self. In an ideal world nobody would have to look at it, they'd just use it. Do you read pg_regress.c when you're writing regression tests? Nope, you read the README and some examples. Same deal. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] checkpointer continuous flushing - V18
I did a quick & small test with random updates on 16 tables with checkpoint_flush_after=16 checkpoint_timeout=30 Another run with more "normal" settings and over 1000 seconds, so less "quick & small" that the previous one. checkpoint_flush_after = 16 checkpoint_timeout = 5min # default shared_buffers = 2GB # 1/8 of available memory Random updates on 16 tables which total to 1.1GB of data, so this is in buffer, no significant "read" traffic. (1) with 16 tablespaces (1 per table) on 1 disk : 680.0 tps per second avg, stddev [ min q1 median d3 max ] <=300tps 679.6 ± 750.4 [0.0, 317.0, 371.0, 438.5, 2724.0] 19.5% (2) with 1 tablespace on 1 disk : 956.0 tps per second avg, stddev [ min q1 median d3 max ] <=300tps 956.2 ± 796.5 [3.0, 488.0, 583.0, 742.0, 2774.0] 2.1% -- Fabien. -- 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] Writing new unit tests with PostgresNode
On Mon, Feb 22, 2016 at 7:55 PM, Craig Ringer wrote: > Er, the patch is attached this time. top_builddir = ../.. include $(top_builddir)/src/Makefile.global -SUBDIRS = regress isolation modules +SUBDIRS = regress isolation modules example_suite I have no problem with a test suite to be included in SUBDIRS, but if that's a template test it would be better if added in ALWAYS_SUBDIRS. I think as well that having it in src/test/perl/example_suite would make more sense. --- /dev/null +++ b/src/test/README @@ -0,0 +1,37 @@ +This directory contains a variety of test infrastructure as well as some of the +tests in PostgreSQL. Not all tests are here - in particular, there are more in +individual contrib/ modules and in src/bin. The usual format for README files is more like that: path/to/README Nice title Text, blah. I think that it would be better to stick with the common style in place. Except that this file is definitely a good addition. +use strict; +use warnings; +use 5.8.8; +use PostgresNode; +use TestLib; I am -1 for including a forced version number in the list of inclusions. We have been careful about not using routines that are not supported with perl < 5.8.8. Let's continue like that. Simply mentioning in the README this minimal requirement is enough IMO. + +=pod + +=head2 Set up a node pod format... Do we really want that? Considering that those modules are only aimed at being dedicated for in-core testing, I would say no. diff --git a/src/test/mb/.gitignore b/src/test/mb/.gitignore new file mode 100644 index 000..6628455 --- /dev/null +++ b/src/test/mb/.gitignore @@ -0,0 +1 @@ +/results/ This should be a separate patch. --- /dev/null +++ b/src/test/modules/README @@ -0,0 +1,10 @@ +src/test/modules contains PostgreSQL extensions that are primarily or entirely +intended for testing PostgreSQL and/or to serve as example code. The extensions +here aren't intended to be installed in a production server and aren't useful +for "real work". Header format should be reworked here as well. This patch is going a bit more far than I expected first, I thought that this would be only a set of README files added into core. And it is not completely clear what we would gain by using perlpod in the case of those in-core modules. -- 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] Optimization for updating foreign tables in Postgres FDW
I did another round of review for the latest patch and well as performed the sanity test, and haven't found any functional issues. Found couple of issue, see in-line comments for the same. On Thu, Feb 18, 2016 at 3:15 PM, Etsuro Fujita wrote: > On 2016/02/12 21:19, Etsuro Fujita wrote: > >> Comments on specific points follow. >>> >>> This seems to need minor rebasing in the wake of the join pushdown patch. >>> >> > Will do. >> > > Done. > > + /* Likewise for ForeignScan that has pushed down >>> INSERT/UPDATE/DELETE */ >>> + if (IsA(plan, ForeignScan) && >>> + (((ForeignScan *) plan)->operation == CMD_INSERT || >>> +((ForeignScan *) plan)->operation == CMD_UPDATE || >>> +((ForeignScan *) plan)->operation == CMD_DELETE)) >>> + return; >>> >>> I don't really understand why this is a good idea. Since target lists >>> are only displayed with EXPLAIN (VERBOSE), I don't really understand >>> what is to be gained by suppressing them in any case at all, and >>> therefore I don't understand why it's a good idea to do it here, >>> either. If there is a good reason, maybe the comment should explain >>> what it is. >>> >> > I think that displaying target lists would be confusing for users. >> > > There seems no objection from you (or anyone), I left that as proposed, > and added more comments. > > + /* Check point 1 */ >>> + if (operation == CMD_INSERT) >>> + return false; >>> + >>> + /* Check point 2 */ >>> + if (nodeTag(subplan) != T_ForeignScan) >>> + return false; >>> + >>> + /* Check point 3 */ >>> + if (subplan->qual != NIL) >>> + return false; >>> + >>> + /* Check point 4 */ >>> + if (operation == CMD_UPDATE) >>> >>> These comments are referring to something in the function header >>> further up, but you could instead just delete the stuff from the >>> header and mention the actual conditions here. >>> >> > Will fix. >> > > Done. > > The patch doesn't allow the postgres_fdw to push down an UPDATE/DELETE on > a foreign join, so I added one more condition here not to handle such > cases. (I'm planning to propose a patch to handle such cases, in the next > CF.) > I think we should place the checking foreign join condition before the target columns, as foreign join condition is less costly then the target columns. > > - If the first condition is supposed accept only CMD_UPDATE or >>> CMD_DELETE, say if (operation != CMD_UPDATE || operation != >>> CMD_DELETE) rather than doing it this way. Is-not-insert may in this >>> context be functionally equivalent to is-update-or-delete, but it's >>> better to write the comment and the code so that they exactly match >>> rather than kinda-match. >>> >>> - For point 2, use IsA(subplan, ForiegnScan). >>> >> > Will fix. >> > > Done. > > + /* >>> +* ignore subplan if the FDW pushes down the >>> command to the remote >>> +* server >>> +*/ >>> >>> This comment states what the code does, instead of explaining why it >>> does it. Please update it so that it explains why it does it. >>> >> > Will update. >> > > Done. > > + List *fdwPushdowns; /* per-target-table FDW >>> pushdown flags */ >>> >>> Isn't a list of booleans an awfully inefficient representation? How >>> about a Bitmapset? >>> >> > OK, will do. >> > > Done. > > + /* >>> +* Prepare remote-parameter expressions for evaluation. >>> (Note: in >>> +* practice, we expect that all these expressions will be just >>> Params, so >>> +* we could possibly do something more efficient than using >>> the full >>> +* expression-eval machinery for this. But probably there >>> would be little >>> +* benefit, and it'd require postgres_fdw to know more than is >>> desirable >>> +* about Param evaluation.) >>> +*/ >>> + dpstate->param_exprs = (List *) >>> + ExecInitExpr((Expr *) fsplan->fdw_exprs, >>> +(PlanState *) node); >>> >>> This is an exact copy of an existing piece of code and its associated >>> comment. A good bit of the surrounding code is copied, too. You need >>> to refactor to avoid duplication, like by putting some of the code in >>> a new function that both postgresBeginForeignScan and >>> postgresBeginForeignModify can call. >>> >>> execute_dml_stmt() has some of the same disease. >>> >> > Will do. >> > > Done. > > Other changes: > > * I fixed docs as discussed before with Rushabh Lathia and Thom Brown. > > * I keep Rushabh's code change that we call PlanDMLPushdown only when all > the required APIs are available with FDW, but for CheckValidResultRel, I > left the code as-is (no changes to that function), to match the docs saying > that the FDW needs to provide the DML pushdown callback functions together > with existing table
Re: [HACKERS] postgres_fdw vs. force_parallel_mode on ppc
On Mon, Feb 22, 2016 at 11:02 AM, Thomas Munro wrote: > On Tue, Feb 16, 2016 at 12:12 PM, Noah Misch wrote: >> On Mon, Feb 15, 2016 at 06:07:48PM -0500, Tom Lane wrote: >>> Noah Misch writes: >>> > I configured a copy of animal "mandrill" that way and launched a test run. >>> > The postgres_fdw suite failed as attached. A manual "make -C contrib >>> > installcheck" fails the same way on a ppc64 GNU/Linux box, but it passes >>> > on >>> > x86_64 and aarch64. Since contrib test suites don't recognize >>> > TEMP_CONFIG, >>> > check-world passes everywhere. >>> >>> Hm, is this with or without the ppc-related atomics fix you just found? >> >> Without those. The ppc64 GNU/Linux configuration used gcc, though, and the >> atomics change affects xlC only. Also, the postgres_fdw behavior does not >> appear probabilistic; it failed twenty times in a row. > > The postgres_fdw failure is a visibility-of-my-own-uncommitted-work > problem. The first command in a transaction updates a row via an FDW, > and then the SELECT expects to see the effects, but when run in a > background worker it creates a new FDW connection that can't see the > uncommitted UPDATE. > > I wonder if parallelism of queries involving an FDW should not be > allowed if your transaction has written through the FDW. Foreign tables are supposed to be categorically excluded from parallelism. Not sure why that's not working in this instance. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Writing new unit tests with PostgresNode
Er, the patch is attached this time. On 22 February 2016 at 18:54, Craig Ringer wrote: > On 22 February 2016 at 15:41, Michael Paquier > wrote: > > >> >> >> > Sound about right? I can tidy that up a bit and turn it into a README >> >> > and >> >> > add a reference to that to the public tap docs to tell users where >> to go >> >> > if >> >> > they want to write more tests. >> >> >> >> Yes, please. >> > >> > Will do that now. >> >> This is definitely independent from the efforts of the other patches. >> > > Done. > > I got a bit carried away and added: > > - src/test/perl/README > - src/test/README > - src/test/modules/README > - POD for src/test/perl/PostgresNode.pm > - $node->info() function (that just returns what dump_info() printed, but > as a string > - $node->backup(...) support for passing -R to pg_basebackup > - $node->backup(...) support for passing -X stream to pg_basebackup > - src/test/example_suite/ with some simple demo tests > > I found that I was writing documentation for how to write tests that'd > bitrot quickly and landed up writing a demo/sample test that can be run as > part of 'make check' with --enable-tap-tests instead. Hopefully it's not > overkill. > > LMK if you think it's too much and I can trim out what's unwanted. > > In the process I noticed a few helpers I think should be added to > PostgresNode. I haven't added them since I didn't want this to turn into a > big patch when it was meant to just be 'add a README', but the main things > are: > > - promote (you've done this) > > - Facilities to make it easier to set a master up as replication-enabled > (your patch does this, but it should use wal_level = 'logical' by default > IMO; also I think setting autovacuum=off is very wrong and will mask > problems) > > - wait_for_recovery_lsn($lsn) to wait until a hot standby passes a given > LSN > > - wait_for_replication_lsn($lsn, $col, $appname) - wait until standby with > name $appname (or any standby, if unspecified) passes $lsn for $col, where > $col can be 'sent', 'write', 'flush' or 'replay' > > > >> > Not committed yet, I see. That's >> https://commitfest.postgresql.org/9/438/ >> > right? >> >> Yeah... That's life. >> >> > I'll comment separately on that thread. > > -- > Craig Ringer http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services > -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services From 0271314446293a61457f0b8e6a77d784804bae43 Mon Sep 17 00:00:00 2001 From: Craig Ringer Date: Mon, 22 Feb 2016 18:54:24 +0800 Subject: [PATCH] Add README and example to document how to create TAP tests --- src/test/Makefile | 2 +- src/test/README | 37 src/test/example_suite/.gitignore | 1 + src/test/example_suite/Makefile | 9 + src/test/example_suite/t/001_minimal.pl | 157 ++ src/test/mb/.gitignore | 1 + src/test/modules/README | 10 + src/test/perl/PostgresNode.pm | 374 +--- src/test/perl/README| 37 9 files changed, 596 insertions(+), 32 deletions(-) create mode 100644 src/test/README create mode 100644 src/test/example_suite/.gitignore create mode 100644 src/test/example_suite/Makefile create mode 100644 src/test/example_suite/t/001_minimal.pl create mode 100644 src/test/mb/.gitignore create mode 100644 src/test/modules/README create mode 100644 src/test/perl/README diff --git a/src/test/Makefile b/src/test/Makefile index b713c2c..e9b0136 100644 --- a/src/test/Makefile +++ b/src/test/Makefile @@ -12,7 +12,7 @@ subdir = src/test top_builddir = ../.. include $(top_builddir)/src/Makefile.global -SUBDIRS = regress isolation modules +SUBDIRS = regress isolation modules example_suite # We don't build or execute examples/, locale/, or thread/ by default, # but we do want "make clean" etc to recurse into them. Likewise for ssl/, diff --git a/src/test/README b/src/test/README new file mode 100644 index 000..485f20b --- /dev/null +++ b/src/test/README @@ -0,0 +1,37 @@ +This directory contains a variety of test infrastructure as well as some of the +tests in PostgreSQL. Not all tests are here - in particular, there are more in +individual contrib/ modules and in src/bin. + +Not all these tests get run by "make check". Check src/test/Makefile to see +which tests get run automatically. + +examples/ + demo programs for libpq that double as regression tests via "make check" + +isolation/ + tests for concurrent behaviours at the SQL level + +locale/ + sanity checks for locale data, encodings, etc + +mb/ + tests for multibyte encoding (UTF-8) support + +modules/ + extensions used only or mainly for test purposes, generally not useful + or suitable for installing in production databases. Some of these have + their own tests, some are used by tests elsewhere.
Re: [HACKERS] Writing new unit tests with PostgresNode
On 22 February 2016 at 15:41, Michael Paquier wrote: > > >> > Sound about right? I can tidy that up a bit and turn it into a README > >> > and > >> > add a reference to that to the public tap docs to tell users where to > go > >> > if > >> > they want to write more tests. > >> > >> Yes, please. > > > > Will do that now. > > This is definitely independent from the efforts of the other patches. > Done. I got a bit carried away and added: - src/test/perl/README - src/test/README - src/test/modules/README - POD for src/test/perl/PostgresNode.pm - $node->info() function (that just returns what dump_info() printed, but as a string - $node->backup(...) support for passing -R to pg_basebackup - $node->backup(...) support for passing -X stream to pg_basebackup - src/test/example_suite/ with some simple demo tests I found that I was writing documentation for how to write tests that'd bitrot quickly and landed up writing a demo/sample test that can be run as part of 'make check' with --enable-tap-tests instead. Hopefully it's not overkill. LMK if you think it's too much and I can trim out what's unwanted. In the process I noticed a few helpers I think should be added to PostgresNode. I haven't added them since I didn't want this to turn into a big patch when it was meant to just be 'add a README', but the main things are: - promote (you've done this) - Facilities to make it easier to set a master up as replication-enabled (your patch does this, but it should use wal_level = 'logical' by default IMO; also I think setting autovacuum=off is very wrong and will mask problems) - wait_for_recovery_lsn($lsn) to wait until a hot standby passes a given LSN - wait_for_replication_lsn($lsn, $col, $appname) - wait until standby with name $appname (or any standby, if unspecified) passes $lsn for $col, where $col can be 'sent', 'write', 'flush' or 'replay' > > Not committed yet, I see. That's > https://commitfest.postgresql.org/9/438/ > > right? > > Yeah... That's life. > > I'll comment separately on that thread. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] psql metaqueries with \gexec
> > FWIW, I also wish we had something better than format() for this stuff. I > did create [1] towards that end, but it currently depends on some C code, > which is cumbersome. For the most party, I'm pretty thrilled with format(), though: - I'll admit to being grumpy about the %1$s notation, but I have no better suggestion. - I'd also like it if there were a %I variant that accepted schema qualified names and %I-ed both, though I see the inability to tell the difference between a schema dot and a really-named-that dot. - I'd love it if there were a %C format that took a pg_class oid and formatted the qualified schema name. As it is i just use %s and cast the parameter as ::regclass The design of the "format" function is not closed. Try to send prototype and patch. The possibility to do PostgreSQL customization was strong reason why we didn't implemented "sprintf" and we implemented "format". Regards Pavel
Re: [HACKERS] Relaxing SSL key permission checks
Re: Tom Lane 2016-02-22 <21507.1456099...@sss.pgh.pa.us> > Stephen Frost writes: > > Just to be clear, I'm not really against this patch as-is, but it > > shouldn't be a precedent or limit us from supporting more permissive > > permissions in other areas (or even here) if there are sensible > > use-cases for more permissive permissions. > > OK, and to be clear, I'm not against considering other use-cases and > trying to do something appropriate for them. I just reject the idea > that it's unnecessary or inappropriate for us to be concerned about > whether secret-holding files are secure. I added the patch to the CF: https://commitfest.postgresql.org/9/532/ (I put it under "System administration" and not under "Security" because it concerns operation.) Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- 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] checkpointer continuous flushing - V18
Hallo Andres, AFAICR I used a "flush context" for each table space in some version I submitted, because I do think that this whole writeback logic really does make sense *per table space*, which suggest that there should be as many write backs contexts as table spaces, otherwise the positive effect may going to be totally lost of tables spaces are used. Any thoughts? Leads to less regular IO, because if your tablespaces are evenly sized (somewhat common) you'll sometimes end up issuing sync_file_range's shortly after each other. For latency outside checkpoints it's important to control the total amount of dirty buffers, and that's obviously independent of tablespaces. I did a quick & small test with random updates on 16 tables with checkpoint_flush_after=16 checkpoint_timeout=30 (1) with 16 tablespaces (1 per table, but same disk) : tps = 1100, 27% time under 100 tps (2) with 1 tablespace : tps = 1200, 3% time under 100 tps This result is logical: with one writeback context shared between tablespaces the sync_file_range is issued on a few buffers per file at a time on the 16 files, no coalescing occurs there, so this result in random IOs, while with one table space all writes are aggregated per file. ISTM that this quick test shows that a writeback context are relevant per tablespace, as I expected. -- Fabien. -- 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: SCRAM authentication
On Mon, Feb 15, 2016 at 11:05 AM, Michael Paquier wrote: > On Mon, Feb 15, 2016 at 10:51 AM, Stephen Frost wrote: >> * Tom Lane (t...@sss.pgh.pa.us) wrote: >>> Stephen Frost writes: >>> > Why do we need pg_shadow or pg_user or related views at all..? >>> >>> A lot of code looks at those just to get usernames. I am not in favor of >>> breaking such stuff without need. >> >> Alright. >> >>> How about we just say that the password in these old views always reads >>> out as '' even when there is a password, and we invent new views >>> that carry real auth information? (Hopefully in an extensible way.) >> >> I'd be alright with that approach, I'd just rather that any clients >> which actually want to read the password field be updated to look at the >> extensible and sensible base catalogs, and not some hacked up array that >> we shoved into that field. > > Well, then let's mask it, and just have pg_auth_verifiers. Another > possible problem that I can see with this patch is what do we do with > valid_until? The last set of patches sent did not switch this field to > be per-verifier settable. I would consider a saner approach to keep > things simple and still do that. Allowing multiple verifiers per > protocol is a problem, and having a solution for it would be nice. > Should this be prioritized before having more protocols like SCRAM? > > FWIW, browsing through pgbouncer, it has a look at pg_shadow for > user's password to build a basic configuration file. > > (My mistake, while pg_user is world-readable, that's not the case of > pg_shadow). FWIW, I am going to create a new thread once I am done with the set of patches I have in mind for the upcoming CF (yes there will be refreshed patches), because this thread has moved on a bit larger discussion than SCRAM itself, summarizing what is more or less the conclusion of this thread, explaining what the patches are doing, what they are not doing, what could be done afterwards, etc, etc. I'll keep a clear scope regarding what I am aiming at. -- 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] psql metaqueries with \gexec
> > I like what you've proposed, though I am wondering if you considered doing > something server-side instead? It seems a shame to do all this work and > exclude all other tools. > I have, but my solutions closely mirror the one you mention in the next paragraph. > I frequently find myself creating a function that is just a wrapper on > EXECUTE for this purpose, but obviously that has transactional limitations. > ...and query text visibility, and result visibility, and error handling, etc. In this case, we're leveraging the psql environment we'd already set up, and if there's an error, \set ECHO queries shows us the errant SQL as if we typed it ourselves.. > > FWIW, I also wish we had something better than format() for this stuff. I > did create [1] towards that end, but it currently depends on some C code, > which is cumbersome. For the most party, I'm pretty thrilled with format(), though: - I'll admit to being grumpy about the %1$s notation, but I have no better suggestion. - I'd also like it if there were a %I variant that accepted schema qualified names and %I-ed both, though I see the inability to tell the difference between a schema dot and a really-named-that dot. - I'd love it if there were a %C format that took a pg_class oid and formatted the qualified schema name. As it is i just use %s and cast the parameter as ::regclass > > [1] > https://github.com/decibel/trunklet-format/blob/master/doc/trunklet-format.asc That's intense. I'll ask you about that in an off-list thread.