CDRIVER-3905 Change estimatedDocumentCount to use collStats on 4.9+ #758
Conversation
| */ | ||
| error = NULL; | ||
| count = 0; | ||
| GOTO (done); |
There was a problem hiding this comment.
Although this swallows the error and returns 0, it does not touch the passed-in reply which still contains the NamespaceNotFound error. This is problematic, as users will not expect the reply to contain an error when the returned error is NULL. I could instead destroy reply and document that reply may be NULL when running against a non-existent collection. I could also mock a server response in reply, but mocking server behavior client-side sounds like trouble.
@alcaeus I'm curious what would make more sense in PHP.
There was a problem hiding this comment.
PHP does not use the CRUD API from libmongoc but implements its own in the PHP library. As such, we're not affected by changes to this method.
There was a problem hiding this comment.
Upon taking a closer look, we'll have to consider the server reply in general: even when the collection exists, the reply will be different from what a user might expect to be returned. I'm not sure how much of this is covered by a BC promise, but users won't see the usual count reply document when running on 4.9+.
There was a problem hiding this comment.
As mentioned in the other thread, CDRIVER-3933 will be used for considering the future of the estimated document count reply.
There was a problem hiding this comment.
Have a couple of notes, but the changes LGTM in general. I'd like to let @bazile-clyde or @kevinAlbs chime in on the reply and error pointer comments once they're available.
| /* Collection does not exist. From spec: return 0 but no err: | ||
| * https://github.com/mongodb/specifications/blob/master/source/crud/crud.rst#estimateddocumentcount | ||
| */ | ||
| error = NULL; |
There was a problem hiding this comment.
I may be mistaken, but I believe that this will cause memory to leak as we're not resetting error information, but rather setting the pointer to NULL. This should either use bson_set_error or memset to make sure there's no error information.
There was a problem hiding this comment.
I'm not sure if this a memory leak, but I'm sure it's a no-op. Because C passes everything by value, this line changes a copy of the parameter's address and not the actual parameter itself or its contents. If this were passed by reference somehow, then this would be a memory leak. Here's a code snippet of what's happening:
void faux_mod(int *ptr) {
ptr = NULL;
printf("faux_mod: address=%p\n", ptr ? ptr : 0);
}
void real_mod(int *ptr) {
*ptr = 2;
printf("real_mod: address=%p,val=%d\n", ptr ? ptr : 0, ptr ? *ptr : 0);
}
int main() {
int* ptr;
ptr = (int*) malloc(sizeof(int));
*ptr = 1;
printf("address=%p,val=%d\n", ptr ? ptr : 0, ptr ? *ptr : 0);
faux_mod(ptr);
printf("address=%p,val=%d\n", ptr ? ptr : 0, ptr ? *ptr : 0);
real_mod(ptr);
printf("address=%p,val=%d\n", ptr ? ptr : 0, ptr ? *ptr : 0);
printf("set to NULL\n");
ptr = NULL;
printf("address=%p,val=%d\n", ptr ? ptr : 0, ptr ? *ptr : 0);
free(ptr);
}
...
# output
address=0x558bcab3e2a0,val=1
faux_mod: address=(nil)
address=0x558bcab3e2a0,val=1
real_mod: address=0x558bcab3e2a0,val=2
address=0x558bcab3e2a0,val=2
set to NULL
address=(nil),val=0
The snippet above prints either a valid address of ptr and its value or nil. Note that faux_mod only changes a local copy of its argument's address, which doesn't have any non-local effects. On the other hand, real_mod uses the address to change the contents of what's being pointed to.
@alcaeus solution of using bson_set_error is correct.
There was a problem hiding this comment.
Yep, you're totally right! Very helpful explanation, thank you @bazile-clyde . I only use bson_set_error now.
There was a problem hiding this comment.
Thanks for that explanation @bazile-clyde, I completely forgot about pass-by-value 🤦♂️
| if (ret && bson_iter_init(&iter, reply_ptr)) { | ||
| if (bson_iter_find_descendant (&iter, "cursor.firstBatch.0.n", &iter)) { | ||
| count = bson_iter_as_int64 (&iter); | ||
| } |
There was a problem hiding this comment.
If the command was successful but we don't find the data we're looking for, we're returning a count of -1 without exposing any additional information. It may be sensible to set error to something that indicates that we didn't receive the information we were looking for.
There was a problem hiding this comment.
An interesting point! The estimated document count function did not return an error on a successful but malformed server response before this added functionality, so I'm not sure what a meaningful error would be. Any thoughts @bazile-clyde ?
There was a problem hiding this comment.
Since this is an extremely narrow use-case (only thing I can imagine is the server not returning an initial batch in the cursor), I'm fine leaving this as is and revisiting as part of CDRIVER-3933 or another ticket.
|
You're a victim of your own success - want to include changes from mongodb/specifications#933 in this PR? |
| */ | ||
| error->code = 0; | ||
| error->domain = 0; | ||
| memset (error->message, 0, strlen (error->message)); |
There was a problem hiding this comment.
I think you could also use memset for the entire error struct like:
memset (error, 0, sizeof *error);
There was a problem hiding this comment.
Ah that seems better! Thanks.
|
I can't remove the skipReason just yet, as the "latest" server version hasn't updated on Evergreen to include this commit. So, I'll do CDRIVER-3932 in the larger versioned API tests sync PR. |
CDRIVER-3905
Test changes in first three commits, code changes in remaining three.
Updates
mongoc_collection_estimated_document_countto use an aggregate with the $collStats pipeline stage when running against server version 4.9 or higher. Adds a new wire version macro for wire version 12.Adds new spec tests for estimatedDocumentCount under
crud/unifiedand moves old CRUD tests tocrud/legacy. Updates unified test runner accordingly. Updates retryable reads and versioned API spec tests involving estimatedDocumentCount.