MySQL Bugs: #120504: Can't create some of tables with SPATIAL KEY because of ER_TOO_BIG

Bug #120504	Can't create some of tables with SPATIAL KEY because of ER_TOO_BIG_ROWSIZE
Submitted:	20 May 11:53	Modified:	29 May 8:46
Reporter:	Pawel Olchawa	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S3 (Non-critical)
Version:	9.7	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
In previous versions of MySQL it was possible to create a table:

CREATE TABLE t1 (
  a VARCHAR(255) NOT NULL,
  b VARCHAR(255) NOT NULL,
  c VARCHAR(255) NOT NULL,
  d VARCHAR(175) NOT NULL,
  g GEOMETRY NOT NULL SRID 4326,
  PRIMARY KEY (a, b, c, d),
  SPATIAL KEY idx_g (g)
) ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=2 CHARSET=latin1;

and then insert some rows, for example:

INSERT INTO t1(a,b,c,d,g) VALUES ('a', 'b', 'c', 'd', ST_GeomFromText('POINT(37.7749 -122.4194)', 4326));

In 9.7, this table can't be created:

CREATE TABLE t1 (
a VARCHAR(255) NOT NULL,
b VARCHAR(255) NOT NULL,
c VARCHAR(255) NOT NULL,
d VARCHAR(175) NOT NULL,
g GEOMETRY NOT NULL SRID 4326,
PRIMARY KEY (a, b, c, d),
SPATIAL KEY idx_g (g)
) ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=2 CHARSET=latin1'
Returned error: ER_TOO_BIG_ROWSIZE(1118) (42000): Row size too large (> 8126). Changing some columns to TEXT or BLOB may help. In current row format, BLOB prefix of 0 bytes is stored inline.

This issue is still there after applying a related bug-fix:
https://github.com/mysql/mysql-server/pull/657#issuecomment-4410209000

The reason is:

1. In previous versions of MySQL there was a possible overflow during estimation of the maximum number of bytes occupied by indexed fields. That's because for a geometry field, ULINT_MAX was returned.

2. The overflow was dangerous when the estimation was executed to decide whether a node should be x-latched.

3. But it was not a big problem when the estimation was executed to check whether a given table might be created.

4. In 9.7, these two use cases started to share implementation (get_field_max_size), which perhaps was not a fortunate decision, because they have different requirements:

- for x-latch decision: it must never underestimate, should take into account only index fields, and should be efficient.

- for DDL decision: it should not break backward compatibility, it might underestimate, doesn't need to be efficient, and should take into account all fields.

5. Because of the shared implementation, the bug fix that was critical for the x-latch decision broke the backward compatibility for the DDL decision.

How to repeat:
Try:

CREATE TABLE t1 (
  a VARCHAR(255) NOT NULL,
  b VARCHAR(255) NOT NULL,
  c VARCHAR(255) NOT NULL,
  d VARCHAR(175) NOT NULL,
  g GEOMETRY NOT NULL SRID 4326,
  PRIMARY KEY (a, b, c, d),
  SPATIAL KEY idx_g (g)
) ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=2 CHARSET=latin1;

INSERT INTO t1(a,b,c,d,g) VALUES ('a', 'b', 'c', 'd', ST_GeomFromText('POINT(37.7749 -122.4194)', 4326));

Suggested fix:
I would suggest a split of implementations for these two different use cases.

Alternatively, let's restore the overflow when the function is being used to check if a table might be created, which would most likely require a new argument.

The following fix shows how to patch the existing solution:
https://github.com/polchawa-percona/mysql-server/pull/1

Assumption:
https://github.com/mysql/mysql-server/pull/657#issuecomment-4410209000
has been merged first (it solves the most critical part of the bug).

Side note: splitting might also be beneficial for efficiency - smaller code used in regular workloads and less branching.

Hi Paweł,

First of all, let me readily admit that the error message text is very misleading and worth fixing - the actual problem has nothing to do with the 8126 number.
The problem that InnoDB notices is that the size of a record in internal node of secondary spatial index might be larger than page_ptr_max=958.
This is because such record might need to store MBR (32 + 1 bytes), together with all the PK fields (255+255+255+175) and child pointer (4 bytes).
255+255+255+175+32 alone is already more than that.

We might fix the error message text to include number 958 and the real condition being checked. But I understand wording is not the main problem you report here.

Btw. the value 958 comes from get_permissible_max_size(...&page_ptr_max).
You've asked for compressed page to have 2k, and we need to ensure that two records (and some metadata) fit such page.

If you really need to CREATE TABLE with definition which describes rows potentially longer than can fit in a page, then you can use SET innodb_strict_mode=OFF;

```
mysql> SET innodb_strict_mode=OFF;
Query OK, 0 rows affected (0.007 sec)

mysql> CREATE TABLE t1 (
    ->   a VARCHAR(255) NOT NULL,
    ->   b VARCHAR(255) NOT NULL,
    ->   c VARCHAR(255) NOT NULL,
    ->   d VARCHAR(175) NOT NULL,
    ->   g GEOMETRY NOT NULL SRID 4326,
    ->   PRIMARY KEY (a, b, c, d),
    ->   SPATIAL KEY idx_g (g)
    -> ) ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=2 CHARSET=latin1;
Query OK, 0 rows affected, 1 warning (0.295 sec)

mysql> INSERT INTO t1(a,b,c,d,g) VALUES ('a', 'b', 'c', 'd', ST_GeomFromText('POINT(37.7749 -122.4194)', 4326));
Query OK, 1 row affected (0.126 sec)
```

May I ask, why do you need a table definition which is wrong in this sense?

There's one more aspect here, somewhat orthogonal, but perhaps worth fixing as it applies to your case:

AFAICT the internal nodes of spatial index contain only MBR and child pointer (no PK values):
```
/**
Gets the number of fields on nonleaf page level in the internal representation
of an index which uniquely determine the position of an index entry in the
index, if we also take multiversioning into account. Note, it doesn't
include page no field.
@param[in]      index   index
@return number of fields */
inline uint16_t dict_index_get_n_unique_in_tree_nonleaf(
    const dict_index_t *index) {
  ut_ad(index != nullptr);
  ut_ad(index->magic_n == DICT_INDEX_MAGIC_N);
  ut_ad(index->cached);

  if (dict_index_is_spatial(index)) {
    /* For spatial index, on non-leaf page, we have only
    2 fields(mbr+page_no). So, except page no field,
    there's one field there. */
    return DICT_INDEX_SPATIAL_NODEPTR_SIZE;
  }

  return dict_index_get_n_unique_in_tree(index);
}
```

Therefore I think this part:
```
    /* Check the size limit on non-leaf pages. Records stored in non-leaf
    B-tree pages consist of the unique columns of the record (the key columns
    of the B-tree) and a node pointer field. When we have processed the unique
    columns, rec_max_size equals the size of the node pointer record minus the
    node pointer column. */
    if (i + 1 == dict_index_get_n_unique_in_tree(index) &&
        rec_max_size + REC_NODE_PTR_SIZE >= page_ptr_max) {
      return (true);
    }
```
is wrong as it should call dict_index_get_n_unique_in_tree_nonleaf.

I think this part should be fixed.

I am marking this as "Verified" in the following sense:

There seems to be the bug in several places in code that we use a wrong of two functions to figure out number of fields of a record in internal node, which affects spatial indexes.
We will fix this issue using this bug.

Separately, and out of scope of this particular bug are the other issues of:
- I've filed Bug#39433634 "Wrong and misleading wording of error message ER_TOO_BIG_ROWSIZE(1118) (42000): Row size too large (> 8126)."
- There is ongoing investigation/discussion in Bug #120323 'Regression : "Row size too large (> 8126)" error with table using InnoDb DYNAMIC row format', which shares big part with this current bug report, w.r.t. strict mode treating as an error something which was permitted in older version of MySQL. I think it's better to continue the discussion about this aspect of the issue there

Hi Kuba,

I've also noticed that for a spatial index, we call the wrong function (take into account more fields than needed), but the consequences are low:
1. Overestimation is safe concerning the latching decision (just not optimal).
2. It was this way before 9.7, so it's not itself a reason why certain tables are prevented from being created (at least in 8.0 and 8.4 that I checked).

Regarding strict-mode=on/off, I suggest keeping the discussion in:
https://bugs.mysql.com/bug.php?id=120323
- not to duplicate here.

I've verified that setting innodb_strict_mode=ON allows me to create the table (and I can insert rows afterwards).

Note that this issue:
- reports very similar consequences to bug#120323,
- is caused by the same two commits that resulted in bug#120323,
- but is caused by the overflow-fix for geometry fields that is also there,
- is not fixed by the fix that was suggested for bug#120323.

Tables with spatial keys are less popular, so this edge case has not been reported by other community members as far as I know. I believe it might make sense to stay consistent here with whatever decision is made for bug#120323.

Side note: if you kept a separate implementation for runtime latching decisions, then you could always improve estimations there while not breaking backward compatibility regarding which tables are allowed to be created (and potentially making it easier to optimize the code for the hot path by moving away checks needed for the DDL-only path).

Hi Paweł,

I agree with your factual statements. 

Let me add some thoughts:
- Internal node size of spatial index seems to be basically constant: MBR+childptr always have, and had, a fixed number of bytes determined at compile time. Also this correct number is so low, that the requirement to fit two such records in the smallest possible page size, even with table compression on, is always satisfied. Thus, making this part of the estimation correct, makes this part of condition only more permissive than it used to be in any version of MySQL. The patch to fix this is already in review.
- You are correct that this part of logic was broken like that for a long time, yet your CREATE TABLE only started failing once I've fixed the other bug which was wrongly estimating it as "-1", so yeah, there's no denying that the change in observable behaviour here is recent. 
- Also, the recent change affects estimation for leaf nodes, too, and leaf nodes store the whole PK. The reason this is not a problem for your particular CREATE TABLE is that for leaf page we require only 1 record per page, not 2, so it still fits in 2k in your case no matter which of the estimation methods we use, and most importantly: in practice.
- We are of opinion that there are multiple readings of the documentation and one of them is that in strict mode we are allowed to reject CREATE TABLE with definition which can lead to rows which can't fit, but yeah, documentation was, and remains very ambiguous. Still, the intent of strict mode, as the name suggest, is to try to be strict, to avoid problems with ambiguity. We had internal discussions prior to release about this bug&fix and concluded that we have to have the right to fix bugs, as opposed to live with them, just because some users used MySQL in a wrong or undocumented way, as long as there's a work-around like using non-strict mode. I still think this is the right balance here, but yeah, let's discuss in the other bug
- You are right that another option would be to split again the single function into two functions, so that one of them could do the right thing, and the other do the wrong thing, which gives us an option to avoid a deadlock while bringing back the buggy (IMO) behaviour of CREATE TABLE... provided that we decide to do it in the first place. Let's discuss there
- There's currently no plan to accept the fix for 120323 as-is, so don't worry. If we decide to do something about it, then we will definitely need to find a solution which satisfies all the needs, not just one edge-case

I think the fundamental issues to figure out in the other bug are:
1. can a given issue be reproduced in non-strict mode?
2. is disabling strict mode easier than upgrading again?
3. (internal decision): what strategy to assume for fixing bugs in areas which are poorly documented (given our constraints in number of supported versions and head count)?