Обсуждение: OK for ABI break of PlannerInfo in 8.4?

Marc Cousins pointed out here
http://archives.postgresql.org/pgsql-general/2010-03/msg01123.php
that the "constraint_exclusion = partition" feature added in 8.4
does not do what you'd expect for the target relation of an UPDATE
or DELETE.  That's because expansion of an inheritance set is managed
differently for an UPDATE/DELETE target rel than for other cases.

I'm intending to apply the attached patch to fix this in HEAD.
I am tempted to back-patch it to 8.4 as well, but there is a potential
problem for external modules that may be touching PlannerInfo (eg,
planner hooks): the added field in that struct is an ABI break for them.
We can minimize the risk by adding the new field at the end rather than
in any more logical position; but it would still be a problem for
modules that palloc'd or copied a PlannerInfo struct.  AFAICS though the
only real risk would be for relation_excluded_by_constraints to see a
garbage value of root->hasInheritedTarget and possibly make an
unexpected choice of what to do.  I think that's probably a small enough
problem to be acceptable.  Comments?

            regards, tom lane

Index: src/backend/nodes/outfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/nodes/outfuncs.c,v
retrieving revision 1.384
diff -c -r1.384 outfuncs.c
*** src/backend/nodes/outfuncs.c    28 Mar 2010 22:59:32 -0000    1.384
--- src/backend/nodes/outfuncs.c    30 Mar 2010 15:18:27 -0000
***************
*** 1559,1564 ****
--- 1559,1565 ----
      WRITE_NODE_FIELD(sort_pathkeys);
      WRITE_FLOAT_FIELD(total_table_pages, "%.0f");
      WRITE_FLOAT_FIELD(tuple_fraction, "%.4f");
+     WRITE_BOOL_FIELD(hasInheritedTarget);
      WRITE_BOOL_FIELD(hasJoinRTEs);
      WRITE_BOOL_FIELD(hasHavingQual);
      WRITE_BOOL_FIELD(hasPseudoConstantQuals);
Index: src/backend/optimizer/plan/planner.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v
retrieving revision 1.266
diff -c -r1.266 planner.c
*** src/backend/optimizer/plan/planner.c    26 Feb 2010 02:00:45 -0000    1.266
--- src/backend/optimizer/plan/planner.c    30 Mar 2010 15:18:27 -0000
***************
*** 307,312 ****
--- 307,313 ----
      root->eq_classes = NIL;
      root->append_rel_list = NIL;
      root->rowMarks = NIL;
+     root->hasInheritedTarget = false;

      root->hasRecursion = hasRecursion;
      if (hasRecursion)
***************
*** 749,754 ****
--- 750,756 ----
              adjust_appendrel_attrs((Node *) parse,
                                     appinfo);
          subroot.init_plans = NIL;
+         subroot.hasInheritedTarget = true;
          /* We needn't modify the child's append_rel_list */
          /* There shouldn't be any OJ info to translate, as yet */
          Assert(subroot.join_info_list == NIL);
Index: src/backend/optimizer/util/plancat.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/optimizer/util/plancat.c,v
retrieving revision 1.162
diff -c -r1.162 plancat.c
*** src/backend/optimizer/util/plancat.c    5 Jan 2010 21:53:58 -0000    1.162
--- src/backend/optimizer/util/plancat.c    30 Mar 2010 15:18:27 -0000
***************
*** 580,586 ****
      /* Skip the test if constraint exclusion is disabled for the rel */
      if (constraint_exclusion == CONSTRAINT_EXCLUSION_OFF ||
          (constraint_exclusion == CONSTRAINT_EXCLUSION_PARTITION &&
!          rel->reloptkind != RELOPT_OTHER_MEMBER_REL))
          return false;

      /*
--- 580,589 ----
      /* Skip the test if constraint exclusion is disabled for the rel */
      if (constraint_exclusion == CONSTRAINT_EXCLUSION_OFF ||
          (constraint_exclusion == CONSTRAINT_EXCLUSION_PARTITION &&
!          !(rel->reloptkind == RELOPT_OTHER_MEMBER_REL ||
!            (root->hasInheritedTarget &&
!             rel->reloptkind == RELOPT_BASEREL &&
!             rel->relid == root->parse->resultRelation))))
          return false;

      /*
Index: src/include/nodes/relation.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/nodes/relation.h,v
retrieving revision 1.185
diff -c -r1.185 relation.h
*** src/include/nodes/relation.h    28 Mar 2010 22:59:33 -0000    1.185
--- src/include/nodes/relation.h    30 Mar 2010 15:18:27 -0000
***************
*** 197,202 ****
--- 197,204 ----

      double        tuple_fraction; /* tuple_fraction passed to query_planner */

+     bool        hasInheritedTarget;    /* true if parse->resultRelation is an
+                                      * inheritance child rel */
      bool        hasJoinRTEs;    /* true if any RTEs are RTE_JOIN kind */
      bool        hasHavingQual;    /* true if havingQual was non-null */
      bool        hasPseudoConstantQuals; /* true if any RestrictInfo has

Tom Lane wrote:
> Marc Cousins pointed out here
> http://archives.postgresql.org/pgsql-general/2010-03/msg01123.php
> that the "constraint_exclusion = partition" feature added in 8.4
> does not do what you'd expect for the target relation of an UPDATE
> or DELETE.  That's because expansion of an inheritance set is managed
> differently for an UPDATE/DELETE target rel than for other cases.
> 
> I'm intending to apply the attached patch to fix this in HEAD.
> I am tempted to back-patch it to 8.4 as well, but there is a potential
> problem for external modules that may be touching PlannerInfo (eg,
> planner hooks): the added field in that struct is an ABI break for them.
> We can minimize the risk by adding the new field at the end rather than
> in any more logical position; but it would still be a problem for
> modules that palloc'd or copied a PlannerInfo struct.  AFAICS though the
> only real risk would be for relation_excluded_by_constraints to see a
> garbage value of root->hasInheritedTarget and possibly make an
> unexpected choice of what to do.  I think that's probably a small enough
> problem to be acceptable.  Comments?

Seems OK to me. It's worth noting though that if a module does do
palloc+memcpy of PlannerInfo, and it's compiled against the new sources
with the extra field, but used on an old server version, it will
memcpy() from beyond the end of the struct. If you're seriously unlucky
and the struct is at the end of allocated address space, that can segfault.

Unfortunately there doesn't seem to be any alignment padding in the
struct either. You could've stuck the new field there and avoided
changing sizeof(PlannerInfo).

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Seems OK to me. It's worth noting though that if a module does do
> palloc+memcpy of PlannerInfo, and it's compiled against the new sources
> with the extra field, but used on an old server version, it will
> memcpy() from beyond the end of the struct. If you're seriously unlucky
> and the struct is at the end of allocated address space, that can segfault.

Yeah, and in the converse case (copied version is too small) the
reference from relation_excluded_by_constraints could segfault.
But actually, neither will happen because palloc rounds the struct
size up to a power of 2, so there will be some memory there; it
just might contain garbage.

> Unfortunately there doesn't seem to be any alignment padding in the
> struct either. You could've stuck the new field there and avoided
> changing sizeof(PlannerInfo).

Yeah, I looked for some pad space :-(

The alternative fix for 8.4 would be to not add the field but instead
grovel through app_info_list to see if the target relation is a child.
I discarded this approach because the whole point of
"constraint_exclusion = partition" is to not add overhead to simple
non-inheritance queries.  However, if we think the ABI break is actually
worth worrying about then maybe that's the better fix.

I'm inclined not to worry, in part because I'm not aware of any
third-party planner hooks actually being used in production.
Does anyone know of one?
        regards, tom lane