On Fri, Mar 17, 2017 at 12:37 AM, Daniel Verite <dan...@manitou-mail.org> wrote:
> Vaishnavi Prabakaran wrote: > > > So, attached the alternative fix for this issue. > > Please share me your thoughts. > > I assume you prefer the alternative fix because it's simpler. > I would like add one more reason for this fix, I think "PQsetSingleRowMode" should be called only when the result is ready to be processed and before starting to consume result as it is documented currently as follows - "To enter single-row mode, call PQsetSingleRowMode immediately after a successful call of PQsendQuery (or a sibling function). This mode selection is effective only for the currently executing query. Then call PQgetResult repeatedly..." I agree that first fix (you shared) will allow user to set single-row mode after PQsendQuery, but it also allows user to set this mode at any time of batch processing(not necessarily "immediately after PQsendQuery"), also "mode selection is effective only for the currently executing query" will be false. Please note that I don't see any problem with this deviation. I like to outline that documentation here anyways needs an update/note. Before going further, I would like to mention that I have modified the documentation of batch processing( in v6 code patch) as below: "To enter single-row mode, call PQsetSingleRowMode immediately after a successful call of PQbatchQueueProcess. This mode selection is effective only for the currently executing query. For more information on the use of PQsetSingleRowMode , refer to Section 33.6, “Retrieving Query Results Row-By-Row”. " Please let me know if you think this is not enough but wanted to update section 33.6 also? > > > I would also like to hear Craig's opinion on it before applying this fix > > to the original patch, just to make sure am not missing anything here. > > +1 > > The main question is whether the predicates enforced > by PQsetSingleRowMode() apply in batch mode in all cases > when it's legit to call that function. Two predicates > that may be problematic are: > if (conn->asyncStatus != PGASYNC_BUSY) > return 0; > and > if (conn->result) > return 0; > > The general case with batch mode is that, from the doc: > "The client interleaves result processing with sending batch queries" > While sending batch queries in middle of result processing, only the query is appended to the list of queries maintained for batch processing and no current connection attribute impacting result processing will be changed. So, calling the PQsetSingleRowMode in-between result processing will fail as it tries to set single-row mode for currently executing query for which result processing is already started. Note that I've not even tested that here, I've tested > batching a bunch of queries in a first step and getting the results > in a second step. > I am not confident that the above predicates will be true > in all cases. Also your alternative fix assumes that we add > a user-visible exception to PQsetSingleRowMode in batch mode, > whereby it must not be called as currently documented: > "call PQsetSingleRowMode immediately after a successful call of > PQsendQuery (or a sibling function)" My gut feeling is that it's not the right direction, I prefer making > the single-row a per-query attribute internally and keep > PQsetSingleRowMode's contract unchanged from the > user's perspective. > > Am going to include the test which you shared in the test patch. Please let me know if you want to cover anymore specific cases to gain confidence. Thanks & Regards, Vaishnavi, Fujitsu Australia.