Re: [GSoC] working status
Hello Charles, >The second review is coming. Here is my working status so far. 1. > Complete the thrift compact protocol implementation using bytea > interface. 2. Thrift type (binary protocol) is almost done, the only > remaining part is struct encoding and decoding. With the thrift type, > you can express your thrift struct using json, but stored using > thrift bytes. 3. Set up travis CI. 4. better documents. > Here is the repo with all recent changes > (https://github.com/charles-cui/pg_thrift) > Let me know if you have any questions. Thanks for keeping us informed! Though it seems that the code is a little bit broken at the moment: ``` pg_thrift.c:1313:26: error: too few arguments to function ‘array_create_iterator’ ArrayIterator iter = array_create_iterator(parray, 0); ^ In file included from pg_thrift.c:5: .../postgresql-install/include/server/utils/array.h:418:22: note: declared here extern ArrayIterator array_create_iterator(ArrayType *arr, int slice_ndim, ArrayMetaState *mstate); ^ ``` -- Best regards, Aleksander Alekseev
Re: [GSoC] working status
Hello Charles, > ``` > pg_thrift.c:1313:26: error: too few arguments to function > ‘array_create_iterator’ ArrayIterator iter = > array_create_iterator(parray, 0); ^ > In file included from pg_thrift.c:5: > .../postgresql-install/include/server/utils/array.h:418:22: > note: declared here extern ArrayIterator > array_create_iterator(ArrayType *arr, int slice_ndim, ArrayMetaState > *mstate); ^ > ``` I should probably explain this in a little more detail. This was an attempt to build pg_thrift with PostgreSQL 10 (compiled from REL_10_STABLE branch). Same issue with upcoming 11 version. I believe you are testing the extension on some 9.x branch and don't see these errors. Corresponding change in the API was done quite a long time ago, in 2015 (see commit 13dbc7a8). To solve this issue you can either use #ifdef's and maintain two versions of the code for different versions of PostgreSQL or just support only 10+. Both options are fine, at least by me personally. -- Best regards, Aleksander Alekseev
Re: [GSoC] working status
Hello Charles, > > ``` > > pg_thrift.c:1313:26: error: too few arguments to function > > ‘array_create_iterator’ ArrayIterator iter = > > array_create_iterator(parray, 0); ^ > > In file included from pg_thrift.c:5: > > .../postgresql-install/include/server/utils/array.h:418:22: > > note: declared here extern ArrayIterator > > array_create_iterator(ArrayType *arr, int slice_ndim, ArrayMetaState > > *mstate); ^ > > ``` > > I should probably explain this in a little more detail. > > This was an attempt to build pg_thrift with PostgreSQL 10 (compiled > from REL_10_STABLE branch). Same issue with upcoming 11 version. I > believe you are testing the extension on some 9.x branch and don't > see these errors. Corresponding change in the API was done quite a > long time ago, in 2015 (see commit 13dbc7a8). > > To solve this issue you can either use #ifdef's and maintain two > versions of the code for different versions of PostgreSQL or just > support only 10+. Both options are fine, at least by me personally. And I should probably explain the "use #ifdef's" part. You can use PG_VERSION_NUM macro to determine PostgreSQL version in this fashion: ``` /* In version 11 these macros have been changed */ #if PG_VERSION_NUM < 11 #define PG_GETARG_JSONB_P(v) PG_GETARG_JSONB(v) #define PG_RETURN_JSONB_P(v) PG_RETURN_JSONB(v) #endif ``` In your case you can declare a wrapper for the array_create_iterator procedure to make it work on both 9.x and 10. -- Best regards, Aleksander Alekseev
Re: project updates
Hello Charles,
> Here is my current working status.
> 1. Complete the thrift_binary_in and thrift_binary_out functions, so
> that users can express their thrift struct using json. These two
> functions support both simple data struct and complex data structure
> like struct and map. 2. added test cases and document to cover
> thrift_binary_in and thrift_binary_out. 3. make the code compile-able
> from 9.4 to 11.0.
I see multiple warnings during compilation with 11.0, e.g:
```
pg_thrift.c:1120:72: warning: implicit declaration of function
‘JsonbGetDatum’; did you mean ‘JsonbPGetDatum’?
[-Wimplicit-function-declaration]
pg_thrift.c:1206:13: warning: the address of ‘is_big_endian’ will
always evaluate as ‘true’ [-Waddress]
pg_thrift.c:1227:18: warning: implicit declaration of function
‘PG_GETARG_JSONB’; did you mean ‘PG_GETARG_JSONB_P’?
[-Wimplicit-function-declaration]
pg_thrift.c:1227:18: warning: initialization of ‘Jsonb *’ {aka ‘struct
*’} from ‘int’ makes pointer from integer without a cast
[-Wint-conversion]
```
Also tests (make clean && make && make install && make installcheck)
don't pass.
> One question though, for custom types, it seems rich operations are
> also very important besides storing and expressing using thrift type.
> What are the most important operators should I support? Any
> suggestions? Here is the updated repo
> https://github.com/charles-cui/pg_thrift
I believe get field / set field are most common ones. Naturally there
are also enumerations, projections, you name it. You can check out list
of operation supported by JSONB (or a list of operations on dict's in
Python) to get a full picture.
--
Best regards,
Aleksander Alekseev
Re: project updates
Hello Charles, > Having tried David's method to install 10.4 and 11 on my mac and > turns out worked for me. The compiling issue posted by Aleksander is > because some json helpers changed function name and is not backward > compatible with 9.4 and 10. Using #if macro resolves the problem, > Here is the commit to solve this. > https://github.com/charles-cui/pg_thrift/commit/dd5b8ad17ab47e3c977943dd2d69e5abad34c6ad > Aleksander, do you want to test again? It's much better now, thank you! :) -- Best regards, Aleksander Alekseev
Re: [GSoC] current working status
Hello Charles, >The first evaluation is coming. Here is my progress so far. During > the first stage of work, I have implemented the thrift binary > protocol as the format of postgresql plugin. Currently, the main > interface is byte. Users who use this plugin need to provide thrift > bytes to the plugin, and there are helpers for each data type to > parse out the value contained in the bytes. This method has been > verified by the use of several unit tests. However, the current > interface needs users understand thrift format very well to use this > plugin. After discussing with my mentors, it will be more useful to > implement the concept of "thrift type", which is a custom type where > user provides user understandable format(e.g., json), then the plugin > converts into byte. I am currently busy with implementing this > feature and still need sometime to complete it. If this part is done, > I will go ahead to implement thrift compact protocol. Let me know if > you have comments. You can always track the progress from github > ( https://github.com/charles-cui/pg_thrift) Thanks for keeping us informed! Unfortunately I'm having difficulties compiling your code on Linux with PostgreSQL 10.4 or 11 and GCC 8.1.1. Here is a full list of warnings and errors: https://afiskon.ru/s/6e/edbefe818e_pg-thrift-errors.txt I also tried a VM with Ubuntu 16.04, PostgreSQL 9.6 and GCC 5.4.0. No luck either. Please make sure that your code compiles on Linux. Users will most likely try to run it on this OS. It might be also a good idea to add your repository to travic-ci.org service. -- Best regards, Aleksander Alekseev
Re: [GSoC] current working status
Hello Charles, >I saw the list of errors you posted. That's because I have some new > function implemented and pushed without testing(in order forget my > change in local machine). > So you are saying it is best to keep each commit workable. Right? Ideally, yes. You can always create a separate branch to experiment with new code and merge it to master branch when it becomes stable. This is not a strict GSoC requirement or something like this, but a good practice. If something goes wrong you can just delete an experimental branch and go back to working master branch. -- Best regards, Aleksander Alekseev
Re: [GSoC] current working status
Hello Charles, > The repo currently can be build on my mac. You can check it out. > Also I am working on the CI config to monitor each commit. Tests pass on Arch Linux with PostgreSQL 11 and Ubuntu 16.04 with PostgreSQL 9.6. However there is a name conflict in case of PostgreSQL 11: https://afiskon.ru/s/7a/fe681b17f0_paste.txt I suggest to rename the procedure to `convert_int8_to_char`. Also GCC still generates a lot of warnings (see my previous message). I would advise to fix all these warnings, otherwise you can miss a really important warning. Ideally, the code should be compiled with -Wall flag. -- Best regards, Aleksander Alekseev
Re: [GSoC] working status
Hello Charles, >Here is my current working status. Resolved all warnings found by > Aleksander previously. Having two threads in parallel. One is the > thrift binary type implementation, the other is thrift compact byte > interface implementation. For these two threads, simple data type has > been implemented and tested, but still need time to focus on > complicated data type like struct, map or list. Let me know if you > have any questions. Here is the repo with latest updates, > https://github.com/charles-cui/pg_thrift Thanks for keeping us informed! I noticed that there are not many comments in your code. It's considered good practice to write at least brief comments for every procedure with 1) short description of what it does 2) what every argument means 3) what does the procedure return 4) whether it changes global state somehow (better avoid it). However, there is no strict requirement to write these comments, especially if there are many similar procedures and (1-4) are obvious. Also I advise to check your code with following tools unless you've already done this: * lcov will show whether the code is covered with tests well. If some part of code is not covered there is probably a test missing or maybe the code is dead and should be removed. * Clang Static Analyzer may find some errors that are difficult to notice with a naked eye. * Valgrind solves the same issue, but unlike Clang Static Analyzer it checks your code not statically but in runtime. There are plenty of corresponding tutorials online. I wrote a few myself [1][2][3]. However, although they should be readable through Google Translate, I guess you may prefer tutorials in English. [1]: https://eax.me/c-code-coverage/ [2]: https://eax.me/c-static-analysis/ [3]: https://eax.me/valgrind/ -- Best regards, Aleksander Alekseev
