At Fri, 17 Jun 2022 20:31:50 +0200, Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote in > On 2022-Jun-16, Kyotaro Horiguchi wrote: > > > The attached is a crude patch to separate the state for PIPELINE-IDLE > > from PGASYNC_IDLE. I haven't found a better way.. > > Ah, yeah, this might be a way to fix this. > > Something very similar to a PIPELINE_IDLE mode was present in Craig's > initial patch for pipeline mode. However, I fought very hard to remove > it, because it seemed to me that failing to handle it correctly > everywhere would lead to more bugs than not having it. (Indeed, there > were some.) > > However, I see now that your patch would not only fix this bug, but also > let us remove the ugly "notionally not-idle" bit in fe-protocol3.c, > which makes me ecstatic. So let's push forward with this. However,
Yey. > this means we'll have to go over all places that use asyncStatus to > ensure that they all handle the new value correctly. Sure. > I did found one bug in your patch: in the switch for asyncStatus in > PQsendQueryStart, you introduce a new error message. With the current > tests, that never fires, which is telling us that our coverage is not > complete. But with the right sequence of libpq calls, which the > attached adds (note that it's for REL_14_STABLE), that can be hit, and # (ah, I wondered why it failed to apply..) > it's easy to see that throwing an error there is a mistake. The right > action to take there is to let the action through. Yeah.. Actulallly I really did it carelessly.. Thanks! > Others to think about: > > PQisBusy (I think no changes are needed), Agreed. > PQfn (I think it should accept a call in PGASYNC_PIPELINE_IDLE mode; > fully untested in pipeline mode), Does a PQ_PIPELINE_OFF path need that? Rather I thought that we need Assert(!conn->asyncStatus != PGASYNC_PIPELINE_IDLE) there. But anyway we might need a test for this path. > PQexitPipelineMode (I think it needs to return error; needs test case), Agreed about test case. Currently the function doesn't handle PGASYNC_IDLE so I thought that PGASYNC_PIPELINE_IDLE also don't need a care. If the function treats PGASYNC_PIPELINE_IDLE state as error, the regression test fails (but I haven't examine it furtuer.) > PQsendFlushRequest (I think it should let through; ditto). Does that mean exit without pushing 'H' message? > I also attach a patch to make the test suite use Test::Differences, if > available. It makes the diffs of the traces much easier to read, when > they fail. (I wish for a simple way to set the context size, but that > would need a shim routine that I'm currently too lazy to write.) Yeah, it was annoying that the script prints expected and result trace separately. It looks pretty good with the patch. I don't think there's much use of context size here. regards. -- Kyotaro Horiguchi NTT Open Source Software Center