Fixed Ubuntu 20.04 build issues and changed Travis CI to test builds on it

[dura0ok]

Looks fantastic!

Thank you!!

Should @df7cb's commits to add support for PostgreSQL 16 be merged here or separately in PR #39?

I think it question for @vitcpp

[df7cb]

Either way is fine with me as long as PG16 gets fixed. :)

[vitcpp]

@stepan-neretin7 thank you for the fix. Concerning cherry-pick, I would like to see @df7cb contribution in a separate PR because the current change is independent. I looked at the spheretrans_out and I see that it should be improved. I propose to use psprintf function or try to find some API functions in postgres core for building strings. Personally speaking, I do not like some magic constants to define the buffer size.

[esabol]

Yeah, I agree with @vitcpp. So this PR should only include the change to the .travis.yml file to test on focal instead of bionic and whatever the correct fix is to spheretrans_out() in src/output.c. Then, after this PR is merged, PR #39 can be rebased on master and merged.

[esabol]

@vitcpp: Do you prefer the titles of the PRs to use past tense verbs or present tense verbs? There seems to be a mixture, but most PR titles seem to use past tense. If past tense is preferred, then the title of this PR should be changed to "Fixed Ubuntu 20.04 build issues and changed Travis CI to test builds on it" for consistency.

[vitcpp]

@esabol I have no preferences concerning past or present tenses. I got used to using the past tense when I described what was done. If you advise me to use the present tense, I will accept it. Thank you.

[esabol]

@vitcpp wrote:

@esabol I have no preferences concerning past or present tenses. I got used to using the past tense when I described what was done. If you advise me to use the present tense, I will accept it. Thank you.

I have no preference other than consistency. Since most of the other PRs use past tense, I propose changing the title of this PR to "Fixed Ubuntu 20.04 build issues and changed Travis CI to test builds on it", but it's really no big deal.

[esabol]

@stepan-neretin7 , can you please make the following requested changes to this PR:

• Remove the cherry-picked commits 6b30147 and 9369afd. I think you should keep 0a32639 in the PR (or commit your own version that does the same thing).
• Modify spheretrans_out() in src/output.c to use psprintf(). https://doxygen.postgresql.org/psprintf_8c.html

Thanks!

[vitcpp]

@esabol It seems github uses the present tense. I've found some instructions how to write good commit messages. There are some instructions in the Internet with the recommendation to use the present tense. So, I propose to adopt github style. It seems it was my fault to add commits with in past tense.

[esabol]

@esabol It seems github uses the present tense. I've found some instructions how to write good commit messages. There are some instructions in the Internet with the recommendation to use the present tense. So, I propose to adopt github style. It seems it was my fault to add commits with in past tense.

OK with me. I just think it's best to be consistent. 👍

[vitcpp]

@stepan-neretin7 Are you still going to complete the fix? I propose to change sphere_output_precision type to int instead of short int. It may fix compiler warnings related to sprintf. I would also propose to keep spheretrans_out unchanged because I'm not sure that the proposed solution significantly improves this function. Thank you in advance!

[esabol]

@vitcpp wrote:

I would also propose to keep spheretrans_out unchanged because I'm not sure that the proposed solution significantly improves this function.

You don't like the psprintf() solution? I guess it is probably less efficient than the char buf[###]/sprintf() version.

[vitcpp]

You don't like the psprintf() solution? I guess it is probably less efficient than the char buf[###]/sprintf() version.

Well, I see no profit in using psprintf solution in the current implementation:

• It is less efficient.
• The output has limited length (strans has 3 numbers, output is short).
• The result buffer is limited by 255 bytes and there is no overflow check.

I think the warnings can be fixed by some other means. A better idea may be to use a string builder but i'm do not know whether postgresql code has such functionality.

[esabol]

@vitcpp wrote:

I think the warnings can be fixed by some other means.

@stepan-neretin7 already fixed the warnings initially by making buf bigger. So go back to the first solution?

A better idea may be to use a string builder but i'm do not know whether postgresql code has such functionality.

There's nothing like that that I could find, and I spent some time looking. I looked up the implementation of psprintf(), and I don't think it's quite as bad as you have characterized (it checks for overflow and reallocs if it needs to be bigger than 255), but I agree it is less efficient. I'm OK with the initial solution of just making buf bigger to avoid the compiler warning.

[vitcpp]

@esabol I guess that the problem is not the too-small buffer size. The sprintf template contains the symbol '*' that defines the precision of the floating value, but the precision is passed as an argument in the arg list of sprintf. The corresponding parameter should be of int type, but the variable sphere_output_precision is of short int type instead. I think that changing the variable type to int will fix the warnings.

[vitcpp]

I'm not sure that psprintf improves the situation because the output is limited by the precision. We can define max precision which is supported. Not sure that 100 digits is the meaningful precision value. In this case buffers in the stack are enought for intermediate calculations. What I would propose is to work without intermediate buffers. Once we palloced the buffer, sprintf is enough for us if we are confident that the output is limited by some length. Just some simple pointer math is needed.

[esabol]

@vitcpp wrote:

I guess that the problem is not the too-small buffer size. The sprintf template contains the symbol '*' that defines the precision of the floating value, but the precision is passed as an argument in the arg list of sprintf. The corresponding parameter should be of int type, but the variable sphere_output_precision is of short int type instead. I think that changing the variable type to int will fix the warnings.

To be honest, I really don't see how, seeing as how the possible range of a short int is smaller than an int's, but maybe I'm being dumb. Would you submit your proposed solution as a separate PR so that we can see?

[esabol]

Not sure that 100 digits is the meaningful precision value.

It's not. What is relevant is DBL_DIG since that's the maximum number of digits of precision a user can set using set_sphere_output_precision(), right? And that's 15, I think?

[vitcpp]

@esabol Yes, sure. There is the maximal number of meaningful digits for double. It is about that what you mentioned. I just wanted to say that 100 meaningful digits is the not real number for double. It was an ironic statement from my side.

BTW, I've made a test PR. Lets see how it goes.

[esabol]

With the mergings of PR #51 and PR #39, this PR is no longer necessary and can be closed without merging.

[vitcpp]

Closed as #51 and #39 are completed.