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

Hi John,

> - * Relies on the assumption that text, VarChar, BpChar, and bytea all have the
> - * same representation. Callers that always use the C collation (e.g.
> - * non-collatable type callers like bytea) may have NUL bytes in their strings;
> - * this will not work with any other collation, though.
> + * Relies on the assumption that text, VarChar, and BpChar all have the
> + * same representation. These text types cannot contain NUL bytes.
>
> AFAICS, the only reaon to mention NUL bytes here before was because of
> bytea -- sirnce bytea is being removed from this path, I don't see a
> need to mention mention NUL bytes here. It'd be relevant if any
> simplification is possible in functions that previously may have had
> to worry about NUL bytes. I don't immediately see any such
> opportunities, though. Are there?

Doesn't seem so. I removed the mention of NUL bytes.

> + * Note: text types (text, varchar, bpchar) cannot contain NUL bytes,
> + * so we don't need to worry about NUL byte handling here.
>
> Same here. Also, "Note" is stronger than a normal comment, maybe to
> call attention to complex edge cases or weird behaviors.

Agree. Fixed.

> +#if SIZEOF_DATUM == 8
>
> We recently made all datums 8 bytes.

Right, I forgot to change the patch accordingly. Fixed.

> Some comments are randomly different than the equivalents in
> varlena.c. It's probably better if same things remain the same, but
> there's nothing wrong either.

I did my best to keep the comments consistent between varlena.c and
bytea.c. I don't think they are going to be that consistent in the
long term anyway, so not sure how much effort we should invest into
this.

> "Lastly, the performance and memory consumption could be optimized a little for
> the bytea case."
>
> Is this a side effect of the patch, or a possibility for future work?
> It's not clear.

The idea was to reflect the fact that bytea-specific code becomes
simpler (less if's, etc) and uses structures of smaller size. This is
probably not that important for the commit message. I removed it in
order to avoid confusion.

PFA patch v4.

--
Best regards,
Aleksander Alekseev