Re: Removing pg_migrator limitations - Mailing list pgsql-hackers

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> The approach I originally suggested was to create the enum type with
> >> *no* members, and then add the values one at a time.
>
> > Well, I was hesitant to modify the grammar, unless we want the ability
> > to create enums with zero values.  Doing enum with only one value will
> > not be too complex for me and I don't think binary upgrade should affect
> > the grammar unless there are other reasons we want to change.
>
> The reason I don't want to do it that way is that then you need two
> ugly kluges in the backend, not just one.  With the zero-and-add-one
> approach there is no need to have a "next enum oid" variable at all.

Uh, I still need that variable because that is how we are going to set
the oid in EnumValuesCreate(), unless we want to add dummy oid-value
arguments to that function for use only by the binary upgrade
server-side function.  I have actually coded the variable case already
so you can see how it looks; attached.  Most of the patch is just
indenting of the existing oid assignment block.

> > We do allow tables with no columns, but we allow the addition of columns
> > to a table, so it makes more sense there.
>
> Well, we might eventually allow addition of values to enums too; the
> fact that it's not implemented outside pg_migrator right now doesn't
> mean we won't ever think of a solution.  In any case I'm not persuaded
> that a zero-element enum is totally without value.  Think of it like a
> domain with a "must be null" constraint.

OK, but that is going to expand the my patch.  I will probably implement
zero-element enums first and then go ahead and do the binary upgrade
part.  Zero-element enums will simplify the pg_dump code.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: src/backend/catalog/pg_enum.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/pg_enum.c,v
retrieving revision 1.11
diff -c -c -r1.11 pg_enum.c
*** src/backend/catalog/pg_enum.c    24 Dec 2009 22:17:58 -0000    1.11
--- src/backend/catalog/pg_enum.c    24 Dec 2009 22:29:17 -0000
***************
*** 25,30 ****
--- 25,32 ----

  static int    oid_cmp(const void *p1, const void *p2);

+ Oid binary_upgrade_next_pg_enum_oid = InvalidOid;
+

  /*
   * EnumValuesCreate
***************
*** 58,82 ****
      tupDesc = pg_enum->rd_att;

      /*
!      * Allocate oids.  While this method does not absolutely guarantee that we
!      * generate no duplicate oids (since we haven't entered each oid into the
!      * table before allocating the next), trouble could only occur if the oid
!      * counter wraps all the way around before we finish. Which seems
!      * unlikely.
       */
      oids = (Oid *) palloc(num_elems * sizeof(Oid));
!     for (elemno = 0; elemno < num_elems; elemno++)
      {
          /*
!          *    The pg_enum.oid is stored in user tables.  This oid must be
!          *    preserved by binary upgrades.
           */
!         oids[elemno] = GetNewOid(pg_enum);
      }

-     /* sort them, just in case counter wrapped from high to low */
-     qsort(oids, num_elems, sizeof(Oid), oid_cmp);
-
      /* and make the entries */
      memset(nulls, false, sizeof(nulls));

--- 60,94 ----
      tupDesc = pg_enum->rd_att;

      /*
!      *    Allocate oids
       */
      oids = (Oid *) palloc(num_elems * sizeof(Oid));
!     if (num_elems == 1 && OidIsValid(binary_upgrade_next_pg_enum_oid))
!     {
!             oids[0] = binary_upgrade_next_pg_enum_oid;
!             binary_upgrade_next_pg_enum_oid = InvalidOid;
!     }
!     else
      {
          /*
!          * While this method does not absolutely guarantee that we generate
!          * no duplicate oids (since we haven't entered each oid into the
!          * table before allocating the next), trouble could only occur if
!          * the oid counter wraps all the way around before we finish. Which
!          * seems unlikely.
           */
!         for (elemno = 0; elemno < num_elems; elemno++)
!         {
!             /*
!              *    The pg_enum.oid is stored in user tables.  This oid must be
!              *    preserved by binary upgrades.
!              */
!             oids[elemno] = GetNewOid(pg_enum);
!         }
!         /* sort them, just in case counter wrapped from high to low */
!         qsort(oids, num_elems, sizeof(Oid), oid_cmp);
      }

      /* and make the entries */
      memset(nulls, false, sizeof(nulls));