release-19.2: sql: repair bogus DropJobID on PUBLIC tables in drainNames

In cockroachdb#50587 we discovered that TRUNCATE would leave a DropJobID on both the new
and old tables. This situation was rectified in cockroachdb#50714. In most cases that job
ID is ignored. However, in release-19.2 and earlier, it would prevent RENAME
operations from completing. To deal with this, we repair the descriptor by
removing the DropJobID. We also, for additional protection, prevent the
failure to update the job from preventing progress.

Release note (bug fix): Fixed a bug which could prevent previously TRUNCATED
tables from being renamed.

pkg/sql/schema_changer.go

@@ -906,6 +906,13 @@ func (sc *SchemaChanger) drainNames(ctx context.Context) error {
 			// Free up the old name(s) for reuse.
 			namesToReclaim = desc.DrainingNames
 			desc.DrainingNames = nil
+			// Note: at one time there was a bug which would populate a DropJobID for
+			// the newly created table in TRUNCATE. A table should never have such a
+			// job ID populated unless it is in the DROP state. We repair that here.
+			// See #50587 for context.
+			if desc.State == sqlbase.TableDescriptor_PUBLIC && desc.GetDropJobID() != 0 {
+				desc.DropJobID = 0
+			}
 			dropJobID = desc.GetDropJobID()
 			return nil
 		},
@@ -923,7 +930,12 @@ func (sc *SchemaChanger) drainNames(ctx context.Context) error {
 					func(context.Context, *client.Txn, *jobs.Job) error {
 						return nil
 					}); err != nil {
-					return err
+					// Failing to update the job should not prevent the rename from
+					// making progress. The above fix to detect the bogus drop job
+					// on a PUBLIC table should prevent this defensive code from being
+					// a problem but nevertheless, it's better than preventing progress
+					// here.
+					log.Warningf(ctx, "failed to update drop job: %v", err)
 				}
 			}
 

pkg/sql/schema_changer_test.go

@@ -43,6 +43,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/jobutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
@@ -56,6 +57,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/pkg/errors"
+	"github.com/stretchr/testify/require"
 )
 
 // asyncSchemaChangerDisabled can be used to disable asynchronous processing
@@ -5046,3 +5048,34 @@ CREATE TABLE t.test (k INT PRIMARY KEY, v INT8);
 		return nil
 	})
 }
+
+// TestPublicTableWithDropJobID tests that dropping/renaming a table will
+// succeed regardless of the existence of a DropJobID. This is critical because
+// old bugs could create such tables and then cause problems.
+func TestTableWithDropJobID(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	ctx := context.Background()
+	tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{})
+	defer tc.Stopper().Stop(ctx)
+
+	// We're going to create a table, then we'll manually muck with its descriptor
+	// as though it had a drop job ID that is bogus, then we'll try to rename it.
+	kvDB := tc.Server(0).DB()
+	tdb := sqlutils.MakeSQLRunner(tc.ServerConn(0))
+	tdb.Exec(t, `CREATE TABLE foo (i INT PRIMARY KEY)`)
+	td := sqlbase.GetTableDescriptor(kvDB, "defaultdb", "foo")
+	require.NotNil(t, td)
+	td.DropJobID = rand.Int63()
+	td.Version++
+	require.NoError(t, kvDB.Txn(ctx, func(ctx context.Context, txn *client.Txn) error {
+		require.NoError(t, txn.SetSystemConfigTrigger())
+		key := sqlbase.MakeDescMetadataKey(td.GetID())
+		wrapped := sqlbase.WrapDescriptor(td)
+		return txn.Put(ctx, key, wrapped)
+	}))
+	idVer, err := tc.Server(0).LeaseManager().(*sql.LeaseManager).WaitForOneVersion(ctx, td.GetID(), retry.Options{})
+	require.Equal(t, td.Version, idVer)
+	require.NoError(t, err)
+	tdb.Exec(t, `ALTER TABLE foo RENAME TO bar`)
+}