Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

I wrote:
> I believe I've identified the reason why skink and some other buildfarm
> members have been failing the pg_upgrade test recently.
> ...
> Not sure what we want to do about it.  One idea is to make
> ALTER SEQUENCE not so transactional when in binary-upgrade mode.

On closer inspection, the only thing that pg_upgrade needs is to be
able to do ALTER SEQUENCE OWNED BY without a relfilenode bump.  PFA
two versions of a patch that fixes this problem, at least to the
extent that it gets through check-world without triggering the Assert
I added to GetNewRelFileNode (which HEAD doesn't).  The first one
is a minimally-invasive hack; the second one puts the responsibility
for deciding if a sequence rewrite is needed into init_params.  That's
bulkier but might be useful if we ever grow additional ALTER SEQUENCE
options that don't need a rewrite.  OTOH, I'm not very clear on what
such options might look like, so maybe the extra flexibility is useless.
Thoughts?

            regards, tom lane

diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 11ee536..43ef4cd 100644
*** a/src/backend/catalog/catalog.c
--- b/src/backend/catalog/catalog.c
*************** GetNewRelFileNode(Oid reltablespace, Rel
*** 391,396 ****
--- 391,403 ----
      bool        collides;
      BackendId    backend;

+     /*
+      * If we ever get here during pg_upgrade, there's something wrong; all
+      * relfilenode assignments during a binary-upgrade run should be
+      * determined by commands in the dump script.
+      */
+     Assert(!IsBinaryUpgrade);
+
      switch (relpersistence)
      {
          case RELPERSISTENCE_TEMP:
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 7e85b69..188429c 100644
*** a/src/backend/commands/sequence.c
--- b/src/backend/commands/sequence.c
*************** AlterSequence(ParseState *pstate, AlterS
*** 473,490 ****
          GetTopTransactionId();

      /*
!      * Create a new storage file for the sequence, making the state changes
!      * transactional.  We want to keep the sequence's relfrozenxid at 0, since
!      * it won't contain any unfrozen XIDs.  Same with relminmxid, since a
!      * sequence will never contain multixacts.
       */
!     RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
!                               InvalidTransactionId, InvalidMultiXactId);

!     /*
!      * Insert the modified tuple into the new storage file.
!      */
!     fill_seq_with_data(seqrel, newdatatuple);

      /* process OWNED BY if given */
      if (owned_by)
--- 473,499 ----
          GetTopTransactionId();

      /*
!      * If we are *only* doing OWNED BY, there is no need to rewrite the
!      * sequence file nor the pg_sequence tuple; and we mustn't do so because
!      * it breaks pg_upgrade by causing unwanted changes in the sequence's
!      * relfilenode.
       */
!     if (!(owned_by && list_length(stmt->options) == 1))
!     {
!         /*
!          * Create a new storage file for the sequence, making the state
!          * changes transactional.  We want to keep the sequence's relfrozenxid
!          * at 0, since it won't contain any unfrozen XIDs.  Same with
!          * relminmxid, since a sequence will never contain multixacts.
!          */
!         RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
!                                   InvalidTransactionId, InvalidMultiXactId);

!         /*
!          * Insert the modified tuple into the new storage file.
!          */
!         fill_seq_with_data(seqrel, newdatatuple);
!     }

      /* process OWNED BY if given */
      if (owned_by)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 11ee536..43ef4cd 100644
*** a/src/backend/catalog/catalog.c
--- b/src/backend/catalog/catalog.c
*************** GetNewRelFileNode(Oid reltablespace, Rel
*** 391,396 ****
--- 391,403 ----
      bool        collides;
      BackendId    backend;

+     /*
+      * If we ever get here during pg_upgrade, there's something wrong; all
+      * relfilenode assignments during a binary-upgrade run should be
+      * determined by commands in the dump script.
+      */
+     Assert(!IsBinaryUpgrade);
+
      switch (relpersistence)
      {
          case RELPERSISTENCE_TEMP:
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 7e85b69..34b8aa2 100644
*** a/src/backend/commands/sequence.c
--- b/src/backend/commands/sequence.c
*************** static Form_pg_sequence_data read_seq_tu
*** 101,107 ****
  static void init_params(ParseState *pstate, List *options, bool for_identity,
              bool isInit,
              Form_pg_sequence seqform,
!             Form_pg_sequence_data seqdataform, List **owned_by);
  static void do_setval(Oid relid, int64 next, bool iscalled);
  static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity);

--- 101,109 ----
  static void init_params(ParseState *pstate, List *options, bool for_identity,
              bool isInit,
              Form_pg_sequence seqform,
!             Form_pg_sequence_data seqdataform,
!             bool *need_newrelfilenode,
!             List **owned_by);
  static void do_setval(Oid relid, int64 next, bool iscalled);
  static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity);

*************** DefineSequence(ParseState *pstate, Creat
*** 115,120 ****
--- 117,123 ----
  {
      FormData_pg_sequence seqform;
      FormData_pg_sequence_data seqdataform;
+     bool        need_newrelfilenode;
      List       *owned_by;
      CreateStmt *stmt = makeNode(CreateStmt);
      Oid            seqoid;
*************** DefineSequence(ParseState *pstate, Creat
*** 153,159 ****
      }

      /* Check and set all option values */
!     init_params(pstate, seq->options, seq->for_identity, true, &seqform, &seqdataform, &owned_by);

      /*
       * Create relation (and fill value[] and null[] for the tuple)
--- 156,164 ----
      }

      /* Check and set all option values */
!     init_params(pstate, seq->options, seq->for_identity, true,
!                 &seqform, &seqdataform,
!                 &need_newrelfilenode, &owned_by);

      /*
       * Create relation (and fill value[] and null[] for the tuple)
*************** AlterSequence(ParseState *pstate, AlterS
*** 417,422 ****
--- 422,428 ----
      HeapTupleData datatuple;
      Form_pg_sequence seqform;
      Form_pg_sequence_data newdataform;
+     bool        need_newrelfilenode;
      List       *owned_by;
      ObjectAddress address;
      Relation    rel;
*************** AlterSequence(ParseState *pstate, AlterS
*** 461,468 ****
      UnlockReleaseBuffer(buf);

      /* Check and set new values */
!     init_params(pstate, stmt->options, stmt->for_identity, false, seqform,
!                 newdataform, &owned_by);

      /* Clear local cache so that we don't think we have cached numbers */
      /* Note that we do not change the currval() state */
--- 467,475 ----
      UnlockReleaseBuffer(buf);

      /* Check and set new values */
!     init_params(pstate, stmt->options, stmt->for_identity, false,
!                 seqform, newdataform,
!                 &need_newrelfilenode, &owned_by);

      /* Clear local cache so that we don't think we have cached numbers */
      /* Note that we do not change the currval() state */
*************** AlterSequence(ParseState *pstate, AlterS
*** 473,490 ****
          GetTopTransactionId();

      /*
!      * Create a new storage file for the sequence, making the state changes
!      * transactional.  We want to keep the sequence's relfrozenxid at 0, since
!      * it won't contain any unfrozen XIDs.  Same with relminmxid, since a
!      * sequence will never contain multixacts.
       */
!     RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
!                               InvalidTransactionId, InvalidMultiXactId);

!     /*
!      * Insert the modified tuple into the new storage file.
!      */
!     fill_seq_with_data(seqrel, newdatatuple);

      /* process OWNED BY if given */
      if (owned_by)
--- 480,500 ----
          GetTopTransactionId();

      /*
!      * If needed, create a new storage file for the sequence, making the state
!      * changes transactional.  We want to keep the sequence's relfrozenxid at
!      * 0, since it won't contain any unfrozen XIDs.  Same with relminmxid,
!      * since a sequence will never contain multixacts.
       */
!     if (need_newrelfilenode)
!     {
!         RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
!                                   InvalidTransactionId, InvalidMultiXactId);

!         /*
!          * Insert the modified tuple into the new storage file.
!          */
!         fill_seq_with_data(seqrel, newdatatuple);
!     }

      /* process OWNED BY if given */
      if (owned_by)
*************** read_seq_tuple(Relation rel, Buffer *buf
*** 1205,1213 ****
   * init_params: process the options list of CREATE or ALTER SEQUENCE, and
   * store the values into appropriate fields of seqform, for changes that go
   * into the pg_sequence catalog, and seqdataform for changes to the sequence
!  * relation itself.  Set *changed_seqform to true if seqform was changed
!  * (interesting for ALTER SEQUENCE).  Also set *owned_by to any OWNED BY
!  * option, or to NIL if there is none.
   *
   * If isInit is true, fill any unspecified options with default values;
   * otherwise, do not change existing options that aren't explicitly overridden.
--- 1215,1224 ----
   * init_params: process the options list of CREATE or ALTER SEQUENCE, and
   * store the values into appropriate fields of seqform, for changes that go
   * into the pg_sequence catalog, and seqdataform for changes to the sequence
!  * relation itself.  Set *need_newrelfilenode to true if we changed any
!  * parameters that require assigning a new sequence relfilenode (interesting
!  * for ALTER SEQUENCE).  Also set *owned_by to any OWNED BY option, or to NIL
!  * if there is none.
   *
   * If isInit is true, fill any unspecified options with default values;
   * otherwise, do not change existing options that aren't explicitly overridden.
*************** init_params(ParseState *pstate, List *op
*** 1217,1222 ****
--- 1228,1234 ----
              bool isInit,
              Form_pg_sequence seqform,
              Form_pg_sequence_data seqdataform,
+             bool *need_newrelfilenode,
              List **owned_by)
  {
      DefElem    *as_type = NULL;
*************** init_params(ParseState *pstate, List *op
*** 1231,1236 ****
--- 1243,1249 ----
      bool        reset_max_value = false;
      bool        reset_min_value = false;

+     *need_newrelfilenode = false;
      *owned_by = NIL;

      foreach(option, options)
*************** init_params(ParseState *pstate, List *op
*** 1245,1250 ****
--- 1258,1264 ----
                           errmsg("conflicting or redundant options"),
                           parser_errposition(pstate, defel->location)));
              as_type = defel;
+             *need_newrelfilenode = true;
          }
          else if (strcmp(defel->defname, "increment") == 0)
          {
*************** init_params(ParseState *pstate, List *op
*** 1254,1259 ****
--- 1268,1274 ----
                           errmsg("conflicting or redundant options"),
                           parser_errposition(pstate, defel->location)));
              increment_by = defel;
+             *need_newrelfilenode = true;
          }
          else if (strcmp(defel->defname, "start") == 0)
          {
*************** init_params(ParseState *pstate, List *op
*** 1263,1268 ****
--- 1278,1284 ----
                           errmsg("conflicting or redundant options"),
                           parser_errposition(pstate, defel->location)));
              start_value = defel;
+             *need_newrelfilenode = true;
          }
          else if (strcmp(defel->defname, "restart") == 0)
          {
*************** init_params(ParseState *pstate, List *op
*** 1272,1277 ****
--- 1288,1294 ----
                           errmsg("conflicting or redundant options"),
                           parser_errposition(pstate, defel->location)));
              restart_value = defel;
+             *need_newrelfilenode = true;
          }
          else if (strcmp(defel->defname, "maxvalue") == 0)
          {
*************** init_params(ParseState *pstate, List *op
*** 1281,1286 ****
--- 1298,1304 ----
                           errmsg("conflicting or redundant options"),
                           parser_errposition(pstate, defel->location)));
              max_value = defel;
+             *need_newrelfilenode = true;
          }
          else if (strcmp(defel->defname, "minvalue") == 0)
          {
*************** init_params(ParseState *pstate, List *op
*** 1290,1295 ****
--- 1308,1314 ----
                           errmsg("conflicting or redundant options"),
                           parser_errposition(pstate, defel->location)));
              min_value = defel;
+             *need_newrelfilenode = true;
          }
          else if (strcmp(defel->defname, "cache") == 0)
          {
*************** init_params(ParseState *pstate, List *op
*** 1299,1304 ****
--- 1318,1324 ----
                           errmsg("conflicting or redundant options"),
                           parser_errposition(pstate, defel->location)));
              cache_value = defel;
+             *need_newrelfilenode = true;
          }
          else if (strcmp(defel->defname, "cycle") == 0)
          {
*************** init_params(ParseState *pstate, List *op
*** 1308,1313 ****
--- 1328,1334 ----
                           errmsg("conflicting or redundant options"),
                           parser_errposition(pstate, defel->location)));
              is_cycled = defel;
+             *need_newrelfilenode = true;
          }
          else if (strcmp(defel->defname, "owned_by") == 0)
          {

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