Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

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

diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index bf8545d..39d7b41 100644
*** a/src/backend/nodes/bitmapset.c
--- b/src/backend/nodes/bitmapset.c
*************** bms_copy(const Bitmapset *a)
*** 115,120 ****
--- 115,121 ----

      if (a == NULL)
          return NULL;
+     Assert(IsA(a, Bitmapset));
      size = BITMAPSET_SIZE(a->nwords);
      result = (Bitmapset *) palloc(size);
      memcpy(result, a, size);
*************** bms_equal(const Bitmapset *a, const Bitm
*** 145,150 ****
--- 146,153 ----
      }
      else if (b == NULL)
          return bms_is_empty(a);
+     Assert(IsA(a, Bitmapset));
+     Assert(IsA(b, Bitmapset));
      /* Identify shorter and longer input */
      if (a->nwords <= b->nwords)
      {
*************** bms_make_singleton(int x)
*** 187,192 ****
--- 190,196 ----
      wordnum = WORDNUM(x);
      bitnum = BITNUM(x);
      result = (Bitmapset *) palloc0(BITMAPSET_SIZE(wordnum + 1));
+     result->type = T_Bitmapset;
      result->nwords = wordnum + 1;
      result->words[wordnum] = ((bitmapword) 1 << bitnum);
      return result;
*************** void
*** 201,207 ****
--- 205,214 ----
  bms_free(Bitmapset *a)
  {
      if (a)
+     {
+         Assert(IsA(a, Bitmapset));
          pfree(a);
+     }
  }


*************** bms_union(const Bitmapset *a, const Bitm
*** 222,227 ****
--- 229,236 ----
      int            otherlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
          return bms_copy(b);
*************** bms_intersect(const Bitmapset *a, const
*** 256,261 ****
--- 265,272 ----
      int            resultlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL || b == NULL)
          return NULL;
*************** bms_difference(const Bitmapset *a, const
*** 287,292 ****
--- 298,305 ----
      int            shortlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
          return NULL;
*************** bms_is_subset(const Bitmapset *a, const
*** 311,316 ****
--- 324,331 ----
      int            longlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
          return true;            /* empty set is a subset of anything */
*************** bms_subset_compare(const Bitmapset *a, c
*** 349,354 ****
--- 364,371 ----
      int            longlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
      {
*************** bms_is_member(int x, const Bitmapset *a)
*** 427,432 ****
--- 444,450 ----
          elog(ERROR, "negative bitmapset member not allowed");
      if (a == NULL)
          return false;
+     Assert(IsA(a, Bitmapset));
      wordnum = WORDNUM(x);
      bitnum = BITNUM(x);
      if (wordnum >= a->nwords)
*************** bms_overlap(const Bitmapset *a, const Bi
*** 445,450 ****
--- 463,470 ----
      int            shortlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL || b == NULL)
          return false;
*************** bms_overlap_list(const Bitmapset *a, con
*** 468,473 ****
--- 488,494 ----
      int            wordnum,
                  bitnum;

+     Assert(a == NULL || IsA(a, Bitmapset));
      if (a == NULL || b == NIL)
          return false;

*************** bms_nonempty_difference(const Bitmapset
*** 496,501 ****
--- 517,524 ----
      int            shortlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
          return false;
*************** bms_singleton_member(const Bitmapset *a)
*** 531,536 ****
--- 554,560 ----

      if (a == NULL)
          elog(ERROR, "bitmapset is empty");
+     Assert(IsA(a, Bitmapset));
      nwords = a->nwords;
      for (wordnum = 0; wordnum < nwords; wordnum++)
      {
*************** bms_get_singleton_member(const Bitmapset
*** 574,579 ****
--- 598,604 ----

      if (a == NULL)
          return false;
+     Assert(IsA(a, Bitmapset));
      nwords = a->nwords;
      for (wordnum = 0; wordnum < nwords; wordnum++)
      {
*************** bms_num_members(const Bitmapset *a)
*** 610,615 ****
--- 635,641 ----

      if (a == NULL)
          return 0;
+     Assert(IsA(a, Bitmapset));
      nwords = a->nwords;
      for (wordnum = 0; wordnum < nwords; wordnum++)
      {
*************** bms_membership(const Bitmapset *a)
*** 639,644 ****
--- 665,671 ----

      if (a == NULL)
          return BMS_EMPTY_SET;
+     Assert(IsA(a, Bitmapset));
      nwords = a->nwords;
      for (wordnum = 0; wordnum < nwords; wordnum++)
      {
*************** bms_is_empty(const Bitmapset *a)
*** 667,672 ****
--- 694,700 ----

      if (a == NULL)
          return true;
+     Assert(IsA(a, Bitmapset));
      nwords = a->nwords;
      for (wordnum = 0; wordnum < nwords; wordnum++)
      {
*************** bms_add_member(Bitmapset *a, int x)
*** 704,709 ****
--- 732,738 ----
          elog(ERROR, "negative bitmapset member not allowed");
      if (a == NULL)
          return bms_make_singleton(x);
+     Assert(IsA(a, Bitmapset));
      wordnum = WORDNUM(x);
      bitnum = BITNUM(x);

*************** bms_del_member(Bitmapset *a, int x)
*** 741,746 ****
--- 770,776 ----
          elog(ERROR, "negative bitmapset member not allowed");
      if (a == NULL)
          return NULL;
+     Assert(IsA(a, Bitmapset));
      wordnum = WORDNUM(x);
      bitnum = BITNUM(x);
      if (wordnum < a->nwords)
*************** bms_add_members(Bitmapset *a, const Bitm
*** 759,764 ****
--- 789,796 ----
      int            otherlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
          return bms_copy(b);
*************** bms_int_members(Bitmapset *a, const Bitm
*** 793,798 ****
--- 825,832 ----
      int            shortlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
          return NULL;
*************** bms_del_members(Bitmapset *a, const Bitm
*** 819,824 ****
--- 853,860 ----
      int            shortlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
          return NULL;
*************** bms_join(Bitmapset *a, Bitmapset *b)
*** 842,847 ****
--- 878,885 ----
      int            otherlen;
      int            i;

+     Assert(a == NULL || IsA(a, Bitmapset));
+     Assert(b == NULL || IsA(b, Bitmapset));
      /* Handle cases where either input is NULL */
      if (a == NULL)
          return b;
*************** bms_first_member(Bitmapset *a)
*** 889,894 ****
--- 927,933 ----

      if (a == NULL)
          return -1;
+     Assert(IsA(a, Bitmapset));
      nwords = a->nwords;
      for (wordnum = 0; wordnum < nwords; wordnum++)
      {
*************** bms_next_member(const Bitmapset *a, int
*** 942,947 ****
--- 981,987 ----

      if (a == NULL)
          return -2;
+     Assert(IsA(a, Bitmapset));
      nwords = a->nwords;
      prevbit++;
      mask = (~(bitmapword) 0) << BITNUM(prevbit);
*************** bms_hash_value(const Bitmapset *a)
*** 987,992 ****
--- 1027,1033 ----

      if (a == NULL)
          return 0;                /* All empty sets hash to 0 */
+     Assert(IsA(a, Bitmapset));
      for (lastword = a->nwords; --lastword >= 0;)
      {
          if (a->words[lastword] != 0)
diff --git a/src/include/nodes/bitmapset.h b/src/include/nodes/bitmapset.h
index 109f7b0..d672ee5 100644
*** a/src/include/nodes/bitmapset.h
--- b/src/include/nodes/bitmapset.h
***************
*** 20,25 ****
--- 20,27 ----
  #ifndef BITMAPSET_H
  #define BITMAPSET_H

+ #include "nodes/nodes.h"
+
  /*
   * Forward decl to save including pg_list.h
   */
*************** typedef int32 signedbitmapword; /* must
*** 36,41 ****
--- 38,44 ----

  typedef struct Bitmapset
  {
+     NodeTag        type;
      int            nwords;            /* number of words in array */
      bitmapword    words[FLEXIBLE_ARRAY_MEMBER];    /* really [nwords] */
  } Bitmapset;
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index f59d719..46408c2 100644
*** a/src/include/nodes/nodes.h
--- b/src/include/nodes/nodes.h
*************** typedef enum NodeTag
*** 291,296 ****
--- 291,297 ----
      T_List,
      T_IntList,
      T_OidList,
+     T_Bitmapset,

      /*
       * TAGS FOR EXTENSIBLE NODES (extensible.h)

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers