Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Separate out bytea sort support from varlena.c
- 9303d62c6db0 19 (unreleased) landed
-
[PATCH] Refactor bytea_sortsupport(), take two
Aleksander Alekseev <aleksander@timescale.com> — 2025-07-02T15:59:02Z
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
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
Aleksander Alekseev <aleksander@tigerdata.com> — 2025-08-07T11:43:48Z
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.
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
John Naylor <johncnaylorls@gmail.com> — 2025-08-11T08:11:21Z
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
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
Aleksander Alekseev <aleksander@tigerdata.com> — 2025-09-15T08:40:19Z
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
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
John Naylor <johncnaylorls@gmail.com> — 2025-09-16T06:46:55Z
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
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
Chao Li <li.evan.chao@gmail.com> — 2025-09-16T08:04:51Z
Hi Aleksander, Overall LGTM. Just a few small comments: > On Sep 15, 2025, at 16:40, Aleksander Alekseev <aleksander@tigerdata.com> wrote: > > -- > Best regards, > Aleksander Alekseev > <v3-0001-Refactor-bytea_sortsupport.patch> 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. 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. */ ``` 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? 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. 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? -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: [PATCH] Refactor bytea_sortsupport(), take two
Aleksander Alekseev <aleksander@tigerdata.com> — 2025-09-16T09:08:57Z
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 -
Re: [PATCH] Refactor bytea_sortsupport(), take two
Aleksander Alekseev <aleksander@tigerdata.com> — 2025-09-16T09:33:10Z
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
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
John Naylor <johncnaylorls@gmail.com> — 2025-10-29T08:54:11Z
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 -
Re: [PATCH] Refactor bytea_sortsupport(), take two
Aleksander Alekseev <aleksander@tigerdata.com> — 2025-11-10T11:22:29Z
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
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
John Naylor <johncnaylorls@gmail.com> — 2025-11-13T09:38:04Z
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 -
Re: [PATCH] Refactor bytea_sortsupport(), take two
Aleksander Alekseev <aleksander@tigerdata.com> — 2025-11-17T10:56:54Z
Hi John, > 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." OK, fixed. > + * 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(). Agree. Fixed. > > > - * 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. OK, I returned the comment to bytea.c, but replaced "string" with "bytea". IMO mentioning "strings" is confusing in this context. I still don't like the fact that the comment is reasoning about "terminating NUL bytes" in the context of bytea, but I kept it as is for now. It seems confusing to me. Do you think we should rewrite this? > 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. Not sure if I follow. Previously we agreed that we disallow NUL bytes within strings, except for terminating ones. What we are doing here is refactoring / simplifying the code. Why keep irrelevant and confusing comments? > 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. Good point. Fixed. -- Best regards, Aleksander Alekseev -
Re: [PATCH] Refactor bytea_sortsupport(), take two
John Naylor <johncnaylorls@gmail.com> — 2025-11-26T07:18:52Z
On Mon, Nov 17, 2025 at 5:57 PM Aleksander Alekseev <aleksander@tigerdata.com> wrote: > > > > - * 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. > > OK, I returned the comment to bytea.c, but replaced "string" with > "bytea". IMO mentioning "strings" is confusing in this context. > > I still don't like the fact that the comment is reasoning about > "terminating NUL bytes" in the context of bytea, but I kept it as is > for now. It seems confusing to me. Do you think we should rewrite > this? + /* + * It's okay that callers can have NUL bytes in bytea because abbreviated + * cmp need not make a distinction between terminating NUL bytes, and NUL + * bytes representing actual NULs in the authoritative representation. I mentioned everything starting with "abbreviated cmp need not ..." was okay, so I agree that "It's okay that callers can have NUL bytes in bytea" is redundant. Was that the confusing part for you? How about this: "Short byteas will have terminating NUL bytes in the abbreviated datum. Abbreviated comparison need not make a distinction between thse NUL bytes, and NUL bytes representing actual NULs in the authoritative representation." [...] After that, the rest seems to flow better at a quick glance. > > 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. > > Not sure if I follow. Previously we agreed that we disallow NUL bytes > within strings, except for terminating ones. What we are doing here is > refactoring / simplifying the code. Why keep irrelevant and confusing > comments? Okay, I'll concede that's it's no longer useful and maybe a distraction.. -- John Naylor Amazon Web Services
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
Aleksander Alekseev <aleksander@tigerdata.com> — 2025-11-27T12:45:15Z
Hi John, > How about this: > > "Short byteas will have terminating NUL bytes in the abbreviated > datum. Abbreviated comparison need not make a distinction between > thse NUL bytes, and NUL bytes representing actual NULs in the > authoritative representation." [...] > > After that, the rest seems to flow better at a quick glance. Yes, it is much better now, thanks! Previously the comment was reasoning about NUL bytes as if normally bytea can't have them which IMO was confusing. -- Best regards, Aleksander Alekseev
-
Re: [PATCH] Refactor bytea_sortsupport(), take two
John Naylor <johncnaylorls@gmail.com> — 2025-12-16T08:28:18Z
On Thu, Nov 27, 2025 at 7:45 PM Aleksander Alekseev <aleksander@tigerdata.com> wrote: > > "Short byteas will have terminating NUL bytes in the abbreviated > > datum. Abbreviated comparison need not make a distinction between > > thse NUL bytes, and NUL bytes representing actual NULs in the > > authoritative representation." [...] > > > > After that, the rest seems to flow better at a quick glance. > > Yes, it is much better now, thanks! Previously the comment was > reasoning about NUL bytes as if normally bytea can't have them which > IMO was confusing. Pushed v7 with a few small adjustments, mostly for pgindent and the new practice to prefer palloc_object(). -- John Naylor Amazon Web Services