Re: [HACKERS] Suggestion: Issue warning when calling SET TRANSACTION outside transaction block
On Wed, Sep 25, 2013 at 2:55 AM, Bruce Momjian wrote: > On Thu, Sep 12, 2013 at 09:38:43AM +0530, Amit Kapila wrote: >> > I have created the attached patch which issues an error when SET >> > TRANSACTION and SET LOCAL are used outside of transactions: >> > >> > test=> set transaction isolation level serializable; >> > ERROR: SET TRANSACTION can only be used in transaction blocks >> > test=> reset transaction isolation level; >> > ERROR: RESET TRANSACTION can only be used in transaction blocks >> > >> > test=> set local effective_cache_size = '3MB'; >> > ERROR: SET LOCAL can only be used in transaction blocks >> > test=> set local effective_cache_size = default; >> > ERROR: SET LOCAL can only be used in transaction blocks >> >> Shouldn't we do it for Set Constraints as well? > > Oh, very good point. I missed that one. Updated patch attached. 1. The function set_config also needs similar functionality, else there will be inconsistency, the SQL statement will give error but equivalent function set_config() will succeed. SQL Command postgres=# set local search_path='public'; ERROR: SET LOCAL can only be used in transaction blocks Function postgres=# select set_config('search_path', 'public', true); set_config public (1 row) 2. I think we should update the documentation as well. For example: The documentation of LOCK TABLE (http://www.postgresql.org/docs/devel/static/sql-lock.html) clearly indicates that it will give error if used outside transaction block. "LOCK TABLE is useless outside a transaction block: the lock would remain held only to the completion of the statement. Therefore PostgreSQL reports an error if LOCK is used outside a transaction block. Use BEGINand COMMIT (or ROLLBACK) to define a transaction block." The documentation of SET TRANSACTION (http://www.postgresql.org/docs/devel/static/sql-set-transaction.html) has below line which doesn't indicate that it will give error if used outside transaction block. "If SET TRANSACTION is executed without a prior START TRANSACTION or BEGIN, it will appear to have no effect, since the transaction will immediately end." 3. void ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel) { .. .. else if (strcmp(stmt->name, "TRANSACTION SNAPSHOT") == 0) { A_Const*con = (A_Const *) linitial(stmt->args); RequireTransactionChain(isTopLevel, "SET TRANSACTION"); if (stmt->is_local) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("SET LOCAL TRANSACTION SNAPSHOT is not implemented"))); .. } .. .. } Wouldn't it be better if call to RequireTransactionChain() is done after if (stmt->is_local)? With Regards, Amit Kapila. EnterpriseDB: 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] 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
Re: [HACKERS] record identical operator - Review
Kevin Grittner wrote: > Bruce Momjian 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); See for details about the meaning !of a row-wise comparison. --- 12739,12745 See for details about the meaning !of a row constructor comparison. *** *** 12795,12806 WHERE EXISTS (SELECT 1 FROM tab2 WHERE col2 = tab1.col2); See for details about the meaning !of a row-wise comparison. !Row-wise Comparison comparison --- 12795,12806 See for details about the meaning !of a row constructor comparison. !Single-row Comparison comparison *** *** 12823,12829 WHERE EXISTS (SELECT 1 FROM tab2 WHERE col2 = tab1.col2); See for details about the meaning !of a row-wise comparison. --- 12823,12829 See for details about the meaning !of a row constructor comparison. *** *** 12853,12864 WHERE EXISTS (SELECT 1 FROM tab2 WHERE col2 = tab1.col2); row-wise comparison comparison !row-wise --- 12853,12874 +composite type +comparison + + + row-wise comparison comparison !composite type ! ! ! !comparison !row constructor *** *** 13023,13029 AND !Row-wise Comparison row_constructor operator row_constructor --- 13033,13039 !Row Constructor Comparison row_constructor operator row_constructor *** *** 13033,13052 AND Each side is a row constructor, as described in . 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 operator is =, <>, <, <=, > or !>=, !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 = member of a B-tree operator !class.) The = and <> cases work slightly differently from the others. Two rows are considered --- 13043,13067 Each side is a row constructor, as described in . 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 operator is =, <>, <, <=, > or !>=. !Every row element must be of a type which has a default B-tree operator !class or the attempted comparison may generate an error. + + + Errors related to the number or types of elements might not occur if + the comparison is resolved using earlier columns. + + + The = and <> cases work slightly differently from the others. Two rows are considered *** *** 13104,13123 AND be either true or false, never null. ! ! ! The SQL specification requires row-wise comparison to return NULL if the ! result depends on comparing two NULL values or a NULL and a non-NULL. ! PostgreSQL does this only when comparing the ! results of two row constructors or comparing a row constructor to the ! output of a subquery (as in ). ! In other contexts where two composite-type values are compared, two ! NULL field values are considered equal, and a NULL is considered larger ! than a non-NULL. This is necessary in order to have consistent sorting ! and indexing behavior for composite types. ! ! --- 13119,13182 be either true or false, never null. ! + +Composite Type Comparison + + + record operator record + + + +The SQL specification requires row-wise comparison to return NULL if the +result depends on comparing two NULL values or a NULL and a non-NULL. +PostgreSQL does this only when comparing the +results of two row constructors (a
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=) at reorderbuffer.c:1162 #4 ReorderBufferCommit (rb=0x1b6e4f8, xid=, commit_lsn=3461001952, end_lsn=) at reorderbuffer.c:1285 #5 0x0065f0f7 in DecodeCommit (xid=, nsubxacts=, sub_xids=, 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] 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
[HACKERS] information schema parameter_default implementation
On 15 September 2013 01:35, Peter Eisentraut 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] 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
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] 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 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] plpgsql.print_strict_params
Hi Sorry for the delay on following up on this. 2013/9/18 Marko Tiikkaja : > 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 Sat, Sep 28, 2013 at 9:44 PM, David Rowley 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
[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: 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 wrote: > Stephen Frost 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