Обсуждение: [PATCH] Refactor bytea_sortsupport(), take two

Hi,

This is a follow-up to b45242fd30ff [1]. Previously we separated
varlena.c into varlena.c and bytea.c. This patch makes
bytea_sortsupport() independent from varlena.c code as it was proposed
before [2][3]. The benefits of this change are summarized in the
commit message that I included to the patch.

As always, your feedback is most appreciated.

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b45242fd30ff
[2]: https://postgr.es/m/1502394.1725398354@sss.pgh.pa.us
[3]: https://postgr.es/m/CAJ7c6TO3X88dGd8C4Tb-Eq2ZDPz%2B9mP%2BKOwdzK_82BEz_cMPZg%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

Hi,

> This is a follow-up to b45242fd30ff [1]. Previously we separated
> varlena.c into varlena.c and bytea.c. This patch makes
> bytea_sortsupport() independent from varlena.c code as it was proposed
> before [2][3]. The benefits of this change are summarized in the
> commit message that I included to the patch.
>
> As always, your feedback is most appreciated.

cfbot indicates that v1 needs a rebase. Here is v2.

On Thu, Aug 7, 2025 at 6:44 PM Aleksander Alekseev
<aleksander@tigerdata.com> wrote:
>
> Hi,
>
> > This is a follow-up to b45242fd30ff [1]. Previously we separated
> > varlena.c into varlena.c and bytea.c. This patch makes
> > bytea_sortsupport() independent from varlena.c code as it was proposed
> > before [2][3]. The benefits of this change are summarized in the
> > commit message that I included to the patch.
> >
> > As always, your feedback is most appreciated.
>
> cfbot indicates that v1 needs a rebase. Here is v2.

- * 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. Callers that use the C collation may have NUL bytes
+ * in their strings; this will not work with any other collation, though.

- * More generally, it's okay that bytea callers can have NUL bytes in
- * strings because abbreviated cmp need not make a distinction between
+ * Generally speaking, it's okay that C locale callers can have NUL bytes
+ * in strings because abbreviated cmp need not make a distinction between

Don't these types disallow NUL bytes regardless of locale / character set?

--
John Naylor
Amazon Web Services

Hi John,

Thanks for the review.

> Don't these types disallow NUL bytes regardless of locale / character set?

You are right, they do. Here is the patch v3 with corrected comments.

-- 
Best regards,
Aleksander Alekseev

On Mon, Sep 15, 2025 at 3:40 PM Aleksander Alekseev
<aleksander@tigerdata.com> wrote:
>
> > Don't these types disallow NUL bytes regardless of locale / character set?
>
> You are right, they do. Here is the patch v3 with corrected comments.

Hi Aleksander,

- * 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?

+ * 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.

+#if SIZEOF_DATUM == 8

We recently made all datums 8 bytes.

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.

"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 rest seems fine at a glance.
--
John Naylor
Amazon Web Services

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

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

On Tue, Sep 16, 2025 at 4:33 PM Aleksander Alekseev
<aleksander@tigerdata.com> wrote:
> > 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.

I see plenty of random differences by diff'ing the relevant functions
beteween the two. In a large, old codebase, consistency is one of the
most important things to maintain. Also, it takes hardly any time at
all to copy something that doesn't need changing.

After looking more closely, some of the differences actually make
things worse for maintenance IMO, so I think we need to be more
thoughtful. For example in *_abbrev_convert:

-        * Maintain approximate cardinality of both abbreviated keys
and original,
-        * authoritative keys using HyperLogLog.  Used as cheap
insurance against
-        * the worst case, where we do many string transformations for no saving
-        * in full strcoll()-based comparisons.  These statistics are used by
-        * varstr_abbrev_abort().
-        *
-        * First, Hash key proper, or a significant fraction of it.
Mix in length
-        * in order to compensate for cases where differences are past
-        * PG_CACHE_LINE_SIZE bytes, so as to limit the overhead of hashing.
+        * Maintain approximate cardinality of both abbreviated keys
and original
+        * keys using HyperLogLog.

The first part of the comment explained why we do this at all. The
bytea type does not use strcoll, so it's possible that a future patch
may decide to skip cardinality estimation entirely. It's obviously not
the responsibility of this refactoring patch to investigate that, but
throwing the reason out makes it harder for someone to discover that
bytea could do something different here. In this case, maybe we could
point to varstr_abbrev_convert instead of repeating the whole comment.

        /*
-        * Clamp cardinality estimates to at least one distinct value.  While
-        * NULLs are generally disregarded, if only NULL values were
seen so far,
-        * that might misrepresent costs if we failed to clamp.
+        * Clamp cardinality estimates to at least one distinct value.
         */

The old comment explained why we clamp, but the new comment just says
what the code is doing, and it's obvious what the code is doing.

-       /*
-        * In the worst case all abbreviated keys are identical, while
at the same
-        * time there are differences within full key strings not captured in
-        * abbreviations.
-        */

Why is this gone? I could go on, but hopefully you get my point. If
it's short, just keep it, and adjust as necessary. If it's long, point
to varlena.c if the idea is the same.

Back to the actual patch:

- * More generally, it's okay that bytea callers can have NUL bytes in
- * strings because abbreviated cmp need not make a distinction between
- * terminating NUL bytes, and NUL bytes representing actual NULs in the
- * authoritative representation.  Hopefully a comparison at or past one
- * abbreviated key's terminating NUL byte will resolve the comparison
- * without consulting the authoritative representation; specifically, some
- * later non-NUL byte in the longer string can resolve the comparison
- * against a subsequent terminating NUL in the shorter string.  There will
- * usually be what is effectively a "length-wise" resolution there and
- * then.
- *
- * If that doesn't work out -- if all bytes in the longer string
- * positioned at or past the offset of the smaller string's (first)
- * terminating NUL are actually representative of NUL bytes in the
- * authoritative binary string (perhaps with some *terminating* NUL bytes
- * towards the end of the longer string iff it happens to still be small)
- * -- then an authoritative tie-breaker will happen, and do the right
- * thing: explicitly consider string length.

Why is this gone -- AFAICT it's still true for bytea, no matter what
file it's located in?

--
John Naylor
Amazon Web Services

Hi John,

Many thanks for the feedback.

> For example in *_abbrev_convert:
>
> [...]
>
> The first part of the comment explained why we do this at all. The
> bytea type does not use strcoll, so it's possible that a future patch
> may decide to skip cardinality estimation entirely. It's obviously not
> the responsibility of this refactoring patch to investigate that, but
> throwing the reason out makes it harder for someone to discover that
> bytea could do something different here. In this case, maybe we could
> point to varstr_abbrev_convert instead of repeating the whole comment.

Fixed. I choose to repeat the entire comment since it is relatively
short in this case.

>         /*
> -        * Clamp cardinality estimates to at least one distinct value.  While
> -        * NULLs are generally disregarded, if only NULL values were
> seen so far,
> -        * that might misrepresent costs if we failed to clamp.
> +        * Clamp cardinality estimates to at least one distinct value.
>          */
>
> The old comment explained why we clamp, but the new comment just says
> what the code is doing, and it's obvious what the code is doing.

Fixed in the same manner.

> -       /*
> -        * In the worst case all abbreviated keys are identical, while
> at the same
> -        * time there are differences within full key strings not captured in
> -        * abbreviations.
> -        */
>
> Why is this gone? I could go on, but hopefully you get my point. If
> it's short, just keep it, and adjust as necessary. If it's long, point
> to varlena.c if the idea is the same.

OK, I made sure all the comments are in place. I referenced
corresponding varlena.c functions where the comments were too long to
my taste to repeat them entirely.

> Back to the actual patch:
>
> - * More generally, it's okay that bytea callers can have NUL bytes in
> - * strings because abbreviated cmp need not make a distinction between
> - * terminating NUL bytes, and NUL bytes representing actual NULs in the
> - * authoritative representation.  Hopefully a comparison at or past one
> - * abbreviated key's terminating NUL byte will resolve the comparison
> - * without consulting the authoritative representation; specifically, some
> - * later non-NUL byte in the longer string can resolve the comparison
> - * against a subsequent terminating NUL in the shorter string.  There will
> - * usually be what is effectively a "length-wise" resolution there and
> - * then.
> - *
> - * If that doesn't work out -- if all bytes in the longer string
> - * positioned at or past the offset of the smaller string's (first)
> - * terminating NUL are actually representative of NUL bytes in the
> - * authoritative binary string (perhaps with some *terminating* NUL bytes
> - * towards the end of the longer string iff it happens to still be small)
> - * -- then an authoritative tie-breaker will happen, and do the right
> - * thing: explicitly consider string length.
>
> Why is this gone -- AFAICT it's still true for bytea, no matter what
> file it's located in?

Actually for bytea_abbrev_convert() this comment makes no sense IMO.
Presumably the reader is aware that '\x0000...' is a valid bytea, and
there is no such thing as "terminating NUL bytes" for bytea.

For varstr_abbrev_convert() as you correctly pointed out before [1] we
actually disallow NUL bytes [2]. The reason why the comment is
currently there is that varstr_abbrev_convert() may have bytea callers
which may have NUL bytes.

So it seems to me that rewriting this particular part is exactly in
scope of the refactoring, unless I'm missing something.

[1]: https://postgr.es/m/CANWCAZbDKESq30bn_6QPZqOyrP7JYxxz27Q5gymv0qJEDVj6_A%40mail.gmail.com
[2]: https://www.postgresql.org/docs/current/datatype-character.html

-- 
Best regards,
Aleksander Alekseev

On Mon, Nov 10, 2025 at 6:22 PM Aleksander Alekseev
<aleksander@tigerdata.com> wrote:

> OK, I made sure all the comments are in place. I referenced
> corresponding varlena.c functions where the comments were too long to
> my taste to repeat them entirely.

Looks mostly okay. The "See varstr_abbrev_abort() for more details."
are repeated several times close together, so I would replace those
with a header comment like "This is based on varstr_abbrev_abort(),
but some comments have been elided for brevity. See there for more
details."

+ * the worst case, where we do many string transformations for no saving
+ * in full strcoll()-based comparisons.  These statistics are used by
+ * bytea_abbrev_abort().

s/strcoll/memcmp/, and maybe s/transformations/abbreviations/ since
that was talking about strxfrm().

> > - * More generally, it's okay that bytea callers can have NUL bytes in
> > - * strings because abbreviated cmp need not make a distinction between
> > - * terminating NUL bytes, and NUL bytes representing actual NULs in the
> > - * authoritative representation.  Hopefully a comparison at or past one

> > Why is this gone -- AFAICT it's still true for bytea, no matter what
> > file it's located in?
>
> Actually for bytea_abbrev_convert() this comment makes no sense IMO.
> Presumably the reader is aware that '\x0000...' is a valid bytea, and
> there is no such thing as "terminating NUL bytes" for bytea.

It makes perfect sense to me. The abbreviated datum has terminating
NUL bytes if the source length is shorter than 8 bytes. The comment is
explaining why comparing bytea abbreviated datums still works. It
seems like everything starting with "abbreviated cmp need not ..." is
still appropriate for bytea.c.

Likewise, this part of varlena.c is still factually accurate as an aside:

- * (Actually, even if there were NUL bytes in the blob it would be
- * okay.  See remarks on bytea case above.)

...although with the above, we'll have to point to the relevant place
in bytea.c.

I only have one real nitpick left (diff'ing between files):

    /* Hash abbreviated key */
    {
-         uint32      tmp;
+         uint32      lohalf,
+                     hihalf;

-         tmp = DatumGetUInt32(res) ^ (uint32) (DatumGetUInt64(res) >> 32);
-         hash = DatumGetUInt32(hash_uint32(tmp));
+         lohalf = (uint32) res;
+         hihalf = (uint32) (res >> 32);
+         hash = DatumGetUInt32(hash_uint32(lohalf ^ hihalf));
    }

If we're going to do anything different here at all, the logical thing
would be to simplify with murmurhash64() since datums are now 8 bytes
everywhere. That's material for a separate patch, though.

--
John Naylor
Amazon Web Services