Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])
On Fri, Aug 30, 2013 at 8:24 PM, Tom Lane t...@sss.pgh.pa.us wrote: Stephen Frost sfr...@snowman.net writes: * Robert Haas (robertmh...@gmail.com) wrote: I think you're getting way too hung up on the fact that the proposed auto.conf will be stored as a flat file. From your comments upthread, I gather that you'd be rejoicing if it were a table. I'd be happy if it was a table which managed an *independent* set of parameters from those used to bootstrap the system, but no one seems to like breaking up the options between things that can be sanely modified without other OS changes and things which require OS support. I agree with Robert's comments upthread that if the new facility can't do everything that can be done today by editing postgresql.conf, it's not going to be adequate. So I'm not in favor of having two sets of parameters. It's also not clear to me that we can make a reliable distinction between parameters that can prevent a server restart vs those that can't; or at least, the set of the latter will be much smaller than one could wish. Now as we have an agreement, I had updated patch for below left issues: 1. ALTER SYSTEM SET should be constrained to only set known GUCs, this point has been discussed on another mail thread (http://www.postgresql.org/message-id/14857.1378523...@sss.pgh.pa.us) In function AlterSystemSetConfigFile(), when we try to get the record using find_option(), pass second parameter as false which will make sure if the parameter doesn't exist it will return NULL. 2. Some indentation issues. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com alter_system_v8.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
[HACKERS] appendStringInfo vs appendStringInfoString
I did some benchmarking earlier in the week for the new patch which was just commited to allow formatting in the log_line_prefix string. In version 0.4 of the patch there was some performance regression as I was doing appendStringInfo(buf, %*s, padding, variable); instead of appendStringInfoString(buf, variable); This regression was fixed in a later version of the patch by only using appendStringInfo when the padding was 0. More details here: http://www.postgresql.org/message-id/CAApHDvreSGYvtXJvqHcXZL8_tXiKKiFXhQyXgqtnQ5Yo=me...@mail.gmail.com Today I started looking through the entire source tree to look for places where appendStringInfo() could be replaced by appendStringInfoString(), I now have a patch which is around 100 KB in size which changes a large number of these instances. Example: diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3107f9c..d0dea4f 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -6174,7 +6174,7 @@ flatten_set_variable_args(const char *name, List *args) A_Const*con; if (l != list_head(args)) - appendStringInfo(buf, , ); + appendStringInfoString(buf, , ); I know lots of these places are not really in performance critical parts of the code, so it's likely not too big a performance gain in most places, though to be honest I didn't pay a great deal of attention to how hot the code path might be when I was editing. I thought I would post here just to test the water on this type of change. I personally don't think it makes the code any less readable, but if performance is not going to be greatly improved then perhaps people would have objections to code churn. I didn't run any benchmarks on the core code, but I did pull out all the stringinfo functions and write my own test application. I also did a trial on a new macro which could further improve performance when the string being appended in a string constant, although I just wrote this to gather opinions about the idea. It is not currently a part of the patch. In my benchmarks I was just appending a 8 char string constant 1000 times onto a stringinfo, I did this 3000 times in my tests. The results were as follows: Results: 1. appendStringInfoString in 0.404000 sec 2. appendStringInfo with %s in 1.118000 sec 3 .appendStringInfo in 1.30 sec 4. appendStringInfoStringConst with in 0.221000 sec You can see that appendStringInfoString() is far faster than appendStringInfo() this will be down to appendStringInfo using va_args whereas appendStringInfoString() just does a strlen() then memcpy(). Test 4 bypasses the strlen() by using sizeof() which of course can only be used with string constants. Actual code: 1. appendStringInfoString(str, test1234); 2. appendStringInfo(str, %s, test1234); 3. appendStringInfo(str, test1234); 4. appendStringInfoStringConst(str, test1234); The macro for test 4 was as follows: #define appendStringInfoStringConst(buf, s) appendBinaryStringInfo(buf, (s), sizeof(s)-1) I should say at this point that I ran these benchmarks on windows 7 64bit, though my tests for the log_line_prefix patch were all on Linux and saw a similar slowdown on appendStringInfo() vs appendStringInfoString(). So let this be the thread to gather opinions on if my 100kb patch which replaces appendStringInfo with appendStringInfoString where possible would be welcomed by the community. Also would using appendStringInfoStringConst() be going 1 step too far? Regards David Rowley
Re: [HACKERS] appendStringInfo vs appendStringInfoString
On Sat, Sep 28, 2013 at 9:44 PM, David Rowley dgrowle...@gmail.com wrote: I did some benchmarking earlier in the week for the new patch which was just commited to allow formatting in the log_line_prefix string. In version 0.4 of the patch there was some performance regression as I was doing appendStringInfo(buf, %*s, padding, variable); instead of appendStringInfoString(buf, variable); This regression was fixed in a later version of the patch by only using appendStringInfo when the padding was 0. More details here: http://www.postgresql.org/message-id/CAApHDvreSGYvtXJvqHcXZL8_tXiKKiFXhQyXgqtnQ5Yo=me...@mail.gmail.com Today I started looking through the entire source tree to look for places where appendStringInfo() could be replaced by appendStringInfoString(), I now have a patch which is around 100 KB in size which changes a large number of these instances. ... Also on making the changes I noticed a possible small bug in the code that could cause a crash if for some reason a translation contained a %s. I know it is an unlikely scenario, never-the-less here is a patch to fix it. diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 562a7c9..91da50b 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -372,7 +372,7 @@ incompatible_module_error(const char *libname, } if (details.len == 0) - appendStringInfo(details, + appendStringInfoString(details, _(Magic block has unexpected length or padding difference.)); David
Re: [HACKERS] plpgsql.print_strict_params
Hi Sorry for the delay on following up on this. 2013/9/18 Marko Tiikkaja ma...@joh.to: Hi, Attached is a patch with the following changes: On 16/09/2013 10:57, I wrote: On 9/16/13 8:04 AM, Ian Lawrence Barwick wrote: However the sample function provided in the documentation throws a runtime error due to a missing FROM-clause entry. Ugh. I'll look into fixing that. Fixed. Confirmed :) This lineis in exec_get_query_params() and exec_get_dynquery_params(): return (no parameters); Presumably the message will escape translation and this line should be better written as: return _((no parameters)); Nice catch. Will look into this. Another option would be to simply omit the DETAIL line if there were no parameters. At this very moment I'm thinking that might be a bit nicer. Decided to remove the DETAIL line in these cases. Yes, on reflection I think that makes most sense. Also, if the offending query parameter contains a single quote, the output is not escaped: postgres=# select get_userid(E'foo'''); Error: query returned more than one row DETAIL: p1 = 'foo'' CONTEXT: PL/pgSQL function get_userid(text) line 9 at SQL statement Not sure if that's a real problem though. Hmm.. I should probably look at what happens when the parameters to a prepared statement are currently logged and imitate that. Yup, they're escaped. Did just that. Also copied the parameters: prefix on the DETAIL line from there. The output looks like this now: postgres=# select get_userid(E'foo'''); ERROR: query returned more than one row DETAIL: parameters: $1 = 'foo''' CONTEXT: PL/pgSQL function get_userid(text) line 6 at SQL statement which looks fine to me. The functions added in pl_exec.c - exec_get_query_params() and exec_get_dynquery_params() do strike me as potentially misnamed, as they don't actually execute anything but are more utility functions for generating formatted output. Maybe format_query_params() etc. would be better? Agreed, they could use some renaming. I went with format_expr_params and format_preparedparamsdata. Thanks. format_preparedparamsdata is a bit hard to scan, but that's just nitpicking on my part ;) * Are the comments sufficient and accurate? exec_get_query_params() and exec_get_dynquery_params() could do with some brief comments explaining their purpose. Agreed. Still agreeing with both of us, added a comment each. Thanks. I also added the new keywords to the unreserved_keywords production, as per the instructions near the beginning of pl_gram.y. Good catch. The patch looks good to me now; does the status need to be changed to Ready for Committer? Regards Ian Barwick -- 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] appendStringInfo vs appendStringInfoString
On 28.09.2013 12:44, David Rowley wrote: The macro for test 4 was as follows: #define appendStringInfoStringConst(buf, s) appendBinaryStringInfo(buf, (s), sizeof(s)-1) If that makes any difference in practice, I wonder if we should just do: #define appendStringInfoString(buf, s) appendBinaryStringInfo(buf, (s), strlen(s)) With a compiler worth its salt, the strlen(s) will be optimized into a constant, if s is a constant. If it's not a constant, we'll just end up calling strlen(), like appendStringInfoString would anyway. That would turn a single function call into two in all of the non-constant callsites, though, bloating the code, so it might not be a win overall. - Heikki -- 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] appendStringInfo vs appendStringInfoString
On Sat, Sep 28, 2013 at 11:11 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 28.09.2013 12:44, David Rowley wrote: The macro for test 4 was as follows: #define appendStringInfoStringConst(**buf, s) appendBinaryStringInfo(buf, (s), sizeof(s)-1) If that makes any difference in practice, I wonder if we should just do: #define appendStringInfoString(buf, s) appendBinaryStringInfo(buf, (s), strlen(s)) With a compiler worth its salt, the strlen(s) will be optimized into a constant, if s is a constant. If it's not a constant, we'll just end up calling strlen(), like appendStringInfoString would anyway. That would turn a single function call into two in all of the non-constant callsites, though, bloating the code, so it might not be a win overall. Nice idea. I quick test shows that this works with the MS compiler I'm using on windows. appendStringInfoString in 0.249000 sec --- appendStringInfo with %s in 1.135000 sec appendStringInfo in 1.295000 sec appendStringInfoStringConst with in 0.245000 sec Regards David - Heikki
Re: [HACKERS] appendStringInfo vs appendStringInfoString
On 2013-09-28 14:11:29 +0300, Heikki Linnakangas wrote: On 28.09.2013 12:44, David Rowley wrote: The macro for test 4 was as follows: #define appendStringInfoStringConst(buf, s) appendBinaryStringInfo(buf, (s), sizeof(s)-1) If that makes any difference in practice, I wonder if we should just do: #define appendStringInfoString(buf, s) appendBinaryStringInfo(buf, (s), strlen(s)) Doesn't that have a bit too much of an multiple evaluation danger? Maybe make it a static inline in the header instead for the platforms that support it? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.print_strict_params
On 2013-09-28 12:31, Ian Lawrence Barwick wrote: The patch looks good to me now; does the status need to be changed to Ready for Committer? Yes. Thanks for reviewing! Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] information schema parameter_default implementation
On 15 September 2013 01:35, Peter Eisentraut pete...@gmx.net wrote: Here is an updated patch which fixes the bug you have pointed out. On Thu, 2013-01-31 at 18:59 +0500, Ali Dar wrote: I checked our your patch. There seems to be an issue when we have OUT parameters after the DEFAULT values. Fixed. Some other minor observations: 1) Some variables are not lined in pg_get_function_arg_default(). Are you referring to indentation issues? I think the indentation is good, but pgindent will fix it anyway. I find only proc variable not aligned. Adding one more tab for all the variables should help. 2) I found the following check a bit confusing, maybe you can make it better if (!argmodes || argmodes[i] == PROARGMODE_IN || argmodes[i] == PROARGMODE_INOUT || argmodes[i] == PROARGMODE_VARIADIC) Factored that out into a separate helper function. The statement needs to be split into 80 columns width lines. 2) inputargn can be assigned in declaration. I'd prefer to initialize it close to where it is used. Me too. 3) Function level comment for pg_get_function_arg_default() is missing. I think the purpose of the function is clear. Unless a reader goes through the definition, it is not obvious whether the second function argument represents the parameter position in input parameters or it is the parameter position in *all* the function parameters (i.e. proargtypes or proallargtypes). I think at least a one-liner description of what this function does should be present. 4) You can also add comments inside the function, for example the comment for the line: nth = inputargn - 1 - (proc-pronargs - proc-pronargdefaults); Suggestion? Yes, it is difficult to understand what's the logic behind this calculation, until we go read the documentation related to pg_proc.proargdefaults. I think this should be sufficient: /* * proargdefaults elements always correspond to the last N input arguments, * where N = pronargdefaults. So calculate the nth_default accordingly. */ 5) I think the line added in the documentation(informational_schema.sgml) is very long. Consider revising. Maybe change from: The default expression of the parameter, or null if none or if the function is not owned by a currently enabled role. TO The default expression of the parameter, or null if none was specified. It will also be null if the function is not owned by a currently enabled role. I don't know what do you exactly mean by: function is not owned by a currently enabled role? I think this style is used throughout the documentation of the information schema. We need to keep the descriptions reasonably compact, but I'm willing to entertain other opinions. This requires first an answer to my earlier question about why the role-related privilege is needed. --- There should be an initial check to see if nth_arg is passed a negative value. It is used as an index into the argmodes array, so a -ve array index can cause a crash. Because pg_get_function_arg_default() can now be called by a user also, we need to validate the input values. I am ok with returning with an error Invalid argument. --- Instead of : deparse_expression_pretty(node, NIL, false, false, 0, 0) you can use : deparse_expression(node, NIL, false, false) We are anyway not using pretty printing. --- Other than this, I have no more issues. --- After the final review is over, the catversion.h requires the appropriate date value. -- 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] logical changeset generation v6.1
On 09/27/2013 11:44 AM, Andres Freund wrote: I'm encountering a make error: Gah. Lastminute changes. Always the same... Updated patch attached. Greetings, Andres Freund I'm still encountering an error in the make. make clean . .make[3]: Entering directory `/usr/local/src/postgresql/src/bin/pg_basebackup' rm -f pg_basebackup pg_receivexlog pg_recvlogical(X) \ pg_basebackup.o pg_receivexlog.o pg_recvlogical.o \ receivelog.o streamutil.o /bin/sh: 1: Syntax error: ( unexpected make[3]: *** [clean] Error 2 I had to add a quotes in to the clean commands to make it work -- 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] logical changeset generation v6
On 09/27/2013 05:18 PM, Andres Freund wrote: Hi Steve, On 2013-09-27 17:06:59 -0400, Steve Singer wrote: I've determined that when in this test the walsender seems to be hitting this when it is decode the transactions that are behind the slonik commands to add tables to replication (set add table, set add sequence). This is before the SUBSCRIBE SET is submitted. I've also noticed something else that is strange (but might be unrelated). If I stop my slon process and restart it I get messages like: WARNING: Starting logical replication from 0/a9321360 ERROR: cannot stream from 0/A9321360, minimum is 0/A9320B00 Where 0/A9321360 was sent in the last packet my slon received from the walsender before the restart. Uh, that looks like I fumbled some comparison. Let me check. I've further narrowed this down to something (or the combination of) what the _disorder_replica.altertableaddTriggers(1); stored function does. (or @SLONYNAMESPACE@.altertableaddTriggers(int); Which is essentially * Get an exclusive lock on sl_config_lock * Get an exclusive lock on the user table in question * create a trigger (the deny access trigger) * create a truncate trigger * create a deny truncate trigger I am not yet able to replicate the error by issuing the same SQL commands from psql, but I must be missing something. I can replicate this when just using the test_decoding plugin. Thanks. That should get me started with debugging. Unless it's possibly fixed in the latest version, one bug fixed there might cause something like this if the moon stands exactly right? The latest version has NOT fixed the problem. Also, I was a bit inaccurate in my previous descriptions. To clarify: 1. I sometimes am getting that 'unexpected duplicate' error 2. The 'set add table ' which triggers those functions that create and configure triggers is actually causing the walsender to hit the following assertion 2 0x00773d47 in ExceptionalCondition ( conditionName=conditionName@entry=0x8cf400 !(ent-cmin == change-tuplecid.cmin), errorType=errorType@entry=0x7ab830 FailedAssertion, fileName=fileName@entry=0x8cecc3 reorderbuffer.c, lineNumber=lineNumber@entry=1162) at assert.c:54 #3 0x00665480 in ReorderBufferBuildTupleCidHash (txn=0x1b6e610, rb=optimized out) at reorderbuffer.c:1162 #4 ReorderBufferCommit (rb=0x1b6e4f8, xid=optimized out, commit_lsn=3461001952, end_lsn=optimized out) at reorderbuffer.c:1285 #5 0x0065f0f7 in DecodeCommit (xid=optimized out, nsubxacts=optimized out, sub_xids=optimized out, ninval_msgs=16, msgs=0x1b637c0, buf=0x7fff54d01530, buf=0x7fff54d01530, ctx=0x1adb928, ctx=0x1adb928) at decode.c:477 I had added an assert(false) to the code where the 'unknown duplicate' error was logged to make spotting this easier but yesterday I didn't double check that I was hitting the assertion I added versus this other one. I can't yet say if this is two unrelated issues or if I'd get to the 'unknown duplicate' message immediately after. 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] record identical operator - Review
Kevin Grittner kgri...@ymail.com wrote: Bruce Momjian br...@momjian.us wrote: I think we need to see a patch from Kevin that shows all the row comparisons documented and we can see the impact of the user-visibile part. First draft attached. I'm inclined to first submit a proposed documentation patch for the existing record operators, and see if we can make everyone happy with that, and *then* see about adding documentation for the new ones. Trying to deal with both at once is likely to increase confusion and wheel-spinning. When I took a stab at it, the new operators only seemed to merit a single paragraph, so that is included after all. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company*** a/doc/src/sgml/func.sgml --- b/doc/src/sgml/func.sgml *** *** 12739,12745 WHERE EXISTS (SELECT 1 FROM tab2 WHERE col2 = tab1.col2); para See xref linkend=row-wise-comparison for details about the meaning !of a row-wise comparison. /para /sect2 --- 12739,12745 para See xref linkend=row-wise-comparison for details about the meaning !of a row constructor comparison. /para /sect2 *** *** 12795,12806 WHERE EXISTS (SELECT 1 FROM tab2 WHERE col2 = tab1.col2); para See xref linkend=row-wise-comparison for details about the meaning !of a row-wise comparison. /para /sect2 sect2 !titleRow-wise Comparison/title indexterm zone=functions-subquery primarycomparison/primary --- 12795,12806 para See xref linkend=row-wise-comparison for details about the meaning !of a row constructor comparison. /para /sect2 sect2 !titleSingle-row Comparison/title indexterm zone=functions-subquery primarycomparison/primary *** *** 12823,12829 WHERE EXISTS (SELECT 1 FROM tab2 WHERE col2 = tab1.col2); para See xref linkend=row-wise-comparison for details about the meaning !of a row-wise comparison. /para /sect2 /sect1 --- 12823,12829 para See xref linkend=row-wise-comparison for details about the meaning !of a row constructor comparison. /para /sect2 /sect1 *** *** 12853,12864 WHERE EXISTS (SELECT 1 FROM tab2 WHERE col2 = tab1.col2); /indexterm indexterm primaryrow-wise comparison/primary /indexterm indexterm primarycomparison/primary !secondaryrow-wise/secondary /indexterm indexterm --- 12853,12874 /indexterm indexterm +primarycomposite type/primary +secondarycomparison/secondary + /indexterm + + indexterm primaryrow-wise comparison/primary /indexterm indexterm primarycomparison/primary !secondarycomposite type/secondary ! /indexterm ! ! indexterm !primarycomparison/primary !secondaryrow constructor/secondary /indexterm indexterm *** *** 13023,13029 AND /sect2 sect2 id=row-wise-comparison !titleRow-wise Comparison/title synopsis replaceablerow_constructor/replaceable replaceableoperator/replaceable replaceablerow_constructor/replaceable --- 13033,13039 /sect2 sect2 id=row-wise-comparison !titleRow Constructor Comparison/title synopsis replaceablerow_constructor/replaceable replaceableoperator/replaceable replaceablerow_constructor/replaceable *** *** 13033,13052 AND Each side is a row constructor, as described in xref linkend=sql-syntax-row-constructors. The two row values must have the same number of fields. !Each side is evaluated and they are compared row-wise. Row comparisons !are allowed when the replaceableoperator/replaceable is literal=/, literallt;gt;/, literallt;/, literallt;=/, literalgt;/ or !literalgt;=/, !or has semantics similar to one of these. (To be specific, an operator !can be a row comparison operator if it is a member of a B-tree operator !class, or is the negator of the literal=/ member of a B-tree operator !class.) /para para The literal=/ and literallt;gt;/ cases work slightly differently from the others. Two rows are considered --- 13043,13067 Each side is a row constructor, as described in xref linkend=sql-syntax-row-constructors. The two row values must have the same number of fields. !Each side is evaluated and they are compared row-wise. Row constructor !comparisons are allowed when the replaceableoperator/replaceable is literal=/, literallt;gt;/, literallt;/, literallt;=/, literalgt;/ or !literalgt;=/. !Every row element must be of a type which has a default B-tree operator !class or the attempted comparison may generate an error. /para + note +para + Errors related to the number or types of elements might not
Re: [HACKERS] logical changeset generation v6.1
Steve Singer wrote: I'm still encountering an error in the make. make clean . .make[3]: Entering directory `/usr/local/src/postgresql/src/bin/pg_basebackup' rm -f pg_basebackup pg_receivexlog pg_recvlogical(X) \ pg_basebackup.o pg_receivexlog.o pg_recvlogical.o \ receivelog.o streamutil.o /bin/sh: 1: Syntax error: ( unexpected make[3]: *** [clean] Error 2 I had to add a quotes in to the clean commands to make it work The proper fix is to add a $ to the pg_recvlogical(X) in clean -- should be $(X) There's another bug in the Makefile: the install target is installing recvlogical$(X) as receivellog$(X). -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
