Re: [HACKERS] SQL/JSON in PostgreSQL
On 2017-02-28 20:08, Oleg Bartunov wrote: > Attached patch is an implementation of SQL/JSON data model from SQL-2016 > standard (ISO/IEC 9075-2:2016(E)) I've faintly started looking into this. > We created repository for reviewing (ask for write access) - > https://github.com/postgrespro/sqljson/tree/sqljson > Examples of usage can be found in src/test/regress/sql/sql_json.sql > The whole documentation about json support should be reorganized and added, > and we plan to do this before release. We need help of community here. > The standard describes SQL/JSON path language, which used by SQL/JSON query > operators to query JSON. It defines path language as string literal. We > implemented the path language as JSONPATH data type, since other > approaches are not friendly to planner and executor. I was a bit sad to discover that I can't PREPARE jsq AS SELECT JSON_QUERY('{}', $1); I assume because of this part of the updated grammar: json_path_specification: Sconst { $$ = $1; } ; Would it make sense, fundamentally, to allow variables there? After Andrew Gierth's analysis of this grammar problem, I understand that it's not reasonable to expect JSON_TABLE() to support variable jsonpaths, but maybe it would be feasible for everything else? From Andrew's changes to the new grammar (see attached) it seems to me that at least that part is possible. Or should I forget about trying to implement the other part? diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index adfe9b1..f459996 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -624,6 +624,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_table_plan_cross json_table_plan_primary json_table_default_plan + json_path_specification %type json_arguments json_passing_clause_opt @@ -635,8 +636,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type json_returning_clause_opt -%type json_path_specification - json_table_column_path_specification_clause_opt +%type json_table_column_path_specification_clause_opt json_table_path_name json_as_path_name_clause_opt @@ -845,6 +845,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); */ %nonassoc UNBOUNDED /* ideally should have same precedence as IDENT */ %nonassoc ERROR_P EMPTY_P DEFAULT ABSENT /* JSON error/empty behavior */ +%nonassoc COLUMNS FALSE_P KEEP OMIT PASSING TRUE_P UNKNOWN %nonassoc IDENT GENERATED NULL_P PARTITION RANGE ROWS PRECEDING FOLLOWING CUBE ROLLUP %left Op OPERATOR /* multi-character ops and user-defined operators */ %left '+' '-' @@ -14472,7 +14473,7 @@ json_context_item: ; json_path_specification: - Sconst { $$ = $1; } + a_expr { $$ = $1; } ; json_as_path_name_clause_opt: @@ -14802,7 +14803,7 @@ json_table_formatted_column_definition: ; json_table_nested_columns: - NESTED path_opt json_path_specification + NESTED path_opt Sconst json_as_path_name_clause_opt json_table_columns_clause { -- 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] recovery_target_time = 'now' is not an error but still impractical setting
On 2017-08-18 20:51, Robert Haas wrote: > On Fri, Aug 18, 2017 at 4:39 AM, Piotr Stefaniak > wrote: >> On 2017-08-17 11:24, Simon Riggs wrote: >>> Your suggestion of "furthest" is already the default behaviour. >>> >>> Why are you using 'now'? Why would you want to pick a randomly >>> selected end time? >> >> The idea in both cases was to stop the recovery in order to prevent the >> standby from selecting new timeline. I want to be able to continue the >> recovery from future WAL files. "Furthest" really meant "as far as >> possible without completing recovery". > > Can you use recovery_target_action='shutdown' instead? The thing I've tried was a combination of recovery_target_action = 'shutdown' and recovery_target_time = 'now'. Do you mean recovery_target_action = 'shutdown' and everything else unset (default)? That leads to the standby selecting new timeline anyway. Adding standby_mode = on prevents that, but then there is no shutdown. Am I missing something? The only way I've managed to get recovery_target_action = 'shutdown' to work as I needed was to follow advice by Matthijs and Christoph to combine recovery_target_action with either setting recovery_target_time to "now" spelled in the usual, non-special way or setting recovery_target_xid to the latest transaction ID pg_xlogdump could find. You could also try recovery_target_lsn, but I don't have that in 9.4. I don't like that line of thought though, so for now I'll use the restore_command hack I mentioned in the first message of this thread. -- 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] recovery_target_time = 'now' is not an error but still impractical setting
On 2017-08-17 11:24, Simon Riggs wrote: > Your suggestion of "furthest" is already the default behaviour. > > Why are you using 'now'? Why would you want to pick a randomly > selected end time? The idea in both cases was to stop the recovery in order to prevent the standby from selecting new timeline. I want to be able to continue the recovery from future WAL files. "Furthest" really meant "as far as possible without completing recovery". -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] recovery_target_time = 'now' is not an error but still impractical setting
First I'll describe my setup just to give you some context. If anyone would like to discuss my ideas or propose their own ideas for discussion, let's do so on -ADMIN or -GENERAL. I have multiple production database clusters which I want to make backups of. Restoring from plain dumps takes too long, so I made an almost typical continuous archiving setup. The unusual assumption in this case is that the standbys are all on a single machine and they are not always running. There are multiple $PGDATA directories on the backups machine, but only one postmaster running in standby mode, replaying archived WAL files from each master. When it's finished replaying them for one $PGDATA, it'll move to another. That way they all will be sufficiently up to date while not requiring resources needed for N replicas running all the time on a single machine. This of course requires that the standbys are never promoted, never change the timeline, etc. - they need to be able to keep replaying WAL files from the masters. I've achieved what I wanted essentially by setting standby_mode = on and restore_command = 'cp /archivedir/%f "%p" || { pg_ctl -D . stop && false ; }', but I was looking for a more elegant solution. Which brings us to the topic. One thing I tried was a combination of recovery_target_action = 'shutdown' and recovery_target_time = 'now'. The result is surprising, because then the standby tries to set the target to year 2000. That's because recovery_target_time depends on timestamptz_in(), the result of which can depend on GetCurrentTransactionStartTimestamp(). But at that point there isn't any transaction yet. Which is why I'm getting "starting point-in-time recovery to 2000-01-01 01:00:00+01". At the very least, I think timestamptz_in() should either complain about being called outside of transaction or return the expected value, because returning year 2000 is unuseful at best. I would also like to become able to do what I'm doing in a less hacky way (assuming there isn't one already but I may be wrong), perhaps once there is a new 'furthest' setting for recovery_target or when recovery_target_time = 'now' works as I expected. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] The error message "sorry, too many clients already" is imprecise
I recently started getting the "sorry, too many clients already" error. There are currently four places that can generate it, but fortunately log_error_verbosity was set to verbose so I was able to see that in this case the warning was generated by proc.c:InitProcess(). But it's still not much, because there are three different lists you can run out of and get the same error message from InitProcess(). I was lucky to be able to rule out two of them. max_connections is set to much more than the sum of possible connections from all relevant pgBouncer instances we have, so it's not hitting max_connections. Also, this is on 9.4 and we don't use any funny extensions, so it's probably not running out of bgworkerFreeProcs either. autovacFreeProcs is what's left but this is still just a guess and I'd prefer to actually know. I've made a hack for myself (attached diff against 9.4) which adds a DETAIL-level message telling me which proc list was saturated. It's not committable in its current form because of a C99 feature and perhaps for other reasons. By the way, I've also noticed that the InitProcess() can segfault upon hitting set_spins_per_delay(procglobal->spins_per_delay). This only happens when I run REL9_4_STABLE under gdb, set a breakpoint on InitProcess, see an autovacuum launcher hit the breakpoint and tell gdb to continue. "p procglobal->spins_per_delay" says "Cannot access memory at address 0xf01b2f90". Maybe this means nothing. diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index e608198..613b36a 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -279,6 +279,14 @@ InitProcess(void) { /* use volatile pointer to prevent code rearrangement */ volatile PROC_HDR *procglobal = ProcGlobal; +#define LIST_TYPE_ENTRY(e) [(e)] = (#e) + enum listType { autovac, bgworker, backend } which; + char const * const listTypeNames[] = { + LIST_TYPE_ENTRY(autovac), + LIST_TYPE_ENTRY(bgworker), + LIST_TYPE_ENTRY(backend) + }; +#undef LIST_TYPE_ENTRY /* * ProcGlobal should be set up already (if we are a backend, we inherit @@ -309,11 +317,11 @@ InitProcess(void) set_spins_per_delay(procglobal->spins_per_delay); if (IsAnyAutoVacuumProcess()) - MyProc = procglobal->autovacFreeProcs; + which = autovac, MyProc = procglobal->autovacFreeProcs; else if (IsBackgroundWorker) - MyProc = procglobal->bgworkerFreeProcs; + which = bgworker, MyProc = procglobal->bgworkerFreeProcs; else - MyProc = procglobal->freeProcs; + which = backend, MyProc = procglobal->freeProcs; if (MyProc != NULL) { @@ -330,13 +338,13 @@ InitProcess(void) /* * If we reach here, all the PGPROCs are in use. This is one of the * possible places to detect "too many backends", so give the standard - * error message. XXX do we need to give a different failure message - * in the autovacuum case? + * error message. */ SpinLockRelease(ProcStructLock); ereport(FATAL, (errcode(ERRCODE_TOO_MANY_CONNECTIONS), - errmsg("sorry, too many clients already"))); + errmsg("sorry, too many clients already"), + errdetail("%s proc list saturated", listTypeNames[which]))); } MyPgXact = &ProcGlobal->allPgXact[MyProc->pgprocno]; -- 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] Preliminary results for proposed new pgindent implementation
On 2017-06-17 21:55, Tom Lane wrote: > I spent some time looking into this. I reverted your commits > 198457848ae5c86bec3336a9437dd5aa30f480c2 (Replace err.h functions with > standard C equivalents) and fb10acb040b90bdcbad09defd303363db29257d1 > (Remove inclusion of sys/cdefs.h) locally and tried to build without > those. I wanted to mirror that move, but forgot to not rebase the repository, so I removed those two commits instead of committing their negatives. Sorry about that. > I've successfully worked around the err.h change by adding > cut-down versions of FreeBSD 11's err.h and err.c to the fileset > (see attached). I thought about something like: #ifdef __FreeBSD__ #include #define ERR(...) err(__VA_ARGS__) #define ERRX(...) errx(__VA_ARGS__) #else #include "err.h" #endif and then call ERR() and ERRX() instead of err() and errx(). But that requires C99. And I would have a very hard time convincing anyone that it makes any sense from FreeBSD's perspective, since indent is part of the base system, where is guaranteed to exist. Perhaps it would be best for everyone if indent was moved out of FreeBSD base, so that portability arguments would make more sense. But that would take time and some debate. > However, it's proving impossible to work around having > "#include " as the first live code in the files. I thought > maybe we could provide a dummy cdefs.h file, but that breaks things on > platforms where cdefs.h is a real thing and is relied on by other system > headers --- which includes both Linux and BSD. It seems we would have > to have something like #ifdef HAVE_SYS_CDEFS_H, but that is already a > departure from FreeBSD practice. I was thinking if I could get away with putting those into #ifdef __FreeBSD__ ... #endif. I think that it might be feasible unlike the idea above. I could be wrong. > So what I'm currently thinking is that we have to diverge from the > FreeBSD sources to the extent of removing #include > and the __FBSDID() calls, and instead inserting #include "c.h" to > pick up PG's own portability definitions. The thing that forced me > into the latter is that there seems no way to avoid compiler warnings > if we don't decorate the declarations of err() and errx() with noreturn > and printf-format attributes --- and we need c.h to provide portable > ways of writing those. But there are probably other portability things > that we'll need c.h for, anyway, especially if we want to make it work > on Windows. So I'm thinking this is a small and easily maintainable > difference from the upstream FreeBSD files. That works for me. > When I inserted #include "c.h", I got duplicate-macro-definition warnings > about "true" and "false", so I would ask you to add this: Done. -- 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] Preliminary results for proposed new pgindent implementation
On 2017-06-17 00:02, Tom Lane wrote: > Piotr Stefaniak writes: >> On 2017-06-16 21:56, Tom Lane wrote: >>> Unless Piotr objects, I propose to add another switch to bsdindent >>> that selects this behavior, and then we can drop entab, removing >>> another impediment to getting pgindent working. > >> I understand the reasoning, but this is a very specific need and I think >> not at all universal for anyone else in the future. One of the bugs >> listed in indent's manpage is that it "has more switches than ls(1)". So >> currently I'm against pushing an option for the above upstream, to the >> FreeBSD repository. > >> Why not add this to the already non-empty list of custom patches? > > Umm ... I thought the idea was to get to the point where the list of > custom patches *is* empty. Except for carrying our own Makefile of > course. I'd be sad if we needed a fork just for this. > > What I'm testing with right now has just four differences from your repo: There are also the "portability fixes" and they're the main problem. I've simply removed things like capsicum or __FBSDID() because I thought it wouldn't be a problem since Postgres will have its own copy of indent anyway (so that its behavior is not a moving target). I can ifdef-out them instead of removing entirely, I just didn't think it was important anymore. I expect to be in trouble for replacing err() and errx(), though. > 1. This workaround for what I believe you agree is a bug: > > - ps.in_decl = ps.decl_on_line = ps.last_token != type_def; > + ps.in_decl = ps.decl_on_line = true; That will need a proper fix... > 2. The long-lines adjustment I just sent you a patch for. That looks very good. > 3. The tab-vs-space difference under discussion here. I can be convinced to make it another option upstream. But I dislike it nevertheless. > 4. A temporary hack affecting the indentation of comments on the same line > (forcing them to a multiple of 8 spaces even though tabsize is 4). I have > every intention of dropping that one later; I just don't want to deal with > comment reindentation at the same time as these other things. Great! -- 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] Preliminary results for proposed new pgindent implementation
On 2017-06-16 20:11, Tom Lane wrote: > Andres Freund writes: >> On 2017-06-16 13:44:30 -0400, Bruce Momjian wrote: >>> Yes, it is all about <80 column output. The current pgindent does >>> everything possible to accomplish that --- the question is whether we >>> want uglier code to do it. > >> For me personally the misindentation is way uglier than a too long line. > > I assume though that Piotr wants an option to preserve that behavior. > I'm happy to write up a patch for bsdindent that adds a switch > controlling this, but is there any rhyme or reason to the way its > switches are named? I don't want to preserve the current behavior at all, but I might need to add an option for choosing one or the other if users of FreeBSD indent protest. I don't have a good name for it. The best I can do is -lpl ("-lp long lines too"). Can I see the patch? -- 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] Preliminary results for proposed new pgindent implementation
On 2017-06-16 21:56, Tom Lane wrote: > One other thing I'd like to do while we're changing this stuff is > to get rid of the need for entab/detab. Right now, after doing > all the other work, my copy of pgindent is running the code through > detab and then entab so as to match the old decisions about how to > represent whitespace (ie, as spaces or tabs). This is grotty as > can be. I managed to tweak bsdindent so that its output matches > what entab would do, by dint of the attached patch, which implements > the rule "use a space instead of a tab if the tab would only move > one column and we don't need another tab after it". (I think entab > is being weird with the second half of that rule, but if I remove it, > I get circa a thousand lines of invisible whitespace changes; probably > better not to deal with those. With no patch at all, just letting > bsdindent do what it does now, there's circa ten thousand changed lines.) > > Unless Piotr objects, I propose to add another switch to bsdindent > that selects this behavior, and then we can drop entab, removing > another impediment to getting pgindent working. I understand the reasoning, but this is a very specific need and I think not at all universal for anyone else in the future. One of the bugs listed in indent's manpage is that it "has more switches than ls(1)". So currently I'm against pushing an option for the above upstream, to the FreeBSD repository. Why not add this to the already non-empty list of custom patches? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-06-14 17:05, Bruce Momjian wrote: > On Wed, Jun 14, 2017 at 10:38:40AM -0400, Tom Lane wrote: >> btw, I was slightly amused to notice that this version still calls itself >> >> $ indent -V >> pg_bsd_indent 1.3 >> >> Don't know what you plan to do with that program name, but we certainly >> need a version number bump so that pgindent can tell that it's got an >> up-to-date copy. 1.4? 2.0? > > For Piotr's reference, we will update src/tools/pgindent/pgindent to > match whatever new version number you use. I would like to go a bit further than that. I see that GNU indent doesn't recognize -V, but prints its version if you use the option --version. I wish to implement the same option for FreeBSD indent, but that would force a change in how pgindent recognizes indent. As for the version bump, I briefly considered "9.7.0", but 2.0 seems more appropriate as the 2 would reflect the fundamental changes that I've done and the .0 would indicate that it's still rough around edges. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-06-14 19:31, Tom Lane wrote: > Does that test case pass for you? No, I broke it recently. Sorry. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-06-13 22:23, Tom Lane wrote: > I could not find any places where reverting this change made the > results worse, so I'm unclear on why you made it. I must admit I'm a bit confused about why it's not fixed yet, but I'll have to analyze that later this week. But the idea was to convince indent that the following is not a declaration and therefore it shouldn't be formatted as such: typedef void (*voidptr) (int *); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-06-13 18:22, Tom Lane wrote: > The Makefile is still BSD-ish of course, but I think > we'll just agree to disagree there. For compiling indent under Linux I use bmake(1). I have no problem with including a Makefile for GNU Make in my repository. As I understand it, there will be a copy of indent maintained by the Postgres group - even less of a problem to include whatever you want, it seems to me. I think that Postgres will have to fork FreeBSD indent anyway, because of nitems(), capsicum, __FBSDID(), etc. Now I only aim for the shortest diff output. > The only thing I could find to > quibble about is that on old macOS versions I get > > In file included from indent.c:49: > indent_globs.h:222:1: warning: "STACKSIZE" redefined > In file included from /usr/include/machine/param.h:30, > from /usr/include/sys/param.h:104, > from indent.c:42: > /usr/include/ppc/param.h:53:1: warning: this is the location of the previous > definition > > Maybe you could rename that symbol to IND_STACKSIZE or some such? I just replaced it with integer constants and nitems(). > Also, I am wondering about the test cases under tests/. I do not > see anything in the Makefile or elsewhere suggesting how those are > to be used. It would sure be nice to have some quick smoke-test > to check that a build on a new platform is working. They'd started out like David Holland's tests for his tradcpp(1), with a similar makefile (again, BSD make). But I was tenaciously asked to use Kyua (a testing framework that is the standard regression test mechanism for FreeBSD) instead, so now the makefile's existence and use is a great secret and the file is not under any source control. Adaption of the indent test suite to Kyua made the makefile more inelegant, but I'm attaching it to this email in hope that you can do something useful with it. I can only guess that you have the option to use Kyua instead, but I don't know the tool at all. INDENT_OBJDIR= ${.OBJDIR:H} INDENT= ${INDENT_OBJDIR}/indent TESTS+= binary TESTS+= comments TESTS+= cppelsecom TESTS+= declarations TESTS+= elsecomment TESTS+= f_decls TESTS+= float TESTS+= label TESTS+= list_head TESTS+= nsac TESTS+= offsetof TESTS+= parens TESTS+= sac TESTS+= struct TESTS+= surplusbad TESTS+= types_from_file TESTS+= wchar all: run-tests .WAIT show-diffs $(INDENT): ${MAKE} -C ${INDENT_OBJDIR} .for T in $(TESTS) run-tests: $(T).diff $(T).diff: $(T).run $(T).0.stdout $(INDENT) -diff -u $(T).0.stdout $(T).run > $(T).diff $(T).run: $(INDENT) $(T).0 $(INDENT) $(T).0 $(T).run -P$(T).pro 2>&1 || echo FAILED >> $(T).run .endfor show-diffs: @echo '*** Test diffs ***' .for T in $(TESTS) @cat $(T).diff .endfor clean: .for T in $(TESTS) rm -f $(T).run $(T).diff $(T).o $(T).plist .endfor good: .for T in $(TESTS) cp $(T).run $(T).0.stdout .endfor .PHONY: all run-tests show-diffs clean good -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
>>> * the comments get formatted differently for -ts4 than -ts8 Still haven't put any thought into it, so I still don't know what to do here. >>> * extra spacing getting inserted for fairly long labels I think the fix is as easy as not producing the space. I committed that. >>> * some enum declarations get the phony extra indentation >>> * surplus indentation for SH_ELEMENT_TYPE * data; I think this is now fixed. >>> * comments on the same line are now run together Indent has worked like for a long time; current pg_bsd_indent does that as well. It was now uncovered by my removing this line from pgindent: # Add tab before comments with no whitespace before them (on a tab stop) $source =~ s!(\S)(/\*.*\*/)$!$1\t$2!gm; > There's also the portability issues: __FBSDID() and bcopy() and > [and err.h]. I think that's fixed as well. I've never been too excited to improve indent and it's increasingly challenging for me to force myself to work on it now, after I've invested so much of my spare time into it. So please bear with me if there are any errors. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-05-22 01:50, Tom Lane wrote: > Being lazy, I just wiped my copy and re-cloned, but it still seems the > same as before ... last commit on the pass3 branch is from Mar 4. > What branch should I be paying attention to? Sorry for the trouble, this is because I interactively git-rebased it in order to rewrite history. I do this to limit the number of commits to the FreeBSD repository. Next time I'll leave fix-ups in chronological order and meld them with what they fix just before committing to the FreeBSD repository. pass3 is the right branch. A fresh clone should have worked as in the attached log. me@t520 /tmp> git clone https://github.com/pstef/freebsd_indent Cloning into 'freebsd_indent'... remote: Counting objects: 640, done. remote: Compressing objects: 100% (55/55), done. remote: Total 640 (delta 128), reused 151 (delta 116), pack-reused 469 Receiving objects: 100% (640/640), 270.61 KiB | 66.00 KiB/s, done. Resolving deltas: 100% (409/409), done. Checking connectivity... done. me@t520 /tmp> cd freebsd_indent me@t520 /t/freebsd_indent> bmake CC=clang CFLAGS='-D__FBSDID="static char *rcsid=" -g -O0' clang -D__FBSDID="static char *rcsid=" -g -O0-c indent.c clang -D__FBSDID="static char *rcsid=" -g -O0-c io.c clang -D__FBSDID="static char *rcsid=" -g -O0-c lexi.c clang -D__FBSDID="static char *rcsid=" -g -O0-c parse.c clang -D__FBSDID="static char *rcsid=" -g -O0-c pr_comment.c clang -D__FBSDID="static char *rcsid=" -g -O0-c args.c clang -o indent indent.o io.o lexi.o parse.o pr_comment.o args.o nroff -man indent.1 > indent.cat1 me@t520 /t/freebsd_indent> cp ~/postgres/freebsd_indent/test.c . me@t520 /t/freebsd_indent> ./indent -bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb -cli1 -U/home/me/pgindent-test/git/src/tools/pgindent/typedefs.list < test.c typedef struct GinBtreeData { /* search methods */ BlockNumber (*findChildPage) (GinBtree, GinBtreeStack *); BlockNumber (*getLeftMostChild) (GinBtree, Page); bool(*isMoveRight) (GinBtree, Page); bool(*findItem) (GinBtree, GinBtreeStack *); /* insert methods */ OffsetNumber (*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber); GinPlaceToPageRC (*beginPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void **, Page *, Page *); void(*execPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void *); } void hardclock_device_poll(void) { const unsigned (*TABLE_index)[2]; if (stat(pg_data, &statbuf) != 0) { /* * The Linux Standard Base Core Specification 3.1 says this should * return '4, program or service status is unknown' * https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g * eneric/iniscrptact.html */ exit(is_status_request ? 4 : 1); } } me@t520 /t/freebsd_indent> cat test.c typedef struct GinBtreeData { /* search methods */ BlockNumber (*findChildPage) (GinBtree, GinBtreeStack *); BlockNumber (*getLeftMostChild) (GinBtree, Page); bool(*isMoveRight) (GinBtree, Page); bool(*findItem) (GinBtree, GinBtreeStack *); /* insert methods */ OffsetNumber (*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber); GinPlaceToPageRC (*beginPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void **, Page *, Page *); void(*execPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void *); } void hardclock_device_poll(void) { const unsigned (*TABLE_index)[2]; if (stat(pg_data, &statbuf) != 0) { /* * The Linux Standard Base Core Specification 3.1 says this should * return '4, program or service status is unknown' * https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g * eneric/iniscrptact.html */ exit(is_status_request ? 4 : 1); } } me@t520 /t/freebsd_indent> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
> * const unsigned(*TABLE_index)[2]; > * OffsetNumber(*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber); > * an overlength comment line is simply busted altogether Fixed and pushed to my github repository. Note that indent won't wrap long lines like links or paths anymore. But obviously it can't join parts of long links that have been wrapped by previous pgindent runs. > * the comments get formatted differently for -ts4 than -ts8 > * extra spacing getting inserted for fairly long labels > * some enum declarations get the phony extra indentation > * comments on the same line are now run together > * surplus indentation for SH_ELEMENT_TYPE * data; Please tell me if the list of your complaints above is incomplete. I will analyze those next. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-05-21 03:00, Tom Lane wrote: > I wrote: >> Also, I found two places where an overlength comment line is simply busted >> altogether --- notice that a character is missing at the split point: > > I found the cause of that: you need to apply this patch: > > --- freebsd_indent/pr_comment.c~ 2017-05-17 14:59:31.548442801 -0400 > +++ freebsd_indent/pr_comment.c 2017-05-20 20:51:16.447332977 -0400 > @@ -344,8 +353,8 @@ pr_comment(void) > { > int len = strlen(t_ptr); > > - CHECK_SIZE_COM(len); > - memmove(e_com, t_ptr, len); > + CHECK_SIZE_COM(len + 1); > + memmove(e_com, t_ptr, len + 1); > last_bl = strpbrk(e_com, " \t"); > e_com += len; > } > > As the code stands, the strpbrk call is being applied to a > not-null-terminated string and therefore is sometimes producing an > insane value of last_bl, messing up decisions later in the comment. > Having the memmove include the trailing \0 resolves that. I have been analyzing this and came to different conclusions. Foremost, a strpbrk() call like that finds the first occurrence of either space or a tab, but last_bl means "last blank" - it's used for marking where to wrap a comment line if it turns out to be too long. The previous coding moved the character sequence byte after byte, updating last_bl every time it was copying one of the two characters. I've rewritten that part as: CHECK_SIZE_COM(len); memmove(e_com, t_ptr, len); - last_bl = strpbrk(e_com, " \t"); e_com += len; + last_bl = NULL; + for (t_ptr = e_com - 1; t_ptr > e_com - len; t_ptr--) + if (*t_ptr == ' ' || *t_ptr == '\t') { + last_bl = t_ptr; + break; + } } But then I also started to wonder if there is any case when there's more than one character to copy and I haven't found one yet. It looks like } while (!memchr("*\n\r\b\t", *buf_ptr, 6) && (now_col <= adj_max_col || !last_bl)); guarantees that if we're past adj_max_col, it'll only be one non-space character. But I'm not sure yet. -- 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] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.
On 2017-05-18 18:13, Tom Lane wrote: > Piotr Stefaniak writes: >> That, I assume, would be me. Coincidentally, I'm about to push my fixes >> upstream (FreeBSD). Before that happens, my changes can be obtained from >> https://github.com/pstef/freebsd_indent and tested, if anyone wishes. > > I spent a little bit of time on portability testing, because we are > certainly going to insist that this tool be portable to more than > just FreeBSD. Things are not very good as it stands: > > * Makefile is 100% BSD-specific. Possibly we could just agree to > disagree on that point, and include a PG-style makefile that is not > like upstream's. I attach the one I used for test purposes. This would have to live outside of FreeBSD for obvious (or not) reasons. Most likely as a part of pg_bsd_indent. I use the original ("BSD") Makefile on Linux, feeding it to bmake(1). But I don't expect bmake to become a requirement for pg_bsd_indent. > * __FBSDID() macros fail to compile anywhere else than FreeBSD. > Couldn't you hide those inside #if 0, as you're already doing for > the ancient sccsid strings? The use of __FBSDID macro won't be going anywhere from FreeBSD indent, I'm afraid. But I think it could be if-def'd out under systems other than FreeBSD. > * Please put the copyright[] string in indent.c inside #if 0 too, > as that draws unreferenced-variable warnings on some compilers. > > * There's one use of bcopy(), please replace with memmove(). These could probably be done upstream. I'd like to convert the strings to comments. > * References to and are problematic, as both > are BSD-isms not found in POSIX. They seem to be available anyway > on Linux, but I bet not on Windows or SysV-tradition boxes. > I removed the includes and didn't see any problems, > but removing exposes calls to err() and errx(), which > we'd have to find replacements for. Maybe just change them to > standard-conforming printf() + exit()? I'll look into why indent includes sys/cdefs.h. I don't expect to be allowed to replace err() and errx() with equivalent solutions based on ISO C and POSIX. I fear the idea of detecting err*() support prior to compilation... Probably a much simpler solution would be to replace err*() with equivalent solutions in the PG's fork of indent. printf() and exit() would probably be insufficient, you'd also need strerror(), I think. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-05-17 23:46, Tom Lane wrote: > I hacked around that by putting back a detab/entab step at the end > using the existing subroutines in pgindent. That about halved the > size of the diff, but it's still too big to post. Much of what > I'm seeing with this version is randomly different decisions about > how far to indent comments pgindent doesn't set the -c indent option ("The column in which comments on code start."), so indent uses the default value of 33 (meaning column 32). If the code pushes the comment further than column 32, indent only places a single tab between the two just to separate them. This, given 4-column tabs, should result in placing the comment on bitSize[INDEX_MAX_KEYS]; from your example onto column 44 - which the newer version of indent does (if you tell it -ts4), unlike the older one. I think that this is an improvement. > It does seem to be handling formatting around sizeof() calls a lot better > than the old code, as well as function pointer typedefs. So those are > huge wins. But can we avoid the changes mentioned above? I'd like the > new version to only differ in ways that are clear improvements. I don't know how to avoid the improvement. Try removing -ts4 as well as putting back detab+entab. -- 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] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.
On 2017-05-17 17:55, Peter Eisentraut wrote: > On 5/17/17 10:14, Tom Lane wrote: >> What I was concerned about was that pgindent will reindent the second >> line so that it's impossible to tell whether the spacing is correct. > > pgindent moving string continuations to the left is a completely > terrible behavior anyway and we should look into changing that. Just > look at the mess it makes out of SELECT queries in pg_dump.c. If I remember correctly, it tries to right-align string literals to whatever -l ("Maximum length of an output line") was set to. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-05-17 22:11, Tom Lane wrote: > Piotr Stefaniak writes: >> The third significant issue I kept in my mind was to shift some >> work-arounds from pgindent to indent. > > Yeah, I was wondering about that too. > >> When I use my indent as the base >> for pgindent and set its options like this: >> -bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb >> -cli1 -sac -ts4 -cp10 > > Ah, thanks, ignore the message I just sent asking for that ... > >> I can remove most of the work-arounds written in the perl script and >> still get pretty decent results. But I expect some debate over a few things. > > ... but what parts of the perl script do you remove? I'm trying > to duplicate whatever results you're currently getting. Full copy of my pgindent attached. Changes commented below. @@ -17,7 +17,7 @@ my $devnull= File::Spec->devnull; # Common indent settings my $indent_opts = - "-bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb"; + "-bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb -cli1 -sac -ts4 -cp10"; # indent-dependent settings my $extra_opts = ""; The additional options are necessary. Mostly they replace the work-arounds. @@ -198,60 +198,16 @@ sub pre_indent { my $source = shift; - # remove trailing whitespace - $source =~ s/[ \t]+$//gm; - I'm not sure there won't be any trailing white-space, but I've committed changes that should limit it. ## Comments # Convert // comments to /* */ $source =~ s!^([ \t]*)//(.*)$!$1/* $2 */!gm; - # 'else' followed by a single-line comment, followed by - # a brace on the next line confuses BSD indent, so we push - # the comment down to the next line, then later pull it - # back up again. Add space before _PGMV or indent will add - # it for us. - # AMD: A symptom of not getting this right is that you see errors like: - # FILE: ../../../src/backend/rewrite/rewriteHandler.c - # Error@2259: - # Stuff missing from end of file - $source =~ - s!(\}|[ \t])else[ \t]*(/\*)(.*\*/)[ \t]*$!$1else\n$2 _PGMV$3!gm; - - # Indent multi-line after-'else' comment so BSD indent will move it - # properly. We already moved down single-line comments above. - # Check for '*' to make sure we are not in a single-line comment that - # has other text on the line. - $source =~ s!(\}|[ \t])else[ \t]*(/\*[^*]*)[ \t]*$!$1else\n $2!gm; These are definitely fixed. ## Other - # Work around bug where function that defines no local variables - # misindents switch() case lines and line after #else. Do not do - # for struct/enum. - my @srclines = split(/\n/, $source); - foreach my $lno (1 .. $#srclines) - { - my $l2 = $srclines[$lno]; - - # Line is only a single open brace in column 0 - next unless $l2 =~ /^\{[ \t]*$/; - - # previous line has a closing paren - next unless $srclines[ $lno - 1 ] =~ /\)/; - - # previous line was struct, etc. - next - if $srclines[ $lno - 1 ] =~ - m!=|^(struct|enum|[ \t]*typedef|extern[ \t]+"C")!; - - $srclines[$lno] = "$l2\nint pgindent_func_no_var_fix;"; - } - $source = join("\n", @srclines) . "\n";# make sure there's a final \n - I don't have time now to double-check, but the above shouldn't be needed anymore. - # Pull up single-line comment after 'else' that was pulled down above - $source =~ s!else\n[ \t]+/\* _PGMV!else\t/*!g; - - # Indent single-line after-'else' comment by only one tab. - $source =~ s!(\}|[ \t])else[ \t]+(/\*.*\*/)[ \t]*$!$1else\t$2!gm; - - # Add tab before comments with no whitespace before them (on a tab stop) - $source =~ s!(\S)(/\*.*\*/)$!$1\t$2!gm; - - # Remove blank line between opening brace and block comment. - $source =~ s!(\t*\{\n)\n([ \t]+/\*)$!$1$2!gm; - These are almost definitely fixed in indent. - # cpp conditionals - - # Reduce whitespace between #endif and comments to one tab - $source =~ s!^\#endif[ \t]+/\*!#endif /*!gm; - ## Functions - # Work around misindenting of function with no variables defined. - $source =~ s!^[ \t]*int[ \t]+pgindent_func_no_var_fix;[ \t]*\n{1,2}!!gm; - These have likely been fixed. - ## Other - - # Remove too much indenting after closing brace. - $source =~ s!^\}\t[ \t]+!}\t!gm; - - # Workaround indent bug that places excessive space before 'static'. - $source =~ s!^static[ \t]+!static !
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
On 2017-05-17 20:31, Tom Lane wrote: > Piotr Stefaniak writes: >> On 2017-05-17 19:16, Alvaro Herrera wrote: >>> We have someone who has studied the BSD indent code and even sent us >>> patches to fix quite a few bugs, but we've largely ignored his efforts >>> so far. I propose we take that indent as part of our repo, and patch it >>> to our preferences. > >> That, I assume, would be me. Coincidentally, I'm about to push my fixes >> upstream (FreeBSD). Before that happens, my changes can be obtained from >> https://github.com/pstef/freebsd_indent and tested, if anyone wishes. > > Yes, I was just reviewing those threads. IIUC, the current proposal is > to adopt FreeBSD's version of indent as being less buggy and better > maintained than NetBSD's, and then patch it as necessary. One of my goals here is to fix bugs in FreeBSD indent so it's easier to develop and maintain. I've also tried hard to not introduce serious regressions for FreeBSD which also uses indent (for some sub-projects even automatically). This is an ongoing effort. What I describe below I believe to have been largely achieved. Another goal is to incorporate all custom changes that make current pg_bsd_indent yet another, unique indent fork, into FreeBSD indent - so that maintaining patches for some other indent by the Postgres project wouldn't be necessary. I understand the need to have control over a copy of it within the Postgres project but it would be nice to not effectively fork it by carrying custom patches around. The third significant issue I kept in my mind was to shift some work-arounds from pgindent to indent. When I use my indent as the base for pgindent and set its options like this: -bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb -cli1 -sac -ts4 -cp10 I can remove most of the work-arounds written in the perl script and still get pretty decent results. But I expect some debate over a few things. -- 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] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.
On 2017-05-17 19:16, Alvaro Herrera wrote: > Tom Lane wrote: > >> Changing the pgindent rule for such cases sounds kind of promising, >> but will anyone pursue it? > > We have someone who has studied the BSD indent code and even sent us > patches to fix quite a few bugs, but we've largely ignored his efforts > so far. I propose we take that indent as part of our repo, and patch it > to our preferences. That, I assume, would be me. Coincidentally, I'm about to push my fixes upstream (FreeBSD). Before that happens, my changes can be obtained from https://github.com/pstef/freebsd_indent and tested, if anyone wishes. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_bsd_indent: implement -lps ("leave preprocessor space")
Hello, this is a patch that Andres asked me for. It makes pg_bsd_indent leave preprocessor space alone, as in this example: #if 0 # if 0 # if 0 # error # endif # endif #else # line 7 #endif diff -ur pg_bsd_indent/args.c pg_bsd_indent_patched/args.c --- pg_bsd_indent/args.c 2014-01-31 04:09:31.0 +0100 +++ pg_bsd_indent_patched/args.c 2017-02-08 01:59:01.921080544 +0100 @@ -221,6 +221,9 @@ "lc", PRO_INT, 0, 0, &block_comment_max_col }, { + "lps", PRO_BOOL, false, ON, &leave_preprocessor_space + }, + { "lp", PRO_BOOL, true, ON, &lineup_to_parens }, { @@ -269,6 +272,9 @@ "nip", PRO_BOOL, true, OFF, &ps.indent_parameters }, { + "nlps", PRO_BOOL, false, OFF, &leave_preprocessor_space + }, + { "nlp", PRO_BOOL, true, OFF, &lineup_to_parens }, { diff -ur pg_bsd_indent/indent.c pg_bsd_indent_patched/indent.c --- pg_bsd_indent/indent.c 2014-01-31 04:06:43.0 +0100 +++ pg_bsd_indent_patched/indent.c 2017-02-08 01:56:59.039931984 +0100 @@ -1091,17 +1091,25 @@ (s_code != e_code)) dump_line(); *e_lab++ = '#'; /* move whole line to 'label' buffer */ + if (leave_preprocessor_space) { +while (isblank((int)*buf_ptr)) { + CHECK_SIZE_LAB; + *e_lab++ = *buf_ptr++; +} + } + else { +while (isblank((int)*buf_ptr)) { + buf_ptr++; +} + } + t_ptr = e_lab; + { int in_comment = 0; int com_start = 0; charquote = 0; int com_end = 0; -while (*buf_ptr == ' ' || *buf_ptr == '\t') { - buf_ptr++; - if (buf_ptr >= buf_end) - fill_buffer(); -} while (*buf_ptr != '\n' || in_comment) { CHECK_SIZE_LAB; *e_lab = *buf_ptr++; @@ -1179,7 +1187,7 @@ ps.pcase = false; } - if (strncmp(s_lab, "#if", 3) == 0) { + if (t_ptr[0] == 'i' && t_ptr[1] == 'f') { if (blanklines_around_conditional_compilation) { int c; prefix_blankline_requested++; @@ -1192,7 +1200,7 @@ } else diag(1, "#if stack overflow"); } else -if (strncmp(s_lab, "#else", 5) == 0) { +if (t_ptr[0] == 'e' && t_ptr[1] == 'l') { if (ifdef_level <= 0) diag(1, "Unmatched #else"); else { @@ -1200,7 +1208,7 @@ ps = state_stack[ifdef_level - 1]; } } else - if (strncmp(s_lab, "#endif", 6) == 0) { + if (strncmp(t_ptr, "endif", 5) == 0) { if (ifdef_level <= 0) diag(1, "Unmatched #endif"); else { diff -ur pg_bsd_indent/indent_globs.h pg_bsd_indent_patched/indent_globs.h --- pg_bsd_indent/indent_globs.h 2005-11-15 01:30:24.0 +0100 +++ pg_bsd_indent_patched/indent_globs.h 2017-02-08 01:57:51.003994806 +0100 @@ -222,6 +222,7 @@ * "for(e;e;e)" should be indented an extra * tab stop so that they don't conflict with * the code that follows */ +EXTERN int leave_preprocessor_space; /* -troff font state information */ -- 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] Possible spelling fixes
On 2017-02-06 10:40, Heikki Linnakangas wrote: > On 02/06/2017 04:50 AM, Josh Soref wrote: >> NUL-terminated -> NULL-terminated > > When we're talking about NUL-terminated strings, NUL refers to the NUL > ASCII character. NULL usually refers to a NULL pointer. We're probably > not consistent about this, but in this context, NUL-terminated isn't > wrong, so let's leave them as they are. The C standard talks about how "a byte with all bits set to 0, called the null character" is used to "terminate a character string"; it mentions '\0' as "commonly used to represent the null character"; and it also talks about when snprintf() produces "null-terminated output". It never mentions ASCII in this context; quite intentionally it avoids assuming ASCII at all, so that a standard-compliant C implementation may co-exist with other encodings (like EBCDIC). -- 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] LLVM Address Sanitizer (ASAN) and valgrind support
On 2016-09-28 00:02, Andres Freund wrote: > On 2015-09-07 17:05:10 +0100, Greg Stark wrote: >> I feel like I remember hearing about this before but I can't find any >> mention of it in my mail archives. It seems pretty simple to add >> support for LLVM's Address Sanitizer (asan) by using the hooks we >> already have for valgrind. > > Any plans to pick this up again? Not remembering the context, I was initially confused about what exactly supposedly needs to be done in order to have ASan support, especially since I've been using it for a couple of years without any kind of modifications. Having read the whole thread now, I assume the discussion is now about getting MSan support, since apparently it's been already concluded that not much is needed for getting ASan support: >> I don't even see any need offhand for a configure flag or autoconf >> test. We could have a configure flag just to be consistent with >> valgrind but it seems pointless. If you're compiling with asan I don't >> see any reason to not use it. I'm building this to see if it works >> now. > > I agree. A flag guards Valgrind client requests, because we'd otherwise have > no idea whether the user plans to run the binary under Valgrind. For ASAN, > all relevant decisions happen at build time. Please correct me if I'm wrong. -- 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: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)
On 2016-08-19 23:55, Tom Lane wrote: > I think you are failing to understand Heikki's point. There is no way > we are committing the change depicted above, because (1) it will mask more > bugs than it fixes; (2) it's an enormously expensive way to fix anything; > and (3) it will effectively disable valgrind testing for missed > initializations. I wasn't disagreeing with Heikki, I was trying to show that while Aleksander's patch may be useless and perhaps harmful if committed, it is not useless in a larger perspective as it has made people look into an issue. And I did that simply because I have more changes of that kind that may end up being deemed as useless for committing, but I want to share them with -hackers anyway. -- 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: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)
On 2016-08-18 20:02, Heikki Linnakangas wrote: > On 03/22/2016 03:27 PM, Aleksander Alekseev wrote: >> diff --git a/src/backend/utils/mmgr/aset.c >> b/src/backend/utils/mmgr/aset.c >> index d26991e..46ab8a2 100644 >> --- a/src/backend/utils/mmgr/aset.c >> +++ b/src/backend/utils/mmgr/aset.c >> @@ -850,7 +850,7 @@ AllocSetAlloc(MemoryContext context, Size size) >> blksize <<= 1; >> >> /* Try to allocate it */ >> -block = (AllocBlock) malloc(blksize); >> +block = (AllocBlock) calloc(1, blksize); >> >> /* >> * We could be asking for pretty big blocks here, so cope if >> malloc > > I think this goes too far. You're zeroing all palloc'd memory, even if > it's going to be passed to palloc0(), and zeroed there. It might even > silence legitimate warnings, if there's code somewhere that does > palloc(), and accesses some of it before initializing. Plus it's a > performance hit. I just did a test where I 1. memset() that block to 0xAC (aset.c:853) 2. compile and run bin/initdb, then bin/postgres 2. run an SQL file, shut down bin/postgres 3. make a copy of the transaction log file 4. change the memset() to 0x0C, repeat steps 2-3 5. compare the two transaction log files with a combination of hexdump(1), cut(1), and diff(1). At the end of the output I can see: -0f34 0010 f5ff ac02 000a +0f34 0010 f5ff 0c02 000a So it looks like the MSan complaint might be a true positive. The SQL file is just this snippet from bit.sql: CREATE TABLE varbit_table (a BIT VARYING(16), b BIT VARYING(16)); COPY varbit_table FROM stdin; X0F X10 X1F X11 X2F X12 X3F X13 X8F X04 X000F X0010 X0123 X X2468 X2468 XFA50 X05AF X1234 XFFF5 \. I realize given information might be a little bit scarce, but I didn't know what else might be interesting to you that you wouldn't be able to reproduce. -- 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] [GENERAL] C++ port of Postgres
On 2016-08-16 18:33, Robert Haas wrote: > It wouldn't be that much work to maintain, either: we'd > just set up some buildfarm members that compiled using C++ and when > they turned red, we'd go fix it. I think that there exist subtle differences between C and C++ that without compile-time diagnostic could potentially lead to different run-time behavior. As an artificial example: $ cat ./test.c #include int main(void) { FILE *f = fopen("test.bin", "w"); if (f == NULL) return 1; fwrite("1", sizeof '1', 1, f); fclose(f); return 0; } $ clang ./test.c -o test $ ./test $ hexdump test.bin 000 0031 004 $ clang++ ./test.c -o test clang-3.9: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated $ ./test $ hexdump test.bin 000 0031 001 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof
On 2016-05-25 21:13, Tom Lane wrote: > Robert Haas writes: >> On Sun, May 22, 2016 at 4:16 PM, Piotr Stefaniak >> wrote: >>> I think I've managed to improve pg_bsd_indent's handling of two types of >>> cases. > >> Wow, that seems pretty great. I haven't scrutinized your changes to >> pg_bsd_indent, but effect_on_pg.diff looks like a large improvement. > Assuming this patch withstands more careful review, we will need to think > about project policy for how/when to apply such fixes. The patches have got committed upstream and work well for Postgres. You can take FreeBSD indent(1) as of SVN r303746, apply patches from https://github.com/pstef/freebsd_indent/commits/pass2 (subject to heavy rebasing) and use as pg_bsd_indent for pgindent. There are more fixes I intend to do, of which the most relevant for Postgres are: 1) fixing "function pointer typedef formatting" 2) adding a -tsn option like in GNU indent, for setting how many columns a tab character will produce. I had a preliminary patch implementing that and I have to say that while it removes the need for entab, it also introduces a lot of seemingly pointless changes in formatting which will be arguably improvements or regressions. -- 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] More parallel-query fun
On 2016-06-16 20:14, Tom Lane wrote: As of HEAD you can exercise quite a lot of parallel query behavior by running the regression tests with these settings applied: force_parallel_mode = regress max_parallel_workers_per_gather = 2-- this is default at the moment min_parallel_relation_size = 0 parallel_setup_cost = 0 parallel_tuple_cost = 0 This results in multiple interesting failures, including a core dump I saw another previously-unreported problem before getting to the crash: Haven't tried to trace that one down yet. As I expected, I'm unable to reproduce anything of the above - please correct me if I'm wrong, but it all seems to have been fixed. -- 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] A couple of cosmetic changes around shared memory code
On 2016-06-29 18:58, Robert Haas wrote: This code predates be7558162acc5578d0b2cf0c8d4c76b6076ce352, prior to which proc_exit(0) forced an immediate, unconditional restart. It's true that, given that commit, changing this code to do proc_exit(0) instead of proc_exit(1) would be harmless. However, people writing background workers that might need to work with 9.3 would be best advised to stick with proc_exit(1). Therefore, I maintain that this is not broken and doesn't need to be fixed. Then I'd argue that it ought to be documented in form of a C comment for people writing background workers and for those who might want to "fix" this in the future. -- 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] A couple of cosmetic changes around shared memory code
On 2016-05-16 21:40, Piotr Stefaniak wrote: Hello, while investigating the shm_mq code and its testing module I made some cosmetic improvements there. You can see them in the attached diff file. Revised patch attached. commit 3ff1afa84e4bc22f153c876e2f0588327a8a004e Author: Piotr Stefaniak Date: Thu Apr 28 18:36:16 2016 +0200 Cosmetic improvements around shm_mq and test_shm_mq. diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c index cc1f04d..64a5475 100644 --- a/src/backend/storage/ipc/shm_mq.c +++ b/src/backend/storage/ipc/shm_mq.c @@ -103,7 +103,7 @@ struct shm_mq * locally by copying the chunks into a backend-local buffer. mqh_buffer is * the buffer, and mqh_buflen is the number of bytes allocated for it. * - * mqh_partial_message_bytes, mqh_expected_bytes, and mqh_length_word_complete + * mqh_partial_bytes, mqh_expected_bytes, and mqh_length_word_complete * are used to track the state of non-blocking operations. When the caller * attempts a non-blocking operation that returns SHM_MQ_WOULD_BLOCK, they * are expected to retry the call at a later time with the same argument; diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c index 5bd2820..a0f3962 100644 --- a/src/test/modules/test_shm_mq/setup.c +++ b/src/test/modules/test_shm_mq/setup.c @@ -125,7 +125,7 @@ setup_dynamic_shared_memory(int64 queue_size, int nworkers, segsize = shm_toc_estimate(&e); /* Create the shared memory segment and establish a table of contents. */ - seg = dsm_create(shm_toc_estimate(&e), 0); + seg = dsm_create(segsize, 0); toc = shm_toc_create(PG_TEST_SHM_MQ_MAGIC, dsm_segment_address(seg), segsize); diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c index 638649b..a94414a 100644 --- a/src/test/modules/test_shm_mq/worker.c +++ b/src/test/modules/test_shm_mq/worker.c @@ -139,7 +139,7 @@ test_shm_mq_main(Datum main_arg) * we can go ahead and exit. */ dsm_detach(seg); - proc_exit(1); + proc_exit(0); } /* -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof
On 2016-05-27 08:13, Piotr Stefaniak wrote: I'm trying to see if FreeBSD indent can successfully do pg_bsd_indent's job. So far I had to fix one thing, which is not adding a space after a cast operator, for which they added no option to turn it off. Currently I'm fighting one other bug, but I think that'll be it. So... after fixing 12 times more bugs that I had anticipated (see the list at the end of this email; also see attached patches.tgz if you want to apply the patches yourself), my "fork" of FreeBSD indent(1) can do pg_bsd_indent's job if you pass it three additional parameters (-nut -cli1 -sac), producing a 6600-line unified diff, mostly of desired changes (see freebsd-indent.diff.gz for details). I'm in the process of pushing my changes upstream, but I was already told that it's too late to get them into 11.0-RELEASE. Personally, I don't mind that, hoping that the upstream will accept them eventually. I'm also hoping it'll be easier to reinvent GNU indent's -tsn ("set tabsize to n spaces") option for FreeBSD indent than it would be for any other of the forks that aren't GNU. I envision that to be the first step to getting rid of some of the work-arounds pgindent does, mainly running entab and detab as pre- and post-processing steps. That and more I'll probably do later. If porting FreeBSD indent to PostgreSQL's sources turns out to be successful, there will be a choice between rebasing pg_bsd_indent on that and picking specific patches and applying it on PG's fork of indent(1). At this point I think it wouldn't make any sense to port any changes to current pg_bsd_indent. The full list of changes I made to FreeBSD's indent(1) as of r289677: [bugfix] Fix typo in keyword "typedef". [bugfix] Avoid out of bound access of array codebuf pointed into by s_code. [bugfix] Avoid out of bound access of array ps.paren_indents. [bugfix] Avoid out of bound access of array in_buffer. [bugfix] Avoid potential use-after-free. [bugfix] Semicolons inside struct declarations don't end the declarations. [bugfix] Support "f" and "F" floating constant suffixes. [bugfix] Removed whitespace shouldn't be considered in column calculations. [bugfix] Don't add surplus space on empty lines in comments. [bugfix] Bail out if there's no more space on the parser stack. [bugfix] Consistently indent declarations. [bugfix] Don't ignore the fact that offsetof is a keyword. [cleanup] Make definition of opchar conditional. [cleanup] Remove dead code relating to unix-style comments. [cleanup] Simplify pr_comment(). [cleanup] Deduplicate code that decided if a comment needs a blank line at the top. [bugfix] Fix wrapping of some lines in comments. [bugfix] Untangle the connection between pr_comment.c and io.c, fixing at least two bugs [feature] Add -sac (space after cast) and -nsac options. [bugfix] Don't newline on cpp lines like #endif unless -bacc is on. [feature] Add -U option for providing a file containing list of types. [optimization] Use bsearch() for looking up type keywords. freebsd-indent.diff.gz Description: application/gzip patches.tgz Description: application/compressed-tar -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof
On 2016-05-25 21:13, Tom Lane wrote: Assuming this patch withstands more careful review, we will need to think about project policy for how/when to apply such fixes. I discovered yesterday that Bruce Evans had done the fix for sizeof in their fork of indent(1) in 2004 (r125623 [1]). The core fix is exactly the same as mine, he just did more fixes around it, which I haven't analyzed. I'm trying to see if FreeBSD indent can successfully do pg_bsd_indent's job. So far I had to fix one thing, which is not adding a space after a cast operator, for which they added no option to turn it off. Currently I'm fighting one other bug, but I think that'll be it. I took interest in FreeBSD's fork of indent(1) because they've fixed more things than NetBSD people have in their fork, it seems. I'm also hoping it'll be easier to reinvent GNU indent's -tsn ("set tabsize to n spaces") option for FreeBSD indent than it would be for any other of the forks that aren't GNU. I envision that to be the first step to getting rid of some of the work-arounds pgindent does, mainly running entab and detab as pre- and post-processing steps. If porting FreeBSD indent to PostgreSQL's sources turns out to be successful, there will be a choice between rebasing pg_bsd_indent on that and picking specific patches and applying it on PG's fork of indent(1). [1] https://svnweb.freebsd.org/base/head/usr.bin/indent/lexi.c?r1=125619&r2=125623 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_bsd_indent - improvements around offsetof and sizeof
On 2016-05-25 21:13, Tom Lane wrote: I'd love to see a fix for its brain damage around function pointer typedef formatting, too. Show me a few examples and I'll look into it. I'm excited about this too, not least because it suggests that maybe bsdindent isn't quite as opaque as it appears. It's old, hacked on many times over the past few decades and historically just a band-aid rather than something designed from the ground up, so it's not easy to work with. Which is why I think that a newer tool (like ClangFormat) should be considered as a replacement for pg_bsd_indent. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_bsd_indent - improvements around offsetof and sizeof
Hello, I think I've managed to improve pg_bsd_indent's handling of two types of cases. The first are like in this example: - hashp = (HTAB *) DynaHashAlloc(sizeof(HTAB) + strlen(tabname) +1); + hashp = (HTAB *) DynaHashAlloc(sizeof(HTAB) + strlen(tabname) + 1); Pristine pg_bsd_indent is inconsistent in masking parentheses as those that are part of a cast and those that "are part of sizeof": seeing a type name following an lparen it always masks that lparen as a part of a cast; seeing an rparen it only removes the bit if it doesn't overlap with sizeof_mask. In the example above, "(HTAB" started both "cast parens" and "sizeof parens" at the same time, and the immediately following rparen ended only the "sizeof parens". According to indent, the cast-to type then ends at "tabname)" and what follows is the cast's operand, including the + operator; in that case it's assumed to be unary and not binary, which is why indent doesn't add the space after it. The fix was to make it consistent about masking parens: - ps.cast_mask |= 1 << ps.p_l_follow; + ps.cast_mask |= (1 << ps.p_l_follow & ~ps.sizeof_mask); The second type of cases are like this: - nse = palloc(offsetof(PLpgSQL_nsitem, name) +strlen(name) + 1); + nse = palloc(offsetof(PLpgSQL_nsitem, name) + strlen(name) + 1); pg_bsd_indent simply hasn't been taught that a parenthesized type name following the offsetof macro and then an lparen is another exception to the rule of thumb that a construction like that generally means a cast. You'll also notice other, seemingly unrelated changes, most notably the rearrangement in numbers assigned to keywords. I've done it that way so that it was easier and simpler to keep the -bs option functioning as designed. I've also renamed "sizeof_mask" to "not_cast_mask", because I think the latter is a better description of what the mask does (it prevents interpreting parenthesized type names as a cast where they aren't, namely where they follow sizeof or offsetof; I haven't done any support for function declarators and I don't plan to - the fact that pg_bsd_indent thinks that "(int" in "char func(int);" begins a cast is amusing but it seems harmless for now). I'm attaching the patch for pg_bsd_indent and also a full diff that shows the change in its behavior when run against PG's sources. diff -Burw indent.c indent.c --- indent.c 2014-01-31 04:06:43.0 +0100 +++ indent.c 2016-05-22 19:24:01.666077311 +0200 @@ -568,7 +568,9 @@ * happy */ if (ps.want_blank && *token != '[' && (ps.last_token != ident || proc_calls_space -|| (ps.its_a_keyword && (!ps.sizeof_keyword || Bill_Shannon + /* offsetof (1) is never allowed a space; sizeof (2) iff -bs; + * all other keywords (>2) always get a space before lparen */ +|| (ps.keyword + Bill_Shannon > 2))) *e_code++ = ' '; if (ps.in_decl && !ps.block_init) { if (troff && !ps.dumped_decl_indent && !is_procname && ps.last_token == decl) { @@ -601,17 +603,19 @@ * structure decl or * initialization */ } - if (ps.sizeof_keyword) -ps.sizeof_mask |= 1 << ps.p_l_follow; + /* a parenthesized type name following sizeof or offsetof is not + * a cast */ + if (ps.keyword == 1 || ps.keyword == 2) +ps.not_cast_mask |= 1 << ps.p_l_follow; break; case rparen: /* got a ')' or ']' */ rparen_count--; - if (ps.cast_mask & (1 << ps.p_l_follow) & ~ps.sizeof_mask) { + if (ps.cast_mask & (1 << ps.p_l_follow) & ~ps.not_cast_mask) { ps.last_u_d = true; ps.cast_mask &= (1 << ps.p_l_follow) - 1; } - ps.sizeof_mask &= (1 << ps.p_l_follow) - 1; + ps.not_cast_mask &= (1 << ps.p_l_follow) - 1; if (--ps.p_l_follow < 0) { ps.p_l_follow = 0; diag(0, "Extra %c", *token); @@ -780,7 +784,7 @@ if (ps.last_token == rparen && rparen_count == 0) ps.in_parameter_declaration = 0; ps.cast_mask = 0; - ps.sizeof_mask = 0; + ps.not_cast_mask = 0; ps.block_init = 0; ps.block_init_level = 0; ps.just_saw_decl--; @@ -1042,7 +1046,7 @@ copy_id: if (ps.want_blank) *e_code++ = ' '; - if (troff && ps.its_a_keyword) { + if (troff && ps.keyword) { e_code = chfont(&bodyf, &keywordf, e_code); for (t_ptr = token; *t_ptr; ++t_ptr) { CHECK_SIZE_CODE; diff -Burw indent_globs.h indent_globs.h --- indent_globs.h 2005-11-15 01:30:24.0 +0100 +++ indent_globs.h 2016-05-22 19:23:45.067093287 +0200 @@ -255,10 +255,10 @@ * comment. In that case, the first non-blank * char should be lined up with the comment / */ int comment_delta, n_comment_delta; - int cast_mask; /* indicates which close parens close off - * casts */ - int sizeof_mask; /* indicates which close parens close off - * sizeof''s */ + int cast_mask; /* indicates which close parens potentially + * close off casts */
Re: [HACKERS] A couple of cosmetic changes around shared memory code
On 2016-05-17 19:05, Tom Lane wrote: Michael Paquier writes: On Tue, May 17, 2016 at 4:40 AM, Piotr Stefaniak wrote: -toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry) +toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry) + allocated_bytes; I don't recall the exact reason, but this is intentional style (memories from a patchwork with Tom). Well, it's not so much intentional as that pgindent will make it look like that no matter what you do --- it's got some weird interaction with sizeof, offsetof, and typedef names versus operators later on the same line. I'd call that a pgindent bug myself, but have no particular desire to try to fix it. From my understanding, it's because pg_bsd_indent thinks that "(shm_toc)" is a cast since shm_toc is a keyword (type alias fetched from the "list of typedefs" file in this case, to be precise), forcing the "+" to be a unary operator instead of binary. One easy way to work around this bug is putting shm_toc into its own parentheses but I'd prefer dropping this particular fix from my "cosmetic changes" patch until pg_bsd_indent is fixed. I may come up with a possible fix for this bug, but don't hold your breath. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] A couple of cosmetic changes around shared memory code
Hello, while investigating the shm_mq code and its testing module I made some cosmetic improvements there. You can see them in the attached diff file. commit 0e202cb6e0eca2e7fb3e1353b550f3d2ace9680e Author: Piotr Stefaniak Date: Thu Apr 28 18:36:16 2016 +0200 Cosmetic improvements around shm_mq and test_shm_mq. diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c index 7859f42..292d515 100644 --- a/src/backend/storage/ipc/shm_mq.c +++ b/src/backend/storage/ipc/shm_mq.c @@ -103,7 +103,7 @@ struct shm_mq * locally by copying the chunks into a backend-local buffer. mqh_buffer is * the buffer, and mqh_buflen is the number of bytes allocated for it. * - * mqh_partial_message_bytes, mqh_expected_bytes, and mqh_length_word_complete + * mqh_partial_bytes, mqh_expected_bytes, and mqh_length_word_complete * are used to track the state of non-blocking operations. When the caller * attempts a non-blocking operation that returns SHM_MQ_WOULD_BLOCK, they * are expected to retry the call at a later time with the same argument; diff --git a/src/backend/storage/ipc/shm_toc.c b/src/backend/storage/ipc/shm_toc.c index 55248c2..e1d6bd1 100644 --- a/src/backend/storage/ipc/shm_toc.c +++ b/src/backend/storage/ipc/shm_toc.c @@ -96,7 +96,7 @@ shm_toc_allocate(shm_toc *toc, Size nbytes) total_bytes = vtoc->toc_total_bytes; allocated_bytes = vtoc->toc_allocated_bytes; nentry = vtoc->toc_nentry; - toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry) + toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry) + allocated_bytes; /* Check for memory exhaustion and overflow. */ @@ -132,7 +132,7 @@ shm_toc_freespace(shm_toc *toc) nentry = vtoc->toc_nentry; SpinLockRelease(&toc->toc_mutex); - toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry); + toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry); Assert(allocated_bytes + BUFFERALIGN(toc_bytes) <= total_bytes); return total_bytes - (allocated_bytes + BUFFERALIGN(toc_bytes)); } @@ -176,7 +176,7 @@ shm_toc_insert(shm_toc *toc, uint64 key, void *address) total_bytes = vtoc->toc_total_bytes; allocated_bytes = vtoc->toc_allocated_bytes; nentry = vtoc->toc_nentry; - toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry) + toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry) + allocated_bytes; /* Check for memory exhaustion and overflow. */ diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c index 5bd2820..a0f3962 100644 --- a/src/test/modules/test_shm_mq/setup.c +++ b/src/test/modules/test_shm_mq/setup.c @@ -125,7 +125,7 @@ setup_dynamic_shared_memory(int64 queue_size, int nworkers, segsize = shm_toc_estimate(&e); /* Create the shared memory segment and establish a table of contents. */ - seg = dsm_create(shm_toc_estimate(&e), 0); + seg = dsm_create(segsize, 0); toc = shm_toc_create(PG_TEST_SHM_MQ_MAGIC, dsm_segment_address(seg), segsize); diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c index 638649b..a94414a 100644 --- a/src/test/modules/test_shm_mq/worker.c +++ b/src/test/modules/test_shm_mq/worker.c @@ -139,7 +139,7 @@ test_shm_mq_main(Datum main_arg) * we can go ahead and exit. */ dsm_detach(seg); - proc_exit(1); + proc_exit(0); } /* -- 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] [BUGS] Breakage with VACUUM ANALYSE + partitions
On 2016-05-05 09:32, Fabien COELHO wrote: I note that C99 specifically mentions this as something a compiler might warn about: [...] Indeed. Neither gcc nor clang emit such warnings... but they might some day, which would be a blow for my suggestion! For what it's worth, newer versions of clang can emit useful warnings for cases like this: int main(void) { enum test { FOUR = 4 }; enum incompatible { INCOMPATIBLE_FOUR = 4 }; enum test variable; variable = INCOMPATIBLE_FOUR; variable = 5; variable = 4; variable = 3; return 0; } enum.c:5:13: warning: implicit conversion from enumeration type 'enum incompatible' to different enumeration type 'enum test' [-Wenum-conversion] variable = INCOMPATIBLE_FOUR; ~ ^ enum.c:6:13: warning: integer constant not in range of enumerated type 'enum test' [-Wassign-enum] variable = 5; ^ enum.c:8:13: warning: integer constant not in range of enumerated type 'enum test' [-Wassign-enum] variable = 3; ^ 3 warnings generated. So with -Wenum-conversion -Wassign-enum you could treat enum types as distinct and incompatible with each other. -- 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] Small fix: avoid passing null pointers to memcpy()
Added a fix for src/backend/storage/ipc/shm_mq.c:shm_mq_receive. commit 936c7268b61460deb201b9e6bbfb60ab5258ec87 Author: Piotr Stefaniak Date: Thu Apr 28 18:35:43 2016 +0200 Don't pass null pointers to functions that require the pointers to be non null. diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c index cfb3b50..600824a 100644 --- a/contrib/pgcrypto/px.c +++ b/contrib/pgcrypto/px.c @@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen, memset(ivbuf, 0, ivs); if (ivlen > ivs) memcpy(ivbuf, iv, ivs); - else + else if (ivlen > 0) memcpy(ivbuf, iv, ivlen); } diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 950bfc8..bf9a7ab 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock) /* * copy the scan key, if appropriate */ - if (key != NULL) + if (key != NULL && scan->rs_nkeys > 0) memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData)); /* diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 95690ff..4ae98e6 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -4890,8 +4890,9 @@ SerializeTransactionState(Size maxsize, char *start_address) { if (TransactionIdIsValid(s->transactionId)) workspace[i++] = s->transactionId; - memcpy(&workspace[i], s->childXids, - s->nChildXids * sizeof(TransactionId)); + if (s->nChildXids > 0) + memcpy(&workspace[i], s->childXids, + s->nChildXids * sizeof(TransactionId)); i += s->nChildXids; } Assert(i == nxids); diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index b4dc617..a72795c 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -1563,9 +1563,12 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn) /* copy running xacts */ sz = sizeof(TransactionId) * builder->running.xcnt_space; - memcpy(ondisk_c, builder->running.xip, sz); - COMP_CRC32C(ondisk->checksum, ondisk_c, sz); - ondisk_c += sz; + if (sz > 0) + { + memcpy(ondisk_c, builder->running.xip, sz); + COMP_CRC32C(ondisk->checksum, ondisk_c, sz); + ondisk_c += sz; + } /* copy committed xacts */ sz = sizeof(TransactionId) * builder->committed.xcnt; diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c index 7d1c9cd..7859f42 100644 --- a/src/backend/storage/ipc/shm_mq.c +++ b/src/backend/storage/ipc/shm_mq.c @@ -492,7 +492,7 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait) shm_mq_result res; Size rb = 0; Size nbytes; - void *rawdata; + void *rawdata = NULL; Assert(mq->mq_receiver == MyProc); @@ -673,7 +673,11 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait) /* Copy as much as we can. */ Assert(mqh->mqh_partial_bytes + rb <= nbytes); - memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb); + if (rb > 0) + { + Assert(rawdata != NULL); + memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata, rb); + } mqh->mqh_partial_bytes += rb; /* diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 0a9a231..ff09171 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -506,12 +506,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid, CurrentSnapshot->xmax = sourcesnap->xmax; CurrentSnapshot->xcnt = sourcesnap->xcnt; Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount()); - memcpy(CurrentSnapshot->xip, sourcesnap->xip, - sourcesnap->xcnt * sizeof(TransactionId)); + if (sourcesnap->xcnt > 0) + memcpy(CurrentSnapshot->xip, sourcesnap->xip, + sourcesnap->xcnt * sizeof(TransactionId)); CurrentSnapshot->subxcnt = sourcesnap->subxcnt; Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount()); - memcpy(CurrentSnapshot->subxip, sourcesnap->subxip, - sourcesnap->subxcnt * sizeof(TransactionId)); + if (sourcesnap->subxcnt > 0) + memcpy(CurrentSnapshot->subxip, sourcesnap->subxip, + sourcesnap->subxcnt * sizeof(TransactionId)); CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed; CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery; /* NB: curcid should NOT be copied, it's a local matter */ diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c index 0e815a9..0f26bd8 100644 --- a/src/backend/utils/time/tqual.c +++ b/src/backend/utils/time/tqual.c @@ -1639,6 +1639,7 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple) static bool TransactionIdInArray(TransactionId xi
Re: [HACKERS] Small fix: avoid passing null pointers to memcpy()
On 2016-04-03 09:24, Piotr Stefaniak wrote: from running the regression test suite (including TAP tests) and also sqlsmith, I've got a couple of places where UBSan reported calls to memcpy() with null pointer passed as either source or destination. Patch attached. Patch updated. Since this time the patch includes fixes for other standard library function calls (memset and bsearch), I'm renaming the patch file to be more generic. commit 75e849e83c8f7d6b4caab13271b7b0fcf124d497 Author: Piotr Stefaniak Date: Sat Apr 16 14:28:34 2016 +0200 Don't pass null pointers to functions that require the pointers to be non null. diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c index cfb3b50..600824a 100644 --- a/contrib/pgcrypto/px.c +++ b/contrib/pgcrypto/px.c @@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen, memset(ivbuf, 0, ivs); if (ivlen > ivs) memcpy(ivbuf, iv, ivs); - else + else if (ivlen > 0) memcpy(ivbuf, iv, ivlen); } diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 29fd31a..2ed56ab 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock) /* * copy the scan key, if appropriate */ - if (key != NULL) + if (key != NULL && scan->rs_nkeys > 0) memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData)); /* diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 7e37331..e7599be 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -4872,8 +4872,9 @@ SerializeTransactionState(Size maxsize, char *start_address) { if (TransactionIdIsValid(s->transactionId)) workspace[i++] = s->transactionId; - memcpy(&workspace[i], s->childXids, - s->nChildXids * sizeof(TransactionId)); + if (s->nChildXids > 0) + memcpy(&workspace[i], s->childXids, + s->nChildXids * sizeof(TransactionId)); i += s->nChildXids; } Assert(i == nxids); diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index b4dc617..a72795c 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -1563,9 +1563,12 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn) /* copy running xacts */ sz = sizeof(TransactionId) * builder->running.xcnt_space; - memcpy(ondisk_c, builder->running.xip, sz); - COMP_CRC32C(ondisk->checksum, ondisk_c, sz); - ondisk_c += sz; + if (sz > 0) + { + memcpy(ondisk_c, builder->running.xip, sz); + COMP_CRC32C(ondisk->checksum, ondisk_c, sz); + ondisk_c += sz; + } /* copy committed xacts */ sz = sizeof(TransactionId) * builder->committed.xcnt; diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 8aa1f49..25ac26f 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -506,12 +506,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid, CurrentSnapshot->xmax = sourcesnap->xmax; CurrentSnapshot->xcnt = sourcesnap->xcnt; Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount()); - memcpy(CurrentSnapshot->xip, sourcesnap->xip, - sourcesnap->xcnt * sizeof(TransactionId)); + if (sourcesnap->xcnt > 0) + memcpy(CurrentSnapshot->xip, sourcesnap->xip, + sourcesnap->xcnt * sizeof(TransactionId)); CurrentSnapshot->subxcnt = sourcesnap->subxcnt; Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount()); - memcpy(CurrentSnapshot->subxip, sourcesnap->subxip, - sourcesnap->subxcnt * sizeof(TransactionId)); + if (sourcesnap->subxcnt > 0) + memcpy(CurrentSnapshot->subxip, sourcesnap->subxip, + sourcesnap->subxcnt * sizeof(TransactionId)); CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed; CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery; /* NB: curcid should NOT be copied, it's a local matter */ diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c index 0e815a9..0f26bd8 100644 --- a/src/backend/utils/time/tqual.c +++ b/src/backend/utils/time/tqual.c @@ -1639,6 +1639,7 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple) static bool TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num) { + Assert(xip != NULL); return bsearch(&xid, xip, num, sizeof(TransactionId), xidComparator) != NULL; } @@ -1675,7 +1676,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot, return false; } /* check if it's one of our txids, toplevel is also in there */ - else if (TransactionIdInArray(xmin, snapshot->subxip, snapshot->subxcnt)) + else if (snapshot->subxcnt > 0 && TransactionI
Re: [HACKERS] [COMMITTERS] pgsql: Allow Pin/UnpinBuffer to operate in a lockfree manner.
On 2016-04-12 07:00, Andres Freund wrote: On 2016-04-12 00:32:13 -0400, Tom Lane wrote: I wrote: This commit has broken buildfarm member gaur, and no doubt pademelon will be equally unhappy once it catches up to HEAD. Spoke too soon, actually: pademelon did not get as far as initdb. cc: "bufmgr.c", line 4032: error 1521: Incorrect initialization. cc: "bufmgr.c", line 4058: error 1521: Incorrect initialization. Apparently, assigning the result of init_spin_delay() is not as portable as you thought. Hm. I'm not sure why not though? Is it because delayStatus isn't static or global scope? It's because C89 requires initializers for aggregate and union types to be constant expressions (3.5.7p3): All the expressions in an initializer for an object that has static storage duration or in an initializer list for an object that has aggregate or union type shall be constant expressions. C99 removes this constraint (6.7.8p4): All the expressions in an initializer for an object that has static storage duration shall be constant expressions or string literals. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Small fix: avoid passing null pointers to memcpy()
Hello, from running the regression test suite (including TAP tests) and also sqlsmith, I've got a couple of places where UBSan reported calls to memcpy() with null pointer passed as either source or destination. Patch attached. diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c index cfb3b50..600824a 100644 --- a/contrib/pgcrypto/px.c +++ b/contrib/pgcrypto/px.c @@ -180,7 +180,7 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen, memset(ivbuf, 0, ivs); if (ivlen > ivs) memcpy(ivbuf, iv, ivs); - else + else if (ivlen > 0) memcpy(ivbuf, iv, ivlen); } diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 34ba385..67c7586 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -305,7 +305,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock) /* * copy the scan key, if appropriate */ - if (key != NULL) + if (key != NULL && scan->rs_nkeys > 0) memcpy(scan->rs_key, key, scan->rs_nkeys * sizeof(ScanKeyData)); /* diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 7e37331..e7599be 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -4872,8 +4872,9 @@ SerializeTransactionState(Size maxsize, char *start_address) { if (TransactionIdIsValid(s->transactionId)) workspace[i++] = s->transactionId; - memcpy(&workspace[i], s->childXids, - s->nChildXids * sizeof(TransactionId)); + if (s->nChildXids > 0) + memcpy(&workspace[i], s->childXids, + s->nChildXids * sizeof(TransactionId)); i += s->nChildXids; } Assert(i == nxids); diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index b88e012..dc7a323 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -402,12 +402,14 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid, CurrentSnapshot->xmax = sourcesnap->xmax; CurrentSnapshot->xcnt = sourcesnap->xcnt; Assert(sourcesnap->xcnt <= GetMaxSnapshotXidCount()); - memcpy(CurrentSnapshot->xip, sourcesnap->xip, - sourcesnap->xcnt * sizeof(TransactionId)); + if (sourcesnap->xcnt > 0) + memcpy(CurrentSnapshot->xip, sourcesnap->xip, + sourcesnap->xcnt * sizeof(TransactionId)); CurrentSnapshot->subxcnt = sourcesnap->subxcnt; Assert(sourcesnap->subxcnt <= GetMaxSnapshotSubxidCount()); - memcpy(CurrentSnapshot->subxip, sourcesnap->subxip, - sourcesnap->subxcnt * sizeof(TransactionId)); + if (sourcesnap->subxcnt > 0) + memcpy(CurrentSnapshot->subxip, sourcesnap->subxip, + sourcesnap->subxcnt * sizeof(TransactionId)); CurrentSnapshot->suboverflowed = sourcesnap->suboverflowed; CurrentSnapshot->takenDuringRecovery = sourcesnap->takenDuringRecovery; /* NB: curcid should NOT be copied, it's a local matter */ -- 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] Two division by 0 errors in optimizer/plan/planner.c and optimizer/path/costsize.c
On 2016-03-28 11:33, Aleksander Alekseev wrote: Hello, Piotr. Thanks for report. But I'm having some difficulties reproducing issues you described. Oh, if you want backtraces then either set a conditional breakpoint or add your own Assert like I did. Also it would be a good idea to include these steps to regression tests. I agree and I generally think that the more test cases touch previously not covered code paths the better, even if it had to be run as a different make(1) target. Although it seems that at least some people would agree (see [1]), the "make check" split somehow isn't happening. [1] ca+tgmoymofe94+3wg3spg9sqajkv4sixjey+rdrmamye-vq...@mail.gmail.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] AssertArg failure in src/backend/executor/functions.c:check_sql_fn_retval()
On 2016-03-27 16:40, Tom Lane wrote: Hm. I would argue that it should have rejected CAST(NULL AS ANYARRAY). That's a pseudotype and so there should never be an actual value of that type, not even a null value. I'm a little confused about what you mean here. I thought reject was exactly what's happening; normally you'd get "ERROR: return type anyarray is not supported for SQL functions". If you mean specifically to forbid CAST(NULL AS ANYARRAY) in general then I'd like to point out that there are columns of type anyarray, at least pg_catalog.pg_statistic.stavalues1 is, so the cast is not the only way to trigger this. -- 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] Two division by 0 errors in optimizer/plan/planner.c and optimizer/path/costsize.c
On 2016-03-26 19:29, Piotr Stefaniak wrote: I'm not saying this is necessarily a bug since the whole function deals with floats, but perhaps it's interesting to note that ndistinct can be 0 in src/backend/utils/adt/selfuncs.c:estimate_hash_bucketsize: On the exact same note, something like this (again reduced from what sqlsmith produced): select 1 from unnest('{}'::boolean[]) a (x) left join ( select * from unnest('{}'::boolean[]) where false ) b (x) on a.x = b.x; leads to vardata.rel->tuples being zero here: if (vardata.rel) ndistinct *= vardata.rel->rows / vardata.rel->tuples; Back-trace attached. #0 estimate_hash_bucketsize (root=0xf3cf98, hashkey=0xf44398, nbuckets=1024) at src/backend/utils/adt/selfuncs.c:3543 #1 0x007141a4 in final_cost_hashjoin (root=0xf3cf98, path=0xf47118, workspace=0x7fffd950, sjinfo=0xf45460, semifactors=0x7fffdae8) at src/backend/optimizer/path/costsize.c:2856 #2 0x0075c134 in create_hashjoin_path (root=0xf3cf98, joinrel=0xf46438, jointype=JOIN_LEFT, workspace=0x7fffd950, sjinfo=0xf45460, semifactors=0x7fffdae8, outer_path=0xf461a0, inner_path=0xf45a98, restrict_clauses=0xf466f0, required_outer=0x0, hashclauses=0xf471d8) at src/backend/optimizer/util/pathnode.c:2133 #3 0x0072074a in try_hashjoin_path (root=0xf3cf98, joinrel=0xf46438, outer_path=0xf461a0, inner_path=0xf45a98, hashclauses=0xf471d8, jointype=JOIN_LEFT, extra=0x7fffdad0) at src/backend/optimizer/path/joinpath.c:523 #4 0x007219e2 in hash_inner_and_outer (root=0xf3cf98, joinrel=0xf46438, outerrel=0xf44cd0, innerrel=0xf44fa8, jointype=JOIN_LEFT, extra=0x7fffdad0) at src/backend/optimizer/path/joinpath.c:1403 #5 0x00720058 in add_paths_to_joinrel (root=0xf3cf98, joinrel=0xf46438, outerrel=0xf44cd0, innerrel=0xf44fa8, jointype=JOIN_LEFT, sjinfo=0xf45460, restrictlist=0xf466f0) at src/backend/optimizer/path/joinpath.c:211 #6 0x00722e38 in make_join_rel (root=0xf3cf98, rel1=0xf44cd0, rel2=0xf44fa8) at src/backend/optimizer/path/joinrels.c:771 #7 0x007222dd in make_rels_by_clause_joins (root=0xf3cf98, old_rel=0xf44cd0, other_rels=0xf463b8) at src/backend/optimizer/path/joinrels.c:274 #8 0x00721f86 in join_search_one_level (root=0xf3cf98, level=2) at src/backend/optimizer/path/joinrels.c:96 #9 0x0070dc4d in standard_join_search (root=0xf3cf98, levels_needed=2, initial_rels=0xf46380) at src/backend/optimizer/path/allpaths.c:2124 #10 0x0070dbc6 in make_rel_from_joinlist (root=0xf3cf98, joinlist=0xf452c8) at src/backend/optimizer/path/allpaths.c:2055 #11 0x0070b304 in make_one_rel (root=0xf3cf98, joinlist=0xf452c8) at src/backend/optimizer/path/allpaths.c:175 #12 0x007352be in query_planner (root=0xf3cf98, tlist=0xf440b8, qp_callback=0x739ec7 , qp_extra=0x7fffde10) at src/backend/optimizer/plan/planmain.c:246 #13 0x00737d89 in grouping_planner (root=0xf3cf98, inheritance_update=0 '\000', tuple_fraction=0) at src/backend/optimizer/plan/planner.c:1673 #14 0x00736535 in subquery_planner (glob=0xf3ad88, parse=0xf3a458, parent_root=0x0, hasRecursion=0 '\000', tuple_fraction=0) at src/backend/optimizer/plan/planner.c:758 #15 0x00735688 in standard_planner (parse=0xf3a458, cursorOptions=256, boundParams=0x0) at src/backend/optimizer/plan/planner.c:307 #16 0x007353ca in planner (parse=0xf3a458, cursorOptions=256, boundParams=0x0) at src/backend/optimizer/plan/planner.c:177 #17 0x00800d28 in pg_plan_query (querytree=0xf3a458, cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:798 #18 0x00800ddb in pg_plan_queries (querytrees=0xf3cf60, cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:857 #19 0x00801080 in exec_simple_query (query_string=0xf077a8 "select 1\nfrom unnest('{}'::boolean[]) a (x)\nleft join (\n select *\n from unnest('{}'::boolean[])\n where false\n) b (x) on a.x = b.x;") at src/backend/tcop/postgres.c:1022 #20 0x00805342 in PostgresMain (argc=1, argv=0xe958d0, dbname=0xe95730 "postgres", username=0xe95710 "me") at src/backend/tcop/postgres.c:4059 #21 0x0077ed31 in BackendRun (port=0xeb2950) at src/backend/postmaster/postmaster.c:4258 #22 0x0077e495 in BackendStartup (port=0xeb2950) at src/backend/postmaster/postmaster.c:3932 #23 0x0077ac19 in ServerLoop () at src/backend/postmaster/postmaster.c:1690 #24 0x0077a24e in PostmasterMain (argc=3, argv=0xe94850) at src/backend/postmaster/postmaster.c:1298 #25 0x006c6216 in main (argc=3, argv=0xe94850) at src/backend/main/main.c:228 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] AssertArg failure in src/backend/executor/functions.c:check_sql_fn_retval()
Hi, using sqlsmith I found a way to induce an AssertArg failure in src/backend/executor/functions.c:check_sql_fn_retval() for assert-enabled builds. It boils down to creating a function and calling it like this: CREATE FUNCTION bad_argument_assert(anyarray, integer) RETURNS anyarray LANGUAGE sql AS $$select $1$$; SELECT bad_argument_assert(CAST(NULL AS ANYARRAY), 0); TRAP: BadArgument("!(!((rettype) == 2283 || (rettype) == 2277 || (rettype) == 2776 || (rettype) == 3500 || (rettype) == 3831))", File: "src/backend/executor/functions.c", Line: 1539) Back-trace attached. I've done a little bit of git-bisecting and this potential failure is there since at least February 2012. The comment above the function definition specifically says that "we should never see a polymorphic pseudotype such as ANYELEMENT as rettype" but given that a non-assert-enabled build seems to cope with such pseudotypes well, I'm unsure the AssertArg is needed there at all. #0 0x77341a98 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55 #1 0x7734369a in __GI_abort () at abort.c:89 #2 0x0094d594 in ExceptionalCondition (conditionName=0xb03c60 "!(!((rettype) == 2283 || (rettype) == 2277 || (rettype) == 2776 || (rettype) == 3500 || (rettype) == 3831))", errorType=0xb03c51 "BadArgument", fileName=0xb03890 "src/backend/executor/functions.c", lineNumber=1539) at src/backend/utils/error/assert.c:54 #3 0x006840c9 in check_sql_fn_retval (func_id=65539, rettype=2277, queryTreeList=0xf3b460, modifyTargetList=0x7fffc676 "", junkFilter=0x0) at src/backend/executor/functions.c:1539 #4 0x0075735c in inline_function (funcid=65539, result_type=2277, result_collid=0, input_collid=0, args=0xf09dd0, funcvariadic=0 '\000', func_tuple=0x77f99c30, context=0x7fffd960) at src/backend/optimizer/util/clauses.c:4706 #5 0x00756508 in simplify_function (funcid=65539, result_type=2277, result_typmod=-1, result_collid=0, input_collid=0, args_p=0x7fffc838, funcvariadic=0 '\000', process_args=1 '\001', allow_non_const=1 '\001', context=0x7fffd960) at src/backend/optimizer/util/clauses.c:4187 #6 0x00753f87 in eval_const_expressions_mutator (node=0xf09960, context=0x7fffd960) at src/backend/optimizer/util/clauses.c:2834 #7 0x006cbb2f in expression_tree_mutator (node=0xf099b8, mutator=0x7539ac , context=0x7fffd960) at src/backend/nodes/nodeFuncs.c:2640 #8 0x00755ead in eval_const_expressions_mutator (node=0xf099b8, context=0x7fffd960) at src/backend/optimizer/util/clauses.c:3806 #9 0x006cbd2a in expression_tree_mutator (node=0xf09928, mutator=0x7539ac , context=0x7fffd960) at src/backend/nodes/nodeFuncs.c:2689 #10 0x00755ead in eval_const_expressions_mutator (node=0xf09928, context=0x7fffd960) at src/backend/optimizer/util/clauses.c:3806 #11 0x00753959 in eval_const_expressions (root=0xf09ae0, node=0xf09928) at src/backend/optimizer/util/clauses.c:2676 #12 0x007365f7 in preprocess_expression (root=0xf09ae0, expr=0xf09928, kind=1) at src/backend/optimizer/plan/planner.c:831 #13 0x00735f1c in subquery_planner (glob=0xf093a0, parse=0xf09048, parent_root=0x0, hasRecursion=0 '\000', tuple_fraction=0) at src/backend/optimizer/plan/planner.c:581 #14 0x00735688 in standard_planner (parse=0xf09048, cursorOptions=256, boundParams=0x0) at src/backend/optimizer/plan/planner.c:307 #15 0x007353ca in planner (parse=0xf09048, cursorOptions=256, boundParams=0x0) at src/backend/optimizer/plan/planner.c:177 #16 0x00800d28 in pg_plan_query (querytree=0xf09048, cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:798 #17 0x00800ddb in pg_plan_queries (querytrees=0xf09aa8, cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:857 #18 0x00801080 in exec_simple_query (query_string=0xf07ee8 "select public.bad_argument_assert(CAST(NULL AS ANYARRAY), 0);") at src/backend/tcop/postgres.c:1022 #19 0x00805342 in PostgresMain (argc=1, argv=0xe95e90, dbname=0xe95cf0 "regression", username=0xe95cd0 "me") at src/backend/tcop/postgres.c:4059 #20 0x0077ed31 in BackendRun (port=0xeb6290) at src/backend/postmaster/postmaster.c:4258 #21 0x0077e495 in BackendStartup (port=0xeb6290) at src/backend/postmaster/postmaster.c:3932 #22 0x0077ac19 in ServerLoop () at src/backend/postmaster/postmaster.c:1690 #23 0x0077a24e in PostmasterMain (argc=3, argv=0xe94e10) at src/backend/postmaster/postmaster.c:1298 #24 0x006c6216 in main (argc=3, argv=0xe94e10) at src/backend/main/main.c:228 -- 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] Two division by 0 errors in optimizer/plan/planner.c and optimizer/path/costsize.c
I'm not saying this is necessarily a bug since the whole function deals with floats, but perhaps it's interesting to note that ndistinct can be 0 in src/backend/utils/adt/selfuncs.c:estimate_hash_bucketsize: /* * Initial estimate of bucketsize fraction is 1/nbuckets as long as the * number of buckets is less than the expected number of distinct values; * otherwise it is 1/ndistinct. */ if (ndistinct > nbuckets) estfract = 1.0 / nbuckets; else estfract = 1.0 / ndistinct; for this query: select subq_0.c1 as c0 from ( select ref_0.a as c0, (select NULL::integer from information_schema.user_defined_types limit 1 offset 1) as c1 from public.rtest_nothn3 as ref_0 limit 130 ) as subq_0 left join ( select sample_0.x as c0 from public.insert_tbl as sample_0 where false ) as subq_1 on subq_0.c1 = subq_1.c0; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Two division by 0 errors in optimizer/plan/planner.c and optimizer/path/costsize.c
Hi, using sqlsmith and UBSan I have found these two division by zero errors: src/backend/optimizer/plan/planner.c:4846 /* Convert absolute # of tuples to a fraction; no need to clamp */ if (tuple_fraction >= 1.0) { tuple_fraction /= best_path->rows; } and src/backend/optimizer/path/costsize.c:3029 if (subplan->subLinkType == EXISTS_SUBLINK) { /* we only need to fetch 1 tuple */ sp_cost.per_tuple += plan_run_cost / plan->plan_rows; } The first is triggered by this query (reduced by me from the original query string generated by sqlsmith): select 1 from ( select ref_0.location as c0 from public.city as ref_0 ) as subq_0 where EXISTS ( select 1 from ( select sample_0.collname as c0 from pg_catalog.pg_collation as sample_0 ) as subq_1 right join public.tt5 as ref_2 inner join pg_catalog.pg_constraint as ref_4 on (ref_2.z = ref_4.coninhcount ) on (subq_1.c0 = ref_4.conname ), lateral ( select 1 from public.shoelace_candelete as ref_5 where false ) as subq_2 ); #0 get_cheapest_fractional_path (rel=0x77ec32a8, tuple_fraction=1) at src/backend/optimizer/plan/planner.c:4846 #1 0x007422a1 in make_subplan (root=0xf49778, orig_subquery=0x77f593c8, subLinkType=EXISTS_SUBLINK, subLinkId=0, testexpr=0x0, isTopQual=1 '\001') at src/backend/optimizer/plan/subselect.c:546 #2 0x0074470d in process_sublinks_mutator (node=0x77f610b0, context=0x7fffd900) at src/backend/optimizer/plan/subselect.c:1974 #3 0x00744670 in SS_process_sublinks (root=0xf49778, expr=0x77f610b0, isQual=1 '\001') at src/backend/optimizer/plan/subselect.c:1947 #4 0x00736621 in preprocess_expression (root=0xf49778, expr=0x77f610b0, kind=0) at src/backend/optimizer/plan/planner.c:848 #5 0x00736700 in preprocess_qual_conditions (root=0xf49778, jtnode=0xf5f790) at src/backend/optimizer/plan/planner.c:893 #6 0x00735ff3 in subquery_planner (glob=0xf3ef70, parse=0xf3e9a0, parent_root=0x0, hasRecursion=0 '\000', tuple_fraction=0) at src/backend/optimizer/plan/planner.c:600 #7 0x0073566b in standard_planner (parse=0xf3e9a0, cursorOptions=256, boundParams=0x0) at src/backend/optimizer/plan/planner.c:307 #8 0x007353ad in planner (parse=0xf3e9a0, cursorOptions=256, boundParams=0x0) at src/backend/optimizer/plan/planner.c:177 #9 0x00800d3b in pg_plan_query (querytree=0xf3e9a0, cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:798 #10 0x00800dee in pg_plan_queries (querytrees=0xf53648, cursorOptions=256, boundParams=0x0) at src/backend/tcop/postgres.c:857 #11 0x00801093 in exec_simple_query (query_string=0xf07dd8 "select 1\nfrom (\n select ref_0.location as c0\n from public.city as ref_0\n) as subq_0\nwhere EXISTS (\n select 1\n from (\nselect sample_0.collname as c0\nfrom pg_catalog.pg_collation as sample_0\n ) as subq_1\n right join public.tt5 as ref_2\ninner join pg_catalog.pg_constraint as ref_4\non (ref_2.z = ref_4.coninhcount )\n on (subq_1.c0 = ref_4.conname ),\n lateral (\nselect 1\n from public.shoelace_candelete as ref_5\nwhere false\n ) as subq_2\n);") at src/backend/tcop/postgres.c:1022 #12 0x00805355 in PostgresMain (argc=1, argv=0xe95ee0, dbname=0xe95d40 "regression", username=0xe95d20 "me") at src/backend/tcop/postgres.c:4059 #13 0x0077ed44 in BackendRun (port=0xeb2f80) at src/backend/postmaster/postmaster.c:4258 #14 0x0077e4a8 in BackendStartup (port=0xeb2f80) at src/backend/postmaster/postmaster.c:3932 #15 0x0077ac2c in ServerLoop () at src/backend/postmaster/postmaster.c:1690 #16 0x0077a261 in PostmasterMain (argc=5, argv=0xe94e10) at src/backend/postmaster/postmaster.c:1298 #17 0x006c623c in main (argc=5, argv=0xe94e10) at src/backend/main/main.c:228 The second one is triggered by this (again, reduced from the original): select 1 from public.tt5 as subq_0 where EXISTS ( select 1 from public.b_star as ref_0 where false ); #0 cost_subplan (root=0xf3e718, subplan=0xf42780, plan=0xf3fcd8) at src/backend/optimizer/path/costsize.c:3029 #1 0x00742eb9 in build_subplan (root=0xf3e718, plan=0xf3fcd8, subroot=0xf3f6a8, plan_params=0x0, subLinkType=EXISTS_SUBLINK, subLinkId=0, testexpr=0x0, adjust_testexpr=1 '\001', unknownEqFalse=1 '\001') at src/backend/optimizer/plan/subselect.c:887 #2 0x007422c0 in make_subplan (root=0xf3e718, orig_subquery=0xf09628, subLinkType=EXISTS_SUBLINK, subLinkId=0, testexpr=0x0, isTopQual=1 '\001') at src/backend/optimizer/plan/subselect.c:551 #3 0x007446d7 in process_sublinks_mutator (node=0xf3f100, context=0x7fffd900) at src/backend/optimizer/plan/subselect.c:1974 #4 0x0074463a in SS_process_sublinks (root=0xf3e718, expr=0xf3f100,
Re: [HACKERS] Proposal: Trigonometric functions in degrees
On 01/31/2016 01:23 PM, Michael Paquier wrote: Per IEEE 754, division by 0 for a double results in Nan or +/-Inf, so that's actually correct. I didn't know that. I guess that in practice that is OK and the case is closed. Interestingly to me, that assumption appears to rely on the C implementation complying to IEC 60559, in which case C99 lets the implementation signal that by defining the __STDC_IEC_559__ macro. C89 doesn't seem to mention any of this. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: Trigonometric functions in degrees
These changes from 65abaab547a5758b0d6d92df4af1663bb47d545f - result = sign * cosd_q1(arg1) / sind_q1(arg1); + result = sign * ((cosd_q1(arg1) / sind_q1(arg1)) / cot_45); and - result = sign * sind_q1(arg1) / cosd_q1(arg1); + result = sign * ((sind_q1(arg1) / cosd_q1(arg1)) / tan_45); both introduce division by zero, don't they? -- 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] Releasing in September
On 01/26/2016 02:09 AM, Peter Eisentraut wrote: > On 1/25/16 2:48 AM, Torsten Zühlsdorff wrote: >> In FreeBSD for example there is an online tool for review >> (http://review.freebsd.org) which was opened to public. There you can >> review any code, left comments in the parts you wanted, submit different >> users to it etc. > I agree better code review tooling could help a bit. The URL you post > above doesn't work at the moment (for me), though. That's a typo, it's reviews.freebsd.org. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add STRICT to some regression test C functions.
On 01/09/2016 12:05 AM, Andreas Seltenreich wrote: Maybe someone can sneak this patch in? Exactly this is done by a part of another patch, by Michael Paquier, that he sent to an off-list thread. Michael had asked for feedback there, but I didn't have the time to test the patch. Just from reading it I think it's good, though. And it still applies and passes HEAD's make check-world tests. -- 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] How to use Makefile - pgxs without gcc -O2 ?
On 07/08/2014 17:53, geohas wrote: > make CFLAGS=-O0 was it. > > now gdb doesn't print . If you're using GCC 4.8 or later, consider using it with -Og for that kind of builds. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Keepalive-related socket options under FreeBSD 9, 10
Since upgrading FreeBSD from 8 to 9, I've noticed the following messages showing up in logs when a connection with pgAdmin3 is made: LOG: getsockopt(TCP_KEEPCNT) failed: Protocol not available STATEMENT: SELECT setting FROM pg_settings WHERE name IN ('autovacuum', 'track_counts') LOG: getsockopt(TCP_KEEPIDLE) failed: Protocol not available STATEMENT: SELECT setting FROM pg_settings WHERE name IN ('autovacuum', 'track_counts') LOG: getsockopt(TCP_KEEPINTVL) failed: Protocol not available STATEMENT: SELECT setting FROM pg_settings WHERE name IN ('autovacuum', 'track_counts') tcp_keepalives_idle, tcp_keepalives_interval, and tcp_keepalives_count are all set to the default (0), which means "system default". My guess as to what causes this: src/backend/libpq/pqcomm.c apparently assumes that if TCP_KEEPIDLE & friends are defined, then the respective options are readable, but according to man tcp, that is not the case for FreeBSD 9 (and 10): TCP_KEEPINIT This write-only setsockopt(2) option accepts a per-socket timeout argument of u_int in seconds, for new, non-estab- lished TCP connections. For the global default in mil- liseconds see keepinit in the MIB Variables section fur- ther down. As a work-around, I've set the keepalive options to the system defaults provided by man tcp. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 03/22/2014 04:00 PM, Tom Lane wrote: On the other side, coding with the explicit cast helps guard against far more dangerous coding errors, which the compiler will*not* help you with. What if myextra is actually of type "int64 *"? Indeed, neither "gcc -Wall -Wextra -std=c89 -pedantic" nor "clang -Weverything -Wno-shadow -std=c89 -pedantic" issues a warning in such case. "clang --analyze", however, does. Perhaps TenDRA would, if it ever worked. This message is meant to be merely informative, since I've put some effort into this test. I'm not trying to argue. #include typedef long int int64; int main(void) { int *myextra; /* with explicit casting */ myextra = (int *) malloc(sizeof (int)); free(myextra); /* with no explicit casting */ myextra = malloc(sizeof (int)); free(myextra); /* myextra now becomes int64 */ { int64 *myextra; /* with explicit casting */ myextra = (int *) malloc(sizeof (int)); /* [1], [2]. and [3] warn here */ free(myextra); /* with no explicit casting */ myextra = malloc(sizeof (int)); /* Only [3] warns here */ free(myextra); } return 0; } /* 1: gcc 4.8.2: gcc -Wall -Wextra -std=c89 -pedantic /tmp/test.c 2: clang 3.5.0: clang -Weverything -Wno-shadow -std=c89 -pedantic /tmp/test.c 3: clang 3.5.0: clang --analyze /tmp/test.c */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
On 03/22/2014 04:00 PM, Tom Lane wrote: That argument is entirely bogus, as it considers only one possible way in which the call could be wrong; a way that is of very low probability in PG usage, since we include in our core headers. Besides which, as noted in the page itself, most modern compilers would warn anyway if you forgot the inclusion. Apart from what the page says, I also think of casting malloc() as bad style and felt the need to bring this up. But since you pointed out why you don't want to remove the cast, I withdraw my previous suggestion. On the other side, coding with the explicit cast helps guard against far more dangerous coding errors, which the compiler will *not* help you with. What if myextra is actually of type "int64 *"? In that case you probably meant to make enough space for an int64 not an int. But without the cast, you won't be told you did anything wrong. This is a particular hazard if you change your mind later on about the type of myextra. (A colleague at Salesforce got burnt in exactly that way, just a couple days ago.) So perhaps this alternative: myextra = malloc(sizeof *myextra); PS. Coding style matters to me, but I was and still am far from insisting on anything. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: plpgsql.extra_warnings, plpgsql.extra_errors
> +myextra = (int *) malloc(sizeof(int)); Please consider not casting malloc(). See http://c-faq.com/malloc/mallocnocast.html -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers