Update message table schema for efficient poller and w/o potential message loss

[mattlord]

This updates the docs based on the work done in: vitessio/vitess#10132

The potential for loss of messages — or more accurately messages never sent — comes from the time_next bigint column definition that we had in the docs, which is effectively time_next bigint DEFAULT NULL. If the time_next value is NULL, then it never gets read from the underlying message table in the poller / results streamer query because < is not NULL safe and that < <time_next_val> clause will never match those rows where the value is NULL (unknown). So while rows INSERTed with time_next being NULL while the binlog streamer is running will catch these rows and process them, any rows INSERTed while the binlog streamer is not running, or the cache is full, will be lost.

We were already using a DEFAULT 0 clause for the time_next column in our tests, but this was not done in the docs.

[netlify]

✅ Deploy Preview for vitess ready!

Name	Link
🔨 Latest commit	e3402b9
🔍 Latest deploy log	https://app.netlify.com/sites/vitess/deploys/6268580fa194c900091ca511
😎 Deploy Preview	https://deploy-preview-1015--vitess.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[derekperkins]

time_scheduled and time_created are no longer populated by Vitess, which impacts the default table. Here's an updated suggestion with comments.

create table my_message(
  # required columns
  id bigint NOT NULL COMMENT 'often an event id, can also be auto-increment or a sequence',
  priority tinyint NOT NULL DEFAULT '50' COMMENT 'lower number priorities process first',
  epoch bigint NOT NULL DEFAULT '0' COMMENT 'Vitess increments this each time it sends a message, and is used for incremental backoff doubling',
  time_next bigint DEFAULT 0 COMMENT 'the earliest time the message will be sent in epoch nanoseconds. Must be null if time_acked is set',
  time_acked bigint DEFAULT NULL COMMENT 'the time the message was acked in epoch nanoseconds. Must be null if time_next is set',

  # add as many custom fields here as required
  # optional - these are suggestions
  tenant_id bigint,
  message json,

  # required indexes
  primary key(id),
  index poller_idx(time_acked, priority, time_next desc)

  # add any secondary indexes or foreign keys - no restrictions
) comment 'vitess_message,vt_min_backoff=30,vt_max_backoff=3600,vt_ack_wait=30,vt_purge_after=86400,vt_batch_size=10,vt_cache_size=10000,vt_poller_interval=30'

[derekperkins]

We don't have to address it in the PR, but I wonder if we could/should introduce a check constraint for anyone >= 8.0.16 that enforces the mutual exclusivity of time_next and time_acked

[mattlord]

@derekperkins could you please review the new page whenever you have time? I went through the entire thing and updated it. All of them are the same, so you can look at this one: https://deploy-preview-1015--vitess.netlify.app/docs/14.0/reference/features/messaging/

Thanks!

[derekperkins]

Should we say "shard" instead of "segment"?

[mattlord]

I feel like it could still be useful when not sharded. Some receivers could only process and ack messages for a given tenant. No?

[derekperkins]

We have lots of unsharded queues - tenant id just feels like the most common sharding id, though I understand your point. My first reaction on reading it was to wonder if "segment" was a specific concept for messaging. I don't feel super strongly about it.

Once we're done with this, I want to blog about and/or add an opinionated design to the docs, with the specific fields and queries we use, so people don't have to architect from scratch.

[mattlord]

We have lots of unsharded queues - tenant id just feels like the most common sharding id, though I understand your point. My first reaction on reading it was to wonder if "segment" was a specific concept for messaging. I don't feel super strongly about it.

I don't have a strong opinion here either. We can always revisit.

Once we're done with this, I want to blog about and/or add an opinionated design to the docs, with the specific fields and queries we use, so people don't have to architect from scratch.

That would be awesome! ❤️

[derekperkins]

Should probably add details for vt_min_backoff and vt_max_backoff here.

Also, maybe we should specify that "some added jitter" is "up to 33% jitter"

[derekperkins]

Not sure if we want to get this technical, but here's a first stab at giving actual details

Basic backoff equation:

jitteredBackoff = `vt_ack_wait` ^ `epoch` * (random value between 0.66 and 1.33)
actualBackoff = `vt_min_backoff` <= `jitteredBackoff` <= `vt_max_backoff`

backoff query if vt_max_backoff is NOT set

update foo set time_next = 
  :time_now + :wait_time + IF(FLOOR((:min_backoff<<ifnull(epoch, 0)) * :jitter) < :min_backoff, :min_backoff, FLOOR((:min_backoff<<ifnull(epoch, 0)) * :jitter)), epoch = ifnull(epoch, 0)+1 where id in ::ids and time_acked is null

backoff query if vt_max_backoff IS set

update foo set time_next = 
  :time_now + :wait_time + IF(FLOOR((:min_backoff<<ifnull(epoch, 0)) * :jitter) < :min_backoff, :min_backoff, IF(FLOOR((:min_backoff<<ifnull(epoch, 0)) * :jitter) > :max_backoff, :max_backoff, FLOOR((:min_backoff<<ifnull(epoch, 0)) * :jitter))), epoch = ifnull(epoch, 0)+1 where id in ::ids and time_acked is null

[derekperkins]

FYI, this is definitely one place where having to handle null epoch adds complexity. If we're ok enforcing non-null epoch, we could change this

[mattlord]

I would rather leave this as it is for now (we're already making a lot of changes) and not share this level of detail in the docs. For those interested, the source code is there.

[derekperkins]

I don't think this is true anymore, but I'm not sure

[derekperkins]

Adding WHERE time_acked IS NULL to the poller query actually solves some issues we've encountered in the past. If someone was requeuing messages after a fix, sometimes they would accidentally set time_next while the service set time_acked, in which case there ended up being an infinite loop of message retries. We should still probably note somewhere that the fields are designed to be mutually exclusive, but at least it's less of a footgun now.