Re: Binary support for pgoutput plugin

On Tue, 7 Jul 2020 at 10:01, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> > On 7 Jul 2020, at 02:16, Dave Cramer <davecramer(at)gmail(dot)com> wrote:
>
> > OK, rebased it down to 2 patches, attached.
>
> I took a look at this patchset today. The feature clearly seems like
> something
> which we'd benefit from having, especially if it allows for the kind of
> extensions that were discussed at the beginning of this thread. In
> general I
> think it's in pretty good shape, there are however a few comments:
>
> The patch lacks any kind of test, which I think is required for it to be
> considered for committing. It also doesn't update the \dRs view in psql to
> include the subbinary column which IMO it should. I took the liberty to
> write
> this as well as tests as I was playing with the patch, the attached 0003
> contains this, while 0001 and 0002 are your patches included to ensure the
> CFBot can do it's thing. This was kind of thrown together to have
> something
> while testing, so it definately need a once-over or two.
>

I have put all your requests other than the indentation as that can be
dealt with by pg_indent into another patch which I reordered ahead of yours
This should make it easier to see that all of your issues have been
addressed.

I did not do the macro for updated, inserted, deleted, will give that a go
tomorrow.

>
> The comment here implies that unchanged is the default value for format,
> but
> isn't this actually setting it to text formatted value?
> + /* default is unchanged */
> + tuple->format = palloc(natts * sizeof(char));
> + memset(tuple->format, 't', natts * sizeof(char));
> Also, since the values member isn't memset() with a default, this seems a
> bit
> misleading at best no?
>
> For the rest of the backend we aren't including the defname in the errmsg
> like
> what is done here. Maybe we should, but I think that should be done
> consistently if so, and we should stick to just "conflicting or redundant
> options" for now. At the very least, this needs a comma between "options"
> and
> the defname and ideally the defname wrapped in \".
> - errmsg("conflicting or redundant options")));
> + errmsg("conflicting or redundant options %s
> already provided", defel->defname)));
>

I added these since this will now be used outside of logical replication
and getting reasonable error messages when setting up
replication is useful. I added the \" and ,

>
> These types of constructs are IMHO quite hard to read:
> + if(
> + #ifdef WORDS_BIGENDIAN
> + true
> + #else
> + false
> + #endif
> + != bigendian)
> How about spelling out the statement completely for both cases, or perhaps
> encapsulating the logic in a macro? Something like the below perhaps?
> + #ifdef WORDS_BIGENDIAN
> + if (bigendian != true)
> + #else
> + if (bigendian != false)
> + #endif
>
> This change needs to be removed before a commit, just highlighting that
> here to
> avoid it going unnoticed.
> -/* #define WAL_DEBUG */
> +#define WAL_DEBUG
>
> Done

> Reading this I'm left wondering if we shoulnd't introduce macros for the
> kinds,
> since we now compare with 'u', 't' etc in quite a few places and add
> comments
> explaining the types everywhere. A descriptive name would make it easier
> to
> grep for all occurrences, and avoid the need for the comment lines. Thats
> not
> necesarily for this patch though, but an observation from reading it.
>

I'll take a look at adding macros tomorrow.

I've taken care of much of this below

>
>
> I found a few smaller nitpicks as well, some of these might go away by a
> pg_indent run but I've included them all here regardless:
>
> This change, and the subsequent whitespace removal later in the same
> function,
> seems a bit pointless:
> - /* read the relation id */
> relid = pq_getmsgint(in, 4);
> -
>
> Braces should go on the next line:
> + if (options->proto.logical.binary) {
>
> This should be a C /* ... */ comment, or perhaps just removed since the
> code
> is quite self explanatory:
> + // default to false
> + *binary_basetypes = false;
>
> Indentation here:
> - errmsg("conflicting or redundant options")));
> + errmsg("conflicting or redundant options %s
> already provided", defel->defname)));
>
> ..as well as here (there are a few like this one):
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("incompatible datum size")));
>
> Capitalization of "after" to make it a proper sentence:
> + * after we know that the subscriber is requesting binary check to make
> sure
>
> Excessive whitespace and indentation in a few places, and not enough in
> some:
> + if (binary_given)
> + {
> + values[Anum_pg_subscription_subbinary - 1]
> =
> ...
> + if ( *binary_basetypes == true )
> ...
> + if (sizeof(int) != int_size)
> ...
> + if( float4_byval !=
> ...
> + if (sizeof(long) != long_size)
> + ereport(ERROR,
> ...
> + if (tupleData->format[remoteattnum] =='u')
> ...
> + bool binary_basetypes;
>
> That's all for now.
>
> cheers ./daniel
>
>