From: | David Rowley <dgrowleyml(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Making aggregate deserialization (and WAL receive) functions slightly faster |
Date: | 2023-10-09 08:17:00 |
Message-ID: | CAApHDvrhzg8W-vrYugCifA23z7CTD2ECByco_e13svvthHpNfA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, 9 Oct 2023 at 17:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Sorry for not having paid more attention to this thread ... but
> I'm pretty desperately unhappy with the patch as-pushed. I agree
> with the criticism that this is a very repetitive coding pattern
> that could have used a macro. But my real problem with this:
>
> + buf.data = VARDATA_ANY(sstate);
> + buf.len = VARSIZE_ANY_EXHDR(sstate);
> + buf.maxlen = 0;
> + buf.cursor = 0;
>
> is that it totally breaks the StringInfo API without even
> attempting to fix the API specs that it falsifies,
> particularly this in stringinfo.h:
>
> * maxlen is the allocated size in bytes of 'data', i.e. the maximum
> * string size (including the terminating '\0' char) that we can
> * currently store in 'data' without having to reallocate
> * more space. We must always have maxlen > len.
>
> I could see inventing a notion of a "read-only StringInfo"
> to legitimize what you've done here, but you didn't bother
> to try. I do not like this one bit. This is a fairly
> fundamental API and we shouldn't be so cavalier about
> breaking it.
You originally called the centralised logic a "loaded foot-gun" [1],
but now you're complaining about a lack of loaded foot-gun and want a
macro? Which part did I misunderstand? Enlighten me, please.
Here are some more thoughts on how we could improve this:
1. Adjust the definition of StringInfoData.maxlen to define that -1
means the StringInfoData's buffer is externally managed.
2. Adjust enlargeStringInfo() to add a check for maxlen = -1 and have
it palloc, say, pg_next_pow2(str->len * 2) bytes and memcpy the
existing (externally managed string) into the newly palloc'd buffer.
3. Add a new function along the lines of what I originally proposed to
allow init of a StringInfoData using an existing allocated string
which sets maxlen = -1.
4. Update all the existing places, including the ones I just committed
(plus the ones you committed in ba1e066e4) to make use of the function
added in #3.
Better ideas?
David
From | Date | Subject | |
---|---|---|---|
Next Message | Laurenz Albe | 2023-10-09 08:29:07 | Re: Fix output of zero privileges in psql |
Previous Message | Laurenz Albe | 2023-10-09 07:54:23 | Re: Fix output of zero privileges in psql |