confluentinc / bottledwater-pg

Change data capture from PostgreSQL into Kafka
http://blog.confluent.io/2015/04/23/bottled-water-real-time-integration-of-postgresql-and-kafka/
Apache License 2.0
2 stars 149 forks source link

Fix extension crashes #98

Closed samstokes closed 8 years ago

samstokes commented 8 years ago

This fixes two known cases that segfault the Bottled Water extension (and thus cause Postgres to restart all backends):

Both were segfaulting due to not checking the return values of Avro library functions.

I also added tests for long table/column names - it turns out Postgres already has a compile-time limit on identifier length, so unless people are routinely patching Postgres to increase the limit, we probably don't need to handle this case.

For invalid Avro identifiers, I chose to sanitise identifiers using an encoding similar to the "percent encoding" used in URLs. Unsupported characters are replaced by a hexadecimal representation: e.g. "person.name" -> "person_2e_name"

A couple of gotchas:

For tables with no columns, ideally we'd represent this by just publishing Avro records with no fields, but that was the existing behaviour of the code, and Avro was bailing out when trying to process the record schema. I'm not sure (and the spec doesn't make it clear) whether a record with zero fields is valid Avro, but it's at least not supported by avro-c at present. So instead I just publish a "dummy" field with the value false.

msakrejda commented 8 years ago

@samstokes the #61 fix is reasonable (like I said, this is a total edge case and basically anything other than crashing is okay). For the other one, does this mean that if we have a Unicode table name like "crêpes", does that mean we can't predict the resulting topic name? I think this is probably fine for now, but it may be something we need to revisit, especially if we're working with a cluster where topic auto-creation is not turned on, so we'd need to know bottledwater's mangled topic name a priori.

samstokes commented 8 years ago

@uhoh-itsmaciek that's weird, "crêpes" was my Unicode test case too! My calling it "unspecified" was mainly punting for now just because I'm pretty sure it's not the only Unicode incompatibility, but your comment persuaded me to dig a bit deeper and I discovered a serious bug in my sanitisation function - thanks!

Now the story is a bit better. The encoding of non-ASCII characters isn't particularly intuitive, but it is deterministic: it's the underscore encoding of the bytes representing those characters in the server encoding (which I'm guessing we never change from UTF-8). e.g.: "crêpes" -> "cr_c3__aa_pes"

Good point re clusters without auto-create. This should help, since obviously everyone can do UTF-8 encoding in their heads.

msakrejda commented 8 years ago

It's the most delicious Unicode test case!

And nice, I think that's certainly good enough for now.