Re: [PATCH] Refactor bytea_sortsupport(), take two

Hi Chao,

> 1.
> ····
> + /* We can't afford to leak memory here. */
> + if (PointerGetDatum(arg1) != x)
> + pfree(arg1);
> + if (PointerGetDatum(arg2) != y)
> + pfree(arg2);
> ···
>
> Should we do:
> ```
>     PG_FREE_IF_COPY(arg1, 0);
>     PG_FREE_IF_COPY(arg2, 1)
> ```
>
> Similar to other places.

Hmmm... to me it doesn't look more readable to be honest. I choose the
same pattern used in varlena.c and suggest keeping it for consistency.
IMO this refactoring can be discussed later in a separate thread. Good
observation though.

> 2.
> ···
> +/*
> + * Conversion routine for sortsupport.
> + */
> +static Datum
> +bytea_abbrev_convert(Datum original, SortSupport ssup)
> ···
>
> The function comment is less descriptive. I would suggest something like:
> ```
> /*
>  * Abbreviated key conversion for bytea sortsupport.
>  *
>  * Returns the abbreviated key as a Datum.  If a detoasted copy was made,
>  * it is freed before returning.
>  */
> ```

Sorry, but I don't think I agree with this either. The comment you are
proposing exposes the implementation details. I don't think that the
caller needs to know them.

This is basically a simplified varstr_abbrev_convert(). I was not
certain if I should mention this in a comment and/or how much text I
should copy from the comment for varstr_abbrev_convert(). I'm open to
suggestions.

> 3.
> ```
> + if (abbrev_distinct <= 1.0)
> + abbrev_distinct = 1.0;
> +
> + if (key_distinct <= 1.0)
> + key_distinct = 1.0;
> ```
>
> Why <= 1.0 then set to 1.0? Shouldn't “if" takes just <1.0?

Good point. I'll fix this in v4. Also I'll modify
varstr_abbrev_abort() for consistency.

One could argue that the change of varstr_abbrev_abort() is irrelevant
in the context of this patch. If there are objections we can easily
change '<' back to '<='. It's not that important but '<' looks cleaner
IMO.

> 4.
> ```
>  Datum
>  bytea_sortsupport(PG_FUNCTION_ARGS)
>  {
>   SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
>   MemoryContext oldcontext;
> + ByteaSortSupport *bss;
> ```
>
> “Bss” can be defined in the “if” clause where it is used.

Agree. I'll fix this in v4.

> 5.
> ```
>  Datum
>  bytea_sortsupport(PG_FUNCTION_ARGS)
>  {
>   SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
>   MemoryContext oldcontext;
> + ByteaSortSupport *bss;
> + bool abbreviate = ssup->abbreviate;
> ```
>
> The local variable “abbreviate” is only used once, do we really need to cache ssup->abbreviate into a local variable?

Agree, thanks.

--
Best regards,
Aleksander Alekseev