On Mon, Feb 20, 2023 at 5:17 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
>
> Thanks for letting me know.
> Attached the fixed version of the patch.
Thanks. I have few comments on v9 patch:
1.
+ /* Do not allow binary = false with copy_format = binary */
+ if (!opts.binary &&
+ sub->copyformat == LOGICALREP_COPY_AS_BINARY &&
+ !IsSet(opts.specified_opts, SUBOPT_COPY_FORMAT))
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot set %s for a
subscription with %s",
+ "binary = false",
"copy_format = binary")));
I don't understand why we'd need to tie an option (binary) that deals
with data types at column-level with another option (copy_format) that
requests the entire table data to be in binary. This'd essentially
make one to set binary = true to use copy_format = binary, no? IMHO,
this inter-dependency is not good for better usability.
2. Why can't the tests that this patch adds be simple? Why would it
need to change the existing tests at all? I'm thinking to create a new
00X_binary_copy_format.pl or such and setting up logical replication
with copy_format = binary and letting table sync worker request
publisher in binary format - you can verify this via publisher server
logs - look for COPY with BINARY option. If required, have the table
with different data types. This greatly reduces the patch's footprint.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
