Change walproposer protocol to run on libpq

[sharnoff]

Corresponding PR for neondatabase/neon#366.

[knizhnik]

I agree that using libpq protocol everywhere is good idea. But...
I think that one of the primary motivation for them is to simplify implementation and reuse existed libpq code. But this patch is adding almost three times more lines then removing. And logic of state machine becomes even more complicated. May be new implementation is more correct and correctly handle problems which are not addressed by old implementation. But at least just by looking at the code I can not ensure than new FSM really works correctly. Now there are three different sets of states: wal keeper state, poll state and socket state. I am not sure if it really needed and make code more clear and maintainable. But I can not propose some alternative solution. Using coroutines is definitely peferrable that manual implementation of FSM, but as far as walproposer is embedded in Postgres and has to be implemented in C, there are almost no other choices...

[sharnoff]

@knizhnik Those are all fair points -- a few things that I think are worth mentioning:

• The solution I've implemented definitely is more complicated. Some of that I think is inherent to libpq (TCP is actually pretty lightweight, in terms of code size), but there are also plenty of cases where we previously did blocking operations (e.g. connecting over TCP, sending small data, etc.) that are now required to be async like the rest.
  • There are a couple ways I think this could be simplified slightly -- the socket state is perhaps unnecessary, because I think it can be derived from the polling type (in most cases -- there'd have to be some slight work done to make it work in all cases).
• Large parts of the added code are due to lots of comments and other nice things -- like formatting functions (specific examples linked). While these aren't strictly necessary, I added them because I was already changing the relevant pieces and I thought it'd be an improvement. Aside from this though, the real meat of the new stuff is ~500 lines with lots of comments -- and it's mostly just replacing what was already a 250-line function.

That being said, there's definitely a lot going on here. I personally think the new structure of AdvancePollState is easier to understand than the previous WalProposerPoll, but that may be largely in part due to familiarity. I'm happy to explain more why I think that's the case if need be, but it may just be my personal opinion.

[arssher]

First of all, I like affluence of comments and general approach is correct and complete. However, I think there is still quite a potential to make things simpler and have less code. Fundamentally, I'd keep the previous structure of single switch per states waiting for read to be available and single switch per states waiting for write to be available instead of two switches on pollstate and state with goto between them. I see why WalKeeperPollState was invented -- the idea is to separate libpq actions common between different higher-level states -- but it turns out there is not many such actions and they can be packed in single switch.
Moreover,

1. All voting packets (essentially everything proposer sends but WAL itself) are small and sent once in the beginning, it is pretty safe IMV to assume sending them won't block, like the earlier code did. Thus, we can PQsetnonblocking only when we start sending WAL. This eliminates all but one states like 'we've sent something, but it is not flushed yet'.
2. SS_EXEC_STARTWALPUSH is not needed as PQsendQuery("START_WAL_PUSH") is done only once immediately after connection establishment; it makes sense to materialize only states which we need to restore after polling.
3. Checking twice for changed socket in SPOLL_CONNECT handling is more code than it could be. I'd structure it like PQconnectPoll(); if failed, bail out; if socket changed, remove it from WaitEventSet; switch on the rest of results, each one making appropriate UpdateEventSet (and in case of SUCCESS, transitioning to further stuff)
4. PQconsumeInput can be moved inside libpqprop_async_read and libpqprop_get_query_result, this eliminates SPOLL_PQ_CONSUME_AND_RETRY.
5. I see the reason behind RequestStateAdvanceNoPoll machinery, but it can be removed by directly walking over SS_VOTING safekeepers and immediately sending message to them like it was done earlier.
6. Some more comments inline.
7. WKSockWaitKind completely submirrors WL_* bitmask, it is redundant.

Also, as inline comment says, we must track reading on socket even in states like SS_IDLE to detect EOF. This point actually increases complexity, so not from the list above :)

So roughly, I imagine the skeleton of AdvancePollState could be like

if (wk->state = SS_CONNECTING) {
  pqconnectpoll, handle result; in case of success, send START_WAL_PUSH right here, set WL_SOCKET_READABLE and switch
  to `SS_WAIT_EXEC_RESULT` 
  return;
}

if (event.events & WL_SOCKET_READABLE) {  
  case SS_WAIT_EXEC_RESULT
    # we are lucky as we have only one state waiting for PQgetResult
    just call `libpqprop_get_result` which chains `PQconsumeInput, PQisBusy, PQgetResult` internally. 
  # this can be just list of all high level states expecting message, or explicitly materialized thing
  # like `pollState` currently, I don't think this matters much.
  case states_expecting_copydata_message
    call `libpqprop_async_read` which chains `PQconsumeInput and PQgetCopyData` internally. If success, another switch (perhaps in separate function) on state deciding allowed message type and taking appropriate action.
  case default (not expecting read):
    complain on EOF (or try to read message and complain on proto violation if that succeeds)
}
if (event.events & WL_SOCKET_WRITEABLE) {
  case SS_SEND_WAL
    send_wal();
    return;
  case SS_SEND_WAL_FLUSH
    pqFlush; on success remove WAL_SOCKET_WRITEABLE interest and switch to SS_RECV_FEEDBACK
  default:
    Assert(false)
}

# called from AdvancePollState when retrying or directly on new wal arrival
send_wal() {
  PQputCopyData() handling all 4 possible results (failed, need retry (sets writeable interest), success but flush needed (sets writeable interest), success (removes it))
  # in async proto, this will loop until all wal is sent or we are blocked
}

As said above, realistically there are no other states except SS_SEND_WAL_[FLUSH] requring resending/flushing. Idly speculating, if we had such, we'd have 1) function async_write(msg, next_state) which performs current walprop_async_write and depending on result either directly transfers to next_state (success) or remembers it but sets current as 'flush_pending' in case of WOULDBLOCK(+ sets WL_SOCKET_WRITEABLE interest) -- we'd use it in all places where to send copydata 2) Similar to state_expecting_copydata_message in place of SS_SEND_WAL_FLUSH we'd have all all such states enumerated (or common marker); after successfull pqFlush it removes WL_SOCKET_WRITEABLE interest and depending on remembered next state takes actions.

All other not-network-triggered advancements (sending new piece of WAL in BroadcastMessage, reconnection) are just done directly. Currently protocol is simple and synchronous (there is only blocking request-response), so we don't need to loop inside such AdvancePollState at all. I hope we will switch at some point to uncoordinated (async) wal and feedback sending, in which case there would be a loop, but that's not hard to wrap things inside of it -- each action just should say whether it makes sense to try advancing state some more or we must return immediately.

Last and least, some bikeshedding:

• Where libpqwalproposer doesn't perform anything smart with result of libpq call I'd just copy result values from libpq instead of inventing one more duplicating layer. The example is libpqprop_connect_poll.
• There are Asserts in PG which on firing crash the instance if it was compiled with --enable-cassert (and ignored otherwse), it many places (but not everywhere) it is better than elog(FATAL, ...).
• In switches it is better to move impossible (unexpected) states to the bottom of the switch, as they are of less importance.
• The accepted PG codestyle is try not to exceed 80 chars limit per line, comments (but not only them) here are often wider.

[arssher]

We actually never WANTS_NO_WAIT, because even if we don't expect anything we still listen to read notifications to notice EOF on it. So this as well as remove_if_nothing is redundant.

[sharnoff]

This makes sense. At first glance, properly making this change would be tricky, so I think the best thing to do is probably to figure out the best way to remove WKSockWaitKind entirely.

Also -- if we always require some kind of valid socket condition, then we introduce an edge case for SS_OFFLINE. Granted, it's a small edge case, but it's the sort of thing that I'd be concerned about introducing because it has the potential to add hard-to-reproduce bugs.

[arssher]

the best way to remove WKSockWaitKind entirely.

+1

then we introduce an edge case for SS_OFFLINE

Which is fine, as it must never get into UpdateEventSet, no?

[sharnoff]

Ah yeah, you're right. UpdateEventSet probably doesn't need anything more than an assertion that the walkeeper isn't offline then

[sharnoff]

@arssher Lots of stuff to go through! Some of this, I mentioned in our chat earlier, but I'm reiterating it here just to have it.

---

In general, the list you've made makes a lot of sense. The big point seems to be that there's many operations where -- even though the returns from libpq allow otherwise -- we expect them to immediately succeed (so we can use non-blocking writes). This seems to encompass points 1, 2, & 5. I'll work through them, commenting if any obstacles show up.

---

As for restructuring to go back to the previous structure, I'd like to provide some of the reasoning behind why I moved away from that.

When I was first trying to understand how the previous implementation worked, I found it very difficult to piece together the state transitions. I've since added some comments to the definition of WalKeeperState to help with this, but I found the structure of the code itself to be difficult to understand, because the flow of logic jumped between both if branches.

This reason is why I've taken special care to order all of the cases in the walkeeper state switch to go in the order that their logic is executed. There's a clear flow in the way that each state (on success) moves to the state listed immediately below it (even if there might be some IO required first, so it's not executed immediately).

There are definitely other things that I think should be changed about this implementation though -- either before merging, or immediately afterwards. I agree that it's overly complex to have three different fields encoding the state. If libpq will actually let us send small CopyData messages without blocking, then I there's maybe few enough states that it makes sense to collapse all of the polling logic into the states themselves.

I understand that your proposed solution would have much less duplicated code if everything was assigned a state as-is, but I think that we can improve this enough by introducing some helper functions that it shouldn't make too much of a difference.

The tl;dr is basically that I originally chose this design because I thought the structure was significantly easier to understand and expresses it more clearly (even if it's currently more messy because it hasn't been polished yet). If you strongly believe that the previous structure is better, I'm happy to change what I've written -- I just do not think it is a good idea.

Re: bikeshedding

• For libpqwalproposer return values, I definitely understand where you're coming from. The reason I added the new types in the way that I did was because there's a lot of extra information that's somewhat specific to libpq as an interface, and I didn't want that to get in the way when trying to understand what's going on in 'walproposer.c'. This is particularly evident in things like get_query_result -- we only care about a couple of those cases. It also extends to connect_poll as well, though -- having exhaustive case matching in switch statements is a really valuable tool, and even though it's only a warning, I designed that interface to take advantage of this. I can completely see how it might not be "typical C style", though. I'm happy to change it if we think it's not worth the readability improvement. (IMO it's a drastic improvement, which is why I did it.)
• Changing things to Asserts makes sense. I hadn't realized that was an option.
• I wasn't aware of that style for switches -- moving unexpected cases to the bottom so that they are evaluated last certainly makes sense from a performance perspective. For the switch over walkeeper states, I think it makes the most sense to keep cases ordered so that states to the case immediately below them, as I've mentioned above.
• For code width -- makes sense. I'll fix it once everything else is ok, because that commit will have a big diff without any change.

[sharnoff]

This last commit (8e30d7d) seems to fix the problem I was having, but I'm not actually sure what the root cause is.

Basically, we were getting this sequence of actions in our tests:

1. Kill one of the WAL acceptors
2. WAL proposer detects failure
3. WAL proposer retries connection: doesn't immediately fail, successfully gets socket, starts waiting on socket to be write-ready
4. WAL acceptor restarts

AFAICT, because the WAL proposer restarts the connection (and starts waiting) before the WAL acceptor is back up, the socket it's waiting on is never write-ready, as is required for the first step of connection polling. (relevant docs)

I'm wondering if the recommended usage given by the docs fails to account for certain kinds of invalid connections, but I would have assumed they already had that handled.

[arssher]

Yeah, I see the same behaviour. However, I don't think this is normal, i.e. timeouts shouldn't be the cure. Note that if you attach with gdb and call print WalProposerFunctions->walprop_connect_poll(walkeeper[0]->conn) directly, connection is reported as failed; the system knows this, but we don't get notification for some reason. I'll try to poke around some more.

[sharnoff]

Oh interesting, ok. Perhaps we have to call PQconnectPoll always without waiting for the socket to become write-ready?

I replicated the same behavior as I found elsewhere in postgres (here), but maybe that's incorrect.

Another possibility: Maybe we check if it's read-ready when we're expecting write-ready -- that could indicate the socket is disconnected. Tossing out some ideas here

[arssher]

Hm-hm. Looks like @knizhnik was right above that (new_sock == old_sock) check is unsafe. At least if you always re-add the socket the problem disappears here.

man epoll says

Q6 Will closing a file descriptor cause it to be removed from all epoll sets automatically?

A6 Yes, ...

that's what most probably happens here: previous socket is removed from waiting set completely as it was closed by libpq.

[sharnoff]

Huh! When I was looking over the logs, I thought we never actually polled the connecting state (and so didn't give the socket a chance to change), but I guess that must have been incorrect. Thanks for the help :)

[arssher]

If you strongly believe that the previous structure is better,

Not 100% the previous, but what I find important is 1) avoid repetition of libpq-related logic, i.e. doing all polling (writing these chatty switches handling all possible outcomes) in a single place; 2) Have clearly trackable flow for each state, which IMV means there should be one top-level switch instead of two -- currently you have switch on pollState, then (sometimes) dispatch on state, and then goto down to dispatch on state again, which is not really needed. I have slightly updated the skeletone in my comment above trying to convey what I have in mind.

Idea to have the states listed sequentially is clear but, well, since they share libpq logic the control flow is mixed anyway. OTOH, you have them listed like that in enum declaration + have English description on the protocol (which should be updated BTW).

As for two switches, I see the idea of separating libpq polling to pollState switch, but it is leaky, it is hard to say what should be in one of them and what in another. walprop_get_query_result, walprop_async_write, ReadPGAsyncIntoValue are libpq actions/polls, but they are in the bottom switch. Hence are weird things like SPOLL_NONE and SPOLL_RETRY.

[arssher]

Re: Re: bikeshedding

It also extends to connect_poll as well, though -- having exhaustive case matching in switch statements is a really valuable tool, and even though it's only a warning, I designed that interface to take advantage of this.

You mean isolating caller from handling PGRES_POLLING_ACTIVE? Well, it is a point, but personally that's not a reason big enough for me to have enum different from libpq one. I don't really care anyway, it is really minor thing.

I wasn't aware of that style for switches -- moving unexpected cases to the bottom so that they are evaluated last certainly makes sense from a performance perspective.

It doesn't; what I meant is from readability POV the impossible states is the last thing you want to know about this switch because they are impossible. Sequential order is another valid point, so here we also have to compromise.

[sharnoff]

Idea to have the states listed sequentially is clear but, well, since they share libpq logic the control flow is mixed anyway

Yeah - I'd really like them to be that way if possible. I have some ideas for unifying control flow and reducing boilerplate that I'll have to flesh out a little more to judge them. The piece you'd mentioned above seems like a good idea:

we'd have async_write(msg, next_state) which performs current walprop_async_write and depending on result either directly transfers to next_state (success) or remembers it but sets current as 'flush_pending'

Reading is more tricky, but a similar idea should be possible, and I think this can cut down on boilerplate.

As for two switches, I see the idea of separating libpq polling to pollState switch, but it is leaky, it is hard to say what should be in one of them and what in another. walprop_get_query_result, walprop_async_write, ReadPGAsyncIntoValue are libpq actions/polls, but they are in the bottom switch. Hence are weird things like SPOLL_NONE and SPOLL_RETRY.

Definitely, yeah. In a similar vein to above, we can (and should) just get rid of this.

---

You mean isolating caller from handling PGRES_POLLING_ACTIVE?

Yup - that case is probably the weakest example of this, so it's maybe not worth it - as you've said. I do think something like the return on PQresultStatus is valuable enough to keep.

[sharnoff]

Going to merge this -- I opened #66 to keep this going. I'll follow up with a new PR for changes once they're somewhat implemented.