Hi,
Thanks for all of your reviews!
So, I made some changes in the v10 according to your comments.
1- copy_format is changed to a boolean parameter copy_binary.
It sounds easier to use a boolean to enable/disable binary copy. Default value is false, so nothing changes in the current behaviour if copy_binary is not specified.
It's still persisted into the catalog. This is needed since its value will be needed by tablesync workers later. And tablesync workers fetch subscription configurations from the catalog.
In the copy_data case, it is not directly stored anywhere but it affects the state of the table which is stored in the catalog. So, I guess this is the convenient way if we decide to go with a new parameter.
2- copy_binary is not tied to another parameter
The patch does not disallow any combination of copy_binary with binary and copy_data options. I tried to explain what binary copy can and cannot do in the docs. Rest is up to the user now.
3- Removed version check for copy_binary
HEAD already does not have any check for binary option. Making binary copy work only if publisher and subscriber are the same major version can be too restricted.
4- Added separate test file
Although I believe
002_types.pl and
014_binary.pl can be improved more for binary enabled subscription cases, this patch might not be the best place to make those changes.
032_binary_copy.pl now has the tests for binary copy option. There are also some regression tests in subscription.sql.
Finally, some other small fixes are done according to the reviews.
Also, thanks Bharath for performance testing [1]. It can be seen that the patch has some benefits.
I appreciate further thoughts/reviews.
Thanks,
--
