[sqlsmith] Planner crash on foreign table join
Hi,
testing master at f0e44021df with a loopback postgres_fdw installed, I
see lots of crashes on queries joining foreign tables with various
expressions. Below is a reduced recipe for the regression database and
a backtrace.
regards,
Andreas
--8<---------------cut here---------------start------------->8---
create extension postgres_fdw;
create server myself foreign data wrapper postgres_fdw;
create schema fdw_postgres;
create user fdw login;
grant all on schema public to fdw;
grant all on all tables in schema public to fdw;
create user mapping for public server myself options (user 'fdw');
import foreign schema public from server myself into fdw_postgres;
explain select from
fdw_postgres.hslot
left join fdw_postgres.num_exp_div
on ((exists (values (1))) and (values (1)) is null);
--8<---------------cut here---------------end--------------->8---
Program terminated with signal SIGSEGV, Segmentation fault.
#0 bms_get_singleton_member (a=0x10, member=member@entry=0x7fffb577cafc) at bitmapset.c:577
#1 0x000056425107b531 in find_relation_from_clauses (clauses=0x564251a68570, root=0x564251a273d8) at clausesel.c:445
#2 clauselist_selectivity (root=root@entry=0x564251a273d8, clauses=0x564251a68570, varRelid=varRelid@entry=0, jointype=JOIN_LEFT, sjinfo=0x564251a661c0) at clausesel.c:128
#3 0x00007f61d3d9f22f in postgresGetForeignJoinPaths (root=<optimized out>, joinrel=0x564251a66ba8, outerrel=<optimized out>, innerrel=<optimized out>, jointype=<optimized out>, extra=0x7fffb577cc50) at postgres_fdw.c:4466
#4 0x000056425108a238 in add_paths_to_joinrel (root=root@entry=0x564251a273d8, joinrel=joinrel@entry=0x564251a66ba8, outerrel=outerrel@entry=0x564251a65378, innerrel=innerrel@entry=0x564251a65f30, jointype=jointype@entry=JOIN_LEFT, sjinfo=sjinfo@entry=0x564251a661c0, restrictlist=0x564251a681c8) at joinpath.c:278
#5 0x000056425108bff2 in populate_joinrel_with_paths (restrictlist=<optimized out>, sjinfo=0x564251a661c0, joinrel=0x564251a66ba8, rel2=0x564251a65f30, rel1=0x564251a65378, root=0x564251a273d8) at joinrels.c:795
#6 make_join_rel (root=root@entry=0x564251a273d8, rel1=rel1@entry=0x564251a65378, rel2=rel2@entry=0x564251a65f30) at joinrels.c:731
#7 0x000056425108c7ef in make_rels_by_clause_joins (other_rels=<optimized out>, old_rel=<optimized out>, root=<optimized out>) at joinrels.c:277
#8 join_search_one_level (root=root@entry=0x564251a273d8, level=level@entry=2) at joinrels.c:99
#9 0x0000564251079bdb in standard_join_search (root=0x564251a273d8, levels_needed=2, initial_rels=<optimized out>) at allpaths.c:2385
#10 0x000056425107ac7b in make_one_rel (root=root@entry=0x564251a273d8, joinlist=joinlist@entry=0x564251a65998) at allpaths.c:184
#11 0x0000564251099ef4 in query_planner (root=root@entry=0x564251a273d8, tlist=tlist@entry=0x0, qp_callback=qp_callback@entry=0x56425109aeb0 <standard_qp_callback>, qp_extra=qp_extra@entry=0x7fffb577cff0) at planmain.c:253
#12 0x000056425109dbc2 in grouping_planner (root=root@entry=0x564251a273d8, inheritance_update=inheritance_update@entry=0 '\000', tuple_fraction=<optimized out>, tuple_fraction@entry=0) at planner.c:1684
#13 0x00005642510a0133 in subquery_planner (glob=glob@entry=0x564251a2a6d0, parse=parse@entry=0x5642519aac60, parent_root=parent_root@entry=0x0, hasRecursion=hasRecursion@entry=0 '\000', tuple_fraction=tuple_fraction@entry=0) at planner.c:833
#14 0x00005642510a0f71 in standard_planner (parse=0x5642519aac60, cursorOptions=256, boundParams=0x0) at planner.c:333
#15 0x00005642511458cd in pg_plan_query (querytree=querytree@entry=0x5642519aac60, cursorOptions=256, boundParams=boundParams@entry=0x0) at postgres.c:802
#16 0x0000564250fa9a40 in ExplainOneQuery (query=0x5642519aac60, cursorOptions=<optimized out>, into=0x0, es=0x564251a513a0, queryString=0x564251a09590 "explain select from\n fdw_postgres.hslot\n left join fdw_postgres.num_exp_div\n on ((exists (values (1))) and (values (1)) is null);", params=0x0, queryEnv=0x0) at explain.c:367
#17 0x0000564250faa005 in ExplainQuery (pstate=pstate@entry=0x564251a511f0, stmt=stmt@entry=0x564251a0aa58, queryString=queryString@entry=0x564251a09590 "explain select from\n fdw_postgres.hslot\n left join fdw_postgres.num_exp_div\n on ((exists (values (1))) and (values (1)) is null);", params=params@entry=0x0, queryEnv=queryEnv@entry=0x0, dest=dest@entry=0x564251a51308) at explain.c:256
#18 0x000056425114b9cb in standard_ProcessUtility (pstmt=0x564251a0b2e0, queryString=0x564251a09590 "explain select from\n fdw_postgres.hslot\n left join fdw_postgres.num_exp_div\n on ((exists (values (1))) and (values (1)) is null);", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x564251a51308, completionTag=0x7fffb577d390 "") at utility.c:680
#19 0x00005642511487b4 in PortalRunUtility (portal=0x564251a07580, pstmt=0x564251a0b2e0, isTopLevel=<optimized out>, setHoldSnapshot=<optimized out>, dest=<optimized out>, completionTag=0x7fffb577d390 "") at pquery.c:1179
#20 0x0000564251149633 in FillPortalStore (portal=portal@entry=0x564251a07580, isTopLevel=isTopLevel@entry=1 '\001') at pquery.c:1039
#21 0x000056425114a21d in PortalRun (portal=portal@entry=0x564251a07580, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=1 '\001', run_once=run_once@entry=1 '\001', dest=dest@entry=0x564251a0b378, altdest=altdest@entry=0x564251a0b378, completionTag=0x7fffb577d5b0 "") at pquery.c:769
#22 0x0000564251145d8a in exec_simple_query (query_string=0x564251a09590 "explain select from\n fdw_postgres.hslot\n left join fdw_postgres.num_exp_div\n on ((exists (values (1))) and (values (1)) is null);") at postgres.c:1105
#23 0x0000564251147ab1 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x5642519b2e00, dbname=<optimized out>, username=<optimized out>) at postgres.c:4075
#24 0x0000564250e5c4cc in BackendRun (port=0x5642519a7d70) at postmaster.c:4317
#25 BackendStartup (port=0x5642519a7d70) at postmaster.c:3989
#26 ServerLoop () at postmaster.c:1729
#27 0x00005642510d07c3 in PostmasterMain (argc=3, argv=0x5642519844d0) at postmaster.c:1337
#28 0x0000564250e5db2d in main (argc=3, argv=0x5642519844d0) at 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
"Andreas" == Andreas Seltenreich <seltenreich@gmx.de> writes:
Andreas> Hi,
Andreas> testing master at f0e44021df with a loopback postgres_fdw
Andreas> installed, I see lots of crashes on queries joining foreign
Andreas> tables with various expressions. Below is a reduced recipe
Andreas> for the regression database and a backtrace.
Commit ac2b095088 assumes that clauselist_selectivity is being passed a
list of RelOptInfo, but postgres_fdw is passing it a list of bare
clauses. One of them is wrong :-)
--
Andrew (irc:RhodiumToad)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
"Andreas" == Andreas Seltenreich <seltenreich@gmx.de> writes:
Andreas> testing master at f0e44021df with a loopback postgres_fdw
Andreas> installed, I see lots of crashes on queries joining foreign
Andreas> tables with various expressions. Below is a reduced recipe
Andreas> for the regression database and a backtrace.
Commit ac2b095088 assumes that clauselist_selectivity is being passed a
list of RelOptInfo, but postgres_fdw is passing it a list of bare
clauses. One of them is wrong :-)
It's a bit scary that apparently none of the committed regression tests
caught that.
More generally, I think the convention up to now has been that
clauselist_selectivity would work on either RestrictInfos or bare boolean
clauses, caching its results in the former case but succeeding anyway.
If we're to standardize on only one of those behaviors it should certainly
be the former, but I think postgres_fdw is probably not the only code that
will be broken if we remove the option for the latter.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
Commit ac2b095088 assumes that clauselist_selectivity is being passed a
list of RelOptInfo, but postgres_fdw is passing it a list of bare
clauses. One of them is wrong :-)
It's a bit scary that apparently none of the committed regression tests
caught that.
Experimentation shows that actually, the standard regression tests
provide dozens of opportunities for find_relation_from_clauses to fail on
non-RestrictInfo input. However, it lacks any IsA check, and the only
thing that it does with the alleged rinfo is
if (bms_get_singleton_member(rinfo->clause_relids, &relid))
As long as there's some kind of object pointer where the clause_relids
field would be, it's highly likely that bms_get_singleton_member will just
return false without crashing, thereby obscuring the fault. Andreas'
example kills it by causing the argument to be a Param node, whose field
layout doesn't put a pointer there.
This makes me wonder whether we were being penny-wise and pound-foolish
by not making Bitmapsets be a kind of Node, so that there could be IsA
assertions in the bitmapset.c routines, as there are for Lists. Most
Bitmapsets in a typical backend probably have only one payload word
(ie highest member < 32), so right now they occupy 8 bytes. Adding
a nodetag would kick them up to the next palloc category, 16 bytes,
which is probably why I didn't do it like that to begin with.
Still, that decision is looking unduly byte-miserly in 2017.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> Experimentation shows that actually, the standard regression tests
Tom> provide dozens of opportunities for find_relation_from_clauses to
Tom> fail on non-RestrictInfo input. However, it lacks any IsA check,
In a discussion with Andres on the hash grouping sets review thread, I
proposed that we should have something of the form
#define lfirst_node(_type_, l) (castNode(_type_,lfirst(l)))
to replace the current idiom of
foreach(l, blah)
{
SomeType *x = (SomeType *) lfirst(l);
(in my code I tend to omit the (SomeType *), which I dislike because it
adds no real protection)
by
foreach(l, blah)
{
SomeType *x = lfirst_node(SomeType, l);
in order to get that IsA check in there in a convenient way.
--
Andrew (irc:RhodiumToad)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
This makes me wonder whether we were being penny-wise and pound-foolish
by not making Bitmapsets be a kind of Node, so that there could be IsA
assertions in the bitmapset.c routines, as there are for Lists. Most
Bitmapsets in a typical backend probably have only one payload word
(ie highest member < 32), so right now they occupy 8 bytes. Adding
a nodetag would kick them up to the next palloc category, 16 bytes,
which is probably why I didn't do it like that to begin with.
Still, that decision is looking unduly byte-miserly in 2017.
I think it's pretty dubious to change this, honestly. Just because it
would have caught this one bug doesn't make it an especially valuable
thing in general. Bytes are still not free.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
"Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> Experimentation shows that actually, the standard regression tests
Tom> provide dozens of opportunities for find_relation_from_clauses to
Tom> fail on non-RestrictInfo input. However, it lacks any IsA check,
In a discussion with Andres on the hash grouping sets review thread, I
proposed that we should have something of the form
#define lfirst_node(_type_, l) (castNode(_type_,lfirst(l)))
That seems like a fairly good idea. A significant fraction of the
existing castNode() calls are being applied to lfirst(something),
and this would shorten that idiom a bit.
There's another noticeable fraction that are being applied to
linitial(something), but I'm not sure if defining linitial_node()
is worth the trouble.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
This makes me wonder whether we were being penny-wise and pound-foolish
by not making Bitmapsets be a kind of Node, so that there could be IsA
assertions in the bitmapset.c routines, as there are for Lists.
I think it's pretty dubious to change this, honestly. Just because it
would have caught this one bug doesn't make it an especially valuable
thing in general. Bytes are still not free.
Yeah, true. OTOH I recall Andres lobbying to change the bitmap word
size to 64 bits on 64-bit hardware, and it *would* be free in that case
due to alignment padding.
What I think I might do is write a trial patch that turns Bitmapsets
into Nodes, and see if it catches any other existing bugs. If it does
not, that would be good evidence for your position.
We could also consider installing the nodetag only in Assert-enabled
builds, although that approach would prevent us from applying followon
simplifications such as not having to treat bitmapset fields specially
in copyfuncs.c and like places.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-04-08 17:20:28 -0400, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
This makes me wonder whether we were being penny-wise and pound-foolish
by not making Bitmapsets be a kind of Node, so that there could be IsA
assertions in the bitmapset.c routines, as there are for Lists.I think it's pretty dubious to change this, honestly. Just because it
would have caught this one bug doesn't make it an especially valuable
thing in general. Bytes are still not free.Yeah, true. OTOH I recall Andres lobbying to change the bitmap word
size to 64 bits on 64-bit hardware, and it *would* be free in that case
due to alignment padding.
Hah, yes, I did. A loong time ago ;) I still think it's a good idea, and
probably has become more useful with just about anyone using 64bits
these days. Also interesting for tidbitmap, which reuses bitmapset's
bitmapword.
We could also consider installing the nodetag only in Assert-enabled
builds, although that approach would prevent us from applying followon
simplifications such as not having to treat bitmapset fields specially
in copyfuncs.c and like places.
Yea, don't like this much.
- Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think it's pretty dubious to change this, honestly. Just because it
would have caught this one bug doesn't make it an especially valuable
thing in general. Bytes are still not free.
What I think I might do is write a trial patch that turns Bitmapsets
into Nodes, and see if it catches any other existing bugs. If it does
not, that would be good evidence for your position.
I made the attached quick-hack patch, and found that check-world
passes just fine with it. That's not complete proof that we have
no other bugs of this ilk, but it definitely supports the idea
that we don't really need to add the overhead. I'll just put this
in the archives for possible future reference.
(Or perhaps Andreas would like to try bashing on a copy with this
installed.)
regards, tom lane
Attachments:
use-isa-checks-for-bitmapsets.patchtext/x-diff; charset=us-ascii; name=use-isa-checks-for-bitmapsets.patchDownload+45-0
On Apr 8, 2017, at 5:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think it's pretty dubious to change this, honestly. Just because it
would have caught this one bug doesn't make it an especially valuable
thing in general. Bytes are still not free.What I think I might do is write a trial patch that turns Bitmapsets
into Nodes, and see if it catches any other existing bugs. If it does
not, that would be good evidence for your position.I made the attached quick-hack patch, and found that check-world
passes just fine with it.
Not so for me. I get a failure almost immediately:
Running in no-clean mode. Mistakes will not be cleaned up.
The files belonging to this database system will be owned by user "mark".
This user must also own the server process.
The database cluster will be initialized with locales
COLLATE: en_US.UTF-8
CTYPE: en_US.UTF-8
MESSAGES: C
MONETARY: en_US.UTF-8
NUMERIC: en_US.UTF-8
TIME: en_US.UTF-8
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".
Data page checksums are disabled.
creating directory /Users/mark/hydra/postgresql/src/test/regress/./tmp_check/data ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... TRAP: FailedAssertion("!(((((const Node*)(a))->type) == T_Bitmapset))", File: "bitmapset.c", Line: 731)
child process was terminated by signal 6: Abort trap
initdb: data directory "/Users/mark/hydra/postgresql/src/test/regress/./tmp_check/data" not removed at user's request
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Apr 8, 2017, at 6:38 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
On Apr 8, 2017, at 5:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think it's pretty dubious to change this, honestly. Just because it
would have caught this one bug doesn't make it an especially valuable
thing in general. Bytes are still not free.What I think I might do is write a trial patch that turns Bitmapsets
into Nodes, and see if it catches any other existing bugs. If it does
not, that would be good evidence for your position.I made the attached quick-hack patch, and found that check-world
passes just fine with it.Not so for me. I get a failure almost immediately:
I recant. Looks like I didn't get the patch applied quite right. So sorry for the noise.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Apr 8, 2017, at 6:48 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
On Apr 8, 2017, at 6:38 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
On Apr 8, 2017, at 5:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think it's pretty dubious to change this, honestly. Just because it
would have caught this one bug doesn't make it an especially valuable
thing in general. Bytes are still not free.What I think I might do is write a trial patch that turns Bitmapsets
into Nodes, and see if it catches any other existing bugs. If it does
not, that would be good evidence for your position.I made the attached quick-hack patch, and found that check-world
passes just fine with it.Not so for me. I get a failure almost immediately:
I recant. Looks like I didn't get the patch applied quite right. So sorry for the noise.
The regression tests now fail on a number of tests due to a server crash:
2017-04-08 18:55:19.826 PDT [90779] pg_regress/errors STATEMENT: select infinite_recurse();
TRAP: FailedAssertion("!(((((const Node*)(a))->type) == T_Bitmapset))", File: "bitmapset.c", Line: 601)
2017-04-08 18:55:22.487 PDT [90242] LOG: server process (PID 90785) was terminated by signal 6: Abort trap
2017-04-08 18:55:22.487 PDT [90242] DETAIL: Failed process was running: explain (costs off)
select * from onek2 where unique2 = 11 and stringu1 = 'ATAAAA';
This is very near where the original crash reported in this thread was crashing, probably only
different due to the extra lines of Assert that were added. Am I missing some portion of the
fix that you are testing? I have only applied the patch that Tom included in the previous email.
mark
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Mark Dilger <hornschnorter@gmail.com> writes:
This is very near where the original crash reported in this thread was crashing, probably only
different due to the extra lines of Assert that were added. Am I missing some portion of the
fix that you are testing? I have only applied the patch that Tom included in the previous email.
No, that was the whole patch --- but did you do a full "make clean" and
rebuild? The patch changed NodeTag numbers which would affect the whole
tree.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Apr 8, 2017, at 7:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Mark Dilger <hornschnorter@gmail.com> writes:
This is very near where the original crash reported in this thread was crashing, probably only
different due to the extra lines of Assert that were added. Am I missing some portion of the
fix that you are testing? I have only applied the patch that Tom included in the previous email.No, that was the whole patch --- but did you do a full "make clean" and
rebuild? The patch changed NodeTag numbers which would affect the whole
tree.
I had trouble applying your patch using
mark$ patch -p 1 < patch
patching file src/backend/nodes/bitmapset.c
Hunk #1 FAILED at 115.
Hunk #2 FAILED at 146.
Hunk #3 FAILED at 190.
Hunk #4 FAILED at 205.
Hunk #5 FAILED at 229.
Hunk #6 FAILED at 265.
Hunk #7 FAILED at 298.
Hunk #8 FAILED at 324.
Hunk #9 FAILED at 364.
Hunk #10 FAILED at 444.
Hunk #11 FAILED at 463.
Hunk #12 FAILED at 488.
Hunk #13 FAILED at 517.
Hunk #14 FAILED at 554.
Hunk #15 FAILED at 598.
Hunk #16 FAILED at 635.
Hunk #17 FAILED at 665.
Hunk #18 FAILED at 694.
Hunk #19 FAILED at 732.
Hunk #20 FAILED at 770.
Hunk #21 FAILED at 789.
Hunk #22 FAILED at 825.
Hunk #23 FAILED at 853.
Hunk #24 FAILED at 878.
Hunk #25 FAILED at 927.
Hunk #26 FAILED at 981.
Hunk #27 FAILED at 1027.
27 out of 27 hunks FAILED -- saving rejects to file src/backend/nodes/bitmapset.c.rej
patching file src/include/nodes/bitmapset.h
Hunk #1 FAILED at 20.
Hunk #2 FAILED at 38.
2 out of 2 hunks FAILED -- saving rejects to file src/include/nodes/bitmapset.h.rej
patching file src/include/nodes/nodes.h
Hunk #1 FAILED at 291.
1 out of 1 hunk FAILED -- saving rejects to file src/include/nodes/nodes.h.rej
So I did the patching manually against my copy of the current master,
aba696d1af9a267eee85d69845c3cdeccf788525, and then ran:
make distclean; ./configure --enable-cassert --enable-tap-tests --enable-depend && make -j4 && make check-world
I am attaching my patch, which I admit on closer inspection differs from yours, but not
in any way I can tell is relevant:
Attachments:
patch.mark.1application/octet-stream; name=patch.mark.1Download+45-0
On Apr 8, 2017, at 7:41 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
On Apr 8, 2017, at 7:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Mark Dilger <hornschnorter@gmail.com> writes:
This is very near where the original crash reported in this thread was crashing, probably only
different due to the extra lines of Assert that were added. Am I missing some portion of the
fix that you are testing? I have only applied the patch that Tom included in the previous email.No, that was the whole patch --- but did you do a full "make clean" and
rebuild? The patch changed NodeTag numbers which would affect the whole
tree.<snip>
I'm going to pull completely fresh sources and reapply and retest, though you are welcome
to review my patch while I do that.
That fixed it. Thanks.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane writes:
I made the attached quick-hack patch, and found that check-world
passes just fine with it. That's not complete proof that we have
no other bugs of this ilk, but it definitely supports the idea
that we don't really need to add the overhead. I'll just put this
in the archives for possible future reference.(Or perhaps Andreas would like to try bashing on a copy with this
installed.)
I certainly do :-). SQLsmith has been fuzzing for couple hours with the
patch applied, and so far none of the assertions fired. I'll leave the
patch on my fuzzing branch until merging becomes burdensome.
regards,
Andreas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Apr 9, 2017 at 8:27 AM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
foreach(l, blah)
{
SomeType *x = (SomeType *) lfirst(l);(in my code I tend to omit the (SomeType *), which I dislike because it
adds no real protection)
Just BTW, without that cast it's not compilable as C++, so I'm
guessing that Peter E will finish up putting it back in wherever you
leave it out...
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"Thomas" == Thomas Munro <thomas.munro@enterprisedb.com> writes:
SomeType *x = (SomeType *) lfirst(l);
(in my code I tend to omit the (SomeType *), which I dislike because
it adds no real protection)
Thomas> Just BTW, without that cast it's not compilable as C++, so I'm
Thomas> guessing that Peter E will finish up putting it back in
Thomas> wherever you leave it out...
There's north of 150 other examples (just grep for '= lfirst' in the
source). Some were even committed by Peter E :-)
In the discussion with Andres the same point came up for palloc, for
which I suggested we add something along the lines of:
#define palloc_object(_type_) (_type_ *) palloc(sizeof(_type_))
#define palloc_array(_type_, n) (_type_ *) palloc((n) * sizeof(_type_))
palloc() without a cast is even more common than lfirst() without one,
and something like half of those (and 80%+ of the pallocs that do have a
cast) are palloc(sizeof(...)) or palloc(something * sizeof(...)).
--
Andrew (irc:RhodiumToad)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
In the discussion with Andres the same point came up for palloc, for
which I suggested we add something along the lines of:
#define palloc_object(_type_) (_type_ *) palloc(sizeof(_type_))
#define palloc_array(_type_, n) (_type_ *) palloc((n) * sizeof(_type_))
I'm far less excited about that, mainly because you'd have to also cover
palloc0, repalloc, MemoryContextAlloc, etc etc. Also I've not seen
very many actual bugs that this would've helped with.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers