another release with the batch API and incremental updates

[tvondra]

Hi @thanodnl,

I've merged the two main improvements waiting in the queue, so maybe it's time to do another release. If you agree, can you do a bit more testing that the current version works fine on Citus etc.? It should be backwards-compatible, but better check.

I'm not entirely sure about the version bump - this is not just a bugfix, it adds features, and it's not a breaking change. So I'd increment the second number, e.g. 1.0.x to 1.1.0. The thing is I've incremented it twice when merging each of the features, so it.s 1.2.0 now. I wonder if that's a bit confusing - maybe we should revert that to just 1.1.0. What do you think?

Note: I noticed I had an incorrect e-mail in the last couple commits, so github did not realize the commits were from me. I had to fix that and force-push to fix that. Sorry if that broke something.

[naude-r]

@tvondra: do you have any idea when a new release would be available?

[tvondra]

@naude-r I don't know - for me this is mostly a hobby project, so I didn't have any particular schedule for the release. If you're interested in getting the release out the door, please do some testing and share your opinions.

@thanodnl, @Godwottery and @min-mwei - any opinions regarding the current code, with the improvements merged?

[naude-r]

@tvondra have run tests using tdigest_union. this worked just fine. have a use case for tdigest_add as well, but that will involve a fair bit more work on our end to use correctly.

[naude-r]

@tvondra: found an issue when performing backups: pg_dump: error: Error message from server: ERROR: unsupported t-digest on-disk format

looks like the change in format caused some incompatibilities.

[thanodnl]

Sorry for the late reply

I quickly ran the Citus test suite on the latest master. Distribution works, which relies on the serialization format. Also tested a stored tdigest value with v1.0.1 was still readable for master.

I did notice some of the more stricter tests from our suite that the combination of centroids seems to be changed:

 -- verifying results - should be stable due to seed while inserting the data, if failure due to data these queries could be removed or check for certain ranges
 SELECT tdigest(latency, 100) FROM latencies;
                                                                                                                                                                                                                                                                                                                                                                                                                                     tdigest                                                                                                                                                                                                                                                                                                                                                                                                                                    
 -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
- flags 0 count 10000 compression 100 centroids 46 (0.287235, 1) (1.025106, 1) (2.058216, 1) (5.335597, 1) (12.707263, 2) (25.302479, 3) (43.435063, 4) (77.987860, 5) (269.478664, 10) (509.417419, 13) (1227.158879, 22) (3408.256171, 35) (7772.721988, 55) (13840.275516, 65) (32937.127607, 108) (64476.403332, 148) (118260.230644, 199) (239584.293240, 292) (562119.836766, 463) (944722.686313, 547) (1751089.620493, 749) (3751264.745959, 1128) (5877270.108576, 1300) (6224557.402567, 1104) (5804999.258033, 874) (5632316.697114, 755) (4648651.050740, 573) (3460055.227950, 402) (2820271.404686, 314) (2676501.012955, 288) (1649845.166017, 173) (1269335.942008, 131) (813964.853243, 83) (484144.878702, 49) (337179.763016, 34) (198775.241901, 20) (149353.499704, 15) (109688.319223, 11) (79855.926155, 8) (49937.731689, 5) (29971.046175, 3) (19982.538737, 2) (9991.467422, 1) (9992.337047, 1) (9995.578357, 1) (9999.700339, 1)
+ flags 1 count 10000 compression 100 centroids 46 (0.287235, 1) (1.025106, 1) (2.058216, 1) (5.335597, 1) (6.353632, 2) (8.434160, 3) (10.858766, 4) (15.597572, 5) (26.947867, 10) (39.185955, 13) (55.779949, 22) (97.378748, 35) (141.322218, 55) (212.927315, 65) (304.973404, 108) (435.651374, 148) (594.272516, 199) (820.494155, 292) (1214.081721, 463) (1727.098147, 547) (2337.903365, 749) (3325.589314, 1128) (4520.977007, 1300) (5638.186053, 1104) (6641.875582, 874) (7460.022116, 755) (8112.829059, 573) (8607.102557, 402) (8981.756066, 314) (9293.406295, 288) (9536.677260, 173) (9689.587343, 131) (9806.805461, 83) (9880.507729, 49) (9917.051853, 34) (9938.762095, 20) (9956.899980, 15) (9971.665384, 11) (9981.990769, 8) (9987.546338, 5) (9990.348725, 3) (9991.269368, 2) (9991.467422, 1) (9992.337047, 1) (9995.578357, 1) (9999.700339, 1)
 (1 row)

(note; flags have changed, but also some of the centroids are changed, the comment above is what we added as instructions for our test, so I might remove these tests given the internal representation is understandably not stable between versions)

I don't think this is a problem, but would you expect such changes based on the difference between master and v1.0.1.

I will start adding support for the aggregate pushdown for the newly added batch API's in Citus. We kind of missed the feature freeze ( hence the late reply, we had a lot of different things going on, finally have some time to maintain our integration here ), so it will land in 10.2 most likely for Citus.

@min-mwei I don't know if you rely on this API to be working in a pushdown fashion (when not grouping on your distribution column). Please reach out to me internally if this is high priority for you, we either land it on our mainline soon and you work from there, or we can see if we can rush it in before release.

@tvondra I think v1.2.0 is ok. I don't think many find it confusing if 1.1.0 is skipped. Given the two new features without a release in between can be understood by many of its users, if they care about the version at all.

Edit: Work in progress branch on the CItus repo for integrating the new APIs: https://github.com/citusdata/citus/compare/feature/batch-add-tdigest Most interesting would be the test output I guess, given the explain plans that push parts of the calculation to the worker nodes.

[naude-r]

@thanodnl : have you tried a backup after upgrading to v1.2.0? in our case older data serialized with v1.0.1 caused a failure (see previous comment).

[tvondra]

@tvondra: found an issue when performing backups: pg_dump: error: Error message from server: ERROR: unsupported t-digest on-disk format

looks like the change in format caused some incompatibilities.

Oh, wow.

I thought this will be just a silly case of not calling tdigest_fix_mean in the output function, but I discovered a much more serious issue with that function - we must not modify the t-digest, because it might be just in the original data block (and we must not modify that - checksums being one reason, etc.). We have to copy the tdigest value, and modify that copy.

Or maybe we could do away without the copy, but then we'd have to teach all the places about the two on-disk formats ...

Will fix, but have to think about it for a bit.

[tvondra]

Sorry for the late reply

I quickly ran the Citus test suite on the latest master. Distribution works, which relies on the serialization format. Also tested a stored tdigest value with v1.0.1 was still readable for master.

I did notice some of the more stricter tests from our suite that the combination of centroids seems to be changed:

 -- verifying results - should be stable due to seed while inserting the data, if failure due to data these queries could be removed or check for certain ranges
 SELECT tdigest(latency, 100) FROM latencies;
                                                                                                                                                                                                                                                                                                                                                                                                                                     tdigest                                                                                                                                                                                                                                                                                                                                                                                                                                    
 -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
- flags 0 count 10000 compression 100 centroids 46 (0.287235, 1) (1.025106, 1) (2.058216, 1) (5.335597, 1) (12.707263, 2) (25.302479, 3) (43.435063, 4) (77.987860, 5) (269.478664, 10) (509.417419, 13) (1227.158879, 22) (3408.256171, 35) (7772.721988, 55) (13840.275516, 65) (32937.127607, 108) (64476.403332, 148) (118260.230644, 199) (239584.293240, 292) (562119.836766, 463) (944722.686313, 547) (1751089.620493, 749) (3751264.745959, 1128) (5877270.108576, 1300) (6224557.402567, 1104) (5804999.258033, 874) (5632316.697114, 755) (4648651.050740, 573) (3460055.227950, 402) (2820271.404686, 314) (2676501.012955, 288) (1649845.166017, 173) (1269335.942008, 131) (813964.853243, 83) (484144.878702, 49) (337179.763016, 34) (198775.241901, 20) (149353.499704, 15) (109688.319223, 11) (79855.926155, 8) (49937.731689, 5) (29971.046175, 3) (19982.538737, 2) (9991.467422, 1) (9992.337047, 1) (9995.578357, 1) (9999.700339, 1)
+ flags 1 count 10000 compression 100 centroids 46 (0.287235, 1) (1.025106, 1) (2.058216, 1) (5.335597, 1) (6.353632, 2) (8.434160, 3) (10.858766, 4) (15.597572, 5) (26.947867, 10) (39.185955, 13) (55.779949, 22) (97.378748, 35) (141.322218, 55) (212.927315, 65) (304.973404, 108) (435.651374, 148) (594.272516, 199) (820.494155, 292) (1214.081721, 463) (1727.098147, 547) (2337.903365, 749) (3325.589314, 1128) (4520.977007, 1300) (5638.186053, 1104) (6641.875582, 874) (7460.022116, 755) (8112.829059, 573) (8607.102557, 402) (8981.756066, 314) (9293.406295, 288) (9536.677260, 173) (9689.587343, 131) (9806.805461, 83) (9880.507729, 49) (9917.051853, 34) (9938.762095, 20) (9956.899980, 15) (9971.665384, 11) (9981.990769, 8) (9987.546338, 5) (9990.348725, 3) (9991.269368, 2) (9991.467422, 1) (9992.337047, 1) (9995.578357, 1) (9999.700339, 1)
 (1 row)

(note; flags have changed, but also some of the centroids are changed, the comment above is what we added as instructions for our test, so I might remove these tests given the internal representation is understandably not stable between versions)

I don't think this is a problem, but would you expect such changes based on the difference between master and v1.0.1.

I think some differences are mostly expected, for two reasons:

1) the switch from "sum" to "mea" can have slightly different rounding errors 2) the rebalancing (commit 7e648a9527a685) may affect centroids close to 0.5

So as long as the results are reasonably accurate, I'd not worry about it too much.

[tvondra]

@tvondra: found an issue when performing backups: pg_dump: error: Error message from server: ERROR: unsupported t-digest on-disk format looks like the change in format caused some incompatibilities.

Oh, wow.

I thought this will be just a silly case of not calling tdigest_fix_mean in the output function, but I discovered a much more serious issue with that function - we must not modify the t-digest, because it might be just in the original data block (and we must not modify that - checksums being one reason, etc.). We have to copy the tdigest value, and modify that copy.

Or maybe we could do away without the copy, but then we'd have to teach all the places about the two on-disk formats ...

Will fix, but have to think about it for a bit.

I've pushed d14642eb573ddd80623b1dc8392587cf3ee2bb43, fixing those issues related to format update. The pg_dump etc. should work fine, now.

There's a question whether "out/send" functions should do the update and only generate the new format (in which case the "in/recv" have to handle both the old and new format, and do the conversion). Or whether to do the conversion in the output functions. The current code does the former.

For the extension itself it probably does not matter much, but if there are external tools parsing the text format it might be easier to handle just one format. Although, if the external tools need to support different extension versions, they have to handle the old format anyway.

[tvondra]

I've tagged the 1.2.0 release.