On Mon, Feb 20, 2023 at 5:17 PM Melih Mutlu <m.melihmu...@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