On Wed, Sep 17, 2025 at 5:57 AM Florents Tselai <[email protected]> wrote: > > > > On Wed, Sep 17, 2025 at 12:56 AM Masahiko Sawada <[email protected]> > wrote: >> >> On Wed, Aug 6, 2025 at 12:43 PM Florents Tselai >> <[email protected]> wrote: >> > >> > >> > >> > On Wed, Aug 6, 2025 at 4:34 PM Florents Tselai <[email protected]> >> > wrote: >> >> >> >> Attaching v6 again because it wasn't picked up the last time. >> >> Trying from Gmail's web page this time. >> >> >> >> >> >> >> >> On Tue, Aug 5, 2025 at 12:40 PM Florents Tselai >> >> <[email protected]> wrote: >> >>> >> >>> >> >>> >> >>> On 1 Aug 2025, at 1:13 PM, Florents Tselai <[email protected]> >> >>> wrote: >> >>> >> >>> >> >>> On Tue, Jul 29, 2025 at 3:25 PM Daniel Gustafsson <[email protected]> >> >>> wrote: >> >>>> >> >>>> > On 12 Jul 2025, at 21:40, David E. Wheeler <[email protected]> >> >>>> > wrote: >> >>>> >> >>>> > Thank you! This looks great. The attached revision makes a a couple >> >>>> > of minor changes: >> >>>> >> >>>> I also had a look at this today and agree that it looks pretty close to >> >>>> being >> >>>> done, and a feature we IMHO would like to have. >> >>> >> >>> >> >>> Thanks for having a look Daniel! >> >>> >> >>>> >> >>>> >> >>>> >> >>>> The attached version also adds a commit message, tweaks the >> >>>> documentation along >> >>>> with a few small changes to error message handling etc. >> >>> >> >>> >> >>> In the doc snippet >> >>> >> >>> > The base64url alphabet use '-' instead of '+' and '_' instead of '/' >> >>> > and also omits the '=' padding character. >> >>> >> >>> Should be >> >>> >> >>> > The base64url alphabet uses '-' instead of '+' and '_' instead of '/', >> >>> > and also omits the '=' padding character. >> >>> >> >>> I'd also add a comma before "and also" >> >>> >> >>>> >> >>>> The base64 code this extends is the RFC 2045 variant while base64url is >> >>>> based >> >>>> on base64 from RFC 3548 (obsoleted by RFC 4648). AFAICT this is not a >> >>>> problem >> >>>> here but has anyone else verified this? >> >>> >> >>> >> >>> I don't see how this can be a problem in practice. >> >>> The conversions are straightforward, >> >>> and the codepath used with url=true is a new one and doesn't change past >> >>> behavior. >> >>> >> >>> >> >>> Here’s a v6; necessary because func.sgml was split . >> >>> No other changes compared to v5. >> > >> > >> > v6 introduced some whitespace errors in the regression files. >> > >> > Here's a v7 that fixes that >> >> While the patch looks good to me I have one question: >> >> - errmsg("invalid symbol \"%.*s\" found while >> decoding base64 sequence", >> - pg_mblen(s - 1), s - 1))); >> + errmsg("invalid symbol \"%.*s\" found while >> decoding %s sequence", >> + pg_mblen(s - 1), s - 1, >> + url ? "base64url" : "base64"))); >> >> The above change makes the error message mention the encoding name >> properly. On the other hand, in pg_base64_decode_internal() there are >> two places where we report invalid data and always mention 'based64' >> in the error message: >> >> ereport(ERROR, >> (errcode(ERRCODE_INVALID_PARAMETER_VALUE), >> errmsg("unexpected \"=\" while decoding base64 sequence"))); >> >> and >> >> ereport(ERROR, >> (errcode(ERRCODE_INVALID_PARAMETER_VALUE), >> errmsg("invalid base64 end sequence"), >> errhint("Input data is missing padding, is truncated, or is >> otherwise corrupted."))); >> >> Do we need to have a similar change for these messages? > > > Good catch, Masahiko-san. They shouldn't be hardcoded either. > > I've updated that and also the wording in the regression tests, too.
Thank you for updating the patch! I've done additional tests in my environment and all test cases passed. One very minor comment is that we might want to add 'BASE64URL' to: /* * BASE64 */ Overall, the patch looks good to me. I'll wait for Daniel as he has polished this patch and might have some comments or want to take it. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
