ReportRemoteError changes could hide data divergence

[jasonmp85]

I was worried about this problem when reviewing #552, but after thinking about it for some time I couldn't convince myself it was actually possible under an all-INSERT workload, so I didn't comment on it.

Basically my thought was that one writer could fail (due to connection) to write a PK value to node A, decide to mark it as inactive, then move on to node B.

Another writer could successfully connect to A (before it's marked as unhealthy), write the same PK value, then move onto node B and get a PK violation.

With the changes from #552, it would error out on the PK violation even though it wrote to node A. Perhaps there's a third placement on node C which should have been updated, but is now divergent from A.

Because I couldn't construct a scenario where A isn't already about to be marked as inactive by a concurrent writer, I didn't think much of this issue.

However, @anarazel points out that certain customers might be creating indexes themselves on shards (manually, on the workers, to e.g. use CONCURRENTLY).

In this case, it's possible one placement has an index created while another doesn't. We could write a duplicate row to one placement and move to another placement where duplicates are already disallowed. We'd error out, marking nothing as inactive. One placement would have an extra row not present on other placements.

Of course, you might say "wait, why is the user connecting to a node directly?" or "but then the index creation should fail, and they should mark the placement as inactive!"

I agree, but realistically we could (possibly) modify #552 slightly to address this: @anarazel suggested only throwing a (e.g. PK violation) error if we haven't written to any nodes yet. If we do have a successful modification, then fall back to the "old" method of warning-and-marking-inactive.

The code change here isn't too big, but we wanted to bring it up as an issue to ensure that e.g. Citus Cloud's error was addressed even with this modification.

[lfittl]

@jasonmp85 If I read this correctly, this would mean that there will still be cases where shards are marked inactive in 5.2 due to DDL changes?

Since Cloud doesn't give access to the workers, at least for now, this is probably an unlikely scenario to occur - but with MX this would change (since you'd randomly connect to one of the workers).

I'm wondering if its worthwhile to re-evaluate what "inactive" means - specifically, if I'm adding an index to a distributed table, and that fails (e.g. since the index was manually added to some nodes) it shouldn't take my database offline.

What I'd expect to happen is that (depending on the DDL change) write loads would fail, but reads would still be allowed through. In the case of an index being partially added we could even allow writes.

That of course assumes we introduce another mechanism that informs the user of the problem, but with Cloud this could even be an email notification - with regular Citus we could show a warning in the logs.

[ozgune]

@lfittl -- I wanted to quickly follow up on this issue and our internal email thread (re: Shard placement marked as invalid with replication_factor=1 (on Cloud)).

Did we conclude that we wouldn't run into data divergence issues on the Cloud side?

[lfittl]

@ozgune It depends on how we handle a user creating unique indices on only one of the workers, as @jasonmp85 was describing.

As of now, without MX, we have no problem here on Cloud, since the customers can't connect to the workers directly - and as soon as we have 2PC for DDL we are safe.

With MX this might become a problem - obviously you shouldn't be running DDL on the workers (thats what the schema node is for), but its an open question how that is enforced.

If you do end up doing DDL on only one of the workers, then we come back to the question "how does it fail" (since a user couldn't recover from an invalid shard placement themselves).

[marcocitus]

No longer relevant after #5379 (or earlier)